From a539c766325dd91e7c27fcd6ffbc55c8cddc607f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 11:39:34 -0400 Subject: [PATCH] Never crash: doc and compose expect() calls become let-else / fallbacks. A parent spread without pages (possible in a .designcraft file) no longer underflows in parent_page_for, and check() rejects a next_id so large that allocating ids would overflow. Both crates deny the panicking lints. Co-Authored-By: Claude Opus 5.5 --- clippy.toml | 4 ++++ crates/compose/src/lib.rs | 8 +++++--- crates/compose/src/table.rs | 2 +- crates/doc/src/build.rs | 15 ++++++++++++++- crates/doc/src/edit.rs | 3 ++- crates/doc/src/endnotes.rs | 3 ++- crates/doc/src/lib.rs | 9 +++++++++ crates/doc/src/story.rs | 7 +++++-- crates/doc/src/styles.rs | 2 +- crates/doc/src/table.rs | 2 +- crates/doc/src/tests.rs | 12 ++++++++++++ 11 files changed, 56 insertions(+), 11 deletions(-) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 00000000..4b6c79f5 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +allow-unwrap-in-tests = true +allow-expect-in-tests = true +allow-panic-in-tests = true +allow-indexing-slicing-in-tests = true diff --git a/crates/compose/src/lib.rs b/crates/compose/src/lib.rs index b48591fc..9f91aa68 100644 --- a/crates/compose/src/lib.rs +++ b/crates/compose/src/lib.rs @@ -7,6 +7,7 @@ //! breaks, vertical justification) → overset detection. //! //! Output coordinates are each frame's *inner* space (apply the frame item's `xf` for spread space). +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] #![forbid(unsafe_code)] pub mod breaker; @@ -2162,13 +2163,14 @@ pub fn path_glyphs( }; let m = place * designcraft_geom::Affine::translate((g.x, l.baseline + g.y)) * skew * designcraft_geom::Affine::scale_non_uniform(g.sx, g.sy); - let bp = match runs.iter_mut().find(|r| r.0 == g.style) { - Some(r) => &mut r.1, + let k = match runs.iter().position(|r| r.0 == g.style) { + Some(k) => k, None => { runs.push((g.style, designcraft_geom::BezPath::new())); - &mut runs.last_mut().expect("pushed").1 + runs.len() - 1 } }; + let bp = &mut runs[k].1; bp.extend((m * outline.as_ref().clone()).elements().iter().copied()); } } diff --git a/crates/compose/src/table.rs b/crates/compose/src/table.rs index b191f29c..bda00182 100644 --- a/crates/compose/src/table.rs +++ b/crates/compose/src/table.rs @@ -339,7 +339,7 @@ fn emit( } else { None }; - let lay = layouts[r * nc + c].as_ref().expect("owner composed"); + let Some(lay) = layouts.get(r * nc + c).and_then(Option::as_ref) else { continue }; let clip = Rect::new(rect.x0 + cell.insets[1], rect.y0 + cell.insets[0], rect.x1 - cell.insets[3], rect.y1 - cell.insets[2]); let space = clip.height() - lay.content_h; let dy = match cell.vj { diff --git a/crates/doc/src/build.rs b/crates/doc/src/build.rs index b7c16de4..ea58043c 100644 --- a/crates/doc/src/build.rs +++ b/crates/doc/src/build.rs @@ -544,7 +544,8 @@ impl Document { let page = self.page(abs)?; let pi = self.parent_index(page.parent?)?; let ps = &self.parents[pi]; - let idx = if ps.pages.len() >= 2 && page.side == PageSide::Left { 0 } else { ps.pages.len() - 1 }; + // A parent spread can come without pages from a file. + let idx = if ps.pages.len() >= 2 && page.side == PageSide::Left { 0 } else { ps.pages.len().checked_sub(1)? }; Some((pi, idx)) } } @@ -567,6 +568,18 @@ mod tests { d.check().unwrap(); } + /// A document whose parent spread has no pages passes `check()`; showing the parent behind a + /// page used to compute index `0 - 1`. + #[test] + fn parent_without_pages_is_not_shown() { + let mut d = Document::new(&NewDocument { pages: 2, ..Default::default() }); + assert!(d.parent_page_for(0).is_some()); + Arc::make_mut(&mut d.parents[0]).pages.clear(); + d.check().unwrap(); + assert_eq!(d.parent_page_for(0), None); + assert_eq!(d.parent_page_for(1), None); + } + #[test] fn non_facing_one_page_per_spread() { let d = Document::new(&NewDocument { pages: 3, facing_pages: false, ..Default::default() }); diff --git a/crates/doc/src/edit.rs b/crates/doc/src/edit.rs index 0f8914fb..a1f21f97 100644 --- a/crates/doc/src/edit.rs +++ b/crates/doc/src/edit.rs @@ -148,7 +148,8 @@ impl Document { for &i in &loc.path[1..loc.path.len() - 1] { it = Arc::make_mut(&mut it.children_mut().ok_or(DocError::NoItem(id))?[i]); } - it.children_mut().ok_or(DocError::NoItem(id))?.remove(*loc.path.last().expect("non-empty")) + let last = *loc.path.last().ok_or(DocError::NoItem(id))?; + it.children_mut().ok_or(DocError::NoItem(id))?.remove(last) } }; let mut frames = Vec::new(); diff --git a/crates/doc/src/endnotes.rs b/crates/doc/src/endnotes.rs index 5e8e5409..b700254a 100644 --- a/crates/doc/src/endnotes.rs +++ b/crates/doc/src/endnotes.rs @@ -176,7 +176,8 @@ impl crate::Document { if cur.text == fresh.text && cur.paras == fresh.paras && cur.chars == fresh.chars { return; } - let st = Arc::make_mut(self.stories.get_mut(&sid).expect("checked")); + let Some(st) = self.stories.get_mut(&sid) else { return }; + let st = Arc::make_mut(st); let (frames, rev) = (std::mem::take(&mut st.frames), st.rev); *st = Story { frames, rev: rev + 1, ..fresh }; } diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index dca3a1ca..271126d5 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -6,6 +6,7 @@ //! //! Coordinates are points, y down. Each spread has its own space: pages sit side by side from //! x = 0 with their tops at y = 0. Items live in spread space via `Item::xf`. +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] #![forbid(unsafe_code)] pub mod anchored; @@ -68,6 +69,10 @@ pub enum DocError { pub type Result = std::result::Result; +/// Largest `next_id` a valid document has (ids count up from it; 2^53 also stays exact in JSON +/// readers that use doubles). +pub const MAX_NEXT_ID: u64 = 1 << 53; + /// Edit › Transparency Blend Space: the colour space transparency is flattened in (CMYK for /// print documents, RGB for screen ones, as InDesign defaults). #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] @@ -716,6 +721,10 @@ impl Document { if self.next_id < ids.iter().copied().max().unwrap_or(0) { return Err(DocError::Invalid("next_id behind existing ids".into())); } + // Ids are allocated by counting up from `next_id`: leave room so that can't overflow. + if self.next_id > MAX_NEXT_ID { + return Err(DocError::Invalid(format!("next_id {} out of range", self.next_id))); + } Ok(()) } } diff --git a/crates/doc/src/story.rs b/crates/doc/src/story.rs index 3fe7bf5b..bbb5b6f5 100644 --- a/crates/doc/src/story.rs +++ b/crates/doc/src/story.rs @@ -58,6 +58,9 @@ impl Default for ParaFormat { } } +/// The format a story without runs reports (a valid story always has one run). +static NO_FORMAT: std::sync::LazyLock = std::sync::LazyLock::new(CharFormat::default); + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct CharFormat { @@ -186,7 +189,7 @@ impl Story { } start = end; } - &self.chars.last().expect("at least one run").format + self.chars.last().map_or(&NO_FORMAT, |r| &r.format) } /// Iterate `(range, format)` of character runs. @@ -310,7 +313,7 @@ impl Story { } start += r.len; } - &self.chars.last().expect("run").format + self.chars.last().map_or(&NO_FORMAT, |r| &r.format) } /// Apply `f` to the character format of every run intersecting `range` (splitting runs at the diff --git a/crates/doc/src/styles.rs b/crates/doc/src/styles.rs index 34de5c66..d294a950 100644 --- a/crates/doc/src/styles.rs +++ b/crates/doc/src/styles.rs @@ -342,7 +342,7 @@ impl Styles { if !existing(base) { return base.to_string(); } - (2..).map(|i| format!("{base} {i}")).find(|n| !existing(n)).expect("infinite") + (2..=usize::MAX).map(|i| format!("{base} {i}")).find(|n| !existing(n)).unwrap_or_else(|| format!("{base} copy")) } } diff --git a/crates/doc/src/table.rs b/crates/doc/src/table.rs index 80774a44..1afa45f7 100644 --- a/crates/doc/src/table.rs +++ b/crates/doc/src/table.rs @@ -298,7 +298,7 @@ impl Table { let mut t = Table::new(id, data.len().max(1), cols, 0, 0, width); for (r, row) in data.iter().enumerate() { for (c, s) in row.iter().enumerate() { - let cell = t.cell_mut(r, c).expect("in range"); + let Some(cell) = t.cell_mut(r, c) else { continue }; cell.text = Story::with_text(StoryId(0), "", ParaFormat { table: None, ..para.clone() }); cell.text.chars[0].format = fmt.clone(); cell.text.insert(0, s); diff --git a/crates/doc/src/tests.rs b/crates/doc/src/tests.rs index 98dee2ac..c3ba44df 100644 --- a/crates/doc/src/tests.rs +++ b/crates/doc/src/tests.rs @@ -100,3 +100,15 @@ fn document_serde_roundtrip() { assert_eq!(d, back); back.check().unwrap(); } + +/// A file can set `next_id` to `u64::MAX`; the next allocated id then overflowed (IDML export +/// adds to it too). Such a document is rejected when it is checked on load. +#[test] +fn next_id_near_the_top_is_rejected() { + let mut d = Document::new(&crate::build::NewDocument::default()); + d.check().unwrap(); + d.next_id = u64::MAX; + assert!(d.check().is_err()); + d.next_id = crate::MAX_NEXT_ID; + d.check().unwrap(); +}