feat: loop must have at least one break (#7126)

compiler/noirc_frontend/src/ast/statement.rs

@@ -46,7 +46,7 @@ pub enum StatementKind {
     Expression(Expression),
     Assign(AssignStatement),
     For(ForLoopStatement),
-    Loop(Expression),
+    Loop(Expression, Span /* loop keyword span */),
     Break,
     Continue,
     /// This statement should be executed at compile-time
@@ -972,7 +972,7 @@ impl Display for StatementKind {
             StatementKind::Expression(expression) => expression.fmt(f),
             StatementKind::Assign(assign) => assign.fmt(f),
             StatementKind::For(for_loop) => for_loop.fmt(f),
-            StatementKind::Loop(block) => write!(f, "loop {}", block),
+            StatementKind::Loop(block, _) => write!(f, "loop {}", block),
             StatementKind::Break => write!(f, "break"),
             StatementKind::Continue => write!(f, "continue"),
             StatementKind::Comptime(statement) => write!(f, "comptime {}", statement.kind),

compiler/noirc_frontend/src/ast/visitor.rs

@@ -1135,7 +1135,7 @@ impl Statement {
             StatementKind::For(for_loop_statement) => {
                 for_loop_statement.accept(visitor);
             }
-            StatementKind::Loop(block) => {
+            StatementKind::Loop(block, _) => {
                 if visitor.visit_loop_statement(block) {
                     block.accept(visitor);
                 }

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -94,6 +94,11 @@ enum UnsafeBlockStatus {
     InUnsafeBlockWithConstrainedCalls,
 }
 
+pub struct Loop {
+    pub is_for: bool,
+    pub has_break: bool,
+}
+
 pub struct Elaborator<'context> {
     scopes: ScopeForest,
 
@@ -107,7 +112,7 @@ pub struct Elaborator<'context> {
     pub(crate) file: FileId,
 
     unsafe_block_status: UnsafeBlockStatus,
-    nested_loops: usize,
+    current_loop: Option<Loop>,
 
     /// Contains a mapping of the current struct or functions's generics to
     /// unique type variables if we're resolving a struct. Empty otherwise.
@@ -230,7 +235,7 @@ impl<'context> Elaborator<'context> {
             crate_graph,
             file: FileId::dummy(),
             unsafe_block_status: UnsafeBlockStatus::NotInUnsafeBlock,
-            nested_loops: 0,
+            current_loop: None,
             generics: Vec::new(),
             lambda_stack: Vec::new(),
             self_type: None,

compiler/noirc_frontend/src/elaborator/statements.rs

@@ -24,7 +24,7 @@ use crate::{
     StructType, Type,
 };
 
-use super::{lints, Elaborator};
+use super::{lints, Elaborator, Loop};
 
 impl<'context> Elaborator<'context> {
     fn elaborate_statement_value(&mut self, statement: Statement) -> (HirStatement, Type) {
@@ -33,7 +33,7 @@ impl<'context> Elaborator<'context> {
             StatementKind::Constrain(constrain) => self.elaborate_constrain(constrain),
             StatementKind::Assign(assign) => self.elaborate_assign(assign),
             StatementKind::For(for_stmt) => self.elaborate_for(for_stmt),
-            StatementKind::Loop(block) => self.elaborate_loop(block, statement.span),
+            StatementKind::Loop(block, span) => self.elaborate_loop(block, span),
             StatementKind::Break => self.elaborate_jump(true, statement.span),
             StatementKind::Continue => self.elaborate_jump(false, statement.span),
             StatementKind::Comptime(statement) => self.elaborate_comptime_statement(*statement),
@@ -227,7 +227,9 @@ impl<'context> Elaborator<'context> {
         let (end_range, end_range_type) = self.elaborate_expression(end);
         let (identifier, block) = (for_loop.identifier, for_loop.block);
 
-        self.nested_loops += 1;
+        let old_loop = std::mem::take(&mut self.current_loop);
+
+        self.current_loop = Some(Loop { is_for: true, has_break: false });
         self.push_scope();
 
         // TODO: For loop variables are currently mutable by default since we haven't
@@ -261,7 +263,7 @@ impl<'context> Elaborator<'context> {
         let (block, _block_type) = self.elaborate_expression(block);
 
         self.pop_scope();
-        self.nested_loops -= 1;
+        self.current_loop = old_loop;
 
         let statement =
             HirStatement::For(HirForStatement { start_range, end_range, block, identifier });
@@ -279,13 +281,19 @@ impl<'context> Elaborator<'context> {
             self.push_err(ResolverError::LoopInConstrainedFn { span });
         }
 
-        self.nested_loops += 1;
+        let old_loop = std::mem::take(&mut self.current_loop);
+        self.current_loop = Some(Loop { is_for: false, has_break: false });
         self.push_scope();
 
         let (block, _block_type) = self.elaborate_expression(block);
 
         self.pop_scope();
-        self.nested_loops -= 1;
+
+        let last_loop =
+            std::mem::replace(&mut self.current_loop, old_loop).expect("Expected a loop");
+        if !last_loop.has_break {
+            self.push_err(ResolverError::LoopWithoutBreak { span });
+        }
 
         let statement = HirStatement::Loop(block);
 
@@ -298,7 +306,12 @@ impl<'context> Elaborator<'context> {
         if in_constrained_function {
             self.push_err(ResolverError::JumpInConstrainedFn { is_break, span });
         }
-        if self.nested_loops == 0 {
+
+        if let Some(current_loop) = &mut self.current_loop {
+            if is_break {
+                current_loop.has_break = true;
+            }
+        } else {
             self.push_err(ResolverError::JumpOutsideLoop { is_break, span });
         }
 

compiler/noirc_frontend/src/hir/comptime/display.rs

@@ -732,8 +732,8 @@ fn remove_interned_in_statement_kind(
             block: remove_interned_in_expression(interner, for_loop.block),
             ..for_loop
         }),
-        StatementKind::Loop(block) => {
-            StatementKind::Loop(remove_interned_in_expression(interner, block))
+        StatementKind::Loop(block, span) => {
+            StatementKind::Loop(remove_interned_in_expression(interner, block), span)
         }
         StatementKind::Comptime(statement) => {
             StatementKind::Comptime(Box::new(remove_interned_in_statement(interner, *statement)))

compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs

@@ -59,7 +59,7 @@ impl HirStatement {
                 block: for_stmt.block.to_display_ast(interner),
                 span,
             }),
-            HirStatement::Loop(block) => StatementKind::Loop(block.to_display_ast(interner)),
+            HirStatement::Loop(block) => StatementKind::Loop(block.to_display_ast(interner), span),
             HirStatement::Break => StatementKind::Break,
             HirStatement::Continue => StatementKind::Continue,
             HirStatement::Expression(expr) => {

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

@@ -98,8 +98,10 @@ pub enum ResolverError {
     DependencyCycle { span: Span, item: String, cycle: String },
     #[error("break/continue are only allowed in unconstrained functions")]
     JumpInConstrainedFn { is_break: bool, span: Span },
-    #[error("loop is only allowed in unconstrained functions")]
+    #[error("`loop` is only allowed in unconstrained functions")]
     LoopInConstrainedFn { span: Span },
+    #[error("`loop` must have at least one `break` in it")]
+    LoopWithoutBreak { span: Span },
     #[error("break/continue are only allowed within loops")]
     JumpOutsideLoop { is_break: bool, span: Span },
     #[error("Only `comptime` globals can be mutable")]
@@ -443,6 +445,13 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
                     *span,
                 )
             },
+            ResolverError::LoopWithoutBreak { span } => {
+                Diagnostic::simple_error(
+                    "`loop` must have at least one `break` in it".into(),
+                    "Infinite loops are disallowed".into(),
+                    *span,
+                )
+            },
             ResolverError::JumpOutsideLoop { is_break, span } => {
                 let item = if *is_break { "break" } else { "continue" };
                 Diagnostic::simple_error(

compiler/noirc_frontend/src/parser/parser/statement.rs

@@ -157,8 +157,8 @@ impl<'a> Parser<'a> {
             return Some(StatementKind::For(for_loop));
         }
 
-        if let Some(block) = self.parse_loop() {
-            return Some(StatementKind::Loop(block));
+        if let Some((block, span)) = self.parse_loop() {
+            return Some(StatementKind::Loop(block, span));
         }
 
         if let Some(kind) = self.parse_if_expr() {
@@ -293,7 +293,7 @@ impl<'a> Parser<'a> {
     }
 
     /// LoopStatement = 'loop' Block
-    fn parse_loop(&mut self) -> Option<Expression> {
+    fn parse_loop(&mut self) -> Option<(Expression, Span)> {
         let start_span = self.current_token_span;
         if !self.eat_keyword(Keyword::Loop) {
             return None;
@@ -312,7 +312,7 @@ impl<'a> Parser<'a> {
             Expression { kind: ExpressionKind::Error, span: self.span_since(block_start_span) }
         };
 
-        Some(block)
+        Some((block, start_span))
     }
 
     /// ForRange
@@ -824,21 +824,23 @@ mod tests {
         let src = "loop { }";
         let mut parser = Parser::for_str(src);
         let statement = parser.parse_statement_or_error();
-        let StatementKind::Loop(block) = statement.kind else {
+        let StatementKind::Loop(block, span) = statement.kind else {
             panic!("Expected loop");
         };
         let ExpressionKind::Block(block) = block.kind else {
             panic!("Expected block");
         };
         assert!(block.statements.is_empty());
+        assert_eq!(span.start(), 0);
+        assert_eq!(span.end(), 4);
     }
 
     #[test]
     fn parses_loop_with_statements() {
         let src = "loop { 1; 2 }";
         let mut parser = Parser::for_str(src);
         let statement = parser.parse_statement_or_error();
-        let StatementKind::Loop(block) = statement.kind else {
+        let StatementKind::Loop(block, _) = statement.kind else {
             panic!("Expected loop");
         };
         let ExpressionKind::Block(block) = block.kind else {

compiler/noirc_frontend/src/tests.rs

@@ -4073,3 +4073,85 @@ fn regression_7088() {
     "#;
     assert_no_errors(src);
 }
+
+#[test]
+fn errors_on_empty_loop_no_break() {
+    let src = r#"
+    fn main() {
+        /// Safety: test
+        unsafe {
+            foo()
+        }
+    }
+
+    unconstrained fn foo() {
+        loop {}
+    }
+    "#;
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+    assert!(matches!(
+        &errors[0].0,
+        CompilationError::ResolverError(ResolverError::LoopWithoutBreak { .. })
+    ));
+}
+
+#[test]
+fn errors_on_loop_without_break() {
+    let src = r#"
+    fn main() {
+        /// Safety: test
+        unsafe {
+            foo()
+        }
+    }
+
+    unconstrained fn foo() {
+        let mut x = 1;
+        loop {
+            x += 1;
+            bar(x);
+        }
+    }
+
+    fn bar(_: Field) {}
+    "#;
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+    assert!(matches!(
+        &errors[0].0,
+        CompilationError::ResolverError(ResolverError::LoopWithoutBreak { .. })
+    ));
+}
+
+#[test]
+fn errors_on_loop_without_break_with_nested_loop() {
+    let src = r#"
+    fn main() {
+        /// Safety: test
+        unsafe {
+            foo()
+        }
+    }
+
+    unconstrained fn foo() {
+        let mut x = 1;
+        loop {
+            x += 1;
+            bar(x);
+            loop {
+                x += 2;
+                break;
+            }
+        }
+    }
+
+    fn bar(_: Field) {}
+    "#;
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+    assert!(matches!(
+        &errors[0].0,
+        CompilationError::ResolverError(ResolverError::LoopWithoutBreak { .. })
+    ));
+}

docs/docs/noir/concepts/control_flow.md

@@ -79,8 +79,9 @@ The iteration variable `i` is still increased by one as normal when `continue` i
 
 ## Loops
 
-In unconstrained code, `loop` is allowed for loops that end with a `break`. This is only allowed
-in unconstrained code since normal constrained code requires that Noir knows exactly how many iterations
+In unconstrained code, `loop` is allowed for loops that end with a `break`. 
+A `loop` must have at least one `break` in it.
+This is only allowed in unconstrained code since normal constrained code requires that Noir knows exactly how many iterations
 a loop may have.
 
 ```rust

tooling/nargo_fmt/src/formatter/statement.rs

@@ -75,7 +75,7 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> {
             StatementKind::For(for_loop_statement) => {
                 group.group(self.format_for_loop(for_loop_statement));
             }
-            StatementKind::Loop(block) => {
+            StatementKind::Loop(block, _) => {
                 group.group(self.format_loop(block));
             }
             StatementKind::Break => {