Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(frontend)!: Restrict numeric generic types to unsigned ints up to u32 #5581

Merged
merged 9 commits into from
Jul 23, 2024
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 @@ -117,7 +117,7 @@
/*env:*/ Box<UnresolvedType>,
),

// The type of quoted code for metaprogramming

Check warning on line 120 in compiler/noirc_frontend/src/ast/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (metaprogramming)
Quoted(crate::QuotedType),

/// An already resolved type. These can only be parsed if they were present in the token stream
Expand Down Expand Up @@ -301,15 +301,12 @@
// 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 @@

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)
}
vezenovm marked this conversation as resolved.
Show resolved Hide resolved

/// 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> {
vezenovm marked this conversation as resolved.
Show resolved Hide resolved
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 @@
#[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 @@
#[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 @@

#[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 @@
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 @@
..
}) = &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 Expand Up @@ -2310,7 +2309,7 @@
}

#[test]
fn underflowing_u8() {

Check warning on line 2312 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (underflowing)
let src = r#"
fn main() {
let _: u8 = -1;
Expand Down Expand Up @@ -2348,7 +2347,7 @@
}

#[test]
fn underflowing_i8() {

Check warning on line 2350 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (underflowing)
let src = r#"
fn main() {
let _: i8 = -129;
Expand Down
Loading