diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index cfcb80c212a..24871208db6 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -3,6 +3,7 @@ pub mod data_bus; use std::{borrow::Cow, collections::BTreeMap, sync::Arc}; use acvm::{FieldElement, acir::circuit::ErrorSelector}; +use fxhash::FxHashSet as HashSet; use noirc_errors::{ Location, call_stack::{CallStack, CallStackId}, @@ -40,6 +41,8 @@ use super::{ pub struct FunctionBuilder { pub current_function: Function, current_block: BasicBlockId, + current_block_closed: bool, + closed_blocked: HashSet<(FunctionId, BasicBlockId)>, finished_functions: Vec, call_stack: CallStackId, error_types: BTreeMap, @@ -61,6 +64,8 @@ impl FunctionBuilder { let new_function = Function::new(function_name, function_id); Self { current_block: new_function.entry_block(), + current_block_closed: false, + closed_blocked: HashSet::default(), current_function: new_function, finished_functions: Vec::new(), call_stack: CallStackId::root(), @@ -126,7 +131,8 @@ impl FunctionBuilder { let call_stack = self.current_function.dfg.get_call_stack(self.call_stack); let mut new_function = Function::new(name, function_id); new_function.set_runtime(runtime_type); - self.current_block = new_function.entry_block(); + self.switch_to_block(new_function.entry_block()); + let old_function = std::mem::replace(&mut self.current_function, new_function); // Copy the call stack to the new function self.call_stack = @@ -197,6 +203,17 @@ impl FunctionBuilder { self.current_function.dfg.make_block() } + /// Prevents any further instructions from being added to the current block. + /// That is, calls to add instructions can be called, but they will have no effect. + pub fn close_block(&mut self) { + self.closed_blocked.insert((self.current_function.id(), self.current_block)); + self.current_block_closed = true; + } + + pub fn current_block_is_closed(&self) -> bool { + self.current_block_closed + } + /// Adds a parameter with the given type to the given block. /// Returns the newly-added parameter. pub fn add_block_parameter(&mut self, block: BasicBlockId, typ: Type) -> ValueId { @@ -214,7 +231,12 @@ impl FunctionBuilder { instruction: Instruction, ctrl_typevars: Option>, ) -> InsertInstructionResult { + if self.current_block_closed { + return InsertInstructionResult::InstructionRemoved; + } + let block = self.current_block(); + if self.simplify { self.current_function.dfg.insert_instruction_and_results( instruction, @@ -237,6 +259,8 @@ impl FunctionBuilder { /// instructions into a new function, call new_function instead. pub fn switch_to_block(&mut self, block: BasicBlockId) { self.current_block = block; + self.current_block_closed = + self.closed_blocked.contains(&(self.current_function.id(), block)); } /// Returns the block currently being inserted into diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 2b27048bd39..c786bba3a4b 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -336,6 +336,12 @@ impl FunctionContext<'_> { let mut result = Self::unit_value(); for expr in block { result = self.codegen_expression(expr)?; + + // A break or continue might happen in a block, in which case we must + // not codegen any further expressions. + if self.builder.current_block_is_closed() { + break; + } } Ok(result) } @@ -576,9 +582,14 @@ impl FunctionContext<'_> { // Compile the loop body self.builder.switch_to_block(loop_body); self.define(for_expr.index_variable, loop_index.into()); + self.codegen_expression(&for_expr.block)?; - let new_loop_index = self.make_offset(loop_index, 1); - self.builder.terminate_with_jmp(loop_entry, vec![new_loop_index]); + + if !self.builder.current_block_is_closed() { + // No need to jump if the current block is already closed + let new_loop_index = self.make_offset(loop_index, 1); + self.builder.terminate_with_jmp(loop_entry, vec![new_loop_index]); + } // Finish by switching back to the end of the loop self.builder.switch_to_block(loop_end); @@ -1087,9 +1098,14 @@ impl FunctionContext<'_> { // Evaluate the rhs first - when we load the expression in the lvalue we want that // to reflect any mutations from evaluating the rhs. let rhs = self.codegen_expression(&assign.expression)?; - let lhs = self.extract_current_value(&assign.lvalue)?; - self.assign_new_value(lhs, rhs); + // Can't assign to a variable if the expression had an unconditional break in it + if !self.builder.current_block_is_closed() { + let lhs = self.extract_current_value(&assign.lvalue)?; + + self.assign_new_value(lhs, rhs); + } + Ok(Self::unit_value()) } @@ -1101,6 +1117,9 @@ impl FunctionContext<'_> { fn codegen_break(&mut self) -> Values { let loop_end = self.current_loop().loop_end; self.builder.terminate_with_jmp(loop_end, Vec::new()); + + self.builder.close_block(); + Self::unit_value() } @@ -1114,6 +1133,9 @@ impl FunctionContext<'_> { } else { self.builder.terminate_with_jmp(loop_.loop_entry, vec![]); } + + self.builder.close_block(); + Self::unit_value() } diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 108f26c8dc8..2a263070281 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -166,9 +166,7 @@ impl Elaborator<'_> { let (id, stmt_type) = self.elaborate_statement_with_target_type(statement, statement_target_type); - if break_or_continue_location.is_none() { - statements.push(id); - } + statements.push(id); let stmt = self.interner.statement(&id); @@ -182,6 +180,8 @@ impl Elaborator<'_> { }); } + let is_break_or_continue = matches!(stmt, HirStatement::Break | HirStatement::Continue); + if let Some(break_or_continue_location) = break_or_continue_location { if !errored_unreachable { self.push_err(ResolverError::UnreachableStatement { @@ -190,11 +190,12 @@ impl Elaborator<'_> { }); errored_unreachable = true; } - } else if matches!(stmt, HirStatement::Break | HirStatement::Continue) { + } else if is_break_or_continue { break_or_continue_location = Some(location); - block_type = stmt_type; - } else if i + 1 == statements.len() { - block_type = stmt_type; + } + + if i + 1 == statements.len() { + block_type = if is_break_or_continue { Type::Unit } else { stmt_type }; } } diff --git a/test_programs/compile_failure/break_in_infix/Nargo.toml b/test_programs/compile_failure/break_in_infix/Nargo.toml new file mode 100644 index 00000000000..e632834352f --- /dev/null +++ b/test_programs/compile_failure/break_in_infix/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "break_in_infix" +type = "bin" +authors = [""] +compiler_version = ">=0.33.0" + +[dependencies] diff --git a/test_programs/compile_failure/break_in_infix/src/main.nr b/test_programs/compile_failure/break_in_infix/src/main.nr new file mode 100644 index 00000000000..d919b09a6c8 --- /dev/null +++ b/test_programs/compile_failure/break_in_infix/src/main.nr @@ -0,0 +1,3 @@ +unconstrained fn main() { + let _ = 1 + { break; }; +} diff --git a/test_programs/compile_success_empty/unconditional_break/Nargo.toml b/test_programs/compile_success_empty/unconditional_break/Nargo.toml new file mode 100644 index 00000000000..88fb764197f --- /dev/null +++ b/test_programs/compile_success_empty/unconditional_break/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "unconditional_break" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/compile_success_empty/unconditional_break/src/main.nr b/test_programs/compile_success_empty/unconditional_break/src/main.nr new file mode 100644 index 00000000000..29aaac736d4 --- /dev/null +++ b/test_programs/compile_success_empty/unconditional_break/src/main.nr @@ -0,0 +1,16 @@ +fn main() { + // Safety: + let x = unsafe { func_1() }; + assert(x); +} + +unconstrained fn func_1() -> bool { + let mut x = true; + loop { + if true { + break; + } + x = false; + } + x +} diff --git a/tooling/ast_fuzzer/src/program/tests.rs b/tooling/ast_fuzzer/src/program/tests.rs index e65a748ce36..d8d1553f45c 100644 --- a/tooling/ast_fuzzer/src/program/tests.rs +++ b/tooling/ast_fuzzer/src/program/tests.rs @@ -91,7 +91,6 @@ fn test_modulo_of_negative_literals_in_range() { v3 = lt v0, i64 18446744073709551615 jmpif v3 then: b2, else: b3 b2(): - v5 = unchecked_add v0, i64 1 jmp b3() b3(): return diff --git a/tooling/nargo_cli/tests/snapshots/compile_failure/break_in_infix/execute__tests__stderr.snap b/tooling/nargo_cli/tests/snapshots/compile_failure/break_in_infix/execute__tests__stderr.snap new file mode 100644 index 00000000000..614321e22b5 --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/compile_failure/break_in_infix/execute__tests__stderr.snap @@ -0,0 +1,19 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: stderr +--- +error: break is only allowed within loops + ┌─ src/main.nr:2:19 + │ +2 │ let _ = 1 + { break; }; + │ ------ + │ + +error: Types in a binary operation should match, but found Field and () + ┌─ src/main.nr:2:13 + │ +2 │ let _ = 1 + { break; }; + │ -------------- + │ + +Aborting due to 2 previous errors diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_empty/brillig_continue_break/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/compile_success_empty/brillig_continue_break/execute__tests__expanded.snap index f537bd8d8e9..d789eb1fdce 100644 --- a/tooling/nargo_cli/tests/snapshots/compile_success_empty/brillig_continue_break/execute__tests__expanded.snap +++ b/tooling/nargo_cli/tests/snapshots/compile_success_empty/brillig_continue_break/execute__tests__expanded.snap @@ -14,9 +14,11 @@ unconstrained fn foo() -> bool { loop { if i == 3 { break; + bug = true; } else if i == 2 { i = i + 1; continue; + bug = true; }; i = i + 1; } diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__expanded.snap new file mode 100644 index 00000000000..d647521eb64 --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__expanded.snap @@ -0,0 +1,18 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: expanded_code +--- +fn main() { + // Safety: comment added by `nargo expand` + let x: bool = unsafe { func_1() }; + assert(x); +} + +unconstrained fn func_1() -> bool { + let mut x: bool = true; + loop { + if true { break; }; + x = false; + } + x +} diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__force_brillig_false_inliner_0.snap b/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__force_brillig_false_inliner_0.snap new file mode 100644 index 00000000000..7de940827c8 --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__force_brillig_false_inliner_0.snap @@ -0,0 +1,26 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: artifact +--- +{ + "noir_version": "[noir_version]", + "hash": "[hash]", + "abi": { + "parameters": [], + "return_type": null, + "error_types": {} + }, + "bytecode": [ + "func 0", + "current witness index : _0", + "private parameters indices : []", + "public parameters indices : []", + "return value indices : []" + ], + "debug_symbols": "XY5BCsQwCEXv4rqLWfcqw1BsaosgJtikMITefWyYQOlK/3/6tcJCc9km1jXuML4rzMYivE0SA2aO6m49B+hyykbkFty4byU00gyjFpEBDpTShvaE2mpGc/oagHTx6oErC13d+XGBge158UBjnIX+ci0abjR/Uyf942Qx0FKMrqTGPPsH", + "file_map": {}, + "names": [ + "main" + ], + "brillig_names": [] +}