From 00f623a417b2b1c5671af30e6af9524aa5df36ca Mon Sep 17 00:00:00 2001 From: Yordan Madzhunkov Date: Fri, 11 Aug 2023 13:22:41 +0300 Subject: [PATCH 1/4] fix: Fix 3 parser test cases in parsing * parse_all_failing test function is now satisfied by errors only ( previous version treated warning as errors ) * Fix 3 parser test cases from failing to passing * Fix type in is_warning --- crates/noirc_errors/src/reporter.rs | 2 +- crates/noirc_frontend/src/parser/parser.rs | 87 ++++++++++++++++------ 2 files changed, 67 insertions(+), 22 deletions(-) diff --git a/crates/noirc_errors/src/reporter.rs b/crates/noirc_errors/src/reporter.rs index 23b6970a913..6a595e5d4f6 100644 --- a/crates/noirc_errors/src/reporter.rs +++ b/crates/noirc_errors/src/reporter.rs @@ -75,7 +75,7 @@ impl CustomDiagnostic { matches!(self.kind, DiagnosticKind::Error) } - pub fn is_warrning(&self) -> bool { + pub fn is_warning(&self) -> bool { matches!(self.kind, DiagnosticKind::Warning) } } diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index efa4e694cdc..c1f33ddd7e3 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -526,9 +526,10 @@ fn where_clause() -> impl NoirParser> { .then_ignore(just(Token::Colon)) .then(ident()) .then(generic_type_args(parse_type())) - .validate(|((typ, trait_name), trait_generics), span, emit| { - emit(ParserError::with_reason(ParserErrorReason::ExperimentalFeature("Traits"), span)); - TraitConstraint { typ, trait_name, trait_generics } + .validate(|((typ, trait_name), trait_generics), _span, _emit| TraitConstraint { + typ, + trait_name, + trait_generics, }); keyword(Keyword::Where) @@ -1470,14 +1471,46 @@ mod test { let message = format!("Failed to parse:\n{}", program); let (op_t, errors) = parse_recover(&parser, program); for e in errors { - if !e.is_warrning() { - panic!("{}", &message); + if !e.is_warning() { + panic!("{} with error {}", &message, e); } } op_t.expect(&message) }) } + fn parse_all_warnings(parser: P, programs: Vec<&str>) -> Vec + where + P: NoirParser, + T: std::fmt::Display, + { + programs + .into_iter() + .flat_map(|program| match parse_recover(&parser, program) { + (None, errors) => errors, + (Some(expr), errors) => { + let mut has_errors = false; + for e in &errors { + if e.is_error() { + has_errors = true; + } + } + match has_errors { + true => { + unreachable!( + "Expected this input to pass with warning:\n{}\nYet it successfully failed with error:\n{}", + program, expr + ) + } + false => {} + } + errors + + } + }) + .collect() + } + fn parse_all_failing(parser: P, programs: Vec<&str>) -> Vec where P: NoirParser, @@ -1485,12 +1518,27 @@ mod test { { programs .into_iter() - .flat_map(|program| match parse_with(&parser, program) { - Ok(expr) => unreachable!( - "Expected this input to fail:\n{}\nYet it successfully parsed as:\n{}", - program, expr - ), - Err(error) => error, + .flat_map(|program| match parse_recover(&parser, program) { + (None, errors) => errors, + (Some(expr), errors) => { + let mut has_errors = false; + for e in &errors { + if e.is_error() { + has_errors = true; + } + } + match has_errors { + false => { + unreachable!( + "Expected this input to fail:\n{}\nYet it successfully parsed as:\n{}", + program, expr + ) + } + true => {} + } + errors + + } }) .collect() } @@ -1679,7 +1727,7 @@ mod test { // The first (inner) `==` is a predicate which returns 0/1 // The outer layer is an infix `==` which is // associated with the Constrain statement - let errors = parse_all_failing( + let errors = parse_all_warnings( constrain(expression()), vec![ "constrain ((x + y) == k) + z == y", @@ -1690,7 +1738,7 @@ mod test { ], ); assert_eq!(errors.len(), 5); - assert!(errors.iter().all(|err| { format!("{}", err).contains("deprecated") })); + assert!(errors.iter().all(|err| { err.is_warning() && format!("{}", err).contains("deprecated") })) } /// This is the standard way to declare an assert statement @@ -1784,6 +1832,8 @@ mod test { "fn f(f: pub Field, y : Field, z : comptime Field) -> u8 { x + a }", "fn f(f: pub Field, y : T, z : comptime Field) -> u8 { x + a }", "fn func_name(f: Field, y : T) where T: SomeTrait {}", + // The following should produce compile error on later stage. From the parser prespective it's fine + "fn func_name(f: Field, y : pub Field, z : pub [u8;5]) where T: SomeTrait {}", ], ); @@ -1798,9 +1848,6 @@ mod test { "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) where SomeTrait {}", "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) SomeTrait {}", "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) where T: SomeTrait {}", - // TODO(GenericParameterNotFoundInFunction) - // Consider making this a compilation error: - // "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) where T: SomeTrait {}", ], ); } @@ -1820,6 +1867,9 @@ mod test { "trait TraitWithAssociatedType { type Element; fn item(self, index: Field) -> Self::Element; }", "trait TraitWithAssociatedConstant { let Size: Field; }", "trait TraitWithAssociatedConstantWithDefaultValue { let Size: Field = 10; }", + "trait GenericTrait { fn elem(&mut self, index: Field) -> T; }", + "trait GenericTraitWithConstraints where T: SomeTrait { fn elem(self, index: Field) -> T; }", + "trait TraitWithMultipleGenericParams where A: SomeTrait, B: AnotherTrait { let Size: Field; fn zero() -> Self; }", ], ); @@ -1829,11 +1879,6 @@ mod test { "trait MissingBody", "trait WrongDelimiter { fn foo() -> u8, fn bar() -> u8 }", "trait WhereClauseWithoutGenerics where A: SomeTrait { }", - // TODO: when implemnt generics in traits the following 3 should pass - "trait GenericTrait { fn elem(&mut self, index: Field) -> T; }", - "trait GenericTraitWithConstraints where T: SomeTrait { fn elem(self, index: Field) -> T; }", - "trait TraitWithMultipleGenericParams where A: SomeTrait, B: AnotherTrait { comptime Size: Field; fn zero() -> Self; }", - ], ); } From 05cd69c52d144d9623b91a58fc9e461ea217e3c8 Mon Sep 17 00:00:00 2001 From: Yordan Madzhunkov Date: Sat, 12 Aug 2023 11:10:44 +0300 Subject: [PATCH 2/4] Fix experimental feature warning when traits are used * Fix spelling * Fix one specific test case * Use parse_with instead of parse_recover in parse_all_failing * Refactor code to be more rusty --- crates/noirc_frontend/src/parser/parser.rs | 69 +++++++++------------- 1 file changed, 27 insertions(+), 42 deletions(-) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index c1f33ddd7e3..37e0670e71f 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -526,10 +526,9 @@ fn where_clause() -> impl NoirParser> { .then_ignore(just(Token::Colon)) .then(ident()) .then(generic_type_args(parse_type())) - .validate(|((typ, trait_name), trait_generics), _span, _emit| TraitConstraint { - typ, - trait_name, - trait_generics, + .validate(|((typ, trait_name), trait_generics), span, emit| { + emit(ParserError::with_reason(ParserErrorReason::ExperimentalFeature("Traits"), span)); + TraitConstraint { typ, trait_name, trait_generics } }); keyword(Keyword::Where) @@ -1489,23 +1488,12 @@ mod test { .flat_map(|program| match parse_recover(&parser, program) { (None, errors) => errors, (Some(expr), errors) => { - let mut has_errors = false; - for e in &errors { - if e.is_error() { - has_errors = true; - } - } - match has_errors { - true => { - unreachable!( - "Expected this input to pass with warning:\n{}\nYet it successfully failed with error:\n{}", - program, expr - ) - } - false => {} - } + if errors.iter().any(|error| error.is_error()) { + unreachable!( + "Expected this input to pass with warning:\n{program}\nYet it successfully failed with error:\n{expr}", + ) + }; errors - } }) .collect() @@ -1518,26 +1506,21 @@ mod test { { programs .into_iter() - .flat_map(|program| match parse_recover(&parser, program) { - (None, errors) => errors, - (Some(expr), errors) => { - let mut has_errors = false; - for e in &errors { - if e.is_error() { - has_errors = true; - } - } - match has_errors { - false => { - unreachable!( - "Expected this input to fail:\n{}\nYet it successfully parsed as:\n{}", - program, expr - ) - } - true => {} - } + .flat_map(|program| match parse_with(&parser, program) { + Ok(expr) => { + unreachable!( + "Expected this input to fail:\n{}\nYet it successfully parsed as:\n{}", + program, expr + ) + } + Err(errors) => { + if errors.iter().all(|error| error.is_warning()) { + unreachable!( + "Expected at least one error when parsing:\n{}\nYet it successfully parsed wiithout errors:\n", + program + ) + }; errors - } }) .collect() @@ -1738,7 +1721,9 @@ mod test { ], ); assert_eq!(errors.len(), 5); - assert!(errors.iter().all(|err| { err.is_warning() && format!("{}", err).contains("deprecated") })) + assert!(errors + .iter() + .all(|err| { err.is_warning() && format!("{}", err).contains("deprecated") })) } /// This is the standard way to declare an assert statement @@ -1832,8 +1817,8 @@ mod test { "fn f(f: pub Field, y : Field, z : comptime Field) -> u8 { x + a }", "fn f(f: pub Field, y : T, z : comptime Field) -> u8 { x + a }", "fn func_name(f: Field, y : T) where T: SomeTrait {}", - // The following should produce compile error on later stage. From the parser prespective it's fine - "fn func_name(f: Field, y : pub Field, z : pub [u8;5]) where T: SomeTrait {}", + // The following should produce compile error on later stage. From the parser's perspective it's fine + "fn func_name(f: Field, y : Field, z : Field) where T: SomeTrait {}", ], ); From 9c44d7b7cad97caf9aeeb8fde602dafe7c44173e Mon Sep 17 00:00:00 2001 From: Yordan Madzhunkov Date: Sat, 12 Aug 2023 12:18:08 +0300 Subject: [PATCH 3/4] Improve code quality --- crates/noirc_frontend/src/parser/parser.rs | 34 ++++++++++------------ 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 37e0670e71f..5daed0e3149 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -1468,12 +1468,12 @@ mod test { { vecmap(programs, move |program| { let message = format!("Failed to parse:\n{}", program); - let (op_t, errors) = parse_recover(&parser, program); - for e in errors { - if !e.is_warning() { - panic!("{} with error {}", &message, e); + let (op_t, diagnostics) = parse_recover(&parser, program); + diagnostics.iter().for_each(|diagnostic| { + if diagnostic.is_error() { + panic!("{} with error {}", &message, diagnostic); } - } + }); op_t.expect(&message) }) } @@ -1485,16 +1485,12 @@ mod test { { programs .into_iter() - .flat_map(|program| match parse_recover(&parser, program) { - (None, errors) => errors, - (Some(expr), errors) => { - if errors.iter().any(|error| error.is_error()) { - unreachable!( - "Expected this input to pass with warning:\n{program}\nYet it successfully failed with error:\n{expr}", - ) - }; - errors - } + .flat_map(|program| { + let (_expr, diagnostics) = parse_recover(&parser, program); + if diagnostics.iter().any(|diagnostic| diagnostic.is_error()) { + unreachable!("Expected this input to pass with warning:\n{program}\nYet it failed with error:\n{diagnostic}") + }; + diagnostics }) .collect() } @@ -1513,14 +1509,14 @@ mod test { program, expr ) } - Err(errors) => { - if errors.iter().all(|error| error.is_warning()) { + Err(diagnostics) => { + if diagnostics.iter().all(|diagnostic: &CustomDiagnostic| diagnostic.is_warning()) { unreachable!( - "Expected at least one error when parsing:\n{}\nYet it successfully parsed wiithout errors:\n", + "Expected at least one error when parsing:\n{}\nYet it successfully parsed without errors:\n", program ) }; - errors + diagnostics } }) .collect() From 9d847473072544e1000c9e8b99be227511936259 Mon Sep 17 00:00:00 2001 From: Yordan Madzhunkov Date: Sat, 12 Aug 2023 12:37:08 +0300 Subject: [PATCH 4/4] Fix build on ubuntu --- crates/noirc_frontend/src/parser/parser.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 5daed0e3149..0c95af77a5a 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -1487,9 +1487,9 @@ mod test { .into_iter() .flat_map(|program| { let (_expr, diagnostics) = parse_recover(&parser, program); - if diagnostics.iter().any(|diagnostic| diagnostic.is_error()) { + diagnostics.iter().for_each(|diagnostic| if diagnostic.is_error() { unreachable!("Expected this input to pass with warning:\n{program}\nYet it failed with error:\n{diagnostic}") - }; + }); diagnostics }) .collect()