From 19e58a91a3b1d0534d8b0198347e3a2fb5488599 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 2 Aug 2024 15:37:37 -0500 Subject: [PATCH] fix: Derive generic types (#5674) # Description ## Problem\* Resolves an issue alluded to in https://github.com/noir-lang/noir/pull/5667 where we still could not derive generic types ## Summary\* This PR allows us to derive generic types by allowing a new "resolved generic" type in the UnresolvedGeneric enum. The issue before was that when we splice `Type`s into quoted snippets we lower them as a special token kind which preserves the original `Type` so that we don't have to worry about re-resolving it if it is a struct type that is not imported into the caller's scope, etc. This also means though that any type variables in the original type are preserved. In the following snippet: ```rs // Lets say this is `MyType` let my_type: Type = ...; quote { impl Foo for $my_type { ... } } ``` The impl will introduce a fresh generic `T` into scope, but the `T` in `MyType` will still be the old `T` with a different TypeVariableId. To fix this there were two options: - Add another function to lower types in a way they'd need to be reparsed, and would thus pick up new type variables but be susceptible to "name not in scope" errors mentioned earlier. - Somehow get the `T` in `impl` to refer to the same `T` in `MyType`. I chose the later approach since it is much simpler from a user's perspective and leads to fewer footguns over forgetting to convert the type into a `Quoted` before splicing it into other code. This way, users just need to ensure the generics inserted as part of the impl generics come from the type which is what is typically done anyway. This means that the original snippet above will actually still fail since it still introduces a new `T`. To fix it, we have to get the T from the type's generics: ```rs let generics = my_struct.generics().map(|g| quote { $g }).join(quote {,}); let my_type = my_struct.as_type(); quote { impl<$generics> Foo for $my_type { ... } } ``` This is what `derive` does already since you don't know the names of the generics ahead of time in practice. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[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: Michael J Klein --- aztec_macros/src/utils/parse_utils.rs | 1 + compiler/noirc_frontend/src/ast/expression.rs | 19 ++++- compiler/noirc_frontend/src/elaborator/mod.rs | 77 ++++++++++++++----- .../src/hir/comptime/interpreter/builtin.rs | 10 +-- .../noirc_frontend/src/hir/comptime/value.rs | 7 +- .../src/hir/resolution/errors.rs | 9 +++ compiler/noirc_frontend/src/hir_def/types.rs | 6 ++ .../src/parser/parser/function.rs | 12 ++- .../noirc_frontend/src/parser/parser/types.rs | 2 +- noir_stdlib/src/cmp.nr | 4 +- noir_stdlib/src/default.nr | 2 +- noir_stdlib/src/meta/struct_def.nr | 5 +- .../derive_impl/src/main.nr | 2 +- .../execution_success/derive/src/main.nr | 19 +++-- tooling/nargo_fmt/src/utils.rs | 3 + 15 files changed, 134 insertions(+), 44 deletions(-) diff --git a/aztec_macros/src/utils/parse_utils.rs b/aztec_macros/src/utils/parse_utils.rs index 06093a4753e..a2c177026c4 100644 --- a/aztec_macros/src/utils/parse_utils.rs +++ b/aztec_macros/src/utils/parse_utils.rs @@ -349,6 +349,7 @@ fn empty_unresolved_generic(unresolved_generic: &mut UnresolvedGeneric) { empty_ident(ident); empty_unresolved_type(typ); } + UnresolvedGeneric::Resolved(..) => (), } } diff --git a/compiler/noirc_frontend/src/ast/expression.rs b/compiler/noirc_frontend/src/ast/expression.rs index e176c7fc8b4..7a324eb2600 100644 --- a/compiler/noirc_frontend/src/ast/expression.rs +++ b/compiler/noirc_frontend/src/ast/expression.rs @@ -7,7 +7,7 @@ use crate::ast::{ }; use crate::hir::def_collector::errors::DefCollectorErrorKind; use crate::macros_api::StructId; -use crate::node_interner::ExprId; +use crate::node_interner::{ExprId, QuotedTypeId}; use crate::token::{Attributes, Token, Tokens}; use crate::{Kind, Type}; use acvm::{acir::AcirField, FieldElement}; @@ -51,7 +51,16 @@ pub type UnresolvedGenerics = Vec; #[derive(Debug, PartialEq, Eq, Clone, Hash)] pub enum UnresolvedGeneric { Variable(Ident), - Numeric { ident: Ident, typ: UnresolvedType }, + Numeric { + ident: Ident, + typ: UnresolvedType, + }, + + /// Already-resolved generics can be parsed as generics when a macro + /// splices existing types into a generic list. In this case we have + /// to validate the type refers to a named generic and treat that + /// as a ResolvedGeneric when this is resolved. + Resolved(QuotedTypeId, Span), } impl UnresolvedGeneric { @@ -61,6 +70,7 @@ impl UnresolvedGeneric { UnresolvedGeneric::Numeric { ident, typ } => { ident.0.span().merge(typ.span.unwrap_or_default()) } + UnresolvedGeneric::Resolved(_, span) => *span, } } @@ -71,6 +81,9 @@ impl UnresolvedGeneric { let typ = self.resolve_numeric_kind_type(typ)?; Ok(Kind::Numeric(Box::new(typ))) } + UnresolvedGeneric::Resolved(..) => { + panic!("Don't know the kind of a resolved generic here") + } } } @@ -94,6 +107,7 @@ impl UnresolvedGeneric { pub(crate) fn ident(&self) -> &Ident { match self { UnresolvedGeneric::Variable(ident) | UnresolvedGeneric::Numeric { ident, .. } => ident, + UnresolvedGeneric::Resolved(..) => panic!("UnresolvedGeneric::Resolved no ident"), } } } @@ -103,6 +117,7 @@ impl Display for UnresolvedGeneric { match self { UnresolvedGeneric::Variable(ident) => write!(f, "{ident}"), UnresolvedGeneric::Numeric { ident, typ } => write!(f, "let {ident}: {typ}"), + UnresolvedGeneric::Resolved(..) => write!(f, "(resolved)"), } } } diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 9c2467d38e6..873da5a0c5e 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -523,35 +523,72 @@ impl<'context> Elaborator<'context> { /// Each generic will have a fresh Shared associated with it. pub fn add_generics(&mut self, generics: &UnresolvedGenerics) -> Generics { vecmap(generics, |generic| { - // Map the generic to a fresh type variable - let id = self.interner.next_type_variable_id(); - let typevar = TypeVariable::unbound(id); - let ident = generic.ident(); - let span = ident.0.span(); + let mut is_error = false; + let (type_var, name, kind) = match self.resolve_generic(generic) { + Ok(values) => values, + Err(error) => { + self.push_err(error); + is_error = true; + let id = self.interner.next_type_variable_id(); + (TypeVariable::unbound(id), Rc::new("(error)".into()), Kind::Normal) + } + }; - // Resolve the generic's kind - let kind = self.resolve_generic_kind(generic); + let span = generic.span(); + let name_owned = name.as_ref().clone(); + let resolved_generic = ResolvedGeneric { name, type_var, kind, span }; // Check for name collisions of this generic - let name = Rc::new(ident.0.contents.clone()); - - let resolved_generic = - ResolvedGeneric { name: name.clone(), type_var: typevar.clone(), kind, span }; - - if let Some(generic) = self.find_generic(&name) { - self.push_err(ResolverError::DuplicateDefinition { - name: ident.0.contents.clone(), - first_span: generic.span, - second_span: span, - }); - } else { - self.generics.push(resolved_generic.clone()); + // Checking `is_error` here prevents DuplicateDefinition errors when + // we have multiple generics from macros which fail to resolve and + // are all given the same default name "(error)". + if !is_error { + if let Some(generic) = self.find_generic(&name_owned) { + self.push_err(ResolverError::DuplicateDefinition { + name: name_owned, + first_span: generic.span, + second_span: span, + }); + } else { + self.generics.push(resolved_generic.clone()); + } } resolved_generic }) } + fn resolve_generic( + &mut self, + generic: &UnresolvedGeneric, + ) -> Result<(TypeVariable, Rc, Kind), ResolverError> { + // Map the generic to a fresh type variable + match generic { + UnresolvedGeneric::Variable(_) | UnresolvedGeneric::Numeric { .. } => { + let id = self.interner.next_type_variable_id(); + let typevar = TypeVariable::unbound(id); + let ident = generic.ident(); + + let kind = self.resolve_generic_kind(generic); + let name = Rc::new(ident.0.contents.clone()); + Ok((typevar, name, kind)) + } + // An already-resolved generic is only possible if it is the result of a + // previous macro call being inserted into a generics list. + UnresolvedGeneric::Resolved(id, span) => { + match self.interner.get_quoted_type(*id).follow_bindings() { + Type::NamedGeneric(type_variable, name, kind) => { + Ok((type_variable, name, kind)) + } + other => Err(ResolverError::MacroResultInGenericsListNotAGeneric { + span: *span, + typ: other.clone(), + }), + } + } + } + } + /// Return the kind of an unresolved generic. /// If a numeric generic has been specified, resolve the annotated type to make /// sure only primitive numeric types are being used. diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 927468e35a6..b66d375d922 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -294,7 +294,7 @@ fn struct_def_as_type( Ok(Value::Type(Type::Struct(struct_def_rc, generics))) } -/// fn generics(self) -> [Quoted] +/// fn generics(self) -> [Type] fn struct_def_generics( interner: &NodeInterner, arguments: Vec<(Value, Location)>, @@ -314,12 +314,10 @@ fn struct_def_generics( let struct_def = interner.get_struct(struct_def); let struct_def = struct_def.borrow(); - let generics = struct_def.generics.iter().map(|generic| { - let name = Token::Ident(generic.type_var.borrow().to_string()); - Value::Quoted(Rc::new(vec![name])) - }); + let generics = + struct_def.generics.iter().map(|generic| Value::Type(generic.clone().as_named_generic())); - let typ = Type::Slice(Box::new(Type::Quoted(QuotedType::Quoted))); + let typ = Type::Slice(Box::new(Type::Quoted(QuotedType::Type))); Ok(Value::Slice(generics.collect(), typ)) } diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index 61475fa60e1..1264cd21635 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -497,7 +497,12 @@ impl<'value, 'interner> Display for ValuePrinter<'value, 'interner> { Value::Quoted(tokens) => { write!(f, "quote {{")?; for token in tokens.iter() { - write!(f, " {token}")?; + match token { + Token::QuotedType(id) => { + write!(f, " {}", self.interner.get_quoted_type(*id))?; + } + other => write!(f, " {other}")?, + } } write!(f, " }}") } diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 1cc1abfa495..cfaa2063c40 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -114,6 +114,8 @@ pub enum ResolverError { MacroIsNotComptime { span: Span }, #[error("Annotation name must refer to a comptime function")] NonFunctionInAnnotation { span: Span }, + #[error("Type `{typ}` was inserted into the generics list from a macro, but is not a generic")] + MacroResultInGenericsListNotAGeneric { span: Span, typ: Type }, } impl ResolverError { @@ -458,6 +460,13 @@ impl<'a> From<&'a ResolverError> for Diagnostic { *span, ) }, + ResolverError::MacroResultInGenericsListNotAGeneric { span, typ } => { + Diagnostic::simple_error( + format!("Type `{typ}` was inserted into a generics list from a macro, but it is not a generic"), + format!("Type `{typ}` is not a generic"), + *span, + ) + } } } } diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index bf46917a58f..177d23c74dd 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -193,6 +193,12 @@ pub struct ResolvedGeneric { pub span: Span, } +impl ResolvedGeneric { + pub fn as_named_generic(self) -> Type { + Type::NamedGeneric(self.type_var, self.name, self.kind) + } +} + impl std::hash::Hash for StructType { fn hash(&self, state: &mut H) { self.id.hash(state); diff --git a/compiler/noirc_frontend/src/parser/parser/function.rs b/compiler/noirc_frontend/src/parser/parser/function.rs index 2fd337e1cb1..3de48d2e02a 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -2,9 +2,10 @@ use super::{ attributes::{attributes, validate_attributes}, block, fresh_statement, ident, keyword, maybe_comp_time, nothing, optional_visibility, parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, + primitives::token_kind, self_parameter, where_clause, NoirParser, }; -use crate::token::{Keyword, Token}; +use crate::token::{Keyword, Token, TokenKind}; use crate::{ast::IntegerBitSize, parser::spanned}; use crate::{ ast::{ @@ -110,8 +111,15 @@ pub(super) fn generic_type() -> impl NoirParser { ident().map(UnresolvedGeneric::Variable) } +pub(super) fn resolved_generic() -> impl NoirParser { + token_kind(TokenKind::QuotedType).map_with_span(|token, span| match token { + Token::QuotedType(id) => UnresolvedGeneric::Resolved(id, span), + _ => unreachable!("token_kind(QuotedType) guarantees we parse a quoted type"), + }) +} + pub(super) fn generic() -> impl NoirParser { - generic_type().or(numeric_generic()) + generic_type().or(numeric_generic()).or(resolved_generic()) } /// non_empty_ident_list: ident ',' non_empty_ident_list diff --git a/compiler/noirc_frontend/src/parser/parser/types.rs b/compiler/noirc_frontend/src/parser/parser/types.rs index fca29bf1e7a..7c2bdcb9fa3 100644 --- a/compiler/noirc_frontend/src/parser/parser/types.rs +++ b/compiler/noirc_frontend/src/parser/parser/types.rs @@ -133,7 +133,7 @@ fn quoted_type() -> impl NoirParser { /// This is the type of an already resolved type. /// The only way this can appear in the token input is if an already resolved `Type` object /// was spliced into a macro's token stream via the `$` operator. -fn resolved_type() -> impl NoirParser { +pub(super) fn resolved_type() -> impl NoirParser { token_kind(TokenKind::QuotedType).map_with_span(|token, span| match token { Token::QuotedType(id) => UnresolvedTypeData::Resolved(id).with_span(span), _ => unreachable!("token_kind(QuotedType) guarantees we parse a quoted type"), diff --git a/noir_stdlib/src/cmp.nr b/noir_stdlib/src/cmp.nr index d2cf6b3836a..94cd284e238 100644 --- a/noir_stdlib/src/cmp.nr +++ b/noir_stdlib/src/cmp.nr @@ -10,9 +10,9 @@ trait Eq { comptime fn derive_eq(s: StructDefinition) -> Quoted { let typ = s.as_type(); - let impl_generics = s.generics().join(quote {,}); + let impl_generics = s.generics().map(|g| quote { $g }).join(quote {,}); - let where_clause = s.generics().map(|name| quote { $name: Default }).join(quote {,}); + let where_clause = s.generics().map(|name| quote { $name: Eq }).join(quote {,}); // `(self.a == other.a) & (self.b == other.b) & ...` let equalities = s.fields().map( diff --git a/noir_stdlib/src/default.nr b/noir_stdlib/src/default.nr index f0d98205a90..4fbde09b512 100644 --- a/noir_stdlib/src/default.nr +++ b/noir_stdlib/src/default.nr @@ -10,7 +10,7 @@ trait Default { comptime fn derive_default(s: StructDefinition) -> Quoted { let typ = s.as_type(); - let impl_generics = s.generics().join(quote {,}); + let impl_generics = s.generics().map(|g| quote { $g }).join(quote {,}); let where_clause = s.generics().map(|name| quote { $name: Default }).join(quote {,}); diff --git a/noir_stdlib/src/meta/struct_def.nr b/noir_stdlib/src/meta/struct_def.nr index 7382aeb8e75..8d3f9ceb8a5 100644 --- a/noir_stdlib/src/meta/struct_def.nr +++ b/noir_stdlib/src/meta/struct_def.nr @@ -4,10 +4,9 @@ impl StructDefinition { #[builtin(struct_def_as_type)] fn as_type(self) -> Type {} - /// Return each generic on this struct. The names of these generics are unchanged - /// so users may need to keep name collisions in mind if this is used directly in a macro. + /// Return each generic on this struct. #[builtin(struct_def_generics)] - fn generics(self) -> [Quoted] {} + fn generics(self) -> [Type] {} /// Returns (name, type) pairs of each field in this struct. Each type is as-is /// with any generic arguments unchanged. diff --git a/test_programs/compile_success_empty/derive_impl/src/main.nr b/test_programs/compile_success_empty/derive_impl/src/main.nr index 62b8a0421e6..69cb641e7c7 100644 --- a/test_programs/compile_success_empty/derive_impl/src/main.nr +++ b/test_programs/compile_success_empty/derive_impl/src/main.nr @@ -1,5 +1,5 @@ comptime fn derive_default(typ: StructDefinition) -> Quoted { - let generics: [Quoted] = typ.generics(); + let generics = typ.generics(); assert_eq( generics.len(), 0, "derive_default: Deriving Default on generic types is currently unimplemented" ); diff --git a/test_programs/execution_success/derive/src/main.nr b/test_programs/execution_success/derive/src/main.nr index e4148f2c944..f344defe41e 100644 --- a/test_programs/execution_success/derive/src/main.nr +++ b/test_programs/execution_success/derive/src/main.nr @@ -8,7 +8,7 @@ struct MyStruct { my_field: u32 } comptime fn derive_do_nothing(s: StructDefinition) -> Quoted { let typ = s.as_type(); - let generics = s.generics().join(quote {,}); + let generics = s.generics().map(|g| quote { $g }).join(quote {,}); quote { impl<$generics> DoNothing for $typ { fn do_nothing(_self: Self) { @@ -21,15 +21,24 @@ comptime fn derive_do_nothing(s: StructDefinition) -> Quoted { // Test stdlib derive fns & multiple traits #[derive(Eq, Default)] -struct MyOtherStruct { - field1: u32, - field2: u64, +struct MyOtherStruct { + field1: A, + field2: B, + field3: MyOtherOtherStruct, +} + +#[derive(Eq, Default)] +struct MyOtherOtherStruct { + x: T, } fn main() { let s = MyStruct { my_field: 1 }; s.do_nothing(); - let o = MyOtherStruct::default(); + let o: MyOtherStruct = MyOtherStruct::default(); + assert_eq(o, o); + + let o: MyOtherStruct]> = MyOtherStruct::default(); assert_eq(o, o); } diff --git a/tooling/nargo_fmt/src/utils.rs b/tooling/nargo_fmt/src/utils.rs index 293351055e1..83634b718e2 100644 --- a/tooling/nargo_fmt/src/utils.rs +++ b/tooling/nargo_fmt/src/utils.rs @@ -188,6 +188,9 @@ impl HasItem for UnresolvedGeneric { result.push_str(&typ); result } + UnresolvedGeneric::Resolved(..) => { + unreachable!("Found macro result UnresolvedGeneric::Resolved in formatter") + } } } }