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 <noreply@anthropic.com>
This commit is contained in:
Brandon Thomas 2026-10-04 13:25:44 -04:00 committed by GitHub
parent 48d1a4f9e5
commit 296ced38d6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 55 additions and 22 deletions

View File

@ -11,33 +11,37 @@ use std::sync::OnceLock;
use photocraft_cms::{Builtin, Intent, Transform}; use photocraft_cms::{Builtin, Intent, Transform};
fn cmyk_to_srgb_transform() -> &'static Transform { // The built-in profiles always link (the tests use these transforms); `None` only guards
static T: OnceLock<Transform> = OnceLock::new(); // against a cms regression, and then the conversions fall back to the naive formulas.
T.get_or_init(|| { fn cmyk_to_srgb_transform() -> Option<&'static Transform> {
Transform::new(Builtin::CoatedCmyk.profile(), Builtin::Srgb.profile(), Intent::RelativeColorimetric, true).expect("built-in profiles link") static T: OnceLock<Option<Transform>> = 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 { fn srgb_to_cmyk_transform() -> Option<&'static Transform> {
static T: OnceLock<Transform> = OnceLock::new(); static T: OnceLock<Option<Transform>> = OnceLock::new();
T.get_or_init(|| { T.get_or_init(|| Transform::new(Builtin::Srgb.profile(), Builtin::CoatedCmyk.profile(), Intent::RelativeColorimetric, true).ok()).as_ref()
Transform::new(Builtin::Srgb.profile(), Builtin::CoatedCmyk.profile(), Intent::RelativeColorimetric, true).expect("built-in profiles link")
})
} }
/// Colour-managed CMYK → sRGB (built-in coated CMYK profile, relative colorimetric + BPC). /// Colour-managed CMYK → sRGB (built-in coated CMYK profile, relative colorimetric + BPC).
#[inline] #[inline]
pub fn cmyk_to_rgb(c: [f32; 4]) -> [f32; 3] { 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]; let mut o = [0.0f32; 3];
cmyk_to_srgb_transform().eval_fast(&c, &mut o); t.eval_fast(&c, &mut o);
o o
} }
/// Colour-managed sRGB → CMYK (built-in coated CMYK profile, relative colorimetric + BPC). /// Colour-managed sRGB → CMYK (built-in coated CMYK profile, relative colorimetric + BPC).
#[inline] #[inline]
pub fn rgb_to_cmyk(rgb: [f32; 3]) -> [f32; 4] { 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]; 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 o
} }

View File

@ -3,6 +3,7 @@
//! Bit depth and colour model are *runtime data* (see `plan/architecture.md` §1.1): //! Bit depth and colour model are *runtime data* (see `plan/architecture.md` §1.1):
//! nothing in the engine assumes 8-bit or RGB. //! nothing in the engine assumes 8-bit or RGB.
#![forbid(unsafe_code)] #![forbid(unsafe_code)]
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
pub mod blend; pub mod blend;
pub mod convert; pub mod convert;

View File

@ -12,6 +12,7 @@
//! This is the reference the GPU backend (milestone M5) must match within 1/255. Compositing //! 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. //! currently happens in display RGB. Mode-native (CMYK/Lab) compositing arrives with ICC in M8.
#![forbid(unsafe_code)] #![forbid(unsafe_code)]
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
pub mod adjust; pub mod adjust;
pub mod bounds; pub mod bounds;

View File

