diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 3a70ff33a35..7695de14d69 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/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), diff --git a/compiler/noirc_frontend/src/ast/visitor.rs b/compiler/noirc_frontend/src/ast/visitor.rs index 5c4781df7a5..d7fe63a6a45 100644 --- a/compiler/noirc_frontend/src/ast/visitor.rs +++ b/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); } diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 0e8850b6543..96ff1597647 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/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, /// 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, diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index a01b24c2f0f..150c6e7de48 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/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 }); } diff --git a/compiler/noirc_frontend/src/hir/comptime/display.rs b/compiler/noirc_frontend/src/hir/comptime/display.rs index ccdfdf00e72..39bcb87e145 100644 --- a/compiler/noirc_frontend/src/hir/comptime/display.rs +++ b/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))) diff --git a/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs b/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs index 71a462c9066..0257ff66ac6 100644 --- a/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs +++ b/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) => { diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 77ba76a0595..f8373080069 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/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( diff --git a/compiler/noirc_frontend/src/parser/parser/statement.rs b/compiler/noirc_frontend/src/parser/parser/statement.rs index 8c6d18d90ef..005216b1deb 100644 --- a/compiler/noirc_frontend/src/parser/parser/statement.rs +++ b/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 { + 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,13 +824,15 @@ 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] @@ -838,7 +840,7 @@ mod tests { 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 { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 087e34fcc64..c2d7e5c5f49 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/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 { .. }) + )); +} diff --git a/docs/docs/noir/concepts/control_flow.md b/docs/docs/noir/concepts/control_flow.md index a11db545e32..3e2d913ec96 100644 --- a/docs/docs/noir/concepts/control_flow.md +++ b/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 diff --git a/tooling/nargo_fmt/src/formatter/statement.rs b/tooling/nargo_fmt/src/formatter/statement.rs index 983fb00db16..0006d24180c 100644 --- a/tooling/nargo_fmt/src/formatter/statement.rs +++ b/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 => {