From 6d9dafc3825bd5db81ce2615de57ee0de5ca4b32 Mon Sep 17 00:00:00 2001 From: Piotr Czarnecki Date: Tue, 25 Apr 2023 14:00:19 +0200 Subject: [PATCH 1/5] Emit an error for the 'return' expression Fixes #1190 --- crates/noirc_frontend/src/parser/parser.rs | 59 +++++++++++++++++++++- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index f4793d0636..1e83223a8b 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -653,8 +653,26 @@ where } fn expression() -> impl ExprParser { - recursive(|expr| expression_with_precedence(Precedence::Lowest, expr, false)) - .labelled("expression") + recursive(|expr| { + choice(( + return_expression(expr.clone()), + expression_with_precedence(Precedence::Lowest, expr, false), + )) + }) + .labelled("expression") +} + +fn return_expression<'a, P>(expr_parser: P) -> impl NoirParser + 'a +where + P: ExprParser + 'a, +{ + keyword(Keyword::Return) + .then(expr_parser) + .validate(|_, span, emit| { + emit(ParserError::with_reason("Early 'return' is unsupported".to_owned(), span)); + Expression::error(span) + }) + .labelled("return expression") } // An expression is a single term followed by 0 or more (OP subexpression)* @@ -1484,4 +1502,41 @@ mod test { ); } } + + #[test] + fn return_validation() { + let cases = vec![ + ("{ return 42 }", 1, "{\n Error\n}"), + ("{ return 1; return 2; }", 2, "{\n Error\n Error;\n}"), + ( + "{ return 123; let foo = 4 + 3; }", + 1, + "{\n Error\n let foo: unspecified = (4 + 3)\n}", + ), + ("{ return return 123; }", 2, "{\n Error;\n}"), + ("{ return 1 + return 2 }", 2, "{\n Error\n}"), + ]; + + let show_errors = |v| vecmap(&v, ToString::to_string).join("\n"); + + let results: Vec<_> = cases.into_iter().map(|(src, expected_errors, expected_result)| { + let (opt, errors) = parse_recover(block(expression()), src); + let actual = opt.map(|ast| ast.to_string()); + let actual = if let Some(s) = &actual { s.to_string() } else { "(none)".to_string() }; + + let result = ((errors.len(), actual.clone()), (expected_errors, expected_result.to_string())); + if result.0 != result.1 { + let num_errors = errors.len(); + let shown_errors = show_errors(errors); + eprintln!( + "\nExpected {} error(s) and got {}:\n\n{}\n\nFrom input: {}\nExpected AST: {}\nActual AST: {}\n", + expected_errors, num_errors, shown_errors, src, expected_result, actual); + } + result + }).collect(); + assert_eq!( + results.iter().cloned().map(|(first, _)| first).collect::>(), + results.into_iter().map(|(_, second)| second).collect::>() + ); + } } From b02d52ced5f001088c55147c31731a84b6b69e50 Mon Sep 17 00:00:00 2001 From: Piotr Czarnecki Date: Sun, 30 Apr 2023 13:50:30 +0200 Subject: [PATCH 2/5] Add fixes for return statement --- crates/noirc_frontend/src/ast/expression.rs | 6 ++++ crates/noirc_frontend/src/parser/parser.rs | 37 ++++++++++----------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index ac6161ddac..3f73604f64 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -106,6 +106,12 @@ impl Recoverable for Expression { } } +impl Recoverable for Option { + fn error(span: Span) -> Self { + Some(Expression::new(ExpressionKind::Error, span)) + } +} + #[derive(Debug, Eq, Clone)] pub struct Expression { pub kind: ExpressionKind, diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 1e83223a8b..447d148828 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -430,6 +430,7 @@ where constrain(expr_parser.clone()), declaration(expr_parser.clone()), assignment(expr_parser.clone()), + return_statement(expr_parser.clone()), expr_parser.map(Statement::Expression), )) } @@ -653,24 +654,18 @@ where } fn expression() -> impl ExprParser { - recursive(|expr| { - choice(( - return_expression(expr.clone()), - expression_with_precedence(Precedence::Lowest, expr, false), - )) - }) - .labelled("expression") + recursive(|expr| expression_with_precedence(Precedence::Lowest, expr, false)) + .labelled("expression") } -fn return_expression<'a, P>(expr_parser: P) -> impl NoirParser + 'a +fn return_statement<'a, P>(expr_parser: P) -> impl NoirParser + 'a where P: ExprParser + 'a, { - keyword(Keyword::Return) - .then(expr_parser) + ignore_then_commit(keyword(Keyword::Return), expr_parser.or_not()) .validate(|_, span, emit| { emit(ParserError::with_reason("Early 'return' is unsupported".to_owned(), span)); - Expression::error(span) + Statement::error(span) }) .labelled("return expression") } @@ -1506,25 +1501,26 @@ mod test { #[test] fn return_validation() { let cases = vec![ - ("{ return 42 }", 1, "{\n Error\n}"), - ("{ return 1; return 2; }", 2, "{\n Error\n Error;\n}"), + ("{ return 42; }", 1, "{\n Error\n}"), + ("{ return 1; return 2; }", 2, "{\n Error\n Error\n}"), ( "{ return 123; let foo = 4 + 3; }", 1, "{\n Error\n let foo: unspecified = (4 + 3)\n}", ), - ("{ return return 123; }", 2, "{\n Error;\n}"), - ("{ return 1 + return 2 }", 2, "{\n Error\n}"), + ("{ return 1 + 2; }", 1, "{\n Error\n}"), + ("{ return; }", 1, "{\n Error\n}"), ]; let show_errors = |v| vecmap(&v, ToString::to_string).join("\n"); - let results: Vec<_> = cases.into_iter().map(|(src, expected_errors, expected_result)| { + let results = vecmap(&cases, |&(src, expected_errors, expected_result)| { let (opt, errors) = parse_recover(block(expression()), src); let actual = opt.map(|ast| ast.to_string()); let actual = if let Some(s) = &actual { s.to_string() } else { "(none)".to_string() }; - let result = ((errors.len(), actual.clone()), (expected_errors, expected_result.to_string())); + let result = + ((errors.len(), actual.clone()), (expected_errors, expected_result.to_string())); if result.0 != result.1 { let num_errors = errors.len(); let shown_errors = show_errors(errors); @@ -1533,10 +1529,11 @@ mod test { expected_errors, num_errors, shown_errors, src, expected_result, actual); } result - }).collect(); + }); + assert_eq!( - results.iter().cloned().map(|(first, _)| first).collect::>(), - results.into_iter().map(|(_, second)| second).collect::>() + vecmap(&results, |t| t.0.clone()), + vecmap(&results, |t| t.1.clone()), ); } } From 1d0080ef77abc7a8345ba0c115090e3db6e679d8 Mon Sep 17 00:00:00 2001 From: Piotr Czarnecki Date: Sun, 7 May 2023 20:11:25 +0200 Subject: [PATCH 3/5] Validate semicolon for `return` properly --- crates/noirc_frontend/src/ast/statement.rs | 22 +++++++++++++++++++ .../src/hir/resolution/resolver.rs | 2 +- crates/noirc_frontend/src/parser/parser.rs | 14 ++++++------ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/crates/noirc_frontend/src/ast/statement.rs b/crates/noirc_frontend/src/ast/statement.rs index 5e0dd4e439..9c8fb6c20c 100644 --- a/crates/noirc_frontend/src/ast/statement.rs +++ b/crates/noirc_frontend/src/ast/statement.rs @@ -25,6 +25,12 @@ pub enum Statement { Assign(AssignStatement), // This is an expression with a trailing semi-colon Semi(Expression), + Return { + expr: Option, + // Initially `false`, but after semicolon validation it says whether + // the semicolon was present. + semi: bool, + }, // This statement is the result of a recovered parse error. // To avoid issuing multiple errors in later steps, it should // be skipped in any future analysis if possible. @@ -65,6 +71,18 @@ impl Statement { self } + Statement::Return { expr, semi: false } => { + if !last_statement_in_block && semi.is_none() { + let reason = "Expected a ; separating these two statements".to_string(); + emit_error(ParserError::with_reason(reason, span)); + } + Statement::Return { expr, semi: semi.is_some() } + } + + Statement::Return { expr: _, semi: true } => { + unreachable!() + } + Statement::Expression(expr) => { match (&expr.kind, semi, last_statement_in_block) { // Semicolons are optional for these expressions @@ -394,6 +412,10 @@ impl Display for Statement { Statement::Expression(expression) => expression.fmt(f), Statement::Assign(assign) => assign.fmt(f), Statement::Semi(semi) => write!(f, "{semi};"), + Statement::Return { expr: Some(expr), semi: true } => write!(f, "return {expr};"), + Statement::Return { expr: None, semi: true } => write!(f, "return;"), + Statement::Return { expr: Some(expr), semi: false } => write!(f, "return {expr}"), + Statement::Return { expr: None, semi: false } => write!(f, "return"), Statement::Error => write!(f, "Error"), } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index cfb354498a..f69e813359 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -803,7 +803,7 @@ impl<'a> Resolver<'a> { let stmt = HirAssignStatement { lvalue: identifier, expression }; HirStatement::Assign(stmt) } - Statement::Error => HirStatement::Error, + Statement::Error | Statement::Return { .. } => HirStatement::Error, } } diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 447d148828..df8eb50010 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -663,9 +663,9 @@ where P: ExprParser + 'a, { ignore_then_commit(keyword(Keyword::Return), expr_parser.or_not()) - .validate(|_, span, emit| { + .validate(|expr, span, emit| { emit(ParserError::with_reason("Early 'return' is unsupported".to_owned(), span)); - Statement::error(span) + Statement::Return { expr, semi: false } }) .labelled("return expression") } @@ -1501,15 +1501,15 @@ mod test { #[test] fn return_validation() { let cases = vec![ - ("{ return 42; }", 1, "{\n Error\n}"), - ("{ return 1; return 2; }", 2, "{\n Error\n Error\n}"), + ("{ return 42; }", 1, "{\n return 42;\n}"), + ("{ return 1; return 2; }", 2, "{\n return 1;\n return 2;\n}"), ( "{ return 123; let foo = 4 + 3; }", 1, - "{\n Error\n let foo: unspecified = (4 + 3)\n}", + "{\n return 123;\n let foo: unspecified = (4 + 3)\n}", ), - ("{ return 1 + 2; }", 1, "{\n Error\n}"), - ("{ return; }", 1, "{\n Error\n}"), + ("{ return 1 + 2 }", 1, "{\n return (1 + 2)\n}"), + ("{ return; }", 1, "{\n return;\n}"), ]; let show_errors = |v| vecmap(&v, ToString::to_string).join("\n"); From 2cfc9391ba1b1b3983b723138228eee6e7b46d28 Mon Sep 17 00:00:00 2001 From: Piotr Czarnecki Date: Sun, 7 May 2023 20:54:46 +0200 Subject: [PATCH 4/5] Ran cargo fmt --- crates/noirc_frontend/src/parser/parser.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index df8eb50010..24ce8bb34e 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -1531,9 +1531,6 @@ mod test { result }); - assert_eq!( - vecmap(&results, |t| t.0.clone()), - vecmap(&results, |t| t.1.clone()), - ); + assert_eq!(vecmap(&results, |t| t.0.clone()), vecmap(&results, |t| t.1.clone()),); } } From e3fb6cc7da30a45ac6e011deadadbb8151a58cf5 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 9 May 2023 15:49:07 -0500 Subject: [PATCH 5/5] Fix test failures from merge commit --- crates/noirc_frontend/src/ast/statement.rs | 34 +++---------------- .../src/hir/resolution/resolver.rs | 2 +- crates/noirc_frontend/src/parser/errors.rs | 2 ++ crates/noirc_frontend/src/parser/parser.rs | 18 +++++----- 4 files changed, 16 insertions(+), 40 deletions(-) diff --git a/crates/noirc_frontend/src/ast/statement.rs b/crates/noirc_frontend/src/ast/statement.rs index be26fa8ebb..2792d51c41 100644 --- a/crates/noirc_frontend/src/ast/statement.rs +++ b/crates/noirc_frontend/src/ast/statement.rs @@ -25,12 +25,6 @@ pub enum Statement { Assign(AssignStatement), // This is an expression with a trailing semi-colon Semi(Expression), - Return { - expr: Option, - // Initially `false`, but after semicolon validation it says whether - // the semicolon was present. - semi: bool, - }, // This statement is the result of a recovered parse error. // To avoid issuing multiple errors in later steps, it should // be skipped in any future analysis if possible. @@ -57,6 +51,8 @@ impl Statement { last_statement_in_block: bool, emit_error: &mut dyn FnMut(ParserError), ) -> Statement { + let missing_semicolon = + ParserError::with_reason(ParserErrorReason::MissingSeparatingSemi, span); match self { Statement::Let(_) | Statement::Constrain(_) @@ -65,26 +61,11 @@ impl Statement { | Statement::Error => { // To match rust, statements always require a semicolon, even at the end of a block if semi.is_none() { - emit_error(ParserError::with_reason( - ParserErrorReason::MissingSeparatingSemi, - span, - )); + emit_error(missing_semicolon); } self } - Statement::Return { expr, semi: false } => { - if !last_statement_in_block && semi.is_none() { - let reason = "Expected a ; separating these two statements".to_string(); - emit_error(ParserError::with_reason(reason, span)); - } - Statement::Return { expr, semi: semi.is_some() } - } - - Statement::Return { expr: _, semi: true } => { - unreachable!() - } - Statement::Expression(expr) => { match (&expr.kind, semi, last_statement_in_block) { // Semicolons are optional for these expressions @@ -103,10 +84,7 @@ impl Statement { // for unneeded expressions like { 1 + 2; 3 } (_, Some(_), false) => Statement::Expression(expr), (_, None, false) => { - emit_error(ParserError::with_reason( - ParserErrorReason::MissingSeparatingSemi, - span, - )); + emit_error(missing_semicolon); Statement::Expression(expr) } @@ -416,10 +394,6 @@ impl Display for Statement { Statement::Expression(expression) => expression.fmt(f), Statement::Assign(assign) => assign.fmt(f), Statement::Semi(semi) => write!(f, "{semi};"), - Statement::Return { expr: Some(expr), semi: true } => write!(f, "return {expr};"), - Statement::Return { expr: None, semi: true } => write!(f, "return;"), - Statement::Return { expr: Some(expr), semi: false } => write!(f, "return {expr}"), - Statement::Return { expr: None, semi: false } => write!(f, "return"), Statement::Error => write!(f, "Error"), } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 3763c97105..d80bca9df1 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -821,7 +821,7 @@ impl<'a> Resolver<'a> { let stmt = HirAssignStatement { lvalue: identifier, expression }; HirStatement::Assign(stmt) } - Statement::Error | Statement::Return { .. } => HirStatement::Error, + Statement::Error => HirStatement::Error, } } diff --git a/crates/noirc_frontend/src/parser/errors.rs b/crates/noirc_frontend/src/parser/errors.rs index d4a294482a..e788893c58 100644 --- a/crates/noirc_frontend/src/parser/errors.rs +++ b/crates/noirc_frontend/src/parser/errors.rs @@ -21,6 +21,8 @@ pub enum ParserErrorReason { ConstrainDeprecated, #[error("Expression is invalid in an array-length type: '{0}'. Only unsigned integer constants, globals, generics, +, -, *, /, and % may be used in this context.")] InvalidArrayLengthExpression(Expression), + #[error("Early 'return' is unsupported")] + EarlyReturn, } /// Represents a parsing error, or a parsing error in the making. diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index dd15c64315..2044a02c68 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -720,11 +720,11 @@ where P: ExprParser + 'a, { ignore_then_commit(keyword(Keyword::Return), expr_parser.or_not()) - .validate(|expr, span, emit| { - emit(ParserError::with_reason("Early 'return' is unsupported".to_owned(), span)); - Statement::Return { expr, semi: false } + .validate(|_, span, emit| { + emit(ParserError::with_reason(ParserErrorReason::EarlyReturn, span)); + Statement::Error }) - .labelled("return expression") + .labelled(ParsingRuleLabel::Statement) } // An expression is a single term followed by 0 or more (OP subexpression)* @@ -1616,15 +1616,15 @@ mod test { #[test] fn return_validation() { let cases = vec![ - ("{ return 42; }", 1, "{\n return 42;\n}"), - ("{ return 1; return 2; }", 2, "{\n return 1;\n return 2;\n}"), + ("{ return 42; }", 1, "{\n Error\n}"), + ("{ return 1; return 2; }", 2, "{\n Error\n Error\n}"), ( "{ return 123; let foo = 4 + 3; }", 1, - "{\n return 123;\n let foo: unspecified = (4 + 3)\n}", + "{\n Error\n let foo: unspecified = (4 + 3)\n}", ), - ("{ return 1 + 2 }", 1, "{\n return (1 + 2)\n}"), - ("{ return; }", 1, "{\n return;\n}"), + ("{ return 1 + 2 }", 2, "{\n Error\n}"), + ("{ return; }", 1, "{\n Error\n}"), ]; let show_errors = |v| vecmap(&v, ToString::to_string).join("\n");