From 9c08e8a3eda18de75f544329a599ef75c7c96c13 Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Mon, 12 Oct 2026 17:53:43 +0100 Subject: [PATCH] fix(blocks): prevent circular block nesting and cut render cycles Introduce would_create_cycle in block_service to prevent recursive block definitions with case-insensitive matching across block records. Enforce cycle refusal at INSERT commit in BEDIT and during block redefinition (redefine_block_raw) with a shared error message. Ensure render-time recursion truncates safely with a one-time session warning on cyclic documents. --- src/app/command_driver/entity_commit.rs | 29 +++++ src/app/update/block_ops_tests.rs | 44 ++++++++ src/app/update/blocks_palette.rs | 6 +- src/app/update/dialog.rs | 7 +- src/scene/block_base_point_tests.rs | 79 ++++++++++--- src/scene/block_service.rs | 141 ++++++++++++++++++++++++ src/scene/cache/block_cache.rs | 123 ++++++++++++++++++++- src/scene/entity.rs | 30 ++++- 8 files changed, 435 insertions(+), 24 deletions(-) diff --git a/src/app/command_driver/entity_commit.rs b/src/app/command_driver/entity_commit.rs index 85c3dfc19..35300a2f3 100644 --- a/src/app/command_driver/entity_commit.rs +++ b/src/app/command_driver/entity_commit.rs @@ -982,6 +982,35 @@ impl OpenCADStudio { pub(super) fn handle_attreq_needed(&mut self, block_name: String) -> Option> { let i = self.active_tab; + // When active space is a block editor session (BEDIT), refuse inserting + // any block that would create a circular nesting dependency. + if let Some(edit_handle) = self.tabs[i].scene.block_edit_block { + let container_name = self.tabs[i] + .scene + .document + .block_records + .iter() + .find(|br| br.handle == edit_handle) + .map(|br| br.name.clone()); + if let Some(container) = container_name { + if crate::scene::block_service::would_create_cycle( + &self.tabs[i].scene.document, + &container, + &block_name, + ) { + self.command_line.push_error( + &crate::scene::block_service::circular_dependency_error( + &container, + &block_name, + ), + ); + self.tabs[i].active_cmd = None; + self.tabs[i].snap_result = None; + self.tabs[i].scene.clear_preview_wire(); + return None; + } + } + } // Collect the full AttributeDefinitions owned by this block // record so each created attribute keeps its geometry (#255). let mut attdefs: Vec = { diff --git a/src/app/update/block_ops_tests.rs b/src/app/update/block_ops_tests.rs index 260b016b1..bfae7258e 100644 --- a/src/app/update/block_ops_tests.rs +++ b/src/app/update/block_ops_tests.rs @@ -1519,3 +1519,47 @@ fn locked_attedit_attribute_flags_cases() { )); assert_eq!(preset_entity.get_value(), "edited_preset"); } + +#[test] +fn locked_bedit_insert_refuses_cycle() { + let mut app = app(); + let name_a = "BEDIT_CYC_A"; + let name_b = "BEDIT_CYC_B"; + + define_block(&mut app, name_a); + + // B contains an insert of A + let ins_a = Insert::new(name_a.to_string(), Vector3::ZERO); + let i = tab(&app); + app.tabs[i] + .scene + .define_block_raw(name_b, Vector3::ZERO, vec![EntityType::Insert(ins_a)]); + + // Now open BEDIT for block A + let insert_a = add_insert(&mut app, name_a); + begin_bedit(&mut app, insert_a); + assert!(bedit_open(&app)); + + // Try committing an INSERT of B into A inside BEDIT space: + // Inserting B into A would create A -> B -> A cycle + let _ = app.dispatch_command("-INSERT"); + let mut feed = |text: &str| { + let _ = app.update(crate::app::Message::CommandInput(text.to_string())); + let _ = app.update(crate::app::Message::CommandSubmit); + }; + feed(name_b); + feed("0,0"); + feed("1"); + feed("1"); + feed("0"); + + // Verify refusal error on command line + let errs = errors(&app); + assert!( + errs.iter() + .any(|e| e.contains("circular dependency detected")), + "expected cycle error in errors: {errs:?}" + ); + + let _ = app.dispatch_command("BEDIT_DISCARD"); +} diff --git a/src/app/update/blocks_palette.rs b/src/app/update/blocks_palette.rs index d128c6707..bbea5ab38 100644 --- a/src/app/update/blocks_palette.rs +++ b/src/app/update/blocks_palette.rs @@ -427,7 +427,11 @@ impl OpenCADStudio { for def in deps.blocks { let scene = &mut self.tabs[i].scene; if redefine && def.name.eq_ignore_ascii_case(name) { - scene.redefine_block_raw(&def.name, def.base_point, def.entities); + if let Err(err) = scene.redefine_block_raw(&def.name, def.base_point, def.entities) + { + self.command_line.push_error(&err); + return Err(err); + } } else if scene.document.block_records.get(&def.name).is_none() { scene.define_block_raw(&def.name, def.base_point, def.entities); } diff --git a/src/app/update/dialog.rs b/src/app/update/dialog.rs index 5d04c72fd..1a33b1db2 100644 --- a/src/app/update/dialog.rs +++ b/src/app/update/dialog.rs @@ -580,11 +580,14 @@ impl OpenCADStudio { .define_block_raw(&def.name, def.base_point, def.entities); } if redefine { - self.tabs[i].scene.redefine_block_raw( + if let Err(err) = self.tabs[i].scene.redefine_block_raw( &name, codec::types::Vector3::new(base.x, base.y, base.z), entities, - ); + ) { + self.command_line.push_error(&err); + return Err(err); + } } else { self.tabs[i] .scene diff --git a/src/scene/block_base_point_tests.rs b/src/scene/block_base_point_tests.rs index 78cfc0b04..d3501771c 100644 --- a/src/scene/block_base_point_tests.rs +++ b/src/scene/block_base_point_tests.rs @@ -538,14 +538,16 @@ fn locked_redefine_with_a_changed_base_moves_the_instance() { ); // The redefine path: the same contents, but a new base point (0,20,0). - scene.redefine_block_raw( - NAME, - v(0.0, 20.0, 0.0), - vec![EntityType::Line(Line::from_points( - v(0.0, 0.0, 0.0), - v(20.0, 0.0, 0.0), - ))], - ); + assert!(scene + .redefine_block_raw( + NAME, + v(0.0, 20.0, 0.0), + vec![EntityType::Line(Line::from_points( + v(0.0, 0.0, 0.0), + v(20.0, 0.0, 0.0), + ))], + ) + .is_ok()); assert_close( block_base_point(&scene.document, NAME), [0.0, 20.0, 0.0], @@ -616,14 +618,16 @@ fn locked_noop_redefine_leaves_the_instance_where_it_is() { assert_close(end, [62.990381, 27.5, 0.0], 1e-6, "line end before"); // No-op redefine: identical contents, same base. - scene.redefine_block_raw( - NAME, - base, - vec![EntityType::Line(Line::from_points( - v(6.0, 4.0, 0.0), - v(16.0, 4.0, 0.0), - ))], - ); + assert!(scene + .redefine_block_raw( + NAME, + base, + vec![EntityType::Line(Line::from_points( + v(6.0, 4.0, 0.0), + v(16.0, 4.0, 0.0), + ))], + ) + .is_ok()); assert_close( block_base_point(&scene.document, NAME), [6.0, 4.0, 0.0], @@ -1153,3 +1157,46 @@ fn locked_nested_insert_with_attributes_exploded_one_level() { "outer attdef insertion point", ); } + +#[test] +fn locked_redefine_block_raw_refuses_cycle() { + let mut scene = Scene::new(); + let blk_a = "REDEF_A"; + let blk_b = "REDEF_B"; + + scene.define_block_raw( + blk_a, + Vector3::ZERO, + vec![EntityType::Line(Line::from_points( + Vector3::ZERO, + Vector3::new(1.0, 0.0, 0.0), + ))], + ); + + // B contains A + scene.define_block_raw( + blk_b, + Vector3::ZERO, + vec![EntityType::Insert(Insert::new( + blk_a.to_string(), + Vector3::ZERO, + ))], + ); + + // Redefining A to contain B would create cycle A -> B -> A; must refuse + let res = scene.redefine_block_raw( + blk_a, + Vector3::ZERO, + vec![EntityType::Insert(Insert::new( + blk_b.to_string(), + Vector3::ZERO, + ))], + ); + + assert!(res.is_err()); + let err = res.unwrap_err(); + assert!( + err.contains("circular dependency detected"), + "expected cycle error, got: {err}" + ); +} diff --git a/src/scene/block_service.rs b/src/scene/block_service.rs index 0f646ead9..f85f71e90 100644 --- a/src/scene/block_service.rs +++ b/src/scene/block_service.rs @@ -598,7 +598,88 @@ fn record_of<'a>( .get(name) .ok_or_else(|| missing(name)) } + +/// Checks whether inserting `inserted_block` into `container_block` would create a circular +/// block nesting dependency, directly or transitively. +/// +/// Matching is case-insensitive, consistent with the rest of the block service. +/// +/// Xrefs (`|` in name or xref flags) and anonymous blocks (`*` prefix) are handled as follows: +/// - Anonymous blocks (`*U...`) are fully traversed like normal blocks because their geometry +/// and nested inserts live in the document. +/// - Externally referenced blocks (xrefs) have definitions that come from external files and +/// are not edited inline in the host document, but if an xref name is tested, it is matched +/// case-insensitively and its local block record entities (if any) are traversed. +pub fn would_create_cycle( + document: &CadDocument, + container_block: &str, + inserted_block: &str, +) -> bool { + let container = container_block.trim(); + let inserted = inserted_block.trim(); + if container.is_empty() || inserted.is_empty() { + return false; + } + if container.eq_ignore_ascii_case(inserted) { + return true; + } + + let mut visited: HashSet = HashSet::default(); + let mut stack: Vec = vec![inserted.to_ascii_uppercase()]; + + while let Some(current_upper) = stack.pop() { + if !visited.insert(current_upper.clone()) { + continue; + } + + // Find block record matching current_upper case-insensitively + let Some(record) = document.block_records.get(¤t_upper) else { + continue; + }; + + // Check entities owned by this block record: either registered in entity_handles + // or having owner_handle == record.handle in the document. + let mut sub_inserts = Vec::new(); + for &handle in &record.entity_handles { + if let Some(EntityType::Insert(sub_ins)) = document.get_entity(handle) { + sub_inserts.push(sub_ins.block_name.trim().to_string()); + } + } + for entity in document.entities() { + if entity.common().owner_handle == record.handle { + if let EntityType::Insert(sub_ins) = entity { + sub_inserts.push(sub_ins.block_name.trim().to_string()); + } + } + } + + for sub_name in sub_inserts { + if sub_name.eq_ignore_ascii_case(container) { + return true; + } + let sub_upper = sub_name.to_ascii_uppercase(); + if !visited.contains(&sub_upper) { + stack.push(sub_upper); + } + } + } + + false +} + +pub const CIRCULAR_BLOCK_DEPENDENCY_ERROR: &str = + "Cannot insert block: circular dependency detected."; + +/// Standard command-line error text when block insertion would create a circular dependency. +pub fn circular_dependency_error(container_block: &str, inserted_block: &str) -> String { + format!( + "Cannot insert block \"{}\" into block \"{}\": circular dependency detected.", + inserted_block, container_block + ) +} + #[cfg(test)] + mod tests { use super::*; use codec::entities::{ @@ -1097,4 +1178,64 @@ mod tests { "block \"A\" is open in the block editor; close the editor first" ); } + + #[test] + fn locked_would_create_cycle_self_insert() { + let mut scene = scene(); + define(&mut scene, "BLK_A"); + assert!(would_create_cycle(&scene.document, "BLK_A", "BLK_A")); + assert!(would_create_cycle(&scene.document, "blk_a", "BLK_A")); + assert!(would_create_cycle(&scene.document, "BLK_A", "blk_a")); + } + + #[test] + fn locked_would_create_cycle_direct_pair() { + let mut scene = scene(); + let blk_a = define(&mut scene, "BLK_A"); + define(&mut scene, "BLK_B"); + // A contains B + add_insert(&mut scene, "BLK_B", Some(blk_a)); + // Inserting A into B would create a cycle + assert!(would_create_cycle(&scene.document, "BLK_B", "BLK_A")); + // Inserting B into A is not allowed either since A already inserts B + assert!(!would_create_cycle(&scene.document, "BLK_A", "BLK_B")); + } + + #[test] + fn locked_would_create_cycle_three_level_chain() { + let mut scene = scene(); + let blk_a = define(&mut scene, "BLK_A"); + let blk_b = define(&mut scene, "BLK_B"); + define(&mut scene, "BLK_C"); + // A contains B, B contains C + add_insert(&mut scene, "BLK_B", Some(blk_a)); + add_insert(&mut scene, "BLK_C", Some(blk_b)); + // Inserting A into C creates cycle C -> A -> B -> C + assert!(would_create_cycle(&scene.document, "BLK_C", "BLK_A")); + // Inserting B into C creates cycle C -> B -> C + assert!(would_create_cycle(&scene.document, "BLK_C", "BLK_B")); + } + + #[test] + fn locked_would_create_cycle_shared_child_not_a_cycle() { + let mut scene = scene(); + let blk_a = define(&mut scene, "BLK_A"); + let blk_b = define(&mut scene, "BLK_B"); + define(&mut scene, "BLK_SHARED"); + // A contains SHARED, B contains SHARED. Inserting B into A is fine (diamond/DAG, not cycle). + add_insert(&mut scene, "BLK_SHARED", Some(blk_a)); + add_insert(&mut scene, "BLK_SHARED", Some(blk_b)); + assert!(!would_create_cycle(&scene.document, "BLK_A", "BLK_B")); + assert!(!would_create_cycle(&scene.document, "BLK_B", "BLK_A")); + } + + #[test] + fn locked_would_create_cycle_case_differences() { + let mut scene = scene(); + let blk_a = define(&mut scene, "BlockA"); + let _blk_b = define(&mut scene, "BlockB"); + add_insert(&mut scene, "BlockB", Some(blk_a)); + assert!(would_create_cycle(&scene.document, "blockb", "BLOCKA")); + assert!(!would_create_cycle(&scene.document, "blocka", "BLOCKB")); + } } diff --git a/src/scene/cache/block_cache.rs b/src/scene/cache/block_cache.rs index c82fe304c..6d8e1c925 100644 --- a/src/scene/cache/block_cache.rs +++ b/src/scene/cache/block_cache.rs @@ -178,6 +178,7 @@ pub struct BlockCache { HashMap>>>>, >, clip_prototypes: std::sync::Mutex), u64>>, + cycle_warnings: std::sync::Mutex>, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -219,6 +220,22 @@ impl BlockCache { Self::default() } + pub fn record_cycle_warning(&self, block_name: &str) -> bool { + let mut warnings = self + .cycle_warnings + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + warnings.insert(block_name.to_ascii_uppercase()) + } + + pub fn drain_cycle_warnings(&self) -> Vec { + let mut warnings = self + .cycle_warnings + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + warnings.drain().collect() + } + pub fn defn(&self, block_name: &str) -> Option<&Arc> { self.defns.get(block_name).or_else(|| { self.defns @@ -1105,7 +1122,7 @@ pub fn expand_insert( ); } let mut batches = Batches::default(); - let mut visited: Vec = Vec::with_capacity(8); + let mut visited: Vec = vec![ins.block_name.clone()]; let offsets = crate::scene::render_graph::array_offsets(ins); let cell_xform = |offset: &[f64; 3]| { @@ -1952,7 +1969,16 @@ fn expand_defn( emit_wire(attachment, &attachment_transform, &inner_ctx, out, d_range); } } - if visited.iter().any(|n| n == &nref.block_name) { + if visited + .iter() + .any(|n| n.eq_ignore_ascii_case(&nref.block_name)) + { + if ctx.cache.record_cycle_warning(&nref.block_name) { + eprintln!( + "Warning: circular block reference detected for block \"{}\"; cutting cycle", + nref.block_name + ); + } continue; } let Some(nested_defn) = ctx.cache.defn(&nref.block_name) else { @@ -3484,4 +3510,97 @@ mod metrics_depth_tests { assert_ne!(root.metrics.aabb_local, [0.0; 4]); assert_eq!(root.metrics.aabb_local, tail.metrics.aabb_local); } + + #[test] + fn locked_cyclic_block_document_renders_without_hanging_and_warns_once() { + // Document built in code with pre-existing cycle: + // A contains B, B contains A + let mut document = CadDocument::new(); + let blk_a = add_block(&mut document, "CYC_A"); + let blk_b = add_block(&mut document, "CYC_B"); + + add_owned( + &mut document, + blk_a, + EntityType::Insert(Insert::new("CYC_B", Vector3::ZERO)), + ); + add_owned( + &mut document, + blk_b, + EntityType::Insert(Insert::new("CYC_A", Vector3::ZERO)), + ); + // Add a line in A so there is some actual geometry + add_owned( + &mut document, + blk_a, + EntityType::Line(Line::from_points( + Vector3::ZERO, + Vector3::new(10.0, 0.0, 0.0), + )), + ); + + // Model space references CYC_A + document + .add_entity(EntityType::Insert(Insert::new("CYC_A", Vector3::ZERO))) + .unwrap(); + + // Building the cache computes defns and metrics without hanging + let cache = BlockCache::build( + &document, + 1.0, + None, + true, + [0.0, 0.0, 0.0, 1.0], + None, + &HashMap::default(), + ); + + // Verify defns exist + assert!(cache.defns.contains_key("CYC_A")); + assert!(cache.defns.contains_key("CYC_B")); + + // Expanding / tessellating into wires does not hang and cuts the cycle + let ins = Insert::new("CYC_A", Vector3::ZERO); + let inherit_style = crate::scene::view::render::InheritStyle { + color: [1.0, 1.0, 1.0, 1.0], + pat_len: 1.0, + pat: [0.0; 8], + lw_px: 1.0, + }; + + let result = expand_insert( + &document, + &cache, + &ins, + Handle::new(1), + [1.0, 1.0, 1.0, 1.0], + 7, + 1.0, + [0.0; 8], + 1.0, + inherit_style, + 7, + true, + false, + 1.0, + None, + None, + false, + [0.0, 0.0, 0.0, 1.0], + 1.0, + crate::scene::BlockScalePolicy::FromInsert, + false, + ); + + assert!(result.is_some()); + + // Warning was recorded once + let warnings = cache.drain_cycle_warnings(); + assert_eq!(warnings.len(), 1); + assert_eq!(warnings[0], "CYC_A"); + + // Subsequent expansion does not warn again because warned set retains it + let warnings_after = cache.drain_cycle_warnings(); + assert!(warnings_after.is_empty()); + } } diff --git a/src/scene/entity.rs b/src/scene/entity.rs index df09813c2..6713fa092 100644 --- a/src/scene/entity.rs +++ b/src/scene/entity.rs @@ -1263,19 +1263,35 @@ impl Scene { /// Replace the contents and base point of block `name`, defining it when /// absent. References keep pointing at it and show the new contents. + /// Refuses and returns Err if the new contents lead back to `name` directly or transitively. pub fn redefine_block_raw( &mut self, name: &str, base_point: codec::types::Vector3, entities: Vec, - ) { + ) -> Result<(), String> { + for entity in &entities { + if let EntityType::Insert(ins) = entity { + if crate::scene::block_service::would_create_cycle( + &self.document, + name, + &ins.block_name, + ) { + return Err(crate::scene::block_service::circular_dependency_error( + name, + &ins.block_name, + )); + } + } + } let Some((br_handle, block_handle)) = self .document .block_records .get(name) .map(|br| (br.handle, br.block_entity_handle)) else { - return self.define_block_raw(name, base_point, entities); + self.define_block_raw(name, base_point, entities); + return Ok(()); }; let owned: Vec = self .document @@ -1299,6 +1315,7 @@ impl Scene { let _ = self.document.add_entity(entity); } self.bump_geometry(); + Ok(()) } pub(super) fn synced_hatch_models( @@ -2228,7 +2245,14 @@ impl Scene { .colors .get(i) .and_then(|e| e.color.rgb()) - .map(|(r, g, b)| [r as f32 / 255.0, g as f32 / 255.0, b as f32 / 255.0, color[3]]) + .map(|(r, g, b)| { + [ + r as f32 / 255.0, + g as f32 / 255.0, + b as f32 / 255.0, + color[3], + ] + }) }; gradient_color1 = stop(0); let color1 = gradient_color1.unwrap_or(color);