From d80962ae6d1b511dacb359bf12997ff1e6d84efd Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 19 Jul 2024 21:29:50 +0000 Subject: [PATCH 1/3] initial changes for signed numeric gen panic --- compiler/noirc_frontend/src/ast/mod.rs | 1 - compiler/noirc_frontend/src/parser/errors.rs | 2 ++ .../src/parser/parser/function.rs | 21 +++++++++++--- compiler/noirc_frontend/src/tests.rs | 29 +++++++++++++++++-- 4 files changed, 46 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index dfe4258744a..aa6d4551a23 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -324,7 +324,6 @@ impl UnresolvedTypeExpression { fn from_expr_helper(expr: Expression) -> Result { match expr.kind { ExpressionKind::Literal(Literal::Integer(int, sign)) => { - assert!(!sign, "Negative literal is not allowed here"); 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 41ea9f88c19..9a7fd143035 100644 --- a/compiler/noirc_frontend/src/parser/errors.rs +++ b/compiler/noirc_frontend/src/parser/errors.rs @@ -44,6 +44,8 @@ pub enum ParserErrorReason { InvalidBitSize(u32), #[error("{0}")] Lexer(LexerErrorKind), + #[error("Only unsigned integers allowed for numeric generics")] + SignedNumericGeneric, } /// 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 3e686ee4c85..fbc983ca176 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -4,13 +4,13 @@ use super::{ parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, self_parameter, where_clause, NoirParser, }; -use crate::ast::{ +use crate::{ast::{ FunctionDefinition, FunctionReturnType, ItemVisibility, NoirFunction, Param, Visibility, -}; +}, macros_api::UnresolvedTypeData, parser::{ParserError, ParserErrorReason}}; use crate::parser::spanned; use crate::token::{Keyword, Token}; use crate::{ - ast::{UnresolvedGeneric, UnresolvedGenerics}, + ast::{UnresolvedGeneric, UnresolvedGenerics, Signedness}, parser::labels::ParsingRuleLabel, }; @@ -84,7 +84,19 @@ pub(super) fn numeric_generic() -> impl NoirParser { .ignore_then(ident()) .then_ignore(just(Token::Colon)) .then(parse_type()) - .map(|(ident, typ)| UnresolvedGeneric::Numeric { ident, typ }) + .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) { + emit(ParserError::with_reason(ParserErrorReason::SignedNumericGeneric, span)); + } + } + } + generic + }) } pub(super) fn generic_type() -> impl NoirParser { @@ -233,6 +245,7 @@ mod test { "fn func_name(y: T) {}", "fn func_name(y: T) {}", "fn func_name(y: T) {}", + "fn func_name() {}", ], ); } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index b4f17489ff7..28126f72e00 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -28,7 +28,7 @@ use crate::hir::def_collector::dc_crate::DefCollector; use crate::hir_def::expr::HirExpression; use crate::hir_def::stmt::HirStatement; use crate::monomorphization::monomorphize; -use crate::parser::ParserErrorReason; +use crate::parser::{ParserError, ParserErrorReason}; use crate::ParsedModule; use crate::{ hir::def_map::{CrateDefMap, LocalModuleId}, @@ -62,7 +62,7 @@ pub(crate) fn get_program(src: &str) -> (ParsedModule, Context, Vec<(Compilation let (program, parser_errors) = parse_program(src); let mut errors = vecmap(parser_errors, |e| (e.into(), root_file_id)); remove_experimental_warnings(&mut errors); - + dbg!(!has_parser_error(&errors)); if !has_parser_error(&errors) { // Allocate a default Module for the root, giving it a ModuleId let mut modules: Arena = Arena::default(); @@ -2459,3 +2459,28 @@ fn no_super() { assert_eq!(span.start(), 4); assert_eq!(span.end(), 9); } + +// TODO(https://github.com/noir-lang/noir/issues/5571): This test can be removed once +// support is expanded for type-level integers. +// This test provides a regression against the panic in https://github.com/noir-lang/noir/issues/5552 +#[test] +fn numeric_generic_signed() { + let src= r"# + struct Foo { } + + fn main() { + let _: Foo<-1> = Foo { }; + } + #"; + + let errors = get_program_errors(src); + dbg!(errors.clone()); + assert_eq!(errors.len(), 1); + let CompilationError::ParseError(parser_error) = &errors[0].0 + else { + panic!("Expected a parser error, got {:?}", errors[0].0); + }; + + let parser_err_reason = parser_error.reason().expect("Should have a parser error reason"); + assert!(matches!(parser_err_reason, ParserErrorReason::SignedNumericGeneric)); +} \ No newline at end of file From 40fa72425c22c020afc4760da913de6312e36a78 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 19 Jul 2024 21:35:57 +0000 Subject: [PATCH 2/3] error rather than panic for signed numeric generics --- compiler/noirc_frontend/src/ast/mod.rs | 11 ++++---- compiler/noirc_frontend/src/parser/errors.rs | 3 +++ .../src/parser/parser/function.rs | 22 +++++++++------ compiler/noirc_frontend/src/tests.rs | 27 +------------------ 4 files changed, 23 insertions(+), 40 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index aa6d4551a23..8cdf377528b 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -323,12 +323,11 @@ impl UnresolvedTypeExpression { fn from_expr_helper(expr: Expression) -> Result { match expr.kind { - ExpressionKind::Literal(Literal::Integer(int, sign)) => { - match int.try_to_u32() { - Some(int) => Ok(UnresolvedTypeExpression::Constant(int, expr.span)), - None => Err(expr), - } - } + // 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), + }, ExpressionKind::Variable(path, _) => Ok(UnresolvedTypeExpression::Variable(path)), ExpressionKind::Prefix(prefix) if prefix.operator == UnaryOp::Minus => { let lhs = Box::new(UnresolvedTypeExpression::Constant(0, expr.span)); diff --git a/compiler/noirc_frontend/src/parser/errors.rs b/compiler/noirc_frontend/src/parser/errors.rs index 9a7fd143035..25919361af8 100644 --- a/compiler/noirc_frontend/src/parser/errors.rs +++ b/compiler/noirc_frontend/src/parser/errors.rs @@ -44,6 +44,9 @@ 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, } diff --git a/compiler/noirc_frontend/src/parser/parser/function.rs b/compiler/noirc_frontend/src/parser/parser/function.rs index fbc983ca176..c856b5f35cd 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -4,13 +4,17 @@ use super::{ parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, self_parameter, where_clause, NoirParser, }; -use crate::{ast::{ - FunctionDefinition, FunctionReturnType, ItemVisibility, NoirFunction, Param, Visibility, -}, macros_api::UnresolvedTypeData, parser::{ParserError, ParserErrorReason}}; use crate::parser::spanned; use crate::token::{Keyword, Token}; use crate::{ - ast::{UnresolvedGeneric, UnresolvedGenerics, Signedness}, + ast::{ + FunctionDefinition, FunctionReturnType, ItemVisibility, NoirFunction, Param, Visibility, + }, + macros_api::UnresolvedTypeData, + parser::{ParserError, ParserErrorReason}, +}; +use crate::{ + ast::{Signedness, UnresolvedGeneric, UnresolvedGenerics}, parser::labels::ParsingRuleLabel, }; @@ -84,14 +88,16 @@ pub(super) fn numeric_generic() -> impl NoirParser { .ignore_then(ident()) .then_ignore(just(Token::Colon)) .then(parse_type()) - .map(|(ident, typ)| { - UnresolvedGeneric::Numeric { ident, typ } - }) + .map(|(ident, typ)| UnresolvedGeneric::Numeric { ident, typ }) .validate(|generic, span, emit| { + dbg!(generic.clone()); if let UnresolvedGeneric::Numeric { typ, .. } = &generic { if let UnresolvedTypeData::Integer(signedness, _) = typ.typ { if matches!(signedness, Signedness::Signed) { - emit(ParserError::with_reason(ParserErrorReason::SignedNumericGeneric, span)); + emit(ParserError::with_reason( + ParserErrorReason::SignedNumericGeneric, + span, + )); } } } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 28126f72e00..4469dece709 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -28,7 +28,7 @@ use crate::hir::def_collector::dc_crate::DefCollector; use crate::hir_def::expr::HirExpression; use crate::hir_def::stmt::HirStatement; use crate::monomorphization::monomorphize; -use crate::parser::{ParserError, ParserErrorReason}; +use crate::parser::ParserErrorReason; use crate::ParsedModule; use crate::{ hir::def_map::{CrateDefMap, LocalModuleId}, @@ -2459,28 +2459,3 @@ fn no_super() { assert_eq!(span.start(), 4); assert_eq!(span.end(), 9); } - -// TODO(https://github.com/noir-lang/noir/issues/5571): This test can be removed once -// support is expanded for type-level integers. -// This test provides a regression against the panic in https://github.com/noir-lang/noir/issues/5552 -#[test] -fn numeric_generic_signed() { - let src= r"# - struct Foo { } - - fn main() { - let _: Foo<-1> = Foo { }; - } - #"; - - let errors = get_program_errors(src); - dbg!(errors.clone()); - assert_eq!(errors.len(), 1); - let CompilationError::ParseError(parser_error) = &errors[0].0 - else { - panic!("Expected a parser error, got {:?}", errors[0].0); - }; - - let parser_err_reason = parser_error.reason().expect("Should have a parser error reason"); - assert!(matches!(parser_err_reason, ParserErrorReason::SignedNumericGeneric)); -} \ No newline at end of file From fb13b660f6ab885af80be848f93496ef109f3ccb Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 19 Jul 2024 21:40:42 +0000 Subject: [PATCH 3/3] cleanup --- compiler/noirc_frontend/src/parser/errors.rs | 3 +-- compiler/noirc_frontend/src/parser/parser/function.rs | 1 - compiler/noirc_frontend/src/tests.rs | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/parser/errors.rs b/compiler/noirc_frontend/src/parser/errors.rs index 25919361af8..9df9a8e15dd 100644 --- a/compiler/noirc_frontend/src/parser/errors.rs +++ b/compiler/noirc_frontend/src/parser/errors.rs @@ -44,8 +44,7 @@ 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. + // 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, diff --git a/compiler/noirc_frontend/src/parser/parser/function.rs b/compiler/noirc_frontend/src/parser/parser/function.rs index c856b5f35cd..fd1c7d5237f 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -90,7 +90,6 @@ pub(super) fn numeric_generic() -> impl NoirParser { .then(parse_type()) .map(|(ident, typ)| UnresolvedGeneric::Numeric { ident, typ }) .validate(|generic, span, emit| { - dbg!(generic.clone()); if let UnresolvedGeneric::Numeric { typ, .. } = &generic { if let UnresolvedTypeData::Integer(signedness, _) = typ.typ { if matches!(signedness, Signedness::Signed) { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 4469dece709..b4f17489ff7 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -62,7 +62,7 @@ pub(crate) fn get_program(src: &str) -> (ParsedModule, Context, Vec<(Compilation let (program, parser_errors) = parse_program(src); let mut errors = vecmap(parser_errors, |e| (e.into(), root_file_id)); remove_experimental_warnings(&mut errors); - dbg!(!has_parser_error(&errors)); + if !has_parser_error(&errors) { // Allocate a default Module for the root, giving it a ModuleId let mut modules: Arena = Arena::default();