fix: warn on and discard unreachable statements after break and continue

compiler/noirc_frontend/src/elaborator/expressions.rs

@@ -151,13 +151,25 @@ impl Elaborator<'_> {
         let statements_len = block.statements.len();
         let mut statements = Vec::with_capacity(statements_len);
 
+        // If we found a break or continue statement, this holds its location (only for the first one)
+        let mut break_or_continue_location = None;
+        // When encountering a statement after a break or continue we'll error saying it's unreachable,
+        // but we only want to error for the first statement.
+        let mut errored_unreachable = false;
+
         for (i, statement) in block.statements.into_iter().enumerate() {
+            let location = statement.location;
             let statement_target_type = if i == statements_len - 1 { target_type } else { None };
             let (id, stmt_type) =
                 self.elaborate_statement_with_target_type(statement, statement_target_type);
-            statements.push(id);
 
-            if let HirStatement::Semi(expr) = self.interner.statement(&id) {
+            if break_or_continue_location.is_none() {
+                statements.push(id);
+            }
+
+            let stmt = self.interner.statement(&id);
+
+            if let HirStatement::Semi(expr) = stmt {
                 let inner_expr_type = self.interner.id_type(expr);
                 let location = self.interner.expr_location(&expr);
 
@@ -167,7 +179,18 @@ impl Elaborator<'_> {
                 });
             }
 
-            if i + 1 == statements.len() {
+            if let Some(break_or_continue_location) = break_or_continue_location {
+                if !errored_unreachable {
+                    self.push_err(ResolverError::UnreachableStatement {
+                        location,
+                        break_or_continue_location,
+                    });
+                    errored_unreachable = true;
+                }
+            } else if matches!(stmt, HirStatement::Break | HirStatement::Continue) {
+                break_or_continue_location = Some(location);
+                block_type = stmt_type;
+            } else if i + 1 == statements.len() {
                 block_type = stmt_type;
             }
         }

compiler/noirc_frontend/src/hir/resolution/errors.rs

@@ -200,6 +200,8 @@ pub enum ResolverError {
         "The type parameter `{ident}` is not constrained by the impl trait, self type, or predicates"
     )]
     UnconstrainedTypeParameter { ident: Ident },
+    #[error("Unreachable statement")]
+    UnreachableStatement { location: Location, break_or_continue_location: Location },
 }
 
 impl ResolverError {
@@ -268,7 +270,8 @@ impl ResolverError {
             | ResolverError::NoPredicatesAttributeOnUnconstrained { location, .. }
             | ResolverError::FoldAttributeOnUnconstrained { location, .. }
             | ResolverError::OracleMarkedAsConstrained { location, .. }
-            | ResolverError::LowLevelFunctionOutsideOfStdlib { location } => *location,
+            | ResolverError::LowLevelFunctionOutsideOfStdlib { location }
+            | ResolverError::UnreachableStatement { location, .. } => *location,
             ResolverError::UnusedVariable { ident }
             | ResolverError::UnusedItem { ident, .. }
             | ResolverError::DuplicateField { field: ident }
@@ -827,6 +830,15 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
                     ident.location(),
                 )
             }
+            ResolverError::UnreachableStatement { location, break_or_continue_location} => {
+                let mut diagnostic = Diagnostic::simple_warning(
+                    "Unreachable statement".to_string(),
+                    "Unreachable statement".to_string(),
+                    *location,
+                );
+                diagnostic.add_secondary("Any code following this expression is unreachable".to_string(), *break_or_continue_location);
+                diagnostic
+            }
         }
     }
 }

compiler/noirc_frontend/src/tests.rs

@@ -284,7 +284,7 @@
 
     let chars = line.chars().collect::<Vec<_>>();
     let first_caret = chars.iter().position(|c| *c == char).unwrap();
     let last_caret = chars.iter().rposition(|c| *c == char).unwrap();
     let start = byte - last_line_length;
     let span = Span::from((start + first_caret - 1) as u32..(start + last_caret) as u32);
     let error = line.trim().trim_start_matches(char).trim().to_string();
@@ -1344,9 +1344,11 @@
 #[test]
 fn break_and_continue_outside_loop() {
     let src = r#"
-        unconstrained fn main() {
+        pub unconstrained fn foo() {
             continue;
             ^^^^^^^^^ continue is only allowed within loops
+        }
+        pub unconstrained fn bar() {
             break;
             ^^^^^^ break is only allowed within loops
         }
@@ -3231,7 +3233,7 @@
     // wouldn't panic due to infinite recursion, but the errors asserted here
     // come from the compilation checks, which does static analysis to catch the
     // problem before it even has a chance to cause a panic.
     let srcs = vec![
         r#"
         fn main() {
            ^^^^ function `main` cannot return without recursing
@@ -3313,7 +3315,7 @@
         "#,
     ];
 
     for (index, src) in srcs.into_iter().enumerate() {
         check_errors(src, Some(&format!("{}_{index}", function_path!())));
     }
 }
@@ -3321,7 +3323,7 @@
 #[named]
 #[test]
 fn unconditional_recursion_pass() {
     let srcs = vec![
         r#"
         fn main() {
             if false { main(); }
@@ -3363,7 +3365,7 @@
         "#,
     ];
 
     for (index, src) in srcs.into_iter().enumerate() {
         assert_no_errors(src, &format!("{}_{index}", function_path!()));
     }
 }
@@ -4403,12 +4405,12 @@
             let mut array = [1, 2, 3];
             borrow(&array);
                    ^^^^^^ This requires the unstable feature 'ownership' which is not enabled
                    ~~~~~~ Pass -Zownership to nargo to enable this feature at your own risk.
         }
 
         fn borrow(_array: &[Field; 3]) {}
                           ^^^^^^^^^^^ This requires the unstable feature 'ownership' which is not enabled
                           ~~~~~~~~~~~ Pass -Zownership to nargo to enable this feature at your own risk.
     "#;
     check_errors!(src);
 }

test_programs/compile_failure/noirc_frontend_tests_break_and_continue_outside_loop/src/main.nr

@@ -1,6 +1,8 @@
 
-        unconstrained fn main() {
+        pub unconstrained fn foo() {
             continue;
+        }
+        pub unconstrained fn bar() {
             break;
         }
 

test_programs/compile_failure/noirc_frontend_tests_break_and_continue_outside_loop/src_hash.txt

@@ -1 +1 @@
-15565082428725147447
+3791504174465283486

test_programs/compile_failure/noirc_frontend_tests_break_and_continue_outside_loop/stderr.txt

@@ -6,9 +6,9 @@ error: continue is only allowed within loops
   │
 
 error: break is only allowed within loops
-  ┌─ src/main.nr:4:13
+  ┌─ src/main.nr:6:13
   │
-4 │             break;
+6 │             break;
   │             ------
   │
 

test_programs/execution_success/brillig_continue_break/Nargo.toml

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

test_programs/execution_success/brillig_continue_break/src/main.nr

@@ -0,0 +1,22 @@
+fn main() {
+    // Safety: test program
+    let bug = unsafe { foo() };
+    assert(!bug);
+}
+
+unconstrained fn foo() -> bool {
+    let mut i = 0;
+    let mut bug = false;
+    loop {
+        if i == 3 {
+            break;
+            bug = true;
+        } else if i == 2 {
+            i += 1;
+            continue;
+            bug = true;
+        }
+        i += 1;
+    }
+    bug
+}