From 078e3cac3c662b79c28a2e75f08e9230b3796855 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Mon, 21 Aug 2023 12:41:10 +0300 Subject: [PATCH 1/9] fix: syntax for environment types now works with generics --- .../closure_explicit_types/src/main.nr | 43 +++++++++++++------ crates/noirc_frontend/src/ast/function.rs | 5 +++ .../src/hir/resolution/errors.rs | 5 +++ .../src/hir/resolution/resolver.rs | 22 +++++++++- crates/noirc_frontend/src/parser/parser.rs | 16 ++----- 5 files changed, 65 insertions(+), 26 deletions(-) diff --git a/crates/nargo_cli/tests/compile_success_empty/closure_explicit_types/src/main.nr b/crates/nargo_cli/tests/compile_success_empty/closure_explicit_types/src/main.nr index 13187adefea..7a500b8362c 100644 --- a/crates/nargo_cli/tests/compile_success_empty/closure_explicit_types/src/main.nr +++ b/crates/nargo_cli/tests/compile_success_empty/closure_explicit_types/src/main.nr @@ -3,49 +3,60 @@ fn ret_normal_lambda1() -> fn() -> Field { || 10 } -// explicitly specified empty capture group -fn ret_normal_lambda2() -> fn[]() -> Field { - || 20 -} - // return lamda that captures a thing -fn ret_closure1() -> fn[Field]() -> Field { +fn ret_closure1() -> fn[(Field,)]() -> Field { let x = 20; || x + 10 } // return lamda that captures two things -fn ret_closure2() -> fn[Field,Field]() -> Field { +fn ret_closure2() -> fn[(Field,Field)]() -> Field { let x = 20; let y = 10; || x + y + 10 } // return lamda that captures two things with different types -fn ret_closure3() -> fn[u32,u64]() -> u64 { +fn ret_closure3() -> fn[(u32,u64)]() -> u64 { let x: u32 = 20; let y: u64 = 10; || x as u64 + y + 10 } // accepts closure that has 1 thing in its env, calls it and returns the result -fn accepts_closure1(f: fn[Field]() -> Field) -> Field { +fn accepts_closure1(f: fn[(Field,)]() -> Field) -> Field { f() } // accepts closure that has 1 thing in its env and returns it -fn accepts_closure2(f: fn[Field]() -> Field) -> fn[Field]() -> Field { +fn accepts_closure2(f: fn[(Field,)]() -> Field) -> fn[(Field,)]() -> Field { f } // accepts closure with different types in the capture group -fn accepts_closure3(f: fn[u32, u64]() -> u64) -> u64 { +fn accepts_closure3(f: fn[(u32, u64)]() -> u64) -> u64 { f() } +// generic over closure environments +fn add_results(f1: fn[Env1]() -> Field, f2: fn[Env2]() -> Field) -> Field { + f1() + f2() +} + +// a *really* generic function +fn map(arr: [T; N], f: fn[Env](T) -> U) -> [U; N] { + let first_elem = f(arr[0]); + let mut ret = [first_elem; N]; + + for i in 1 .. N { + ret[i] = f(arr[i]); + } + + ret +} + fn main() { assert(ret_normal_lambda1()() == 10); - assert(ret_normal_lambda2()() == 20); assert(ret_closure1()() == 30); assert(ret_closure2()() == 40); assert(ret_closure3()() == 40); @@ -57,4 +68,12 @@ fn main() { let y: u32 = 30; let z: u64 = 40; assert(accepts_closure3(|| y as u64 + z) == 70); + + assert(add_results(|| 100, || x ) == 150); + assert(add_results(|| x + 100, || w + x ) == 250); + + let arr = [1,2,3,4]; + + assert(map(arr, |n| n + 1) == [2, 3, 4, 5]); + assert(map(arr, |n| n + x) == [51, 52, 53, 54]); } diff --git a/crates/noirc_frontend/src/ast/function.rs b/crates/noirc_frontend/src/ast/function.rs index cedb48b1714..29a1804baf1 100644 --- a/crates/noirc_frontend/src/ast/function.rs +++ b/crates/noirc_frontend/src/ast/function.rs @@ -1,5 +1,7 @@ use std::fmt::Display; +use noirc_errors::Span; + use crate::{token::Attribute, FunctionReturnType, Ident, Pattern, Visibility}; use super::{FunctionDefinition, UnresolvedType}; @@ -67,6 +69,9 @@ impl NoirFunction { pub fn number_of_statements(&self) -> usize { self.def.body.0.len() } + pub fn span(&self) -> Span { + self.def.span + } pub fn foreign(&self) -> Option<&FunctionDefinition> { match &self.kind { diff --git a/crates/noirc_frontend/src/hir/resolution/errors.rs b/crates/noirc_frontend/src/hir/resolution/errors.rs index 1215d897eda..93304ac151c 100644 --- a/crates/noirc_frontend/src/hir/resolution/errors.rs +++ b/crates/noirc_frontend/src/hir/resolution/errors.rs @@ -74,6 +74,8 @@ pub enum ResolverError { ContractFunctionInternalInNormalFunction { span: Span }, #[error("Numeric constants should be printed without formatting braces")] NumericConstantInFormatString { name: String, span: Span }, + #[error("Closure environment must be a tuple or unit type")] + InvalidClosureEnvironment { typ: Type, span: Span }, } impl ResolverError { @@ -283,6 +285,9 @@ impl From for Diagnostic { "Numeric constants should be printed without formatting braces".to_string(), span, ), + ResolverError::InvalidClosureEnvironment { span, typ } => Diagnostic::simple_error( + format!("{typ} is not a valid closure environment type"), + "Closure environment must be a tuple or unit type".to_string(), span), } } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 47d624e028e..21f8fb6f1f3 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -92,6 +92,10 @@ pub struct Resolver<'a> { /// that are captured. We do this in order to create the hidden environment /// parameter for the lambda function. lambda_stack: Vec, + + /// UnresolvedTypes currently don't have Spans, so if we hit an error while resolving a type + /// the best we can do is show the expression where the type is used + current_expr_span: Option, } /// ResolverMetas are tagged onto each definition to track how many times they are used @@ -119,6 +123,7 @@ impl<'a> Resolver<'a> { errors: Vec::new(), lambda_stack: Vec::new(), file, + current_expr_span: None, } } @@ -365,7 +370,19 @@ impl<'a> Resolver<'a> { let args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); let ret = Box::new(self.resolve_type_inner(*ret, new_variables)); let env = Box::new(self.resolve_type_inner(*env, new_variables)); - Type::Function(args, ret, env) + + match *env { + Type::Unit | Type::Tuple(_) | Type::NamedGeneric(_, _) => { + Type::Function(args, ret, env) + } + _ => { + self.push_err(ResolverError::InvalidClosureEnvironment { + typ: *env, + span: self.current_expr_span.unwrap(), + }); + Type::Error + } + } } UnresolvedType::MutableReference(element) => { Type::MutableReference(Box::new(self.resolve_type_inner(*element, new_variables))) @@ -675,6 +692,7 @@ impl<'a> Resolver<'a> { parameter_types.push(typ); } + self.current_expr_span = Some(func.span()); let return_type = Box::new(self.resolve_type(func.return_type())); self.declare_numeric_generics(¶meter_types, &return_type); @@ -975,6 +993,8 @@ impl<'a> Resolver<'a> { } pub fn resolve_expression(&mut self, expr: Expression) -> ExprId { + self.current_expr_span = Some(expr.span); + let hir_expr = match expr.kind { ExpressionKind::Literal(literal) => HirExpression::Literal(match literal { Literal::Bool(b) => HirLiteral::Bool(b), diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index f3b58617e45..b0d4682a9e8 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -969,23 +969,13 @@ fn function_type(type_parser: T) -> impl NoirParser where T: NoirParser, { - let types = type_parser.clone().separated_by(just(Token::Comma)).allow_trailing(); - let args = parenthesized(types.clone()); + let args = parenthesized(type_parser.clone().separated_by(just(Token::Comma)).allow_trailing()); let env = just(Token::LeftBracket) - .ignore_then(types) + .ignore_then(type_parser.clone()) .then_ignore(just(Token::RightBracket)) .or_not() - .map(|args| match args { - Some(args) => { - if args.is_empty() { - UnresolvedType::Unit - } else { - UnresolvedType::Tuple(args) - } - } - None => UnresolvedType::Unit, - }); + .map(|t| t.unwrap_or(UnresolvedType::Unit)); keyword(Keyword::Fn) .ignore_then(env) From 7d96303db2d2948be4f84689e14bc037edece690 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Mon, 21 Aug 2023 12:51:18 +0300 Subject: [PATCH 2/9] fix: code style suggestion --- crates/noirc_frontend/src/ast/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/ast/mod.rs b/crates/noirc_frontend/src/ast/mod.rs index 0a4e69aa55f..8cb9b909c94 100644 --- a/crates/noirc_frontend/src/ast/mod.rs +++ b/crates/noirc_frontend/src/ast/mod.rs @@ -116,7 +116,7 @@ impl std::fmt::Display for UnresolvedType { Function(args, ret, env) => { let args = vecmap(args, ToString::to_string); - match &**env { + match env.as_ref() { UnresolvedType::Unit => { write!(f, "fn({}) -> {ret}", args.join(", ")) } From 94e6a797b7a39586d1ec0e72d5e63008558bbdb7 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Mon, 21 Aug 2023 12:54:32 +0300 Subject: [PATCH 3/9] test: add missing let statement to closure test --- .../compile_success_empty/closure_explicit_types/src/main.nr | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/nargo_cli/tests/compile_success_empty/closure_explicit_types/src/main.nr b/crates/nargo_cli/tests/compile_success_empty/closure_explicit_types/src/main.nr index 7a500b8362c..133bc1b4206 100644 --- a/crates/nargo_cli/tests/compile_success_empty/closure_explicit_types/src/main.nr +++ b/crates/nargo_cli/tests/compile_success_empty/closure_explicit_types/src/main.nr @@ -69,6 +69,7 @@ fn main() { let z: u64 = 40; assert(accepts_closure3(|| y as u64 + z) == 70); + let w = 50; assert(add_results(|| 100, || x ) == 150); assert(add_results(|| x + 100, || w + x ) == 250); From c5053317c3a057a61ad48bed0ea60eaa92ccd0c8 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Tue, 22 Aug 2023 08:22:55 +0300 Subject: [PATCH 4/9] fix: set current_expr_span earlier --- crates/noirc_frontend/src/hir/resolution/resolver.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 21f8fb6f1f3..9166b798c26 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -674,6 +674,7 @@ impl<'a> Resolver<'a> { } }); + self.current_expr_span = Some(func.span()); let mut parameters = vec![]; let mut parameter_types = vec![]; @@ -692,7 +693,6 @@ impl<'a> Resolver<'a> { parameter_types.push(typ); } - self.current_expr_span = Some(func.span()); let return_type = Box::new(self.resolve_type(func.return_type())); self.declare_numeric_generics(¶meter_types, &return_type); From 95ec81062d838aae15a568de8721c53d397a1ba1 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Wed, 23 Aug 2023 08:52:00 +0300 Subject: [PATCH 5/9] feat(parser): add spans to UnresolvedType --- crates/noirc_frontend/src/ast/function.rs | 6 +- crates/noirc_frontend/src/ast/mod.rs | 50 ++++++++++++---- .../src/hir/resolution/resolver.rs | 41 +++++++------ crates/noirc_frontend/src/parser/mod.rs | 4 +- crates/noirc_frontend/src/parser/parser.rs | 58 ++++++++++++------- 5 files changed, 103 insertions(+), 56 deletions(-) diff --git a/crates/noirc_frontend/src/ast/function.rs b/crates/noirc_frontend/src/ast/function.rs index 29a1804baf1..ea6d92f0afd 100644 --- a/crates/noirc_frontend/src/ast/function.rs +++ b/crates/noirc_frontend/src/ast/function.rs @@ -4,7 +4,7 @@ use noirc_errors::Span; use crate::{token::Attribute, FunctionReturnType, Ident, Pattern, Visibility}; -use super::{FunctionDefinition, UnresolvedType}; +use super::{FunctionDefinition, UnresolvedType, UnresolvedTypeData}; // A NoirFunction can be either a foreign low level function or a function definition // A closure / function definition will be stored under a name, so we do not differentiate between their variants @@ -44,7 +44,9 @@ impl NoirFunction { pub fn return_type(&self) -> UnresolvedType { match &self.def.return_type { - FunctionReturnType::Default(_) => UnresolvedType::Unit, + FunctionReturnType::Default(_) => { + UnresolvedType::without_span(UnresolvedTypeData::Unit) + } FunctionReturnType::Ty(ty, _) => ty.clone(), } } diff --git a/crates/noirc_frontend/src/ast/mod.rs b/crates/noirc_frontend/src/ast/mod.rs index 8cb9b909c94..411397a9bb4 100644 --- a/crates/noirc_frontend/src/ast/mod.rs +++ b/crates/noirc_frontend/src/ast/mod.rs @@ -31,7 +31,7 @@ use iter_extended::vecmap; /// require name resolution to resolve any type names used /// for structs within, but are otherwise identical to Types. #[derive(Debug, PartialEq, Eq, Clone, Hash)] -pub enum UnresolvedType { +pub enum UnresolvedTypeData { FieldElement, Array(Option, Box), // [4]Witness = Array(4, Witness) Integer(Signedness, u32), // u32 = Integer(unsigned, 32) @@ -60,6 +60,12 @@ pub enum UnresolvedType { Error, } +#[derive(Debug, PartialEq, Eq, Clone, Hash)] +pub struct UnresolvedType { + pub typ: UnresolvedTypeData, + pub span: Option, +} + /// The precursor to TypeExpression, this is the type that the parser allows /// to be used in the length position of an array type. Only constants, variables, /// and numeric binary operators are allowed here. @@ -76,14 +82,14 @@ pub enum UnresolvedTypeExpression { } impl Recoverable for UnresolvedType { - fn error(_: Span) -> Self { - UnresolvedType::Error + fn error(span: Span) -> Self { + UnresolvedType { typ: UnresolvedTypeData::Error, span: Some(span) } } } -impl std::fmt::Display for UnresolvedType { +impl std::fmt::Display for UnresolvedTypeData { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - use UnresolvedType::*; + use UnresolvedTypeData::*; match self { FieldElement => write!(f, "Field"), Array(len, typ) => match len { @@ -95,7 +101,7 @@ impl std::fmt::Display for UnresolvedType { Signedness::Unsigned => write!(f, "u{num_bits}"), }, Named(s, args) => { - let args = vecmap(args, ToString::to_string); + let args = vecmap(args, |arg| ToString::to_string(&arg.typ)); if args.is_empty() { write!(f, "{s}") } else { @@ -116,12 +122,12 @@ impl std::fmt::Display for UnresolvedType { Function(args, ret, env) => { let args = vecmap(args, ToString::to_string); - match env.as_ref() { - UnresolvedType::Unit => { + match &env.as_ref().typ { + UnresolvedTypeData::Unit => { write!(f, "fn({}) -> {ret}", args.join(", ")) } - UnresolvedType::Tuple(env_types) => { - let env_types = vecmap(env_types, ToString::to_string); + UnresolvedTypeData::Tuple(env_types) => { + let env_types = vecmap(env_types, |arg| ToString::to_string(&arg.typ)); write!(f, "fn[{}]({}) -> {ret}", env_types.join(", "), args.join(", ")) } _ => unreachable!(), @@ -135,6 +141,12 @@ impl std::fmt::Display for UnresolvedType { } } +impl std::fmt::Display for UnresolvedType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.typ.fmt(f) + } +} + impl std::fmt::Display for UnresolvedTypeExpression { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -148,13 +160,27 @@ impl std::fmt::Display for UnresolvedTypeExpression { } impl UnresolvedType { - pub fn from_int_token(token: IntType) -> UnresolvedType { - use {IntType::*, UnresolvedType::Integer}; + pub fn without_span(typ: UnresolvedTypeData) -> UnresolvedType { + UnresolvedType { typ, span: None } + } + + pub fn unspecified() -> UnresolvedType { + UnresolvedType { typ: UnresolvedTypeData::Unspecified, span: None } + } +} + +impl UnresolvedTypeData { + pub fn from_int_token(token: IntType) -> UnresolvedTypeData { + use {IntType::*, UnresolvedTypeData::Integer}; match token { Signed(num_bits) => Integer(Signedness::Signed, num_bits), Unsigned(num_bits) => Integer(Signedness::Unsigned, num_bits), } } + + pub fn with_span(&self, span: Span) -> UnresolvedType { + UnresolvedType { typ: self.clone(), span: Some(span) } + } } #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 9166b798c26..dd30f9dd7fe 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -36,7 +36,8 @@ use crate::{ use crate::{ ArrayLiteral, ContractFunctionType, Distinctness, Generics, LValue, NoirStruct, NoirTypeAlias, Path, Pattern, Shared, StructType, Trait, Type, TypeAliasType, TypeBinding, TypeVariable, - UnaryOp, UnresolvedGenerics, UnresolvedType, UnresolvedTypeExpression, Visibility, ERROR_IDENT, + UnaryOp, UnresolvedGenerics, UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression, + Visibility, ERROR_IDENT, }; use fm::FileId; use iter_extended::vecmap; @@ -336,9 +337,11 @@ impl<'a> Resolver<'a> { /// Translates an UnresolvedType into a Type and appends any /// freshly created TypeVariables created to new_variables. fn resolve_type_inner(&mut self, typ: UnresolvedType, new_variables: &mut Generics) -> Type { - match typ { - UnresolvedType::FieldElement => Type::FieldElement, - UnresolvedType::Array(size, elem) => { + use UnresolvedTypeData::*; + + match typ.typ { + FieldElement => Type::FieldElement, + Array(size, elem) => { let elem = Box::new(self.resolve_type_inner(*elem, new_variables)); let size = if size.is_none() { Type::NotConstant @@ -347,26 +350,26 @@ impl<'a> Resolver<'a> { }; Type::Array(Box::new(size), elem) } - UnresolvedType::Expression(expr) => self.convert_expression_type(expr), - UnresolvedType::Integer(sign, bits) => Type::Integer(sign, bits), - UnresolvedType::Bool => Type::Bool, - UnresolvedType::String(size) => { + Expression(expr) => self.convert_expression_type(expr), + Integer(sign, bits) => Type::Integer(sign, bits), + Bool => Type::Bool, + String(size) => { let resolved_size = self.resolve_array_size(size, new_variables); Type::String(Box::new(resolved_size)) } - UnresolvedType::FormatString(size, fields) => { + FormatString(size, fields) => { let resolved_size = self.convert_expression_type(size); let fields = self.resolve_type_inner(*fields, new_variables); Type::FmtString(Box::new(resolved_size), Box::new(fields)) } - UnresolvedType::Unit => Type::Unit, - UnresolvedType::Unspecified => Type::Error, - UnresolvedType::Error => Type::Error, - UnresolvedType::Named(path, args) => self.resolve_named_type(path, args, new_variables), - UnresolvedType::Tuple(fields) => { + Unit => Type::Unit, + Unspecified => Type::Error, + Error => Type::Error, + Named(path, args) => self.resolve_named_type(path, args, new_variables), + Tuple(fields) => { Type::Tuple(vecmap(fields, |field| self.resolve_type_inner(field, new_variables))) } - UnresolvedType::Function(args, ret, env) => { + Function(args, ret, env) => { let args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); let ret = Box::new(self.resolve_type_inner(*ret, new_variables)); let env = Box::new(self.resolve_type_inner(*env, new_variables)); @@ -384,7 +387,7 @@ impl<'a> Resolver<'a> { } } } - UnresolvedType::MutableReference(element) => { + MutableReference(element) => { Type::MutableReference(Box::new(self.resolve_type_inner(*element, new_variables))) } } @@ -593,9 +596,9 @@ impl<'a> Resolver<'a> { /// Translates a (possibly Unspecified) UnresolvedType to a Type. /// Any UnresolvedType::Unspecified encountered are replaced with fresh type variables. fn resolve_inferred_type(&mut self, typ: UnresolvedType) -> Type { - match typ { - UnresolvedType::Unspecified => self.interner.next_type_variable(), - other => self.resolve_type_inner(other, &mut vec![]), + match &typ.typ { + UnresolvedTypeData::Unspecified => self.interner.next_type_variable(), + _ => self.resolve_type_inner(typ, &mut vec![]), } } diff --git a/crates/noirc_frontend/src/parser/mod.rs b/crates/noirc_frontend/src/parser/mod.rs index ad519836b39..64f8c077648 100644 --- a/crates/noirc_frontend/src/parser/mod.rs +++ b/crates/noirc_frontend/src/parser/mod.rs @@ -403,7 +403,7 @@ impl ForRange { // let fresh1 = array; let let_array = Statement::Let(LetStatement { pattern: Pattern::Identifier(array_ident.clone()), - r#type: UnresolvedType::Unspecified, + r#type: UnresolvedType::unspecified(), expression: array, }); @@ -436,7 +436,7 @@ impl ForRange { // let elem = array[i]; let let_elem = Statement::Let(LetStatement { pattern: Pattern::Identifier(identifier), - r#type: UnresolvedType::Unspecified, + r#type: UnresolvedType::unspecified(), expression: Expression::new(loop_element, array_span), }); diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index b0d4682a9e8..3b8e6828120 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -30,7 +30,9 @@ use super::{ ForRange, NoirParser, ParsedModule, ParserError, ParserErrorReason, Precedence, SubModule, TopLevelStatement, }; -use crate::ast::{Expression, ExpressionKind, LetStatement, Statement, UnresolvedType}; +use crate::ast::{ + Expression, ExpressionKind, LetStatement, Statement, UnresolvedType, UnresolvedTypeData, +}; use crate::lexer::Lexer; use crate::parser::{force, ignore_then_commit, statement_recovery}; use crate::token::{Attribute, Keyword, Token, TokenKind}; @@ -256,7 +258,7 @@ fn lambda_return_type() -> impl NoirParser { just(Token::Arrow) .ignore_then(parse_type()) .or_not() - .map(|ret| ret.unwrap_or(UnresolvedType::Unspecified)) + .map(|ret| ret.unwrap_or_else(UnresolvedType::unspecified)) } fn function_return_type() -> impl NoirParser<((Distinctness, Visibility), FunctionReturnType)> { @@ -295,7 +297,7 @@ fn lambda_parameters() -> impl NoirParser> { let parameter = pattern() .recover_via(parameter_name_recovery()) - .then(typ.or_not().map(|typ| typ.unwrap_or(UnresolvedType::Unspecified))); + .then(typ.or_not().map(|typ| typ.unwrap_or_else(UnresolvedType::unspecified))); parameter .separated_by(just(Token::Comma)) @@ -345,12 +347,13 @@ fn self_parameter() -> impl NoirParser<(Pattern, UnresolvedType, Visibility)> { .map(|(pattern_keyword, span)| { let ident = Ident::new("self".to_string(), span); let path = Path::from_single("Self".to_owned(), span); - let mut self_type = UnresolvedType::Named(path, vec![]); + let mut self_type = UnresolvedTypeData::Named(path, vec![]).with_span(span); let mut pattern = Pattern::Identifier(ident); match pattern_keyword { Some((Token::Ampersand, _)) => { - self_type = UnresolvedType::MutableReference(Box::new(self_type)); + self_type = + UnresolvedTypeData::MutableReference(Box::new(self_type)).with_span(span) } Some((Token::Keyword(_), span)) => { pattern = Pattern::Mutable(Box::new(pattern), span); @@ -582,7 +585,7 @@ fn check_statements_require_semicolon( fn optional_type_annotation<'a>() -> impl NoirParser + 'a { ignore_then_commit(just(Token::Colon), parse_type()) .or_not() - .map(|r#type| r#type.unwrap_or(UnresolvedType::Unspecified)) + .map(|r#type| r#type.unwrap_or_else(UnresolvedType::unspecified)) } fn module_declaration() -> impl NoirParser { @@ -859,11 +862,15 @@ fn maybe_comp_time() -> impl NoirParser<()> { } fn field_type() -> impl NoirParser { - maybe_comp_time().then_ignore(keyword(Keyword::Field)).map(|_| UnresolvedType::FieldElement) + maybe_comp_time() + .then_ignore(keyword(Keyword::Field)) + .map_with_span(|_, span| UnresolvedTypeData::FieldElement.with_span(span)) } fn bool_type() -> impl NoirParser { - maybe_comp_time().then_ignore(keyword(Keyword::Bool)).map(|_| UnresolvedType::Bool) + maybe_comp_time() + .then_ignore(keyword(Keyword::Bool)) + .map_with_span(|_, span| UnresolvedTypeData::Bool.with_span(span)) } fn string_type() -> impl NoirParser { @@ -871,7 +878,7 @@ fn string_type() -> impl NoirParser { .ignore_then( type_expression().delimited_by(just(Token::Less), just(Token::Greater)).or_not(), ) - .map(UnresolvedType::String) + .map_with_span(|expr, span| UnresolvedTypeData::String(expr).with_span(span)) } fn format_string_type( @@ -884,7 +891,9 @@ fn format_string_type( .then(type_parser) .delimited_by(just(Token::Less), just(Token::Greater)), ) - .map(|(size, fields)| UnresolvedType::FormatString(size, Box::new(fields))) + .map_with_span(|(size, fields), span| { + UnresolvedTypeData::FormatString(size, Box::new(fields)).with_span(span) + }) } fn int_type() -> impl NoirParser { @@ -896,8 +905,8 @@ fn int_type() -> impl NoirParser { } })) .validate(|(_, token), span, emit| { - let typ = UnresolvedType::from_int_token(token); - if let UnresolvedType::Integer(crate::Signedness::Signed, _) = &typ { + let typ = UnresolvedTypeData::from_int_token(token).with_span(span); + if let UnresolvedTypeData::Integer(crate::Signedness::Signed, _) = &typ.typ { let reason = ParserErrorReason::ExperimentalFeature("Signed integer types"); emit(ParserError::with_reason(reason, span)); } @@ -908,7 +917,7 @@ fn int_type() -> impl NoirParser { fn named_type(type_parser: impl NoirParser) -> impl NoirParser { path() .then(generic_type_args(type_parser)) - .map(|(path, args)| UnresolvedType::Named(path, args)) + .map_with_span(|(path, args), span| UnresolvedTypeData::Named(path, args).with_span(span)) } fn generic_type_args( @@ -920,7 +929,8 @@ fn generic_type_args( // 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(UnresolvedType::Expression)) + .or(type_expression() + .map_with_span(|expr, span| UnresolvedTypeData::Expression(expr).with_span(span))) .separated_by(just(Token::Comma)) .allow_trailing() .at_least(1) @@ -934,7 +944,9 @@ fn array_type(type_parser: impl NoirParser) -> impl NoirParser impl NoirParser { @@ -956,11 +968,11 @@ where T: NoirParser, { let fields = type_parser.separated_by(just(Token::Comma)).allow_trailing(); - parenthesized(fields).map(|fields| { + parenthesized(fields).map_with_span(|fields, span| { if fields.is_empty() { - UnresolvedType::Unit + UnresolvedTypeData::Unit.with_span(span) } else { - UnresolvedType::Tuple(fields) + UnresolvedTypeData::Tuple(fields).with_span(span) } }) } @@ -975,14 +987,16 @@ where .ignore_then(type_parser.clone()) .then_ignore(just(Token::RightBracket)) .or_not() - .map(|t| t.unwrap_or(UnresolvedType::Unit)); + .map_with_span(|t, span| t.unwrap_or_else(|| UnresolvedTypeData::Unit.with_span(span))); keyword(Keyword::Fn) .ignore_then(env) .then(args) .then_ignore(just(Token::Arrow)) .then(type_parser) - .map(|((env, args), ret)| UnresolvedType::Function(args, Box::new(ret), Box::new(env))) + .map_with_span(|((env, args), ret), span| { + UnresolvedTypeData::Function(args, Box::new(ret), Box::new(env)).with_span(span) + }) } fn mutable_reference_type(type_parser: T) -> impl NoirParser @@ -992,7 +1006,9 @@ where just(Token::Ampersand) .ignore_then(keyword(Keyword::Mut)) .ignore_then(type_parser) - .map(|element| UnresolvedType::MutableReference(Box::new(element))) + .map_with_span(|element, span| { + UnresolvedTypeData::MutableReference(Box::new(element)).with_span(span) + }) } fn expression() -> impl ExprParser { From 0cc224c629dbb1317543f7fa5ae25363cfbedff8 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Wed, 23 Aug 2023 09:06:56 +0300 Subject: [PATCH 6/9] fix: use UnresolvedType::span for a nicer error --- .../src/hir/resolution/resolver.rs | 16 +++++++--------- crates/noirc_frontend/src/parser/parser.rs | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index dd30f9dd7fe..3c5a41bd2c3 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -93,10 +93,6 @@ pub struct Resolver<'a> { /// that are captured. We do this in order to create the hidden environment /// parameter for the lambda function. lambda_stack: Vec, - - /// UnresolvedTypes currently don't have Spans, so if we hit an error while resolving a type - /// the best we can do is show the expression where the type is used - current_expr_span: Option, } /// ResolverMetas are tagged onto each definition to track how many times they are used @@ -124,7 +120,6 @@ impl<'a> Resolver<'a> { errors: Vec::new(), lambda_stack: Vec::new(), file, - current_expr_span: None, } } @@ -372,6 +367,12 @@ impl<'a> Resolver<'a> { Function(args, ret, env) => { let args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); let ret = Box::new(self.resolve_type_inner(*ret, new_variables)); + + // unwrap() here is valid, because the only places we don't have a span are omitted types + // e.g. a function without return type implicitly has a spanless UnresolvedType::Unit return type + // To get an invalid env type, the user must explicitly specify the type, which will have a span + let env_span = env.span.unwrap(); + let env = Box::new(self.resolve_type_inner(*env, new_variables)); match *env { @@ -381,7 +382,7 @@ impl<'a> Resolver<'a> { _ => { self.push_err(ResolverError::InvalidClosureEnvironment { typ: *env, - span: self.current_expr_span.unwrap(), + span: env_span, }); Type::Error } @@ -677,7 +678,6 @@ impl<'a> Resolver<'a> { } }); - self.current_expr_span = Some(func.span()); let mut parameters = vec![]; let mut parameter_types = vec![]; @@ -996,8 +996,6 @@ impl<'a> Resolver<'a> { } pub fn resolve_expression(&mut self, expr: Expression) -> ExprId { - self.current_expr_span = Some(expr.span); - let hir_expr = match expr.kind { ExpressionKind::Literal(literal) => HirExpression::Literal(match literal { Literal::Bool(b) => HirLiteral::Bool(b), diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 3b8e6828120..cd9cfc47886 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -353,7 +353,7 @@ fn self_parameter() -> impl NoirParser<(Pattern, UnresolvedType, Visibility)> { match pattern_keyword { Some((Token::Ampersand, _)) => { self_type = - UnresolvedTypeData::MutableReference(Box::new(self_type)).with_span(span) + UnresolvedTypeData::MutableReference(Box::new(self_type)).with_span(span); } Some((Token::Keyword(_), span)) => { pattern = Pattern::Mutable(Box::new(pattern), span); From a89b2e5fef7568ba42d52695ec5f70f8f1a45aee Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Wed, 23 Aug 2023 09:18:11 +0300 Subject: [PATCH 7/9] fix: add clarification for when UnresolvedType::span is None --- crates/noirc_frontend/src/ast/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/noirc_frontend/src/ast/mod.rs b/crates/noirc_frontend/src/ast/mod.rs index 411397a9bb4..e92d333fd69 100644 --- a/crates/noirc_frontend/src/ast/mod.rs +++ b/crates/noirc_frontend/src/ast/mod.rs @@ -63,6 +63,10 @@ pub enum UnresolvedTypeData { #[derive(Debug, PartialEq, Eq, Clone, Hash)] pub struct UnresolvedType { pub typ: UnresolvedTypeData, + + // The span is None in the cases where the User omitted a type: + // fn Foo() {} --- return type is UnresolvedType::Unit without a span + // let x = 100; --- type is UnresolvedType::Unspecified without a span pub span: Option, } From c33be8f8dddf26548ae825c3b8baa38f9b308fa3 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Fri, 25 Aug 2023 08:03:02 +0300 Subject: [PATCH 8/9] fix: use expect() instead of unwrap() --- crates/noirc_frontend/src/hir/resolution/resolver.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 3c5a41bd2c3..d815018ea64 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -368,10 +368,10 @@ impl<'a> Resolver<'a> { let args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); let ret = Box::new(self.resolve_type_inner(*ret, new_variables)); - // unwrap() here is valid, because the only places we don't have a span are omitted types + // expect() here is valid, because the only places we don't have a span are omitted types // e.g. a function without return type implicitly has a spanless UnresolvedType::Unit return type // To get an invalid env type, the user must explicitly specify the type, which will have a span - let env_span = env.span.unwrap(); + let env_span = env.span.expect("Invalid closure environment type"); let env = Box::new(self.resolve_type_inner(*env, new_variables)); From f2a90163498c9f8a0d47202e3b5be85c655da889 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Mon, 28 Aug 2023 08:04:32 +0300 Subject: [PATCH 9/9] fix: better error msg --- crates/noirc_frontend/src/hir/resolution/resolver.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index d815018ea64..c37c51805b7 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -371,7 +371,7 @@ impl<'a> Resolver<'a> { // expect() here is valid, because the only places we don't have a span are omitted types // e.g. a function without return type implicitly has a spanless UnresolvedType::Unit return type // To get an invalid env type, the user must explicitly specify the type, which will have a span - let env_span = env.span.expect("Invalid closure environment type"); + let env_span = env.span.expect("Unexpected missing span for closure environment type"); let env = Box::new(self.resolve_type_inner(*env, new_variables));