Skip to content

Commit

Permalink
fix(frontend)!: Restrict numeric generic types to unsigned ints up to…
Browse files Browse the repository at this point in the history
… `u32` (#5581)

# 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<let N: u64>() -> u64 {
        N
    }

    fn main() {
        let _ = big::<18446744073709551615>();
    }
```
gives the following error:
<img width="972" alt="Screenshot 2024-07-23 at 12 18 00 PM"
src="https://github.com/user-attachments/assets/fbbaf78a-ab86-4250-9008-ba4d2751409d">

The previous errors were very unclear:
<img width="725" alt="Screenshot 2024-07-22 at 5 59 04 PM"
src="https://github.com/user-attachments/assets/c532de18-0240-4a4a-acbc-f0be72b5b2db">



## 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 <[email protected]>
  • Loading branch information
vezenovm and jfecher authored Jul 23, 2024
1 parent ff8ca91 commit b85e764
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 28 deletions.
8 changes: 2 additions & 6 deletions compiler/noirc_frontend/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<UnresolvedTypeExpression, ParserError> {
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)
})
}

Expand All @@ -323,7 +320,6 @@ impl UnresolvedTypeExpression {

fn from_expr_helper(expr: Expression) -> Result<UnresolvedTypeExpression, Expression> {
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),
Expand Down
10 changes: 4 additions & 6 deletions compiler/noirc_frontend/src/parser/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand All @@ -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.
Expand Down
18 changes: 12 additions & 6 deletions compiler/noirc_frontend/src/parser/parser/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -91,10 +91,12 @@ pub(super) fn numeric_generic() -> impl NoirParser<UnresolvedGeneric> {
.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,
));
}
Expand Down Expand Up @@ -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<T>(x: impl Eq, y : T) where T: SomeTrait + Eq {}",
"fn func_name<let N: u64>(x: [Field; N]) {}",
"fn func_name<let N: u32>(x: [Field; N]) {}",
],
);

Expand All @@ -249,8 +251,12 @@ mod test {
"fn func_name<let T>(y: T) {}",
"fn func_name<let T:>(y: T) {}",
"fn func_name<T:>(y: T) {}",
"fn func_name<T: u64>(y: T) {}",
// Test failure of missing `let`
"fn func_name<T: u32>(y: T) {}",
// Test that signed numeric generics are banned
"fn func_name<let N: i8>() {}",
// Test that `u64` is banned
"fn func_name<let N: u64>(x: [Field; N]) {}",
],
);
}
Expand Down
29 changes: 24 additions & 5 deletions compiler/noirc_frontend/src/parser/parser/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -234,7 +235,26 @@ pub(super) fn slice_type(
})
}

pub(super) fn type_expression() -> impl NoirParser<UnresolvedTypeExpression> {
fn type_expression() -> impl NoirParser<UnresolvedTypeExpression> {
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<UnresolvedType> {
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<Expression> {
recursive(|expr| {
expression_with_precedence(
Precedence::lowest_type_precedence(),
Expand All @@ -246,7 +266,6 @@ pub(super) fn type_expression() -> impl NoirParser<UnresolvedTypeExpression> {
)
})
.labelled(ParsingRuleLabel::TypeExpression)
.try_map(UnresolvedTypeExpression::from_expr)
}

pub(super) fn tuple_type<T>(type_parser: T) -> impl NoirParser<UnresolvedType>
Expand Down
9 changes: 4 additions & 5 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1634,7 +1634,7 @@ fn numeric_generic_in_function_signature() {
#[test]
fn numeric_generic_as_struct_field_type() {
let src = r#"
struct Foo<let N: u64> {
struct Foo<let N: u32> {
a: Field,
b: N,
}
Expand Down Expand Up @@ -1695,7 +1695,7 @@ fn numeric_generic_as_param_type() {
#[test]
fn numeric_generic_used_in_nested_type_fail() {
let src = r#"
struct Foo<let N: u64> {
struct Foo<let N: u32> {
a: Field,
b: Bar<N>,
}
Expand Down Expand Up @@ -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<let N: u64, T> Deserialize<N, T>` does
// We want to make sure that `N` in `impl<let N: u32, T> Deserialize<N, T>` does
// not trigger `expected type, found numeric generic parameter N` as the trait
// does in fact expect a numeric generic.
let src = r#"
Expand All @@ -1756,7 +1756,7 @@ fn numeric_generic_used_in_trait() {
d: T,
}
impl<let N: u64, T> Deserialize<N, T> for MyType<T> {
impl<let N: u32, T> Deserialize<N, T> for MyType<T> {
fn deserialize(fields: [Field; N], other: T) -> Self {
MyType { a: fields[0], b: fields[1], c: fields[2], d: other }
}
Expand Down Expand Up @@ -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"));
Expand Down

0 comments on commit b85e764

Please sign in to comment.