From b85e764c2156ebb68acb7fba68e63856f9d1235b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 23 Jul 2024 12:31:00 -0400 Subject: [PATCH] fix(frontend)!: Restrict numeric generic types to unsigned ints up to `u32` (#5581) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem\* Resolves #5571 The issue references expanding the supported type-level integers, however, after a team discussion we decided to simply restrict the supported numeric generics. If other kinds of numeric generics become highly requested we can then re-assess whether we want to expand support for type level constants. ## Summary\* A new parser combinator for numeric generics was introduced to differentiate with the existing `type_expression` combinator. This is due to usage of `or_not` in the `generic_type_args` parser. Due to the `or_not` combinator the expression error was being ignored. Thus, the new numeric generic expression combinator returns an optional expression along with its parsed type. ## Additional Context This program: ```rust fn big() -> u64 { N } fn main() { let _ = big::<18446744073709551615>(); } ``` gives the following error: Screenshot 2024-07-23 at 12 18 00 PM The previous errors were very unclear: Screenshot 2024-07-22 at 5 59 04 PM ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [X] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher --- compiler/noirc_frontend/src/ast/mod.rs | 8 ++--- compiler/noirc_frontend/src/parser/errors.rs | 10 +++---- .../src/parser/parser/function.rs | 18 ++++++++---- .../noirc_frontend/src/parser/parser/types.rs | 29 +++++++++++++++---- compiler/noirc_frontend/src/tests.rs | 9 +++--- 5 files changed, 46 insertions(+), 28 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index 8cdf377528b..038a13529d7 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -301,15 +301,12 @@ impl UnresolvedTypeExpression { // This large error size is justified because it improves parsing speeds by around 40% in // release mode. See `ParserError` definition for further explanation. #[allow(clippy::result_large_err)] - pub fn from_expr( + pub(crate) fn from_expr( expr: Expression, span: Span, ) -> Result { Self::from_expr_helper(expr).map_err(|err_expr| { - ParserError::with_reason( - ParserErrorReason::InvalidArrayLengthExpression(err_expr), - span, - ) + ParserError::with_reason(ParserErrorReason::InvalidTypeExpression(err_expr), span) }) } @@ -323,7 +320,6 @@ impl UnresolvedTypeExpression { fn from_expr_helper(expr: Expression) -> Result { match expr.kind { - // TODO(https://github.com/noir-lang/noir/issues/5571): The `sign` bool from `Literal::Integer` should be used to construct the constant type expression. ExpressionKind::Literal(Literal::Integer(int, _)) => match int.try_to_u32() { Some(int) => Ok(UnresolvedTypeExpression::Constant(int, expr.span)), None => Err(expr), diff --git a/compiler/noirc_frontend/src/parser/errors.rs b/compiler/noirc_frontend/src/parser/errors.rs index 9df9a8e15dd..c566489eb40 100644 --- a/compiler/noirc_frontend/src/parser/errors.rs +++ b/compiler/noirc_frontend/src/parser/errors.rs @@ -20,8 +20,8 @@ pub enum ParserErrorReason { MissingSeparatingSemi, #[error("constrain keyword is deprecated")] 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("Invalid type expression: '{0}'. Only unsigned integer constants up to `u32`, globals, generics, +, -, *, /, and % may be used in this context.")] + InvalidTypeExpression(Expression), #[error("Early 'return' is unsupported")] EarlyReturn, #[error("Patterns aren't allowed in a trait's function declarations")] @@ -44,10 +44,8 @@ pub enum ParserErrorReason { InvalidBitSize(u32), #[error("{0}")] Lexer(LexerErrorKind), - // TODO(https://github.com/noir-lang/noir/issues/5571): This error can be removed once support is expanded for type-level integers. - // This error reason was added to prevent the panic outline in this issue: https://github.com/noir-lang/noir/issues/5552. - #[error("Only unsigned integers allowed for numeric generics")] - SignedNumericGeneric, + #[error("The only supported numeric generic types are `u1`, `u8`, `u16`, and `u32`")] + ForbiddenNumericGenericType, } /// Represents a parsing error, or a parsing error in the making. diff --git a/compiler/noirc_frontend/src/parser/parser/function.rs b/compiler/noirc_frontend/src/parser/parser/function.rs index fd1c7d5237f..2fd337e1cb1 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -4,8 +4,8 @@ use super::{ parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, self_parameter, where_clause, NoirParser, }; -use crate::parser::spanned; use crate::token::{Keyword, Token}; +use crate::{ast::IntegerBitSize, parser::spanned}; use crate::{ ast::{ FunctionDefinition, FunctionReturnType, ItemVisibility, NoirFunction, Param, Visibility, @@ -91,10 +91,12 @@ pub(super) fn numeric_generic() -> impl NoirParser { .map(|(ident, typ)| UnresolvedGeneric::Numeric { ident, typ }) .validate(|generic, span, emit| { if let UnresolvedGeneric::Numeric { typ, .. } = &generic { - if let UnresolvedTypeData::Integer(signedness, _) = typ.typ { - if matches!(signedness, Signedness::Signed) { + if let UnresolvedTypeData::Integer(signedness, bit_size) = typ.typ { + if matches!(signedness, Signedness::Signed) + || matches!(bit_size, IntegerBitSize::SixtyFour) + { emit(ParserError::with_reason( - ParserErrorReason::SignedNumericGeneric, + ParserErrorReason::ForbiddenNumericGenericType, span, )); } @@ -228,7 +230,7 @@ mod test { // fn func_name(x: impl Eq) {} with error Expected an end of input but found end of input // "fn func_name(x: impl Eq) {}", "fn func_name(x: impl Eq, y : T) where T: SomeTrait + Eq {}", - "fn func_name(x: [Field; N]) {}", + "fn func_name(x: [Field; N]) {}", ], ); @@ -249,8 +251,12 @@ mod test { "fn func_name(y: T) {}", "fn func_name(y: T) {}", "fn func_name(y: T) {}", - "fn func_name(y: T) {}", + // Test failure of missing `let` + "fn func_name(y: T) {}", + // Test that signed numeric generics are banned "fn func_name() {}", + // Test that `u64` is banned + "fn func_name(x: [Field; N]) {}", ], ); } diff --git a/compiler/noirc_frontend/src/parser/parser/types.rs b/compiler/noirc_frontend/src/parser/parser/types.rs index 3bd59608b12..cecc1cbcd4c 100644 --- a/compiler/noirc_frontend/src/parser/parser/types.rs +++ b/compiler/noirc_frontend/src/parser/parser/types.rs @@ -3,7 +3,9 @@ use super::{ expression_with_precedence, keyword, nothing, parenthesized, path, NoirParser, ParserError, ParserErrorReason, Precedence, }; -use crate::ast::{Recoverable, UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression}; +use crate::ast::{ + Expression, Recoverable, UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression, +}; use crate::QuotedType; use crate::parser::labels::ParsingRuleLabel; @@ -201,8 +203,7 @@ pub(super) fn generic_type_args<'a>( // separator afterward. Failing early here ensures we try the `type_expression` // parser afterward. .then_ignore(one_of([Token::Comma, Token::Greater]).rewind()) - .or(type_expression() - .map_with_span(|expr, span| UnresolvedTypeData::Expression(expr).with_span(span))) + .or(type_expression_validated()) .separated_by(just(Token::Comma)) .allow_trailing() .at_least(1) @@ -234,7 +235,26 @@ pub(super) fn slice_type( }) } -pub(super) fn type_expression() -> impl NoirParser { +fn type_expression() -> impl NoirParser { + type_expression_inner().try_map(UnresolvedTypeExpression::from_expr) +} + +/// This parser is the same as `type_expression()`, however, it continues parsing and +/// emits a parser error in the case of an invalid type expression rather than halting the parser. +fn type_expression_validated() -> impl NoirParser { + type_expression_inner().validate(|expr, span, emit| { + let type_expr = UnresolvedTypeExpression::from_expr(expr, span); + match type_expr { + Ok(type_expression) => UnresolvedTypeData::Expression(type_expression).with_span(span), + Err(parser_error) => { + emit(parser_error); + UnresolvedType::error(span) + } + } + }) +} + +fn type_expression_inner() -> impl NoirParser { recursive(|expr| { expression_with_precedence( Precedence::lowest_type_precedence(), @@ -246,7 +266,6 @@ pub(super) fn type_expression() -> impl NoirParser { ) }) .labelled(ParsingRuleLabel::TypeExpression) - .try_map(UnresolvedTypeExpression::from_expr) } pub(super) fn tuple_type(type_parser: T) -> impl NoirParser diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 3767dad103c..8cf98f4f609 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1634,7 +1634,7 @@ fn numeric_generic_in_function_signature() { #[test] fn numeric_generic_as_struct_field_type() { let src = r#" - struct Foo { + struct Foo { a: Field, b: N, } @@ -1695,7 +1695,7 @@ fn numeric_generic_as_param_type() { #[test] fn numeric_generic_used_in_nested_type_fail() { let src = r#" - struct Foo { + struct Foo { a: Field, b: Bar, } @@ -1745,7 +1745,7 @@ fn numeric_generic_used_in_nested_type_pass() { #[test] fn numeric_generic_used_in_trait() { - // We want to make sure that `N` in `impl Deserialize` does + // We want to make sure that `N` in `impl Deserialize` does // not trigger `expected type, found numeric generic parameter N` as the trait // does in fact expect a numeric generic. let src = r#" @@ -1756,7 +1756,7 @@ fn numeric_generic_used_in_trait() { d: T, } - impl Deserialize for MyType { + impl Deserialize for MyType { fn deserialize(fields: [Field; N], other: T) -> Self { MyType { a: fields[0], b: fields[1], c: fields[2], d: other } } @@ -2224,7 +2224,6 @@ fn impl_stricter_than_trait_different_trait_generics() { .. }) = &errors[0].0 { - dbg!(constraint_name.as_str()); assert!(matches!(constraint_typ.to_string().as_str(), "A")); assert!(matches!(constraint_name.as_str(), "T2")); assert!(matches!(constraint_generics[0].to_string().as_str(), "B"));