fix: stop inserting instructions after break and continue

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<Function>,
     call_stack: CallStackId,
     error_types: BTreeMap<ErrorSelector, HirType>,
@@ -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<Vec<Type>>,
     ) -> 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

compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs

@@ -336,6 +336,12 @@
         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)
     }
@@ -520,7 +526,7 @@
     ///   br loop_entry(v0)
     /// loop_entry(i: Field):
     ///   v2 = lt i v1
     ///   brif v2, then: loop_body, else: loop_end
     /// loop_body():
     ///   v3 = ... codegen body ...
     ///   v4 = add 1, i
@@ -576,9 +582,14 @@
         // 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);
@@ -665,7 +676,7 @@
     ///
     /// ```text
     ///   v0 = ... codegen cond ...
     ///   brif v0, then: then_block, else: else_block
     /// then_block():
     ///   v1 = ... codegen a ...
     ///   br end_if(v1)
@@ -680,7 +691,7 @@
     ///
     /// ```text
     ///   v0 = ... codegen cond ...
     ///   brif v0, then: then_block, else: end_if
     /// then_block:
     ///   v1 = ... codegen a ...
     ///   br end_if()
@@ -1087,9 +1098,14 @@
         // 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 @@
     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 @@
         } else {
             self.builder.terminate_with_jmp(loop_.loop_entry, vec![]);
         }
+
+        self.builder.close_block();
+
         Self::unit_value()
     }
 

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 };
             }
         }
 

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]

test_programs/compile_failure/break_in_infix/src/main.nr

@@ -0,0 +1,3 @@
+unconstrained fn main() {
+    let _ = 1 + { break; };
+}

test_programs/compile_success_empty/unconditional_break/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "unconditional_break"
+type = "bin"
+authors = [""]
+
+[dependencies]

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
+}

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