From 296ced38d61c63357aad57c64429d986960fa566 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 13:25:44 -0400 Subject: [PATCH] Never crash: text, color, doc, paint, geom, raster, compose, vector (#39) - Real crash: build_engine_data took its template from the PSD's text engine data and did expect("EngineDict"), so a text layer whose engine data lacked EngineDict (or wasn't a dictionary) crashed PSD export. The template is now repaired; regression test engine_data_template_without_engine_dict. - color: the built-in CMYK<->sRGB transforms fall back to the naive formulas instead of expect if they ever fail to link. - doc: insert_above falls back to the top of the root instead of expect("valid path"); next_layer_name searches a bounded range. - paint: Mips::new loops with while-let instead of last().expect. - All eight crates deny unwrap/expect/panic/unimplemented/todo/unreachable. Co-authored-by: Claude Opus 5.5 --- crates/color/src/convert.rs | 28 ++++++++++++++++------------ crates/color/src/lib.rs | 1 + crates/compose/src/lib.rs | 1 + crates/doc/src/lib.rs | 11 +++++------ crates/geom/src/lib.rs | 1 + crates/paint/src/lib.rs | 1 + crates/paint/src/tile.rs | 3 +-- crates/raster/src/lib.rs | 1 + crates/text/src/lib.rs | 1 + crates/text/src/psd.rs | 14 ++++++++++++-- crates/text/src/tests.rs | 14 ++++++++++++++ crates/vector/src/lib.rs | 1 + 12 files changed, 55 insertions(+), 22 deletions(-) diff --git a/crates/color/src/convert.rs b/crates/color/src/convert.rs index 4b3b1303..70d52279 100644 --- a/crates/color/src/convert.rs +++ b/crates/color/src/convert.rs @@ -11,33 +11,37 @@ use std::sync::OnceLock; use photocraft_cms::{Builtin, Intent, Transform}; -fn cmyk_to_srgb_transform() -> &'static Transform { - static T: OnceLock = OnceLock::new(); - T.get_or_init(|| { - Transform::new(Builtin::CoatedCmyk.profile(), Builtin::Srgb.profile(), Intent::RelativeColorimetric, true).expect("built-in profiles link") - }) +// The built-in profiles always link (the tests use these transforms); `None` only guards +// against a cms regression, and then the conversions fall back to the naive formulas. +fn cmyk_to_srgb_transform() -> Option<&'static Transform> { + static T: OnceLock> = OnceLock::new(); + T.get_or_init(|| Transform::new(Builtin::CoatedCmyk.profile(), Builtin::Srgb.profile(), Intent::RelativeColorimetric, true).ok()).as_ref() } -fn srgb_to_cmyk_transform() -> &'static Transform { - static T: OnceLock = OnceLock::new(); - T.get_or_init(|| { - Transform::new(Builtin::Srgb.profile(), Builtin::CoatedCmyk.profile(), Intent::RelativeColorimetric, true).expect("built-in profiles link") - }) +fn srgb_to_cmyk_transform() -> Option<&'static Transform> { + static T: OnceLock> = OnceLock::new(); + T.get_or_init(|| Transform::new(Builtin::Srgb.profile(), Builtin::CoatedCmyk.profile(), Intent::RelativeColorimetric, true).ok()).as_ref() } /// Colour-managed CMYK → sRGB (built-in coated CMYK profile, relative colorimetric + BPC). #[inline] pub fn cmyk_to_rgb(c: [f32; 4]) -> [f32; 3] { + let Some(t) = cmyk_to_srgb_transform() else { + return cmyk_to_rgb_naive(c); + }; let mut o = [0.0f32; 3]; - cmyk_to_srgb_transform().eval_fast(&c, &mut o); + t.eval_fast(&c, &mut o); o } /// Colour-managed sRGB → CMYK (built-in coated CMYK profile, relative colorimetric + BPC). #[inline] pub fn rgb_to_cmyk(rgb: [f32; 3]) -> [f32; 4] { + let Some(t) = srgb_to_cmyk_transform() else { + return rgb_to_cmyk_naive(rgb); + }; let mut o = [0.0f32; 4]; - srgb_to_cmyk_transform().eval_fast(&[rgb[0].clamp(0.0, 1.0), rgb[1].clamp(0.0, 1.0), rgb[2].clamp(0.0, 1.0)], &mut o); + t.eval_fast(&[rgb[0].clamp(0.0, 1.0), rgb[1].clamp(0.0, 1.0), rgb[2].clamp(0.0, 1.0)], &mut o); o } diff --git a/crates/color/src/lib.rs b/crates/color/src/lib.rs index d8fb0140..50e969f6 100644 --- a/crates/color/src/lib.rs +++ b/crates/color/src/lib.rs @@ -3,6 +3,7 @@ //! Bit depth and colour model are *runtime data* (see `plan/architecture.md` §1.1): //! nothing in the engine assumes 8-bit or RGB. #![forbid(unsafe_code)] +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] pub mod blend; pub mod convert; diff --git a/crates/compose/src/lib.rs b/crates/compose/src/lib.rs index 7d65613d..c7eb9bac 100644 --- a/crates/compose/src/lib.rs +++ b/crates/compose/src/lib.rs @@ -12,6 +12,7 @@ //! This is the reference the GPU backend (milestone M5) must match within 1/255. Compositing //! currently happens in display RGB. Mode-native (CMYK/Lab) compositing arrives with ICC in M8. #![forbid(unsafe_code)] +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] pub mod adjust; pub mod bounds; diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 875238d4..eba846fc 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -6,6 +6,7 @@ //! Non-destructive kinds (adjustment, fill, text, shape, smart object) and colour modes beyond RGB are //! part of the model from day one, even where rendering support lands later (architecture §1.1). #![forbid(unsafe_code)] +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] pub mod adjust; pub mod analysis; @@ -767,11 +768,8 @@ impl Document { /// Insert `layer` directly above the layer `above` (or at the top of the root if None). pub fn insert_above(&mut self, above: Option, layer: Layer) -> LayerId { let id = layer.id; - match above.and_then(|a| self.path_of(a)) { - Some(path) => { - let (sib, idx) = self.siblings_mut(&path).expect("valid path"); - sib.insert(idx + 1, layer); - } + match above.and_then(|a| self.path_of(a)).and_then(|path| self.siblings_mut(&path)) { + Some((sib, idx)) => sib.insert(idx + 1, layer), None => self.layers.push(layer), } id @@ -799,7 +797,8 @@ impl Document { /// Next unused "Layer N" name. pub fn next_layer_name(&self, base: &str) -> String { let names: std::collections::HashSet<&str> = self.walk().into_iter().map(|(_, _, l)| l.name.as_str()).collect(); - (1..).map(|n| format!("{base} {n}")).find(|n| !names.contains(n.as_str())).expect("infinite") + // At most `names.len() + 1` candidates are needed, so the search always succeeds. + (1..=names.len() + 1).map(|n| format!("{base} {n}")).find(|n| !names.contains(n.as_str())).unwrap_or_else(|| base.to_string()) } /// Top-most layer id, useful as the default active layer. diff --git a/crates/geom/src/lib.rs b/crates/geom/src/lib.rs index c57584c0..d655d33c 100644 --- a/crates/geom/src/lib.rs +++ b/crates/geom/src/lib.rs @@ -3,6 +3,7 @@ //! Document space is integer pixels with the origin at the canvas top-left. //! Layers may extend beyond the canvas (negative coordinates are valid). #![forbid(unsafe_code)] +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] use serde::{Deserialize, Serialize}; diff --git a/crates/paint/src/lib.rs b/crates/paint/src/lib.rs index f051b86b..48a3111e 100644 --- a/crates/paint/src/lib.rs +++ b/crates/paint/src/lib.rs @@ -12,6 +12,7 @@ //! stroke buffer → pixels), [`presets`] (built-ins), [`mixer`] (Mixer Brush), [`replace`] (Color //! Replacement), [`retouch`] (sequential/accumulating helpers for the retouch tools). #![forbid(unsafe_code)] +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] use photocraft_geom::{Point, Rect}; use photocraft_raster::Surface; diff --git a/crates/paint/src/tile.rs b/crates/paint/src/tile.rs index ecabcbb3..f9587d64 100644 --- a/crates/paint/src/tile.rs +++ b/crates/paint/src/tile.rs @@ -158,8 +158,7 @@ pub struct Mips { impl Mips { pub fn new(t: &GrayTile) -> Self { let mut levels = vec![(t.width as usize, t.height as usize, t.to_f32())]; - loop { - let (w, h, v) = levels.last().expect("level"); + while let Some((w, h, v)) = levels.last() { if *w <= 1 && *h <= 1 { break; } diff --git a/crates/raster/src/lib.rs b/crates/raster/src/lib.rs index 36d612df..925d9518 100644 --- a/crates/raster/src/lib.rs +++ b/crates/raster/src/lib.rs @@ -5,6 +5,7 @@ //! so cloning a surface is O(tiles) and mutation copies only the touched tiles. That makes //! undo snapshots, background jobs and autosave cheap. #![forbid(unsafe_code)] +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] use std::collections::BTreeMap; use std::sync::Arc; diff --git a/crates/text/src/lib.rs b/crates/text/src/lib.rs index 3b18ff4f..75562872 100644 --- a/crates/text/src/lib.rs +++ b/crates/text/src/lib.rs @@ -12,6 +12,7 @@ //! The usual entry point is [`TextEngine::render_layer`], which refreshes a //! [`photocraft_doc::TextLayer`]'s cache from its style model. #![forbid(unsafe_code)] +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] pub mod engine_data; pub mod fonts; diff --git a/crates/text/src/psd.rs b/crates/text/src/psd.rs index 6de79e35..5845f7c8 100644 --- a/crates/text/src/psd.rs +++ b/crates/text/src/psd.rs @@ -432,7 +432,15 @@ pub(crate) fn font_entry(name: &str) -> E { /// Updates (or creates) an EngineData tree for `layer`. pub fn build_engine_data(layer: &TextLayer, template: Option, dpi: f32) -> E { let k = 72.0 / if dpi > 0.0 { dpi } else { 72.0 }; - let mut e = template.unwrap_or_else(|| E::Dict(vec![("EngineDict".into(), E::dict()), ("ResourceDict".into(), E::dict())])); + let fresh = || E::Dict(vec![("EngineDict".into(), E::dict()), ("ResourceDict".into(), E::dict())]); + // The template comes from the PSD: it may not be a dictionary or may lack `EngineDict`. + let mut e = match template { + Some(t @ E::Dict(_)) => t, + _ => fresh(), + }; + if !matches!(e.get("EngineDict"), Some(E::Dict(_))) { + e.set("EngineDict", E::dict()); + } let runs = layer.char_runs(); let paras = layer.paragraph_runs(); let text = layer.text.replace('\n', "\r"); @@ -483,7 +491,9 @@ pub fn build_engine_data(layer: &TextLayer, template: Option, dpi: f32) -> E } let fontset = E::Array(fonts.iter().map(|f| font_entry(f)).collect()); - let dict = e.get_mut("EngineDict").expect("EngineDict"); + let Some(dict) = e.get_mut("EngineDict") else { + return fresh(); + }; let mut editor = dict.get("Editor").cloned().unwrap_or_else(E::dict); editor.set("Text", E::String(ed_text)); dict.set("Editor", editor); diff --git a/crates/text/src/tests.rs b/crates/text/src/tests.rs index bdfba50a..2b706398 100644 --- a/crates/text/src/tests.rs +++ b/crates/text/src/tests.rs @@ -471,3 +471,17 @@ fn psd_round_trips_antialias_opentype_and_warp() { assert_eq!(tags, ["frac", "swsh"]); } } + +/// Regression: a PSD whose text engine data has no `EngineDict` (or isn't a dictionary) +/// panicked with `expect("EngineDict")` when the layer was written back (PSD export). +#[test] +fn engine_data_template_without_engine_dict() { + use crate::engine_data::{self as ed, Value as E}; + let t = styled("Hi", CharStyle::default()); + let no_engine_dict = ed::parse(b"<< /ResourceDict << >> >>").unwrap(); + for template in [no_engine_dict, E::Dict(vec![]), E::Int(3)] { + let e = crate::psd::build_engine_data(&t, Some(template), 72.0); + let text = e.path(&["EngineDict", "Editor", "Text"]); + assert!(matches!(text, Some(E::String(s)) if s == "Hi\r"), "{text:?}"); + } +} diff --git a/crates/vector/src/lib.rs b/crates/vector/src/lib.rs index 35059fd2..c50c87ca 100644 --- a/crates/vector/src/lib.rs +++ b/crates/vector/src/lib.rs @@ -15,6 +15,7 @@ //! [`PixelFormat`] (only tiles that differ from the default are allocated). Pure Rust, no //! platform dependencies; builds for `wasm32` (single-threaded there, rayon elsewhere). #![forbid(unsafe_code)] +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] pub mod flatten; pub mod raster;