From f0de367499a3d431b24d7b403f84da4fc690af14 Mon Sep 17 00:00:00 2001 From: teddy Date: Mon, 5 Jan 2026 20:04:40 -0300 Subject: [PATCH 1/5] test: replicate the bug --- crates/bulloak/tests/check.rs | 21 +++++++ .../tests/check/hoisted_hook_regression.tree | 9 +++ .../check/hoisted_hook_regression_test.nr | 22 +++++++ crates/bulloak/tests/scaffold.rs | 24 +++++++ .../scaffold/hoisted_hook_regression.tree | 9 +++ .../scaffold/hoisted_hook_regression_test.nr | 26 ++++++++ crates/noir/src/test_structure.rs | 62 +++++++++++++++++++ 7 files changed, 173 insertions(+) create mode 100644 crates/bulloak/tests/check/hoisted_hook_regression.tree create mode 100644 crates/bulloak/tests/check/hoisted_hook_regression_test.nr create mode 100644 crates/bulloak/tests/scaffold/hoisted_hook_regression.tree create mode 100644 crates/bulloak/tests/scaffold/hoisted_hook_regression_test.nr diff --git a/crates/bulloak/tests/check.rs b/crates/bulloak/tests/check.rs index 42b39e0..fed7a38 100644 --- a/crates/bulloak/tests/check.rs +++ b/crates/bulloak/tests/check.rs @@ -736,3 +736,24 @@ fn checks_no_submodule_multi_root_error() { assert!(stderr.contains(r#"an error occurred while parsing the tree: separator missing at tree root #1 "no_submodule_multi_root". Expected to find `::` between the contract name and the function name when multiple roots exist"#)); } + +/// Regression test for https://github.com/defi-wonderland/bulloak/pull/9#issuecomment-3710452952 +/// When multiple roots share the same condition, check should report the missing hoisted setup hook. +#[test] +fn checks_missing_hoisted_setup_hook_for_shared_condition() { + let cwd = env::current_dir().unwrap(); + let binary_path = get_binary_path(); + let tree_path = cwd + .join("tests") + .join("check") + .join("hoisted_hook_regression.tree"); + + let output = cmd(&binary_path, "check", &tree_path, &["-l", "noir"]); + let stderr = String::from_utf8(output.stderr).unwrap(); + + assert!( + stderr.contains(r#"Missing setup hook "when_passing_valid_parameters""#), + "Expected check to report missing setup hook 'when_passing_valid_parameters', but it did not.\nStderr:\n{}", + stderr + ); +} diff --git a/crates/bulloak/tests/check/hoisted_hook_regression.tree b/crates/bulloak/tests/check/hoisted_hook_regression.tree new file mode 100644 index 0000000..b007cbf --- /dev/null +++ b/crates/bulloak/tests/check/hoisted_hook_regression.tree @@ -0,0 +1,9 @@ +hoisted_hook_regression::constructor_with_minter +└── when passing valid parameters + ├── it sets name + └── it sets symbol + +hoisted_hook_regression::constructor_with_initial_supply +└── when passing valid parameters + ├── it sets name + └── it sets symbol diff --git a/crates/bulloak/tests/check/hoisted_hook_regression_test.nr b/crates/bulloak/tests/check/hoisted_hook_regression_test.nr new file mode 100644 index 0000000..9a86324 --- /dev/null +++ b/crates/bulloak/tests/check/hoisted_hook_regression_test.nr @@ -0,0 +1,22 @@ +// Generated by bulloak + + +mod constructor_with_minter { + #[test] + unconstrained fn test_when_passing_valid_parameters() { + when_passing_valid_parameters(); + // it sets name + // it sets symbol + } + +} + +mod constructor_with_initial_supply { + #[test] + unconstrained fn test_when_passing_valid_parameters() { + when_passing_valid_parameters(); + // it sets name + // it sets symbol + } + +} diff --git a/crates/bulloak/tests/scaffold.rs b/crates/bulloak/tests/scaffold.rs index b5d6179..3f98693 100644 --- a/crates/bulloak/tests/scaffold.rs +++ b/crates/bulloak/tests/scaffold.rs @@ -350,3 +350,27 @@ fn scaffold_dissambiguates_function_name_collisions() { assert_eq!(expected.trim(), actual.trim()); } } + +/// Regression test for https://github.com/defi-wonderland/bulloak/pull/9#issuecomment-3710452952 +/// When multiple roots share the same condition, the hoisted setup hook should be generated. +#[test] +fn scaffold_noir_generates_hoisted_setup_hook_for_shared_condition() { + let cwd = env::current_dir().unwrap(); + let binary_path = get_binary_path(); + let tests_path = cwd.join("tests").join("scaffold"); + let tree_path = tests_path.join("hoisted_hook_regression.tree"); + + let output = cmd(&binary_path, "scaffold", &tree_path, &["-l", "noir"]); + let actual = String::from_utf8(output.stdout).unwrap(); + + assert!( + actual.contains("unconstrained fn when_passing_valid_parameters()"), + "Expected hoisted setup hook 'when_passing_valid_parameters' to be generated, but it was not.\nActual output:\n{}", + actual + ); + + // Also verify the full expected output matches + let noir_filename = tests_path.join("hoisted_hook_regression_test.nr"); + let expected = fs::read_to_string(noir_filename).unwrap(); + assert_eq!(expected.trim(), actual.trim()); +} diff --git a/crates/bulloak/tests/scaffold/hoisted_hook_regression.tree b/crates/bulloak/tests/scaffold/hoisted_hook_regression.tree new file mode 100644 index 0000000..b007cbf --- /dev/null +++ b/crates/bulloak/tests/scaffold/hoisted_hook_regression.tree @@ -0,0 +1,9 @@ +hoisted_hook_regression::constructor_with_minter +└── when passing valid parameters + ├── it sets name + └── it sets symbol + +hoisted_hook_regression::constructor_with_initial_supply +└── when passing valid parameters + ├── it sets name + └── it sets symbol diff --git a/crates/bulloak/tests/scaffold/hoisted_hook_regression_test.nr b/crates/bulloak/tests/scaffold/hoisted_hook_regression_test.nr new file mode 100644 index 0000000..a17c1e9 --- /dev/null +++ b/crates/bulloak/tests/scaffold/hoisted_hook_regression_test.nr @@ -0,0 +1,26 @@ +// Generated by bulloak + +/// Setup hook for condition +unconstrained fn when_passing_valid_parameters() { +} + + +mod constructor_with_minter { + #[test] + unconstrained fn test_when_passing_valid_parameters() { + when_passing_valid_parameters(); + // it sets name + // it sets symbol + } + +} + +mod constructor_with_initial_supply { + #[test] + unconstrained fn test_when_passing_valid_parameters() { + when_passing_valid_parameters(); + // it sets name + // it sets symbol + } + +} diff --git a/crates/noir/src/test_structure.rs b/crates/noir/src/test_structure.rs index 9c5ab5c..200b1e9 100644 --- a/crates/noir/src/test_structure.rs +++ b/crates/noir/src/test_structure.rs @@ -456,4 +456,66 @@ Contract::bar assert_eq!(bar_module.functions[2].name(), "test_when_c"); assert!(matches!(bar_module.functions[2], Function::TestFunction(_))); } + + /// Regression test for https://github.com/defi-wonderland/bulloak/pull/9#issuecomment-3710452952 + /// When multiple roots share the same leaf condition (a condition with only action children), + /// the shared setup hook should be hoisted to root.functions. + #[test] + fn test_hoist_shared_leaf_condition_setup() { + let tree = r" +hoisted_hook_regression::constructor_with_minter +└── when passing valid parameters + ├── it sets name + └── it sets symbol + +hoisted_hook_regression::constructor_with_initial_supply +└── when passing valid parameters + ├── it sets name + └── it sets symbol +"; + let forest = parse(tree).unwrap(); + let root = Root::new(&forest).unwrap(); + + // The shared condition should be hoisted to root.functions as a SetupHook + assert_eq!( + root.functions.len(), + 1, + "Expected 1 hoisted setup hook, found {}. Root functions: {:?}", + root.functions.len(), + root.functions + ); + assert_eq!(root.functions[0].name(), "when_passing_valid_parameters"); + assert!( + matches!(root.functions[0], Function::SetupHook(_)), + "Expected SetupHook, got {:?}", + root.functions[0] + ); + + // Verify modules are created correctly + assert_eq!(root.modules.len(), 2); + + let minter_module = &root.modules[0]; + assert_eq!(minter_module.name, "constructor_with_minter"); + assert_eq!(minter_module.functions.len(), 1); + assert_eq!( + minter_module.functions[0].name(), + "test_when_passing_valid_parameters" + ); + assert!(matches!( + minter_module.functions[0], + Function::TestFunction(_) + )); + + let supply_module = &root.modules[1]; + assert_eq!(supply_module.name, "constructor_with_initial_supply"); + assert_eq!(supply_module.functions.len(), 1); + assert_eq!( + supply_module.functions[0].name(), + "test_when_passing_valid_parameters" + ); + assert!(matches!( + supply_module.functions[0], + Function::TestFunction(_) + )); + } } From 90b5393c4058201f9da6cc1986da2f230d6457a9 Mon Sep 17 00:00:00 2001 From: teddy Date: Tue, 6 Jan 2026 16:12:38 -0300 Subject: [PATCH 2/5] test: ensure setup hooks are generated even if their setup would not be shared --- crates/noir/src/test_structure.rs | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/crates/noir/src/test_structure.rs b/crates/noir/src/test_structure.rs index 200b1e9..21ff6c9 100644 --- a/crates/noir/src/test_structure.rs +++ b/crates/noir/src/test_structure.rs @@ -518,4 +518,42 @@ hoisted_hook_regression::constructor_with_initial_supply Function::TestFunction(_) )); } + + /// Related to https://github.com/defi-wonderland/bulloak/pull/9#issuecomment-3710452952 + /// check the modifier would be generated in the case of a single-root testfile + #[test] + fn test_single_root_setup_hook_generation() { + let tree = r" +hoisted_hook_regression::constructor_with_minter +└── when passing valid parameters + ├── it sets name + └── it sets symbol +"; + let forest = parse(tree).unwrap(); + let root = Root::new(&forest).unwrap(); + dbg!(&root); + + assert_eq!( + root.functions.len(), + 2, + "Expected 1 setup hook and 1 test function, found {}. Root functions: {:?}", + root.functions.len(), + root.functions + ); + assert_eq!(root.functions[0].name(), "when_passing_valid_parameters"); + assert!( + matches!(root.functions[0], Function::SetupHook(_)), + "Expected SetupHook, got {:?}", + root.functions[0] + ); + + // Verify no modules are defined + assert_eq!(root.modules.len(), 0); + + assert_eq!( + root.functions[1].name(), + "test_when_passing_valid_parameters" + ); + assert!(matches!(root.functions[1], Function::TestFunction(_))); + } } From 7d369eb9e8a29a765900390018066b5d9d0a6580 Mon Sep 17 00:00:00 2001 From: teddy Date: Tue, 6 Jan 2026 16:16:47 -0300 Subject: [PATCH 3/5] feat: extract helpers only once this mitigates the case where inconsistencies between the logic in `collect_helpers` and `collect_tests` caused setup hooks to be invoked but not defined, as was the trigger for this PR. This also mean we will generate more helpers, as we'll now generate them when there's no real need for reuse of their logic, as exemplified in the previous commit --- crates/noir/src/test_structure.rs | 108 +++++++++++++++--------------- 1 file changed, 53 insertions(+), 55 deletions(-) diff --git a/crates/noir/src/test_structure.rs b/crates/noir/src/test_structure.rs index 21ff6c9..14af84f 100644 --- a/crates/noir/src/test_structure.rs +++ b/crates/noir/src/test_structure.rs @@ -23,15 +23,19 @@ impl Root { match forest.iter().len() { 0 => Ok(Root { functions, modules }), 1 => { + let test_functions = collect_tests(forest, &[]); + functions.extend( - collect_helpers(forest) + collect_helpers(&test_functions) .into_iter() - .map(|x| Function::SetupHook(x)), + .map(|x| Function::SetupHook(x)) + .collect::>(), ); functions.extend( - collect_tests(forest, &[]) + test_functions .into_iter() - .map(|x| Function::TestFunction(x)), + .map(|x| Function::TestFunction(x)) + .collect::>(), ); Ok(Root { functions, modules }) } @@ -57,38 +61,69 @@ impl Root { ); } - let tree_slice = std::slice::from_ref(ast); - let helpers = collect_helpers(tree_slice); + let local_tests = + collect_tests(std::slice::from_ref(ast), &[]); - for helper in &helpers { + let helpers = collect_helpers(&local_tests); + for hook in &helpers { // returns false if the key is already present - if !all_hooks.insert(helper.name.clone()) { + if !all_hooks.insert(hook.name.clone()) { // we don't care if it's repeated one or multiple times - repeated_hooks.insert(helper.name.clone()); + repeated_hooks.insert(hook.name.clone()); } } let mut local_functions = Vec::new(); - local_functions.extend( - helpers.into_iter().map(|x| Function::SetupHook(x)), + local_functions.extend(helpers + .into_iter() + .map(|x| Function::SetupHook(x)) + .collect::>(), ); local_functions.extend( - collect_tests(tree_slice, &[]) + local_tests .into_iter() - .map(|x| Function::TestFunction(x)), + .map(|x| Function::TestFunction(x)) + .collect::>(), ); modules.push(Module { name, functions: local_functions }); } - modules = modules.into_iter().map(|module| Module{ - name: module.name.clone(), - functions: module.functions.into_iter().filter(|fun| !repeated_hooks.contains(&fun.name())).collect() - }).collect(); + modules = modules + .into_iter() + .map(|module| Module { + name: module.name.clone(), + functions: module + .functions + .into_iter() + .filter(|fun| !repeated_hooks.contains(&fun.name())) + .collect(), + }) + .collect(); + + Ok(Root { + modules, + functions: repeated_hooks + .into_iter() + .map(|name| Function::SetupHook(SetupHook { name })) + .collect(), + }) + } + } + } +} - Ok(Root { modules, functions: repeated_hooks.into_iter().map(|name| Function::SetupHook(SetupHook{name})).collect() }) +fn collect_helpers(test_functions: &Vec) -> Vec { + let mut hooks = Vec::new(); + let mut all_hooks: HashSet = HashSet::new(); + for func in test_functions { + for hook in &func.setup_hooks { + // skip repeated hooks + if all_hooks.insert(hook.name.clone()) { + hooks.push(hook.clone()); } } } + hooks } /// Used for both definition and invocation @@ -126,43 +161,6 @@ impl Function { } } -/// Collect all unique helper names from conditions. -fn collect_helpers(children: &[Ast]) -> Vec { - let mut helpers = HashSet::new(); - collect_helpers_recursive(children, &mut helpers); - let mut sorted: Vec = helpers.into_iter().collect(); - sorted.sort(); - sorted -} - -/// Recursively collect helper names. -fn collect_helpers_recursive( - children: &[Ast], - helpers: &mut HashSet, -) { - for child in children { - match child { - Ast::Condition(condition) => { - // only produce helpers for a branch if any of its children is also a branch, meaning - // there's a potential need to reuse them - if condition.children.iter().any(|c| match c { - Ast::Condition(_) => true, - _ => false, - }) { - helpers.insert(SetupHook { - name: to_snake_case(&condition.title), - }); - } - collect_helpers_recursive(&condition.children, helpers); - } - Ast::Root(root) => { - collect_helpers_recursive(&root.children, helpers); - } - _ => {} - } - } -} - fn collect_tests( children: &[Ast], parent_helpers: &[SetupHook], From 6c06e69e9bf224e2e3f98626d1d2406d8bb55825 Mon Sep 17 00:00:00 2001 From: teddy Date: Tue, 6 Jan 2026 18:46:50 -0300 Subject: [PATCH 4/5] feat: track which modifiers were hoisted so we can auto-import them --- .../noir/src/check/rules/structural_match.rs | 21 +- crates/noir/src/noir/parser.rs | 1 + crates/noir/src/test_structure.rs | 338 +++++++++++++++++- 3 files changed, 343 insertions(+), 17 deletions(-) diff --git a/crates/noir/src/check/rules/structural_match.rs b/crates/noir/src/check/rules/structural_match.rs index 940fd87..60163b4 100644 --- a/crates/noir/src/check/rules/structural_match.rs +++ b/crates/noir/src/check/rules/structural_match.rs @@ -906,6 +906,7 @@ mod compare_trees_test { functions: vec![], modules: vec![Module { name: "module1".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::SetupHook(SetupHook { name: "when_something".to_string(), })], @@ -938,6 +939,7 @@ mod compare_trees_test { functions: vec![], modules: vec![Module { name: "wrong_name".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::TestFunction(TestFunction { name: "test_something".to_string(), expect_fail: false, @@ -950,6 +952,7 @@ mod compare_trees_test { functions: vec![], modules: vec![Module { name: "correct_name".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::TestFunction(TestFunction { name: "test_something".to_string(), expect_fail: false, @@ -974,6 +977,7 @@ mod compare_trees_test { modules: vec![ Module { name: "module_b".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::TestFunction(TestFunction { name: "test_b".to_string(), expect_fail: false, @@ -983,6 +987,7 @@ mod compare_trees_test { }, Module { name: "module_a".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::TestFunction(TestFunction { name: "test_a".to_string(), expect_fail: false, @@ -997,6 +1002,7 @@ mod compare_trees_test { modules: vec![ Module { name: "module_a".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::TestFunction(TestFunction { name: "test_a".to_string(), expect_fail: false, @@ -1006,6 +1012,7 @@ mod compare_trees_test { }, Module { name: "module_b".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::TestFunction(TestFunction { name: "test_b".to_string(), expect_fail: false, @@ -1040,9 +1047,14 @@ mod compare_trees_test { let actual = Root { functions: vec![], modules: vec![ - Module { name: "module_a".to_string(), functions: vec![] }, + Module { + name: "module_a".to_string(), + imported_hooks: Vec::new(), + functions: vec![], + }, Module { name: "module_b".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::TestFunction(TestFunction { name: "test_foo".to_string(), expect_fail: false, @@ -1057,6 +1069,7 @@ mod compare_trees_test { modules: vec![ Module { name: "module_a".to_string(), + imported_hooks: Vec::new(), functions: vec![Function::TestFunction(TestFunction { name: "test_foo".to_string(), expect_fail: false, @@ -1064,7 +1077,11 @@ mod compare_trees_test { actions: vec![], })], }, - Module { name: "module_b".to_string(), functions: vec![] }, + Module { + name: "module_b".to_string(), + imported_hooks: Vec::new(), + functions: vec![], + }, ], }; let violations = diff --git a/crates/noir/src/noir/parser.rs b/crates/noir/src/noir/parser.rs index 7e190eb..98de562 100644 --- a/crates/noir/src/noir/parser.rs +++ b/crates/noir/src/noir/parser.rs @@ -62,6 +62,7 @@ impl ParsedNoirFile { self.find_functions_recursive(node, &mut functions); modules.push(Module { name: self.extract_module_name(node), + imported_hooks: Vec::new(), functions, }); } diff --git a/crates/noir/src/test_structure.rs b/crates/noir/src/test_structure.rs index 14af84f..69d7195 100644 --- a/crates/noir/src/test_structure.rs +++ b/crates/noir/src/test_structure.rs @@ -74,7 +74,8 @@ impl Root { } let mut local_functions = Vec::new(); - local_functions.extend(helpers + local_functions.extend( + helpers .into_iter() .map(|x| Function::SetupHook(x)) .collect::>(), @@ -85,23 +86,15 @@ impl Root { .map(|x| Function::TestFunction(x)) .collect::>(), ); - modules.push(Module { name, functions: local_functions }); + modules.push(Module { + name, + functions: local_functions, + imported_hooks: Vec::new(), + }); } - modules = modules - .into_iter() - .map(|module| Module { - name: module.name.clone(), - functions: module - .functions - .into_iter() - .filter(|fun| !repeated_hooks.contains(&fun.name())) - .collect(), - }) - .collect(); - Ok(Root { - modules, + modules: hoist_setup_hooks(modules, &repeated_hooks), functions: repeated_hooks .into_iter() .map(|name| Function::SetupHook(SetupHook { name })) @@ -112,6 +105,48 @@ impl Root { } } +fn hoist_setup_hooks( + modules: Vec, + repeated_hooks: &HashSet, +) -> Vec { + modules + .into_iter() + .map(|module| Module { + name: module.name.clone(), + imported_hooks: module + .functions + .iter() + .filter_map(|fun| { + if let Function::SetupHook(f) = fun { + if repeated_hooks.contains(&f.name) { + Some(f.clone()) + } else { + None + } + } else { + None + } + }) + .collect(), + functions: module + .functions + .into_iter() + .filter(|fun| { + if let Function::SetupHook(f) = fun { + if repeated_hooks.contains(&f.name) { + return false; + } else { + return true; + } + } else { + return true; + } + }) + .collect(), + }) + .collect() +} + fn collect_helpers(test_functions: &Vec) -> Vec { let mut hooks = Vec::new(); let mut all_hooks: HashSet = HashSet::new(); @@ -135,6 +170,7 @@ pub(crate) struct SetupHook { #[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Debug)] pub(crate) struct Module { pub name: String, + pub imported_hooks: Vec, pub functions: Vec, } @@ -555,3 +591,275 @@ hoisted_hook_regression::constructor_with_minter assert!(matches!(root.functions[1], Function::TestFunction(_))); } } + +#[cfg(test)] +mod tests_hoist_setup_hooks { + use super::*; + + #[test] + fn test_hoist_setup_hooks_empty_modules() { + let modules: Vec = vec![]; + let repeated_hooks: HashSet = HashSet::new(); + + let result = hoist_setup_hooks(modules, &repeated_hooks); + + assert!(result.is_empty()); + } + + #[test] + fn test_hoist_setup_hooks_empty_repeated_hooks() { + let modules = vec![Module { + name: "module_a".to_string(), + imported_hooks: Vec::new(), + functions: vec![ + Function::SetupHook(SetupHook { name: "when_a".to_string() }), + Function::TestFunction(TestFunction { + name: "test_when_a".to_string(), + expect_fail: false, + setup_hooks: vec![], + actions: vec![], + }), + ], + }]; + let repeated_hooks: HashSet = HashSet::new(); + + let result = hoist_setup_hooks(modules, &repeated_hooks); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name, "module_a"); + assert!(result[0].imported_hooks.is_empty()); + assert_eq!(result[0].functions.len(), 2); + assert_eq!(result[0].functions[0].name(), "when_a"); + assert_eq!(result[0].functions[1].name(), "test_when_a"); + } + + #[test] + fn test_hoist_setup_hooks_single_module_no_repeated_hooks() { + let modules = vec![Module { + name: "my_module".to_string(), + imported_hooks: Vec::new(), + functions: vec![ + Function::SetupHook(SetupHook { + name: "when_something".to_string(), + }), + Function::TestFunction(TestFunction { + name: "test_when_something".to_string(), + expect_fail: false, + setup_hooks: vec![SetupHook { + name: "when_something".to_string(), + }], + actions: vec!["it does something".to_string()], + }), + ], + }]; + let repeated_hooks: HashSet = HashSet::new(); + + let result = hoist_setup_hooks(modules, &repeated_hooks); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name, "my_module"); + assert!(result[0].imported_hooks.is_empty()); + assert_eq!(result[0].functions.len(), 2); + assert_eq!(result[0].functions[0].name(), "when_something"); + assert_eq!(result[0].functions[1].name(), "test_when_something"); + } + + // this borders on testing a bug, since there should be no repeated hooks + // if there is only one module + #[test] + fn test_hoist_setup_hooks_single_module_with_one_repeated_hook() { + let modules = vec![Module { + name: "my_module".to_string(), + imported_hooks: Vec::new(), + functions: vec![ + Function::SetupHook(SetupHook { + name: "when_shared".to_string(), + }), + Function::SetupHook(SetupHook { + name: "when_local".to_string(), + }), + Function::TestFunction(TestFunction { + name: "test_when_shared".to_string(), + expect_fail: false, + setup_hooks: vec![], + actions: vec![], + }), + ], + }]; + let mut repeated_hooks: HashSet = HashSet::new(); + repeated_hooks.insert("when_shared".to_string()); + + let result = hoist_setup_hooks(modules, &repeated_hooks); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name, "my_module"); + + // The repeated hook should be in imported_hooks + assert_eq!(result[0].imported_hooks.len(), 1); + assert_eq!(result[0].imported_hooks[0].name, "when_shared"); + + // The repeated hook should be removed from functions, but local hook and test remain + assert_eq!(result[0].functions.len(), 2); + assert_eq!(result[0].functions[0].name(), "when_local"); + assert_eq!(result[0].functions[1].name(), "test_when_shared"); + } + + #[test] + fn test_hoist_setup_hooks_multiple_modules_with_shared_hooks() { + let modules = vec![ + Module { + name: "foo".to_string(), + imported_hooks: Vec::new(), + functions: vec![ + Function::SetupHook(SetupHook { + name: "when_shared".to_string(), + }), + Function::TestFunction(TestFunction { + name: "test_foo".to_string(), + expect_fail: false, + setup_hooks: vec![], + actions: vec![], + }), + ], + }, + Module { + name: "bar".to_string(), + imported_hooks: Vec::new(), + functions: vec![ + Function::SetupHook(SetupHook { + name: "when_shared".to_string(), + }), + Function::SetupHook(SetupHook { + name: "when_bar_only".to_string(), + }), + Function::TestFunction(TestFunction { + name: "test_bar".to_string(), + expect_fail: false, + setup_hooks: vec![], + actions: vec![], + }), + ], + }, + ]; + let mut repeated_hooks: HashSet = HashSet::new(); + repeated_hooks.insert("when_shared".to_string()); + + let result = hoist_setup_hooks(modules, &repeated_hooks); + + assert_eq!(result.len(), 2); + + // First module (foo) + assert_eq!(result[0].name, "foo"); + assert_eq!(result[0].imported_hooks.len(), 1); + assert_eq!(result[0].imported_hooks[0].name, "when_shared"); + assert_eq!(result[0].functions.len(), 1); + assert_eq!(result[0].functions[0].name(), "test_foo"); + + // Second module (bar) + assert_eq!(result[1].name, "bar"); + assert_eq!(result[1].imported_hooks.len(), 1); + assert_eq!(result[1].imported_hooks[0].name, "when_shared"); + assert_eq!(result[1].functions.len(), 2); + assert_eq!(result[1].functions[0].name(), "when_bar_only"); + assert_eq!(result[1].functions[1].name(), "test_bar"); + } + + #[test] + fn test_hoist_setup_hooks_module_with_only_test_functions() { + let modules = vec![Module { + name: "tests_only".to_string(), + imported_hooks: Vec::new(), + functions: vec![ + Function::TestFunction(TestFunction { + name: "test_one".to_string(), + expect_fail: false, + setup_hooks: vec![], + actions: vec!["action one".to_string()], + }), + Function::TestFunction(TestFunction { + name: "test_two".to_string(), + expect_fail: true, + setup_hooks: vec![], + actions: vec!["action two".to_string()], + }), + ], + }]; + let mut repeated_hooks: HashSet = HashSet::new(); + repeated_hooks.insert("when_something".to_string()); // Not present in module + + let result = hoist_setup_hooks(modules, &repeated_hooks); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name, "tests_only"); + assert!(result[0].imported_hooks.is_empty()); + assert_eq!(result[0].functions.len(), 2); + assert_eq!(result[0].functions[0].name(), "test_one"); + assert_eq!(result[0].functions[1].name(), "test_two"); + } + + #[test] + fn test_hoist_setup_hooks_test_function_with_repeated_hook_name_not_removed( + ) { + // Edge case: a TestFunction has the same name as a repeated hook + // The function should NOT be removed because only SetupHooks are hoisted + let modules = vec![ + Module { + name: "edge".to_string(), + imported_hooks: Vec::new(), + functions: vec![ + Function::SetupHook(SetupHook { + name: "when_shared".to_string(), + }), + Function::TestFunction(TestFunction { + name: "when_shared".to_string(), // Same name as hook + expect_fail: false, + setup_hooks: vec![], + actions: vec![], + }), + ], + }, + Module { + name: "case".to_string(), + imported_hooks: Vec::new(), + functions: vec![ + Function::SetupHook(SetupHook { + name: "when_shared".to_string(), + }), + Function::TestFunction(TestFunction { + name: "when_shared".to_string(), // Same name as hook + expect_fail: false, + setup_hooks: vec![], + actions: vec![], + }), + ], + }, + ]; + let mut repeated_hooks: HashSet = HashSet::new(); + repeated_hooks.insert("when_shared".to_string()); + + let result = hoist_setup_hooks(modules, &repeated_hooks); + + assert_eq!(result.len(), 2); + assert_eq!(result[0].name, "edge"); + + // The SetupHook should be in imported_hooks + assert_eq!(result[0].imported_hooks.len(), 1); + assert_eq!(result[0].imported_hooks[0].name, "when_shared"); + + // the setup hook was removed but the test function remains in the module + assert_eq!(result[0].functions.len(), 1); + assert!(matches!(result[0].functions[0], Function::TestFunction(_))); + assert_eq!(result[0].functions[0].name(), "when_shared"); + + assert_eq!(result[1].name, "case"); + + // The SetupHook should be in imported_hooks + assert_eq!(result[1].imported_hooks.len(), 1); + assert_eq!(result[1].imported_hooks[0].name, "when_shared"); + + // the setup hook was removed but the test function remains in the module + assert_eq!(result[0].functions.len(), 1); + assert!(matches!(result[0].functions[0], Function::TestFunction(_))); + assert_eq!(result[0].functions[0].name(), "when_shared"); + } +} From e2cd583553d1c7543e3f9e8af2c996e4126e0d17 Mon Sep 17 00:00:00 2001 From: teddy Date: Tue, 6 Jan 2026 19:11:05 -0300 Subject: [PATCH 5/5] feat: include imports in scaffold output, ignore them in check --- .../tests/check/hoisted_setup_hooks_test.nr | 2 ++ crates/bulloak/tests/scaffold.rs | 16 +++++++++++++++ .../scaffold/hoisted_hook_regression_test.nr | 2 ++ .../scaffold/hoisted_hook_skip_modifiers.tree | 9 +++++++++ .../hoisted_hook_skip_modifiers_test.nr | 20 +++++++++++++++++++ crates/noir/src/scaffold/scaffoldable.rs | 5 +++++ 6 files changed, 54 insertions(+) create mode 100644 crates/bulloak/tests/scaffold/hoisted_hook_skip_modifiers.tree create mode 100644 crates/bulloak/tests/scaffold/hoisted_hook_skip_modifiers_test.nr diff --git a/crates/bulloak/tests/check/hoisted_setup_hooks_test.nr b/crates/bulloak/tests/check/hoisted_setup_hooks_test.nr index 4559165..d1de9ff 100644 --- a/crates/bulloak/tests/check/hoisted_setup_hooks_test.nr +++ b/crates/bulloak/tests/check/hoisted_setup_hooks_test.nr @@ -6,6 +6,7 @@ unconstrained fn when_b() { mod foo { + use super::when_b; #[test] unconstrained fn test_when_b() { when_b(); @@ -21,6 +22,7 @@ mod foo { } mod bar { + // import is not present in this module. `check` ignores imports. #[test] unconstrained fn test_when_b() { when_b(); diff --git a/crates/bulloak/tests/scaffold.rs b/crates/bulloak/tests/scaffold.rs index 3f98693..7597095 100644 --- a/crates/bulloak/tests/scaffold.rs +++ b/crates/bulloak/tests/scaffold.rs @@ -374,3 +374,19 @@ fn scaffold_noir_generates_hoisted_setup_hook_for_shared_condition() { let expected = fs::read_to_string(noir_filename).unwrap(); assert_eq!(expected.trim(), actual.trim()); } + +#[test] +fn scaffold_noir_generates_skips_modifiers_on_imported_hoks() { + let cwd = env::current_dir().unwrap(); + let binary_path = get_binary_path(); + let tests_path = cwd.join("tests").join("scaffold"); + let tree_path = tests_path.join("hoisted_hook_skip_modifiers.tree"); + + let output = cmd(&binary_path, "scaffold", &tree_path, &["-l", "noir", "-m"]); + let actual = String::from_utf8(output.stdout).unwrap(); + + // Verify the full expected output matches + let noir_filename = tests_path.join("hoisted_hook_skip_modifiers_test.nr"); + let expected = fs::read_to_string(noir_filename).unwrap(); + assert_eq!(expected.trim(), actual.trim()); +} diff --git a/crates/bulloak/tests/scaffold/hoisted_hook_regression_test.nr b/crates/bulloak/tests/scaffold/hoisted_hook_regression_test.nr index a17c1e9..c7732b1 100644 --- a/crates/bulloak/tests/scaffold/hoisted_hook_regression_test.nr +++ b/crates/bulloak/tests/scaffold/hoisted_hook_regression_test.nr @@ -6,6 +6,7 @@ unconstrained fn when_passing_valid_parameters() { mod constructor_with_minter { + use super::when_passing_valid_parameters; #[test] unconstrained fn test_when_passing_valid_parameters() { when_passing_valid_parameters(); @@ -16,6 +17,7 @@ mod constructor_with_minter { } mod constructor_with_initial_supply { + use super::when_passing_valid_parameters; #[test] unconstrained fn test_when_passing_valid_parameters() { when_passing_valid_parameters(); diff --git a/crates/bulloak/tests/scaffold/hoisted_hook_skip_modifiers.tree b/crates/bulloak/tests/scaffold/hoisted_hook_skip_modifiers.tree new file mode 100644 index 0000000..5a31aab --- /dev/null +++ b/crates/bulloak/tests/scaffold/hoisted_hook_skip_modifiers.tree @@ -0,0 +1,9 @@ +hoisted_hook_skip_modifiers::constructor_with_minter +└── when passing valid parameters + ├── it sets name + └── it sets symbol + +hoisted_hook_skip_modifiers::constructor_with_initial_supply +└── when passing valid parameters + ├── it sets name + └── it sets symbol diff --git a/crates/bulloak/tests/scaffold/hoisted_hook_skip_modifiers_test.nr b/crates/bulloak/tests/scaffold/hoisted_hook_skip_modifiers_test.nr new file mode 100644 index 0000000..6b4b725 --- /dev/null +++ b/crates/bulloak/tests/scaffold/hoisted_hook_skip_modifiers_test.nr @@ -0,0 +1,20 @@ +// Generated by bulloak + + +mod constructor_with_minter { + #[test] + unconstrained fn test_when_passing_valid_parameters() { + // it sets name + // it sets symbol + } + +} + +mod constructor_with_initial_supply { + #[test] + unconstrained fn test_when_passing_valid_parameters() { + // it sets name + // it sets symbol + } + +} diff --git a/crates/noir/src/scaffold/scaffoldable.rs b/crates/noir/src/scaffold/scaffoldable.rs index 85880ad..276e0fb 100644 --- a/crates/noir/src/scaffold/scaffoldable.rs +++ b/crates/noir/src/scaffold/scaffoldable.rs @@ -69,6 +69,11 @@ impl Scaffoldable for Module { fn scaffold(&self, generate_setup_hooks: bool) -> String { let mut output = format!("\nmod {} {{\n", self.name); + if generate_setup_hooks { + for import in &self.imported_hooks { + output.push_str(format!(" use super::{};\n", import.name).as_str()); + } + } // indent the definitions inside the module for func in &self.functions { for line in func.scaffold(generate_setup_hooks).lines() {