From 4d77db2f1b740786650103b1f19230a3f65fa8fc Mon Sep 17 00:00:00 2001 From: AztecBot Date: Thu, 7 Mar 2024 19:56:43 +0000 Subject: [PATCH] [1 changes] chore: add regression test for issue 4449 (https://github.com/noir-lang/noir/pull/4503) chore: pass macro processors by reference (https://github.com/noir-lang/noir/pull/4501) chore: bump bb to 0.26.3 (https://github.com/noir-lang/noir/pull/4488) fix: handling of gh deps in noir_wasm (https://github.com/noir-lang/noir/pull/4499) fix: iterative flattening pass (https://github.com/noir-lang/noir/pull/4492) chore: Move templated code for assert_message into the stdlib (https://github.com/noir-lang/noir/pull/4475) chore: pull out separate function for compiling and running a test chore: update cargo deny config (https://github.com/noir-lang/noir/pull/4486) feat: run tests in parallel in `nargo test` (https://github.com/noir-lang/noir/pull/4484) --- noir/noir-repo/.gitrepo | 4 +- noir/noir-repo/Cargo.lock | 9 - noir/noir-repo/Cargo.toml | 3 +- noir/noir-repo/bootstrap.sh | 22 - noir/noir-repo/bootstrap_cache.sh | 13 - .../compiler/noirc_driver/Cargo.toml | 1 - .../compiler/noirc_driver/src/lib.rs | 10 +- .../src/ssa/opt/flatten_cfg.rs | 543 ++++++++++-------- .../src/hir/def_collector/dc_crate.rs | 10 +- .../noirc_frontend/src/hir/def_map/mod.rs | 12 +- .../src/hir/resolution/resolver.rs | 8 +- .../compiler/noirc_frontend/src/tests.rs | 2 +- .../compiler/wasm/src/noir/package.ts | 7 +- noir/noir-repo/deny.toml | 13 +- noir/noir-repo/noir_stdlib/src/internal.nr | 12 + noir/noir-repo/noir_stdlib/src/lib.nr | 1 + noir/noir-repo/noirc_macros/Cargo.toml | 14 - noir/noir-repo/noirc_macros/src/lib.rs | 73 --- .../regression_4449/Nargo.toml | 6 + .../regression_4449/Prover.toml | 3 + .../regression_4449/src/main.nr | 14 + .../tooling/bb_abstraction_leaks/build.rs | 2 +- .../tooling/lsp/src/requests/test_run.rs | 2 +- noir/noir-repo/tooling/nargo/src/ops/test.rs | 14 +- .../tooling/nargo_cli/src/cli/test_cmd.rs | 139 +++-- .../noir_js_backend_barretenberg/package.json | 2 +- noir/noir-repo/yarn.lock | 10 +- 27 files changed, 499 insertions(+), 450 deletions(-) delete mode 100755 noir/noir-repo/bootstrap.sh delete mode 100755 noir/noir-repo/bootstrap_cache.sh create mode 100644 noir/noir-repo/noir_stdlib/src/internal.nr delete mode 100644 noir/noir-repo/noirc_macros/Cargo.toml delete mode 100644 noir/noir-repo/noirc_macros/src/lib.rs create mode 100644 noir/noir-repo/test_programs/execution_success/regression_4449/Nargo.toml create mode 100644 noir/noir-repo/test_programs/execution_success/regression_4449/Prover.toml create mode 100644 noir/noir-repo/test_programs/execution_success/regression_4449/src/main.nr diff --git a/noir/noir-repo/.gitrepo b/noir/noir-repo/.gitrepo index 24317f85d26..c11aa7b6e84 100644 --- a/noir/noir-repo/.gitrepo +++ b/noir/noir-repo/.gitrepo @@ -6,7 +6,7 @@ [subrepo] remote = https://github.com/noir-lang/noir branch = master - commit = 5f57ebb7ff4b810802f90699a10f4325ef904f2e - parent = 8307dadd853d5091841e169c841ab6b09c223efb + commit = 2a555a041dbd3e40cbf768ca131f508570f4ba50 + parent = 481e46b2091080e5c9498f248a29caafc122508e method = merge cmdver = 0.4.6 diff --git a/noir/noir-repo/Cargo.lock b/noir/noir-repo/Cargo.lock index b2b6f8037bb..18c1f7ad40e 100644 --- a/noir/noir-repo/Cargo.lock +++ b/noir/noir-repo/Cargo.lock @@ -2946,7 +2946,6 @@ dependencies = [ "noirc_errors", "noirc_evaluator", "noirc_frontend", - "noirc_macros", "rust-embed", "serde", "thiserror", @@ -3012,14 +3011,6 @@ dependencies = [ "tracing", ] -[[package]] -name = "noirc_macros" -version = "0.24.0" -dependencies = [ - "iter-extended", - "noirc_frontend", -] - [[package]] name = "noirc_printable_type" version = "0.24.0" diff --git a/noir/noir-repo/Cargo.toml b/noir/noir-repo/Cargo.toml index 7d5da7b00d0..38f39137360 100644 --- a/noir/noir-repo/Cargo.toml +++ b/noir/noir-repo/Cargo.toml @@ -1,9 +1,8 @@ [workspace] members = [ - # Macros crates for metaprogramming + # Aztec Macro crate for metaprogramming "aztec_macros", - "noirc_macros", # Compiler crates "compiler/noirc_evaluator", "compiler/noirc_frontend", diff --git a/noir/noir-repo/bootstrap.sh b/noir/noir-repo/bootstrap.sh deleted file mode 100755 index 54129c3d61a..00000000000 --- a/noir/noir-repo/bootstrap.sh +++ /dev/null @@ -1,22 +0,0 @@ -#!/usr/bin/env bash -set -eu - -cd $(dirname "$0") - -CMD=${1:-} - -if [ -n "$CMD" ]; then - if [ "$CMD" = "clean" ]; then - git clean -fdx - exit 0 - else - echo "Unknown command: $CMD" - exit 1 - fi -fi - -# Attempt to just pull artefacts from CI and exit on success. -[ -n "${USE_CACHE:-}" ] && ./bootstrap_cache.sh && exit - -./scripts/bootstrap_native.sh -./scripts/bootstrap_packages.sh diff --git a/noir/noir-repo/bootstrap_cache.sh b/noir/noir-repo/bootstrap_cache.sh deleted file mode 100755 index 1cec6c81d8e..00000000000 --- a/noir/noir-repo/bootstrap_cache.sh +++ /dev/null @@ -1,13 +0,0 @@ -#!/usr/bin/env bash -set -eu - -cd "$(dirname "$0")" -source ../build-system/scripts/setup_env '' '' mainframe_$USER > /dev/null - -echo -e "\033[1mRetrieving noir packages from remote cache...\033[0m" -extract_repo noir-packages /usr/src/noir/packages ./ -echo -e "\033[1mRetrieving nargo from remote cache...\033[0m" -extract_repo noir /usr/src/noir/target/release ./target/ - -remove_old_images noir-packages -remove_old_images noir diff --git a/noir/noir-repo/compiler/noirc_driver/Cargo.toml b/noir/noir-repo/compiler/noirc_driver/Cargo.toml index 681976735f3..a7fe0b4b610 100644 --- a/noir/noir-repo/compiler/noirc_driver/Cargo.toml +++ b/noir/noir-repo/compiler/noirc_driver/Cargo.toml @@ -26,4 +26,3 @@ tracing.workspace = true thiserror.workspace = true aztec_macros = { path = "../../aztec_macros" } -noirc_macros = { path = "../../noirc_macros" } diff --git a/noir/noir-repo/compiler/noirc_driver/src/lib.rs b/noir/noir-repo/compiler/noirc_driver/src/lib.rs index 11f53cdb749..fa134f8a0dd 100644 --- a/noir/noir-repo/compiler/noirc_driver/src/lib.rs +++ b/noir/noir-repo/compiler/noirc_driver/src/lib.rs @@ -237,14 +237,8 @@ pub fn check_crate( deny_warnings: bool, disable_macros: bool, ) -> CompilationResult<()> { - let macros: Vec<&dyn MacroProcessor> = if disable_macros { - vec![&noirc_macros::AssertMessageMacro as &dyn MacroProcessor] - } else { - vec![ - &aztec_macros::AztecMacro as &dyn MacroProcessor, - &noirc_macros::AssertMessageMacro as &dyn MacroProcessor, - ] - }; + let macros: &[&dyn MacroProcessor] = + if disable_macros { &[] } else { &[&aztec_macros::AztecMacro as &dyn MacroProcessor] }; let mut errors = vec![]; let diagnostics = CrateDefMap::collect_defs(crate_id, context, macros); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 943a57c1bc0..46f1e7a2765 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -199,22 +199,47 @@ struct Context<'f> { /// found, the top of this conditions stack is popped since we are no longer under that /// condition. If we are under multiple conditions (a nested if), the topmost condition is /// the most recent condition combined with all previous conditions via `And` instructions. - conditions: Vec<(BasicBlockId, ValueId)>, + condition_stack: Vec, /// Maps SSA array values with a slice type to their size. /// This is maintained by appropriate calls to the `SliceCapacityTracker` and is used by the `ValueMerger`. slice_sizes: HashMap, + + /// Stack of block arguments + /// When processing a block, we pop this stack to get its arguments + /// and at the end we push the arguments for his successor + arguments_stack: Vec>, } +#[derive(Clone)] pub(crate) struct Store { old_value: ValueId, new_value: ValueId, } -struct Branch { - condition: ValueId, +#[derive(Clone)] +struct ConditionalBranch { + // Contains the last processed block during the processing of the branch. last_block: BasicBlockId, + // The unresolved condition of the branch + old_condition: ValueId, + // The condition of the branch + condition: ValueId, + // The store values accumulated when processing the branch store_values: HashMap, + // The allocations accumulated when processing the branch + local_allocations: HashSet, +} + +struct ConditionalContext { + // Condition from the conditional statement + condition: ValueId, + // Block containing the conditional statement + entry_block: BasicBlockId, + // First block of the then branch + then_branch: ConditionalBranch, + // First block of the else branch + else_branch: Option, } fn flatten_function_cfg(function: &mut Function) { @@ -233,90 +258,117 @@ fn flatten_function_cfg(function: &mut Function) { store_values: HashMap::default(), local_allocations: HashSet::new(), branch_ends, - conditions: Vec::new(), slice_sizes: HashMap::default(), + condition_stack: Vec::new(), + arguments_stack: Vec::new(), }; context.flatten(); } impl<'f> Context<'f> { fn flatten(&mut self) { - // Start with following the terminator of the entry block since we don't - // need to flatten the entry block into itself. - self.handle_terminator(self.inserter.function.entry_block()); + // Flatten the CFG by inlining all instructions from the queued blocks + // until all blocks have been flattened. + // We follow the terminator of each block to determine which blocks to + // process next + let mut queue = vec![self.inserter.function.entry_block()]; + while let Some(block) = queue.pop() { + self.inline_block(block); + let to_process = self.handle_terminator(block, &queue); + for incoming_block in to_process { + if !queue.contains(&incoming_block) { + queue.push(incoming_block); + } + } + } } - /// Check the terminator of the given block and recursively inline any blocks reachable from - /// it. Since each block from a jmpif terminator is inlined successively, we must handle - /// instructions with side effects like constrain and store specially to preserve correctness. - /// For these instructions we must keep track of what the current condition is and modify - /// the instructions according to the module-level comment at the top of this file. Note that - /// the current condition is all the jmpif conditions required to reach the current block, - /// combined via `And` instructions. - /// - /// Returns the last block to be inlined. This is either the return block of the function or, - /// if self.conditions is not empty, the end block of the most recent condition. - fn handle_terminator(&mut self, block: BasicBlockId) -> BasicBlockId { - // As we recursively flatten inner blocks, we need to track the slice information - // for the outer block before we start recursively inlining - let outer_block_instructions = self.inserter.function.dfg[block].instructions(); - let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - for instruction in outer_block_instructions { - let results = self.inserter.function.dfg.instruction_results(*instruction); - let instruction = &self.inserter.function.dfg[*instruction]; + /// Returns the updated condition so that + /// it is 'AND-ed' with the previous condition (if any) + fn link_condition(&mut self, condition: ValueId) -> ValueId { + // Retrieve the previous condition + if let Some(context) = self.condition_stack.last() { + let previous_branch = context.else_branch.as_ref().unwrap_or(&context.then_branch); + let and = Instruction::binary(BinaryOp::And, previous_branch.condition, condition); + self.insert_instruction(and, CallStack::new()) + } else { + condition + } + } + + /// Returns the current condition + fn get_last_condition(&self) -> Option { + self.condition_stack.last().map(|context| match &context.else_branch { + Some(else_branch) => else_branch.condition, + None => context.then_branch.condition, + }) + } + + // Inline all instructions from the given block into the entry block, and track slice capacities + fn inline_block(&mut self, block: BasicBlockId) { + if self.inserter.function.entry_block() == block { + // we do not inline the entry block into itself + // for the outer block before we start inlining + let outer_block_instructions = self.inserter.function.dfg[block].instructions(); + let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); + for instruction in outer_block_instructions { + let results = self.inserter.function.dfg.instruction_results(*instruction); + let instruction = &self.inserter.function.dfg[*instruction]; + capacity_tracker.collect_slice_information( + instruction, + &mut self.slice_sizes, + results.to_vec(), + ); + } + + return; + } + + let arguments = self.arguments_stack.pop().unwrap(); + self.inserter.remember_block_params(block, &arguments); + + // If this is not a separate variable, clippy gets confused and says the to_vec is + // unnecessary, when removing it actually causes an aliasing/mutability error. + let instructions = self.inserter.function.dfg[block].instructions().to_vec(); + for instruction in instructions.iter() { + let results = self.push_instruction(*instruction); + let (instruction, _) = self.inserter.map_instruction(*instruction); + let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); capacity_tracker.collect_slice_information( - instruction, + &instruction, &mut self.slice_sizes, - results.to_vec(), + results, ); } + } - match self.inserter.function.dfg[block].unwrap_terminator() { + /// Returns the list of blocks that need to be processed after the given block + /// For a normal block, it would be its successor + /// For blocks related to a conditional statement, we ensure to process + /// the 'then-branch', then the 'else-branch' (if it exists), and finally the end block + fn handle_terminator( + &mut self, + block: BasicBlockId, + work_list: &[BasicBlockId], + ) -> Vec { + let terminator = self.inserter.function.dfg[block].unwrap_terminator().clone(); + match &terminator { TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { - let old_condition = *condition; - let then_block = *then_destination; - let else_block = *else_destination; - let then_condition = self.inserter.resolve(old_condition); - - let one = FieldElement::one(); - let then_branch = - self.inline_branch(block, then_block, old_condition, then_condition, one); - - let else_condition = - self.insert_instruction(Instruction::Not(then_condition), CallStack::new()); - let zero = FieldElement::zero(); - - // Make sure the else branch sees the previous values of each store - // rather than any values created in the 'then' branch. - self.undo_stores_in_then_branch(&then_branch); - - let else_branch = - self.inline_branch(block, else_block, old_condition, else_condition, zero); - - // We must remember to reset whether side effects are enabled when both branches - // end, in addition to resetting the value of old_condition since it is set to - // known to be true/false within the then/else branch respectively. - self.insert_current_side_effects_enabled(); - - // We must map back to `then_condition` here. Mapping `old_condition` to itself would - // lose any previous mappings. - self.inserter.map_value(old_condition, then_condition); - - // While there is a condition on the stack we don't compile outside the condition - // until it is popped. This ensures we inline the full then and else branches - // before continuing from the end of the conditional here where they can be merged properly. - let end = self.branch_ends[&block]; - self.inline_branch_end(end, then_branch, else_branch) + self.arguments_stack.push(vec![]); + self.if_start(condition, then_destination, else_destination, &block) } TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { - if let Some((end_block, _)) = self.conditions.last() { - if destination == end_block { - return block; + let arguments = vecmap(arguments.clone(), |value| self.inserter.resolve(value)); + self.arguments_stack.push(arguments); + if work_list.contains(destination) { + if work_list.last() == Some(destination) { + self.else_stop(&block) + } else { + self.then_stop(&block) } + } else { + vec![*destination] } - let destination = *destination; - let arguments = vecmap(arguments.clone(), |value| self.inserter.resolve(value)); - self.inline_block(destination, &arguments) } TerminatorInstruction::Return { return_values, call_stack } => { let call_stack = call_stack.clone(); @@ -326,133 +378,133 @@ impl<'f> Context<'f> { let entry = self.inserter.function.entry_block(); self.inserter.function.dfg.set_block_terminator(entry, new_return); - block + vec![] } } } - /// Push a condition to the stack of conditions. - /// - /// This condition should be present while we're inlining each block reachable from the 'then' - /// branch of a jmpif instruction, until the branches eventually join back together. Likewise, - /// !condition should be present while we're inlining each block reachable from the 'else' - /// branch of a jmpif instruction until the join block. - fn push_condition(&mut self, start_block: BasicBlockId, condition: ValueId) { - let end_block = self.branch_ends[&start_block]; - - if let Some((_, previous_condition)) = self.conditions.last() { - let and = Instruction::binary(BinaryOp::And, *previous_condition, condition); - let new_condition = self.insert_instruction(and, CallStack::new()); - self.conditions.push((end_block, new_condition)); - } else { - self.conditions.push((end_block, condition)); + /// Process a conditional statement + fn if_start( + &mut self, + condition: &ValueId, + then_destination: &BasicBlockId, + else_destination: &BasicBlockId, + if_entry: &BasicBlockId, + ) -> Vec { + // manage conditions + let old_condition = *condition; + let then_condition = self.inserter.resolve(old_condition); + + let one = FieldElement::one(); + let old_stores = std::mem::take(&mut self.store_values); + let old_allocations = std::mem::take(&mut self.local_allocations); + let branch = ConditionalBranch { + old_condition, + condition: self.link_condition(then_condition), + store_values: old_stores, + local_allocations: old_allocations, + last_block: *then_destination, + }; + let cond_context = ConditionalContext { + condition: then_condition, + entry_block: *if_entry, + then_branch: branch, + else_branch: None, + }; + self.condition_stack.push(cond_context); + self.insert_current_side_effects_enabled(); + // Optimization: within the then branch we know the condition to be true, so replace + // any references of it within this branch with true. Likewise, do the same with false + // with the else branch. We must be careful not to replace the condition if it is a + // known constant, otherwise we can end up setting 1 = 0 or vice-versa. + if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() { + let known_value = self.inserter.function.dfg.make_constant(one, Type::bool()); + + self.inserter.map_value(old_condition, known_value); } + vec![self.branch_ends[if_entry], *else_destination, *then_destination] } - /// Insert a new instruction into the function's entry block. - /// Unlike push_instruction, this function will not map any ValueIds. - /// within the given instruction, nor will it modify self.values in any way. - fn insert_instruction(&mut self, instruction: Instruction, call_stack: CallStack) -> ValueId { - let block = self.inserter.function.entry_block(); - self.inserter - .function - .dfg - .insert_instruction_and_results(instruction, block, None, call_stack) - .first() + /// Switch context to the 'else-branch' + fn then_stop(&mut self, block: &BasicBlockId) -> Vec { + let mut cond_context = self.condition_stack.pop().unwrap(); + cond_context.then_branch.last_block = *block; + + let else_condition = + self.insert_instruction(Instruction::Not(cond_context.condition), CallStack::new()); + let else_condition = self.link_condition(else_condition); + + let zero = FieldElement::zero(); + // Make sure the else branch sees the previous values of each store + // rather than any values created in the 'then' branch. + let old_stores = std::mem::take(&mut cond_context.then_branch.store_values); + cond_context.then_branch.store_values = std::mem::take(&mut self.store_values); + self.undo_stores_in_then_branch(&cond_context.then_branch.store_values); + + let old_allocations = std::mem::take(&mut self.local_allocations); + let else_branch = ConditionalBranch { + old_condition: cond_context.then_branch.old_condition, + condition: else_condition, + store_values: old_stores, + local_allocations: old_allocations, + last_block: *block, + }; + let old_condition = else_branch.old_condition; + cond_context.then_branch.local_allocations.clear(); + cond_context.else_branch = Some(else_branch); + self.condition_stack.push(cond_context); + + self.insert_current_side_effects_enabled(); + // Optimization: within the then branch we know the condition to be true, so replace + // any references of it within this branch with true. Likewise, do the same with false + // with the else branch. We must be careful not to replace the condition if it is a + // known constant, otherwise we can end up setting 1 = 0 or vice-versa. + if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() { + let known_value = self.inserter.function.dfg.make_constant(zero, Type::bool()); + + self.inserter.map_value(old_condition, known_value); + } + assert_eq!(self.cfg.successors(*block).len(), 1); + vec![self.cfg.successors(*block).next().unwrap()] } - /// Inserts a new instruction into the function's entry block, using the given - /// control type variables to specify result types if needed. - /// Unlike push_instruction, this function will not map any ValueIds. - /// within the given instruction, nor will it modify self.values in any way. - fn insert_instruction_with_typevars( - &mut self, - instruction: Instruction, - ctrl_typevars: Option>, - ) -> InsertInstructionResult { - let block = self.inserter.function.entry_block(); - self.inserter.function.dfg.insert_instruction_and_results( - instruction, - block, - ctrl_typevars, - CallStack::new(), - ) - } + /// Process the 'exit' block of a conditional statement + fn else_stop(&mut self, block: &BasicBlockId) -> Vec { + let mut cond_context = self.condition_stack.pop().unwrap(); + if cond_context.else_branch.is_none() { + // then_stop() has not been called, this means that the conditional statement has no else branch + // so we simply do the then_stop() now + self.condition_stack.push(cond_context); + self.then_stop(block); + cond_context = self.condition_stack.pop().unwrap(); + } - /// Checks the branch condition on the top of the stack and uses it to build and insert an - /// `EnableSideEffects` instruction into the entry block. - /// - /// If the stack is empty, a "true" u1 constant is taken to be the active condition. This is - /// necessary for re-enabling side-effects when re-emerging to a branch depth of 0. - fn insert_current_side_effects_enabled(&mut self) { - let condition = match self.conditions.last() { - Some((_, cond)) => *cond, - None => { - self.inserter.function.dfg.make_constant(FieldElement::one(), Type::unsigned(1)) - } - }; - let enable_side_effects = Instruction::EnableSideEffects { condition }; - self.insert_instruction_with_typevars(enable_side_effects, None); - } + let mut else_branch = cond_context.else_branch.unwrap(); + let stores_in_branch = std::mem::replace(&mut self.store_values, else_branch.store_values); + self.local_allocations = std::mem::take(&mut else_branch.local_allocations); + else_branch.last_block = *block; + else_branch.store_values = stores_in_branch; + cond_context.else_branch = Some(else_branch); - /// Inline one branch of a jmpif instruction. - /// - /// This will continue inlining recursively until the next end block is reached where each branch - /// of the jmpif instruction is joined back into a single block. - /// - /// Within a branch of a jmpif instruction, we can assume the condition of the jmpif to be - /// always true or false, depending on which branch we're in. - /// - /// Returns the ending block / join block of this branch. - fn inline_branch( - &mut self, - jmpif_block: BasicBlockId, - destination: BasicBlockId, - old_condition: ValueId, - new_condition: ValueId, - condition_value: FieldElement, - ) -> Branch { - if destination == self.branch_ends[&jmpif_block] { - // If the branch destination is the same as the end of the branch, this must be the - // 'else' case of an if with no else - so there is no else branch. - Branch { - condition: new_condition, - // The last block here is somewhat arbitrary. It only matters that it has no Jmp - // args that will be merged by inline_branch_end. Since jmpifs don't have - // block arguments, it is safe to use the jmpif block here. - last_block: jmpif_block, - store_values: HashMap::default(), - } - } else { - self.push_condition(jmpif_block, new_condition); - self.insert_current_side_effects_enabled(); - let old_stores = std::mem::take(&mut self.store_values); - let old_allocations = std::mem::take(&mut self.local_allocations); - - // Optimization: within the then branch we know the condition to be true, so replace - // any references of it within this branch with true. Likewise, do the same with false - // with the else branch. We must be careful not to replace the condition if it is a - // known constant, otherwise we can end up setting 1 = 0 or vice-versa. - if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() { - let known_value = - self.inserter.function.dfg.make_constant(condition_value, Type::bool()); - - self.inserter.map_value(old_condition, known_value); - } + // We must remember to reset whether side effects are enabled when both branches + // end, in addition to resetting the value of old_condition since it is set to + // known to be true/false within the then/else branch respectively. + self.insert_current_side_effects_enabled(); - let final_block = self.inline_block(destination, &[]); + // We must map back to `then_condition` here. Mapping `old_condition` to itself would + // lose any previous mappings. + self.inserter + .map_value(cond_context.then_branch.old_condition, cond_context.then_branch.condition); - self.conditions.pop(); + // While there is a condition on the stack we don't compile outside the condition + // until it is popped. This ensures we inline the full then and else branches + // before continuing from the end of the conditional here where they can be merged properly. + let end = self.branch_ends[&cond_context.entry_block]; - let stores_in_branch = std::mem::replace(&mut self.store_values, old_stores); - self.local_allocations = old_allocations; + // Merge arguments and stores from the else/end branches + self.inline_branch_end(end, cond_context); - Branch { - condition: new_condition, - last_block: final_block, - store_values: stores_in_branch, - } - } + vec![self.cfg.successors(*block).next().unwrap()] } /// Inline the ending block of a branch, the point where all blocks from a jmpif instruction @@ -467,15 +519,17 @@ impl<'f> Context<'f> { fn inline_branch_end( &mut self, destination: BasicBlockId, - then_branch: Branch, - else_branch: Branch, + cond_context: ConditionalContext, ) -> BasicBlockId { assert_eq!(self.cfg.predecessors(destination).len(), 2); + let last_then = cond_context.then_branch.last_block; + let mut else_args = Vec::new(); + if cond_context.else_branch.is_some() { + let last_else = cond_context.else_branch.clone().unwrap().last_block; + else_args = self.inserter.function.dfg[last_else].terminator_arguments().to_vec(); + } - let then_args = - self.inserter.function.dfg[then_branch.last_block].terminator_arguments().to_vec(); - let else_args = - self.inserter.function.dfg[else_branch.last_block].terminator_arguments().to_vec(); + let then_args = self.inserter.function.dfg[last_then].terminator_arguments().to_vec(); let params = self.inserter.function.dfg.block_parameters(destination); assert_eq!(params.len(), then_args.len()); @@ -500,17 +554,64 @@ impl<'f> Context<'f> { // Cannot include this in the previous vecmap since it requires exclusive access to self let args = vecmap(args, |(then_arg, else_arg)| { value_merger.merge_values( - then_branch.condition, - else_branch.condition, + cond_context.then_branch.condition, + cond_context.else_branch.clone().unwrap().condition, then_arg, else_arg, ) }); - self.merge_stores(then_branch, else_branch); + self.merge_stores(cond_context.then_branch, cond_context.else_branch); + self.arguments_stack.pop(); + self.arguments_stack.pop(); + self.arguments_stack.push(args); + destination + } - // insert merge instruction - self.inline_block(destination, &args) + /// Insert a new instruction into the function's entry block. + /// Unlike push_instruction, this function will not map any ValueIds. + /// within the given instruction, nor will it modify self.values in any way. + fn insert_instruction(&mut self, instruction: Instruction, call_stack: CallStack) -> ValueId { + let block = self.inserter.function.entry_block(); + self.inserter + .function + .dfg + .insert_instruction_and_results(instruction, block, None, call_stack) + .first() + } + + /// Inserts a new instruction into the function's entry block, using the given + /// control type variables to specify result types if needed. + /// Unlike push_instruction, this function will not map any ValueIds. + /// within the given instruction, nor will it modify self.values in any way. + fn insert_instruction_with_typevars( + &mut self, + instruction: Instruction, + ctrl_typevars: Option>, + ) -> InsertInstructionResult { + let block = self.inserter.function.entry_block(); + self.inserter.function.dfg.insert_instruction_and_results( + instruction, + block, + ctrl_typevars, + CallStack::new(), + ) + } + + /// Checks the branch condition on the top of the stack and uses it to build and insert an + /// `EnableSideEffects` instruction into the entry block. + /// + /// If the stack is empty, a "true" u1 constant is taken to be the active condition. This is + /// necessary for re-enabling side-effects when re-emerging to a branch depth of 0. + fn insert_current_side_effects_enabled(&mut self) { + let condition = match self.get_last_condition() { + Some(cond) => cond, + None => { + self.inserter.function.dfg.make_constant(FieldElement::one(), Type::unsigned(1)) + } + }; + let enable_side_effects = Instruction::EnableSideEffects { condition }; + self.insert_instruction_with_typevars(enable_side_effects, None); } /// Merge any store instructions found in each branch. @@ -518,7 +619,11 @@ impl<'f> Context<'f> { /// This function relies on the 'then' branch being merged before the 'else' branch of a jmpif /// instruction. If this ordering is changed, the ordering that store values are merged within /// this function also needs to be changed to reflect that. - fn merge_stores(&mut self, then_branch: Branch, else_branch: Branch) { + fn merge_stores( + &mut self, + then_branch: ConditionalBranch, + else_branch: Option, + ) { // Address -> (then_value, else_value, value_before_the_if) let mut new_map = BTreeMap::new(); @@ -526,11 +631,13 @@ impl<'f> Context<'f> { new_map.insert(address, (store.new_value, store.old_value, store.old_value)); } - for (address, store) in else_branch.store_values { - if let Some(entry) = new_map.get_mut(&address) { - entry.1 = store.new_value; - } else { - new_map.insert(address, (store.old_value, store.new_value, store.old_value)); + if else_branch.is_some() { + for (address, store) in else_branch.clone().unwrap().store_values { + if let Some(entry) = new_map.get_mut(&address) { + entry.1 = store.new_value; + } else { + new_map.insert(address, (store.old_value, store.new_value, store.old_value)); + } } } @@ -544,8 +651,11 @@ impl<'f> Context<'f> { } let then_condition = then_branch.condition; - let else_condition = else_branch.condition; - + let else_condition = if let Some(branch) = else_branch { + branch.condition + } else { + self.inserter.function.dfg.make_constant(FieldElement::zero(), Type::bool()) + }; let block = self.inserter.function.entry_block(); let mut value_merger = @@ -607,35 +717,6 @@ impl<'f> Context<'f> { } } - /// Inline all instructions from the given destination block into the entry block. - /// Afterwards, check the block's terminator and continue inlining recursively. - /// - /// Returns the final block that was inlined. - /// - /// Expects that the `arguments` given are already translated via self.inserter.resolve. - /// If they are not, it is possible some values which no longer exist, such as block - /// parameters, will be kept in the program. - fn inline_block(&mut self, destination: BasicBlockId, arguments: &[ValueId]) -> BasicBlockId { - self.inserter.remember_block_params(destination, arguments); - - // If this is not a separate variable, clippy gets confused and says the to_vec is - // unnecessary, when removing it actually causes an aliasing/mutability error. - let instructions = self.inserter.function.dfg[destination].instructions().to_vec(); - - for instruction in instructions.iter() { - let results = self.push_instruction(*instruction); - let (instruction, _) = self.inserter.map_instruction(*instruction); - let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - capacity_tracker.collect_slice_information( - &instruction, - &mut self.slice_sizes, - results, - ); - } - - self.handle_terminator(destination) - } - /// Push the given instruction to the end of the entry block of the current function. /// /// Note that each ValueId of the instruction will be mapped via self.inserter.resolve. @@ -666,7 +747,7 @@ impl<'f> Context<'f> { instruction: Instruction, call_stack: CallStack, ) -> Instruction { - if let Some((_, condition)) = self.conditions.last().copied() { + if let Some(condition) = self.get_last_condition() { match instruction { Instruction::Constrain(lhs, rhs, message) => { // Replace constraint `lhs == rhs` with `condition * lhs == condition * rhs`. @@ -741,8 +822,8 @@ impl<'f> Context<'f> { } } - fn undo_stores_in_then_branch(&mut self, then_branch: &Branch) { - for (address, store) in &then_branch.store_values { + fn undo_stores_in_then_branch(&mut self, store_values: &HashMap) { + for (address, store) in store_values { let address = *address; let value = store.old_value; self.insert_instruction_with_typevars(Instruction::Store { address, value }, None); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 7f36af5b30e..4a6ca5c6993 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -207,7 +207,7 @@ impl DefCollector { context: &mut Context, ast: SortedModule, root_file_id: FileId, - macro_processors: Vec<&dyn MacroProcessor>, + macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; let crate_id = def_map.krate; @@ -220,11 +220,7 @@ impl DefCollector { let crate_graph = &context.crate_graph[crate_id]; for dep in crate_graph.dependencies.clone() { - errors.extend(CrateDefMap::collect_defs( - dep.crate_id, - context, - macro_processors.clone(), - )); + errors.extend(CrateDefMap::collect_defs(dep.crate_id, context, macro_processors)); let dep_def_root = context.def_map(&dep.crate_id).expect("ice: def map was just created").root; @@ -257,7 +253,7 @@ impl DefCollector { context.def_maps.insert(crate_id, def_collector.def_map); // TODO(#4653): generalize this function - for macro_processor in ¯o_processors { + for macro_processor in macro_processors { macro_processor .process_unresolved_traits_impls( &crate_id, diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/mod.rs index 8721bdb6c3c..1326ffca9f7 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -73,7 +73,7 @@ impl CrateDefMap { pub fn collect_defs( crate_id: CrateId, context: &mut Context, - macro_processors: Vec<&dyn MacroProcessor>, + macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { // Check if this Crate has already been compiled // XXX: There is probably a better alternative for this. @@ -90,7 +90,7 @@ impl CrateDefMap { let (ast, parsing_errors) = context.parsed_file_results(root_file_id); let mut ast = ast.into_sorted(); - for macro_processor in ¯o_processors { + for macro_processor in macro_processors { match macro_processor.process_untyped_ast(ast.clone(), &crate_id, context) { Ok(processed_ast) => { ast = processed_ast; @@ -115,13 +115,7 @@ impl CrateDefMap { }; // Now we want to populate the CrateDefMap using the DefCollector - errors.extend(DefCollector::collect( - def_map, - context, - ast, - root_file_id, - macro_processors.clone(), - )); + errors.extend(DefCollector::collect(def_map, context, ast, root_file_id, macro_processors)); errors.extend( parsing_errors.iter().map(|e| (e.clone().into(), root_file_id)).collect::>(), diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 7f9e48353a7..7789c06ca69 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -1256,13 +1256,17 @@ impl<'a> Resolver<'a> { let is_in_stdlib = self.path_resolver.module_id().krate.is_stdlib(); let assert_msg_call_path = if is_in_stdlib { ExpressionKind::Variable(Path { - segments: vec![Ident::from("resolve_assert_message")], + segments: vec![Ident::from("internal"), Ident::from("resolve_assert_message")], kind: PathKind::Crate, span, }) } else { ExpressionKind::Variable(Path { - segments: vec![Ident::from("std"), Ident::from("resolve_assert_message")], + segments: vec![ + Ident::from("std"), + Ident::from("internal"), + Ident::from("resolve_assert_message"), + ], kind: PathKind::Dep, span, }) diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs index c661cc92eef..0c8c677d9af 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs @@ -81,7 +81,7 @@ mod test { &mut context, program.clone().into_sorted(), root_file_id, - Vec::new(), // No macro processors + &[], // No macro processors )); } (program, context, errors) diff --git a/noir/noir-repo/compiler/wasm/src/noir/package.ts b/noir/noir-repo/compiler/wasm/src/noir/package.ts index 81178e6ae96..2856798273a 100644 --- a/noir/noir-repo/compiler/wasm/src/noir/package.ts +++ b/noir/noir-repo/compiler/wasm/src/noir/package.ts @@ -105,7 +105,12 @@ export class Package { handles .filter((handle) => SOURCE_EXTENSIONS.find((ext) => handle.endsWith(ext))) .map(async (file) => { - const suffix = file.replace(this.#srcPath, ''); + // Github deps are directly added to the file manager, which causes them to be missing the absolute path to the source file + // and only include the extraction directory relative to the fm root directory + // This regexp ensures we remove the "real" source path for all dependencies, providing the compiler with what it expects for each source file: + // -> for bin/contract packages + // -> for libs + const suffix = file.replace(new RegExp(`.*${this.#srcPath}`), ''); return { path: this.getType() === 'lib' ? `${alias ? alias : this.#config.package.name}${suffix}` : file, source: (await fm.readFile(file, 'utf-8')).toString(), diff --git a/noir/noir-repo/deny.toml b/noir/noir-repo/deny.toml index 72150f08a3c..578f8427263 100644 --- a/noir/noir-repo/deny.toml +++ b/noir/noir-repo/deny.toml @@ -2,11 +2,13 @@ # More documentation for the advisories section can be found here: # https://embarkstudios.github.io/cargo-deny/checks/advisories/cfg.html [advisories] -vulnerability = "deny" -unmaintained = "warn" -unsound = "warn" +version = 2 yanked = "warn" -notice = "warn" + +ignore = [ + "RUSTSEC-2020-0168", # mach unmaintained + "RUSTSEC-2020-0016" # net2 unmaintained +] # This section is considered when running `cargo deny check bans`. # More documentation about the 'bans' section can be found here: @@ -32,9 +34,8 @@ skip = [] skip-tree = [] [licenses] -unlicensed = "deny" +version = 2 confidence-threshold = 0.9 -# copyleft = "deny" # List of explicitly allowed licenses # See https://spdx.org/licenses/ for list of possible licenses diff --git a/noir/noir-repo/noir_stdlib/src/internal.nr b/noir/noir-repo/noir_stdlib/src/internal.nr new file mode 100644 index 00000000000..8d5c01dda7f --- /dev/null +++ b/noir/noir-repo/noir_stdlib/src/internal.nr @@ -0,0 +1,12 @@ +// This file contains functions which should only be used in calls injected by the Noir compiler. +// These functions should not be called manually in user code. +// +// Changes to this file will not be considered breaking. + +#[oracle(assert_message)] +unconstrained fn assert_message_oracle(_input: T) {} +unconstrained pub fn resolve_assert_message(input: T, condition: bool) { + if !condition { + assert_message_oracle(input); + } +} diff --git a/noir/noir-repo/noir_stdlib/src/lib.nr b/noir/noir-repo/noir_stdlib/src/lib.nr index ebde4b88858..90c04472066 100644 --- a/noir/noir-repo/noir_stdlib/src/lib.nr +++ b/noir/noir-repo/noir_stdlib/src/lib.nr @@ -26,6 +26,7 @@ mod default; mod prelude; mod uint128; mod bigint; +mod internal; // Oracle calls are required to be wrapped in an unconstrained function // Thus, the only argument to the `println` oracle is expected to always be an ident diff --git a/noir/noir-repo/noirc_macros/Cargo.toml b/noir/noir-repo/noirc_macros/Cargo.toml deleted file mode 100644 index 699e6b01cae..00000000000 --- a/noir/noir-repo/noirc_macros/Cargo.toml +++ /dev/null @@ -1,14 +0,0 @@ -[package] -name = "noirc_macros" -version.workspace = true -authors.workspace = true -edition.workspace = true -rust-version.workspace = true -license.workspace = true -repository.workspace = true - -# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html - -[dependencies] -noirc_frontend.workspace = true -iter-extended.workspace = true \ No newline at end of file diff --git a/noir/noir-repo/noirc_macros/src/lib.rs b/noir/noir-repo/noirc_macros/src/lib.rs deleted file mode 100644 index 9a916843200..00000000000 --- a/noir/noir-repo/noirc_macros/src/lib.rs +++ /dev/null @@ -1,73 +0,0 @@ -use noirc_frontend::hir::def_collector::dc_crate::UnresolvedFunctions; -use noirc_frontend::hir::def_collector::dc_crate::UnresolvedTraitImpl; -use noirc_frontend::macros_api::parse_program; -use noirc_frontend::macros_api::HirContext; -use noirc_frontend::macros_api::SortedModule; -use noirc_frontend::macros_api::{CrateId, FileId}; -use noirc_frontend::macros_api::{MacroError, MacroProcessor}; - -pub struct AssertMessageMacro; - -impl MacroProcessor for AssertMessageMacro { - fn process_untyped_ast( - &self, - ast: SortedModule, - crate_id: &CrateId, - _context: &HirContext, - ) -> Result { - transform(ast, crate_id) - } - - fn process_unresolved_traits_impls( - &self, - _crate_id: &CrateId, - _context: &mut HirContext, - _unresolved_traits_impls: &[UnresolvedTraitImpl], - _collected_functions: &mut Vec, - ) -> Result<(), (MacroError, FileId)> { - Ok(()) - } - - // This macro does not need to process any information after name resolution - fn process_typed_ast( - &self, - _crate_id: &CrateId, - _context: &mut HirContext, - ) -> Result<(), (MacroError, FileId)> { - Ok(()) - } -} - -fn transform(ast: SortedModule, crate_id: &CrateId) -> Result { - let ast = add_resolve_assert_message_funcs(ast, crate_id)?; - - Ok(ast) -} - -fn add_resolve_assert_message_funcs( - mut ast: SortedModule, - crate_id: &CrateId, -) -> Result { - if !crate_id.is_stdlib() { - return Ok(ast); - } - let assert_message_oracles = " - #[oracle(assert_message)] - unconstrained fn assert_message_oracle(_input: T) {} - unconstrained pub fn resolve_assert_message(input: T, condition: bool) { - if !condition { - assert_message_oracle(input); - } - }"; - - let (assert_msg_funcs_ast, errors) = parse_program(assert_message_oracles); - assert_eq!(errors.len(), 0, "Failed to parse Noir macro code. This is either a bug in the compiler or the Noir macro code"); - - let assert_msg_funcs_ast = assert_msg_funcs_ast.into_sorted(); - - for func in assert_msg_funcs_ast.functions { - ast.functions.push(func) - } - - Ok(ast) -} diff --git a/noir/noir-repo/test_programs/execution_success/regression_4449/Nargo.toml b/noir/noir-repo/test_programs/execution_success/regression_4449/Nargo.toml new file mode 100644 index 00000000000..925420a03a8 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/regression_4449/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "regression_4449" +type = "bin" +authors = [""] + +[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/regression_4449/Prover.toml b/noir/noir-repo/test_programs/execution_success/regression_4449/Prover.toml new file mode 100644 index 00000000000..81af476bcc9 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/regression_4449/Prover.toml @@ -0,0 +1,3 @@ + +x = 0xbd +result = [204, 59, 83, 197, 18, 1, 128, 43, 247, 28, 104, 225, 106, 13, 20, 187, 42, 26, 67, 150, 48, 75, 238, 168, 121, 247, 142, 160, 71, 222, 97, 188] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/regression_4449/src/main.nr b/noir/noir-repo/test_programs/execution_success/regression_4449/src/main.nr new file mode 100644 index 00000000000..454a93f5d1a --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/regression_4449/src/main.nr @@ -0,0 +1,14 @@ +// Regression test for issue #4449 +use dep::std; + +fn main(x: u8, result: [u8; 32]) { + let x = x % 31; + let mut digest = [0; 32]; + for i in 0..70 { + let y = x + i; + let a = [y, x, 32, 0, y + 1, y - 1, y - 2, 5]; + digest = std::sha256::digest(a); + } + + assert(digest == result); +} diff --git a/noir/noir-repo/tooling/bb_abstraction_leaks/build.rs b/noir/noir-repo/tooling/bb_abstraction_leaks/build.rs index f9effd5d991..0bd2b1c076a 100644 --- a/noir/noir-repo/tooling/bb_abstraction_leaks/build.rs +++ b/noir/noir-repo/tooling/bb_abstraction_leaks/build.rs @@ -10,7 +10,7 @@ use const_format::formatcp; const USERNAME: &str = "AztecProtocol"; const REPO: &str = "aztec-packages"; -const VERSION: &str = "0.24.0"; +const VERSION: &str = "0.26.3"; const TAG: &str = formatcp!("aztec-packages-v{}", VERSION); const API_URL: &str = diff --git a/noir/noir-repo/tooling/lsp/src/requests/test_run.rs b/noir/noir-repo/tooling/lsp/src/requests/test_run.rs index 0b88d814265..1844a3d9bf0 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/test_run.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/test_run.rs @@ -84,7 +84,7 @@ fn on_test_run_request_inner( let test_result = run_test( &state.solver, &mut context, - test_function, + &test_function, false, None, &CompileOptions::default(), diff --git a/noir/noir-repo/tooling/nargo/src/ops/test.rs b/noir/noir-repo/tooling/nargo/src/ops/test.rs index 92c09ec889e..8ddcb5cf8d2 100644 --- a/noir/noir-repo/tooling/nargo/src/ops/test.rs +++ b/noir/noir-repo/tooling/nargo/src/ops/test.rs @@ -14,10 +14,16 @@ pub enum TestStatus { CompileError(FileDiagnostic), } +impl TestStatus { + pub fn failed(&self) -> bool { + !matches!(self, TestStatus::Pass) + } +} + pub fn run_test( blackbox_solver: &B, context: &mut Context, - test_function: TestFunction, + test_function: &TestFunction, show_output: bool, foreign_call_resolver_url: Option<&str>, config: &CompileOptions, @@ -45,7 +51,7 @@ pub fn run_test( /// that a constraint was never satisfiable. /// An example of this is the program `assert(false)` /// In that case, we check if the test function should fail, and if so, we return `TestStatus::Pass`. -fn test_status_program_compile_fail(err: CompileError, test_function: TestFunction) -> TestStatus { +fn test_status_program_compile_fail(err: CompileError, test_function: &TestFunction) -> TestStatus { // The test has failed compilation, but it should never fail. Report error. if !test_function.should_fail() { return TestStatus::CompileError(err.into()); @@ -70,7 +76,7 @@ fn test_status_program_compile_fail(err: CompileError, test_function: TestFuncti /// We now check whether execution passed/failed and whether it should have /// passed/failed to determine the test status. fn test_status_program_compile_pass( - test_function: TestFunction, + test_function: &TestFunction, debug: DebugInfo, circuit_execution: Result, ) -> TestStatus { @@ -109,7 +115,7 @@ fn test_status_program_compile_pass( } fn check_expected_failure_message( - test_function: TestFunction, + test_function: &TestFunction, failed_assertion: Option, error_diagnostic: Option, ) -> TestStatus { diff --git a/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs b/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs index 68660d62d23..2828aaf01eb 100644 --- a/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs @@ -5,17 +5,18 @@ use bn254_blackbox_solver::Bn254BlackBoxSolver; use clap::Args; use fm::FileManager; use nargo::{ - insert_all_files_for_workspace_into_file_manager, - ops::{run_test, TestStatus}, - package::Package, - parse_all, prepare_package, + insert_all_files_for_workspace_into_file_manager, ops::TestStatus, package::Package, parse_all, + prepare_package, }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_driver::{ + check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, +}; use noirc_frontend::{ graph::CrateName, hir::{FunctionNameMatch, ParsedFiles}, }; +use rayon::prelude::{IntoParallelIterator, ParallelBridge, ParallelIterator}; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; use crate::{backends::Backend, cli::check_cmd::check_crate_and_report_errors, errors::CliError}; @@ -82,15 +83,13 @@ pub(crate) fn run( None => FunctionNameMatch::Anything, }; - let blackbox_solver = Bn254BlackBoxSolver::new(); - let test_reports: Vec> = workspace .into_iter() + .par_bridge() .map(|package| { - run_tests( + run_tests::( &workspace_file_manager, &parsed_files, - &blackbox_solver, package, pattern, args.show_output, @@ -116,24 +115,95 @@ pub(crate) fn run( }; } - if test_report.iter().any(|(_, status)| matches!(status, TestStatus::Fail { .. })) { + if test_report.iter().any(|(_, status)| status.failed()) { Err(CliError::Generic(String::new())) } else { Ok(()) } } -#[allow(clippy::too_many_arguments)] -fn run_tests( +fn run_tests( file_manager: &FileManager, parsed_files: &ParsedFiles, - blackbox_solver: &S, package: &Package, fn_name: FunctionNameMatch, show_output: bool, foreign_call_resolver_url: Option<&str>, compile_options: &CompileOptions, ) -> Result, CliError> { + let test_functions = + get_tests_in_package(file_manager, parsed_files, package, fn_name, compile_options)?; + + let count_all = test_functions.len(); + + let plural = if count_all == 1 { "" } else { "s" }; + println!("[{}] Running {count_all} test function{plural}", package.name); + + let test_report: Vec<(String, TestStatus)> = test_functions + .into_par_iter() + .map(|test_name| { + let status = run_test::( + file_manager, + parsed_files, + package, + &test_name, + show_output, + foreign_call_resolver_url, + compile_options, + ); + + (test_name, status) + }) + .collect(); + + display_test_report(file_manager, package, compile_options, &test_report)?; + Ok(test_report) +} + +fn run_test( + file_manager: &FileManager, + parsed_files: &ParsedFiles, + package: &Package, + fn_name: &str, + show_output: bool, + foreign_call_resolver_url: Option<&str>, + compile_options: &CompileOptions, +) -> TestStatus { + // This is really hacky but we can't share `Context` or `S` across threads. + // We then need to construct a separate copy for each test. + + let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); + check_crate( + &mut context, + crate_id, + compile_options.deny_warnings, + compile_options.disable_macros, + ) + .expect("Any errors should have occurred when collecting test functions"); + + let test_functions = context + .get_all_test_functions_in_crate_matching(&crate_id, FunctionNameMatch::Exact(fn_name)); + let (_, test_function) = test_functions.first().expect("Test function should exist"); + + let blackbox_solver = S::default(); + + nargo::ops::run_test( + &blackbox_solver, + &mut context, + test_function, + show_output, + foreign_call_resolver_url, + compile_options, + ) +} + +fn get_tests_in_package( + file_manager: &FileManager, + parsed_files: &ParsedFiles, + package: &Package, + fn_name: FunctionNameMatch, + compile_options: &CompileOptions, +) -> Result, CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); check_crate_and_report_errors( &mut context, @@ -143,30 +213,27 @@ fn run_tests( compile_options.silence_warnings, )?; - let test_functions = context.get_all_test_functions_in_crate_matching(&crate_id, fn_name); - let count_all = test_functions.len(); - - let plural = if count_all == 1 { "" } else { "s" }; - println!("[{}] Running {count_all} test function{plural}", package.name); + Ok(context + .get_all_test_functions_in_crate_matching(&crate_id, fn_name) + .into_iter() + .map(|(test_name, _)| test_name) + .collect()) +} +fn display_test_report( + file_manager: &FileManager, + package: &Package, + compile_options: &CompileOptions, + test_report: &[(String, TestStatus)], +) -> Result<(), CliError> { let writer = StandardStream::stderr(ColorChoice::Always); let mut writer = writer.lock(); - let mut test_report: Vec<(String, TestStatus)> = Vec::new(); - for (test_name, test_function) in test_functions { + for (test_name, test_status) in test_report { write!(writer, "[{}] Testing {test_name}... ", package.name) .expect("Failed to write to stderr"); writer.flush().expect("Failed to flush writer"); - let test_status = run_test( - blackbox_solver, - &mut context, - test_function, - show_output, - foreign_call_resolver_url, - compile_options, - ); - match &test_status { TestStatus::Pass { .. } => { writer @@ -181,7 +248,7 @@ fn run_tests( writeln!(writer, "FAIL\n{message}\n").expect("Failed to write to stderr"); if let Some(diag) = error_diagnostic { noirc_errors::reporter::report_all( - context.file_manager.as_file_map(), + file_manager.as_file_map(), &[diag.clone()], compile_options.deny_warnings, compile_options.silence_warnings, @@ -190,23 +257,21 @@ fn run_tests( } TestStatus::CompileError(err) => { noirc_errors::reporter::report_all( - context.file_manager.as_file_map(), + file_manager.as_file_map(), &[err.clone()], compile_options.deny_warnings, compile_options.silence_warnings, ); } } - - test_report.push((test_name, test_status)); - writer.reset().expect("Failed to reset writer"); } write!(writer, "[{}] ", package.name).expect("Failed to write to stderr"); - let count_failed = - test_report.iter().filter(|(_, status)| !matches!(status, TestStatus::Pass)).count(); + let count_all = test_report.len(); + let count_failed = test_report.iter().filter(|(_, status)| status.failed()).count(); + let plural = if count_all == 1 { "" } else { "s" }; if count_failed == 0 { writer.set_color(ColorSpec::new().set_fg(Some(Color::Green))).expect("Failed to set color"); write!(writer, "{count_all} test{plural} passed").expect("Failed to write to stderr"); @@ -231,5 +296,5 @@ fn run_tests( writer.reset().expect("Failed to reset writer"); } - Ok(test_report) + Ok(()) } diff --git a/noir/noir-repo/tooling/noir_js_backend_barretenberg/package.json b/noir/noir-repo/tooling/noir_js_backend_barretenberg/package.json index 28c3609fd14..06c034725e3 100644 --- a/noir/noir-repo/tooling/noir_js_backend_barretenberg/package.json +++ b/noir/noir-repo/tooling/noir_js_backend_barretenberg/package.json @@ -42,7 +42,7 @@ "lint": "NODE_NO_WARNINGS=1 eslint . --ext .ts --ignore-path ./.eslintignore --max-warnings 0" }, "dependencies": { - "@aztec/bb.js": "0.24.0", + "@aztec/bb.js": "0.26.3", "@noir-lang/types": "workspace:*", "fflate": "^0.8.0" }, diff --git a/noir/noir-repo/yarn.lock b/noir/noir-repo/yarn.lock index f5f3a29f08a..49485193532 100644 --- a/noir/noir-repo/yarn.lock +++ b/noir/noir-repo/yarn.lock @@ -221,9 +221,9 @@ __metadata: languageName: node linkType: hard -"@aztec/bb.js@npm:0.24.0": - version: 0.24.0 - resolution: "@aztec/bb.js@npm:0.24.0" +"@aztec/bb.js@npm:0.26.3": + version: 0.26.3 + resolution: "@aztec/bb.js@npm:0.26.3" dependencies: comlink: ^4.4.1 commander: ^10.0.1 @@ -231,7 +231,7 @@ __metadata: tslib: ^2.4.0 bin: bb.js: dest/node/main.js - checksum: a086dabf30084cfa526e512148b9c02f0a0770dcc19b7dca4af9a3e98612b716acc7eaac6b52c0f12d985932e866d1cb9e534ded6ac9d747f3dd021afe25de27 + checksum: 74c2b7ef5405f56472cf7c41d1c13261df07b1d5019e3ede9b63d218378e0fb73ccf5c52f1cc524505efad5799b347b497349d7c9b6fe82286014b1574f12309 languageName: node linkType: hard @@ -4396,7 +4396,7 @@ __metadata: version: 0.0.0-use.local resolution: "@noir-lang/backend_barretenberg@workspace:tooling/noir_js_backend_barretenberg" dependencies: - "@aztec/bb.js": 0.24.0 + "@aztec/bb.js": 0.26.3 "@noir-lang/types": "workspace:*" "@types/node": ^20.6.2 "@types/prettier": ^3