From 0b5e15e2b9547692953b54565f5ca040dd58a793 Mon Sep 17 00:00:00 2001 From: Matt Date: Sun, 11 Oct 2026 07:29:16 -0700 Subject: [PATCH] fix: continue the plane depth count through model geometry A construction plane built from model geometry resolves each of its sides through the document again. That hop restarted the recursion depth at 0, so a plane that is its own side never exceeded depth 1 and the 32-level cap never fired: a ~600-byte design file recursed until the stack overflowed and the process aborted, which no panic guard can intercept. Carry the depth on the construction context so the geometry arms continue the caller's count instead of restarting it. Fixes #143 Co-Authored-By: Claude Opus 5 (cherry picked from commit 4bca1d49f27dfe9ef0140132746e1c7415daba8e) --- apps/solvecraft-cli/tests/design_corpus.rs | 18 ++++++++++++ crates/doc/src/construct.rs | 8 ++++-- crates/doc/src/document.rs | 2 +- crates/doc/src/eval.rs | 4 +-- crates/doc/src/tests.rs | 33 ++++++++++++++++++++++ 5 files changed, 60 insertions(+), 5 deletions(-) diff --git a/apps/solvecraft-cli/tests/design_corpus.rs b/apps/solvecraft-cli/tests/design_corpus.rs index 7cc2fb24..93ab92cd 100644 --- a/apps/solvecraft-cli/tests/design_corpus.rs +++ b/apps/solvecraft-cli/tests/design_corpus.rs @@ -150,6 +150,23 @@ fn try_bytes(bytes: &[u8]) { } } +/// Hostile shapes that no mutation of a saved file produces: plane references that close a +/// cycle, which resolve through the model and back into the document. +fn generated() -> Vec<(String, Vec)> { + let to = |name: &str| json!({"type": "plane", "plane": {"type": "construction", "name": name}}); + let plane = |id: u64, name: &str, side: Value| { + json!({"id": id, "name": name, "suppressed": false, "type": "construction_plane", + "plane": {"type": "midplane", "a": side, "b": {"type": "plane", "plane": {"type": "origin", "name": "XZ"}}}}) + }; + let design = |name: &str, features: Value| { + json!({"format": "solvecraft/11", "name": name, "units": "mm", "params": [], "next_id": 9, "features": features}).to_string().into_bytes() + }; + vec![ + ("construction plane is its own side".into(), design("selfplane", json!([plane(1, "P1", to("P1"))]))), + ("two construction planes refer to each other".into(), design("twoplanes", json!([plane(1, "P1", to("P2")), plane(2, "P2", to("P1"))]))), + ] +} + #[test] fn damaged_and_hostile_design_files_never_panic() { let mut rng = Rng(0x5eed_cafe_f00d_d00d); @@ -201,6 +218,7 @@ fn damaged_and_hostile_design_files_never_panic() { cases.push(("binary".into(), (0..4096u32).map(|i| (i.wrapping_mul(2_654_435_761) >> 24) as u8).collect())); all_cases.extend(cases.into_iter().map(|(what, b)| (name.clone(), what, b))); } + all_cases.extend(generated().into_iter().map(|(what, b)| ("generated".to_string(), what, b))); let next = std::sync::atomic::AtomicUsize::new(0); let threads = std::thread::available_parallelism().map_or(4, |n| n.get()); let failures: Vec = std::thread::scope(|scope| { diff --git a/crates/doc/src/construct.rs b/crates/doc/src/construct.rs index c666faec..d6a7ee8e 100644 --- a/crates/doc/src/construct.rs +++ b/crates/doc/src/construct.rs @@ -105,6 +105,10 @@ pub(crate) struct Ctx<'a> { pub doc: &'a Document, pub vals: &'a BTreeMap, pub st: &'a ModelState, + /// How deep the plane resolution that led here already is. A plane built from model + /// geometry resolves its sides through the document again, so the count has to continue + /// across that hop; restarting it lets a plane that refers to itself recurse forever. + pub depth: usize, } fn bad(m: impl Into) -> DocError { @@ -281,7 +285,7 @@ pub(crate) fn line(ctx: &Ctx, g: &GeoRef) -> Result<(Vec3, Vec3)> { /// A reference as a plane. pub(crate) fn plane(ctx: &Ctx, g: &GeoRef) -> Result { match g { - GeoRef::Plane { plane } => ctx.doc.resolve_plane_in(ctx.vals, plane, 0, Some(ctx.st)), + GeoRef::Plane { plane } => ctx.doc.resolve_plane_in(ctx.vals, plane, ctx.depth + 1, Some(ctx.st)), GeoRef::Face { at, name } => { let (b, fi, q) = face_of(ctx.st, *at, name)?; match kernel::face_surface(&b.body, fi) { @@ -527,7 +531,7 @@ pub(crate) fn resolve_geo_plane(ctx: &Ctx, p: &PlaneRef) -> Result { let (p, tan) = along(&path_points(ctx, path)?, t).ok_or_else(|| bad("the path has no length"))?; Plane::from_normal(p, tan).ok_or_else(degenerate) } - _ => ctx.doc.resolve_plane_in(ctx.vals, p, 0, Some(ctx.st)), + _ => ctx.doc.resolve_plane_in(ctx.vals, p, ctx.depth + 1, Some(ctx.st)), } } diff --git a/crates/doc/src/document.rs b/crates/doc/src/document.rs index fc9b9e29..41cff42a 100644 --- a/crates/doc/src/document.rs +++ b/crates/doc/src/document.rs @@ -2132,7 +2132,7 @@ impl Document { | PlaneRef::AlongPath { .. } | PlaneRef::Perpendicular { .. } => { let st = st.ok_or_else(|| DocError::Invalid("this plane is built from the model: evaluate first".into()))?; - crate::construct::resolve_geo_plane(&crate::construct::Ctx { doc: self, vals, st }, p) + crate::construct::resolve_geo_plane(&crate::construct::Ctx { doc: self, vals, st, depth }, p) } } } diff --git a/crates/doc/src/eval.rs b/crates/doc/src/eval.rs index a54e83a1..90aca9f0 100644 --- a/crates/doc/src/eval.rs +++ b/crates/doc/src/eval.rs @@ -2905,7 +2905,7 @@ fn eval_feature(doc: &Document, vals: &BTreeMap, f: &Feature, st: Ok(()) } FeatureKind::ConstructionAxis { def } => { - let (origin, dir) = crate::construct::resolve_axis(&crate::construct::Ctx { doc, vals, st }, def)?; + let (origin, dir) = crate::construct::resolve_axis(&crate::construct::Ctx { doc, vals, st, depth: 0 }, def)?; st.construct.push(crate::construct::Construct { feature: f.id, name: f.name.clone(), @@ -2914,7 +2914,7 @@ fn eval_feature(doc: &Document, vals: &BTreeMap, f: &Feature, st: Ok(()) } FeatureKind::ConstructionPoint { def } => { - let p = crate::construct::resolve_point(&crate::construct::Ctx { doc, vals, st }, def)?; + let p = crate::construct::resolve_point(&crate::construct::Ctx { doc, vals, st, depth: 0 }, def)?; st.construct.push(crate::construct::Construct { feature: f.id, name: f.name.clone(), geom: crate::construct::ConstructGeom::Point(p) }); Ok(()) } diff --git a/crates/doc/src/tests.rs b/crates/doc/src/tests.rs index 3ea65a68..c4bea724 100644 --- a/crates/doc/src/tests.rs +++ b/crates/doc/src/tests.rs @@ -367,3 +367,36 @@ fn timeline_clones_share_meshes_even_when_cloned_before_tessellation() { assert!(std::sync::Arc::ptr_eq(&a.mesh(), &b.mesh())); assert!(!a.mesh().triangles.is_empty()); } + +/// A construction plane that is its own side resolves through the model and back into the +/// document. The depth count has to survive that hop, or the recursion overflows the stack and +/// aborts the process, which no panic guard can intercept. +#[test] +fn self_referencing_construction_planes_are_errors() { + let selfref = |name: &str| Box::new(construct::GeoRef::Plane { plane: PlaneRef::Construction { name: name.into() } }); + let xz = || Box::new(construct::GeoRef::Plane { plane: PlaneRef::Origin { name: "XZ".into() } }); + + // Directly its own first side. + let mut doc = Document::new("selfplane"); + doc.add_feature(FeatureKind::ConstructionPlane { plane: PlaneRef::Midplane { a: selfref("P1"), b: xz() } }, Some("P1")).unwrap(); + let mut m = Model::new(); + m.evaluate(&doc); + assert!(m.results.iter().any(|r| r.error.is_some()), "a plane that is its own side must report an error: {:?}", m.results); + + // Two planes that are each other's side. + let mut doc = Document::new("twoplanes"); + doc.add_feature(FeatureKind::ConstructionPlane { plane: PlaneRef::Midplane { a: selfref("P2"), b: xz() } }, Some("P1")).unwrap(); + doc.add_feature(FeatureKind::ConstructionPlane { plane: PlaneRef::Midplane { a: selfref("P1"), b: xz() } }, Some("P2")).unwrap(); + let mut m = Model::new(); + m.evaluate(&doc); + assert!(m.results.iter().any(|r| r.error.is_some()), "a two-plane cycle must report an error: {:?}", m.results); + + // A perpendicular plane whose base is itself, and a sketch placed on it. + let mut doc = Document::new("perpplane"); + let line = Box::new(construct::GeoRef::World { axis: "X".into() }); + doc.add_feature(FeatureKind::ConstructionPlane { plane: PlaneRef::Perpendicular { plane: selfref("P1"), line } }, Some("P1")).unwrap(); + doc.add_feature(FeatureKind::Sketch { plane: PlaneRef::Construction { name: "P1".into() }, sketch: Sketch::new() }, Some("Sketch1")).unwrap(); + let mut m = Model::new(); + m.evaluate(&doc); + assert!(m.results.iter().any(|r| r.error.is_some()), "a cyclic perpendicular plane must report an error: {:?}", m.results); +}