@ -6,6 +6,7 @@
//! Non-destructive kinds (adjustment, fill, text, shape, smart object) and colour modes beyond RGB are //! 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). //! part of the model from day one, even where rendering support lands later (architecture §1.1).
#![forbid(unsafe_code)] #![forbid(unsafe_code)]
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
pub mod adjust; pub mod adjust;
pub mod analysis; 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). /// Insert `layer` directly above the layer `above` (or at the top of the root if None).
pub fn insert_above(&mut self, above: Option<LayerId>, layer: Layer) -> LayerId { pub fn insert_above(&mut self, above: Option<LayerId>, layer: Layer) -> LayerId {
let id = layer.id; let id = layer.id;
match above.and_then(|a| self.path_of(a)) { match above.and_then(|a| self.path_of(a)).and_then(|path| self.siblings_mut(&path)) {
Some(path) => { Some((sib, idx)) => sib.insert(idx + 1, layer),
let (sib, idx) = self.siblings_mut(&path).expect("valid path");
sib.insert(idx + 1, layer);
}
None => self.layers.push(layer), None => self.layers.push(layer),
} }
id id
@ -799,7 +797,8 @@ impl Document {
/// Next unused "Layer N" name. /// Next unused "Layer N" name.
pub fn next_layer_name(&self, base: &str) -> String { 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(); 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. /// Top-most layer id, useful as the default active layer.

View File

@ -3,6 +3,7 @@
//! Document space is integer pixels with the origin at the canvas top-left. //! Document space is integer pixels with the origin at the canvas top-left.
//! Layers may extend beyond the canvas (negative coordinates are valid). //! Layers may extend beyond the canvas (negative coordinates are valid).
#![forbid(unsafe_code)] #![forbid(unsafe_code)]
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};

View File

@ -12,6 +12,7 @@
//! stroke buffer → pixels), [`presets`] (built-ins), [`mixer`] (Mixer Brush), [`replace`] (Color //! stroke buffer → pixels), [`presets`] (built-ins), [`mixer`] (Mixer Brush), [`replace`] (Color
//! Replacement), [`retouch`] (sequential/accumulating helpers for the retouch tools). //! Replacement), [`retouch`] (sequential/accumulating helpers for the retouch tools).
#![forbid(unsafe_code)] #![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_geom::{Point, Rect};
use photocraft_raster::Surface; use photocraft_raster::Surface;

View File

@ -158,8 +158,7 @@ pub struct Mips {
impl Mips { impl Mips {
pub fn new(t: &GrayTile) -> Self { pub fn new(t: &GrayTile) -> Self {
let mut levels = vec![(t.width as usize, t.height as usize, t.to_f32())]; let mut levels = vec![(t.width as usize, t.height as usize, t.to_f32())];
loop { while let Some((w, h, v)) = levels.last() {
let (w, h, v) = levels.last().expect("level");
if *w <= 1 && *h <= 1 { if *w <= 1 && *h <= 1 {
break; break;
} }

View File

@ -5,6 +5,7 @@
//! so cloning a surface is O(tiles) and mutation copies only the touched tiles. That makes //! so cloning a surface is O(tiles) and mutation copies only the touched tiles. That makes
//! undo snapshots, background jobs and autosave cheap. //! undo snapshots, background jobs and autosave cheap.
#![forbid(unsafe_code)] #![forbid(unsafe_code)]
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
use std::collections::BTreeMap; use std::collections::BTreeMap;
use std::sync::Arc; use std::sync::Arc;

View File

@ -12,6 +12,7 @@
//! The usual entry point is [`TextEngine::render_layer`], which refreshes a //! The usual entry point is [`TextEngine::render_layer`], which refreshes a
//! [`photocraft_doc::TextLayer`]'s cache from its style model. //! [`photocraft_doc::TextLayer`]'s cache from its style model.
#![forbid(unsafe_code)] #![forbid(unsafe_code)]
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
pub mod engine_data; pub mod engine_data;
pub mod fonts; pub mod fonts;

View File

@ -432,7 +432,15 @@ pub(crate) fn font_entry(name: &str) -> E {
/// Updates (or creates) an EngineData tree for `layer`. /// Updates (or creates) an EngineData tree for `layer`.
pub fn build_engine_data(layer: &TextLayer, template: Option<E>, dpi: f32) -> E { pub fn build_engine_data(layer: &TextLayer, template: Option<E>, dpi: f32) -> E {
let k = 72.0 / if dpi > 0.0 { dpi } else { 72.0 }; 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 runs = layer.char_runs();
let paras = layer.paragraph_runs(); let paras = layer.paragraph_runs();
let text = layer.text.replace('\n', "\r"); let text = layer.text.replace('\n', "\r");
@ -483,7 +491,9 @@ pub fn build_engine_data(layer: &TextLayer, template: Option<E>, dpi: f32) -> E
} }
let fontset = E::Array(fonts.iter().map(|f| font_entry(f)).collect()); 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); let mut editor = dict.get("Editor").cloned().unwrap_or_else(E::dict);
editor.set("Text", E::String(ed_text)); editor.set("Text", E::String(ed_text));
dict.set("Editor", editor); dict.set("Editor", editor);

View File

@ -471,3 +471,17 @@ fn psd_round_trips_antialias_opentype_and_warp() {
assert_eq!(tags, ["frac", "swsh"]); 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:?}");
}
}

View File

@ -15,6 +15,7 @@
//! [`PixelFormat`] (only tiles that differ from the default are allocated). Pure Rust, no //! [`PixelFormat`] (only tiles that differ from the default are allocated). Pure Rust, no
//! platform dependencies; builds for `wasm32` (single-threaded there, rayon elsewhere). //! platform dependencies; builds for `wasm32` (single-threaded there, rayon elsewhere).
#![forbid(unsafe_code)] #![forbid(unsafe_code)]
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
pub mod flatten; pub mod flatten;
pub mod raster; pub mod raster;