From 555d27da71fe9a514a9528df475adcfeaf6d0c0c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 8 Jan 2025 11:57:17 -0600 Subject: [PATCH 01/10] Get a function with a where clause not specifying assoc types working --- compiler/noirc_frontend/src/elaborator/mod.rs | 108 ++++++++++++++---- .../noirc_frontend/src/elaborator/traits.rs | 2 +- .../noirc_frontend/src/elaborator/types.rs | 14 ++- 3 files changed, 96 insertions(+), 28 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index aeee417789..a2b025116d 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -475,7 +475,7 @@ impl<'context> Elaborator<'context> { self.add_existing_variable_to_scope(name, parameter.clone(), warn_if_unused); } - self.add_trait_constraints_to_scope(&func_meta); + self.add_trait_constraints_to_scope(&func_meta.trait_constraints, func_meta.location.span); let (hir_func, body_type) = match kind { FunctionKind::Builtin @@ -501,7 +501,7 @@ impl<'context> Elaborator<'context> { // when multiple impls are available. Instead we default first to choose the Field or u64 impl. self.check_and_pop_function_context(); - self.remove_trait_constraints_from_scope(&func_meta); + self.remove_trait_constraints_from_scope(&func_meta.trait_constraints); let func_scope_tree = self.scopes.end_function(); @@ -733,24 +733,88 @@ impl<'context> Elaborator<'context> { None } - /// TODO: This is currently only respected for generic free functions - /// there's a bunch of other places where trait constraints can pop up fn resolve_trait_constraints( &mut self, where_clause: &[UnresolvedTraitConstraint], + add_constraints_to_scope: bool, ) -> Vec { where_clause .iter() - .filter_map(|constraint| self.resolve_trait_constraint(constraint)) + .filter_map(|constraint| { + self.resolve_trait_constraint(constraint, add_constraints_to_scope) + }) .collect() } - pub fn resolve_trait_constraint( + fn desugar_trait_constraints( + &mut self, + where_clause: &mut [UnresolvedTraitConstraint], + generics: &mut Vec, + ) { + for constraint in where_clause { + self.add_missing_named_generics(&mut constraint.trait_bound, generics); + } + } + + /// For each associated type that isn't mentioned in a trait bound, this adds + /// the type as an implicit generic to the function. For example, this will + /// turn a function using a trait with 2 associated types: + /// + /// `fn foo() where T: Foo { ... }` + /// + /// into: + /// `fn foo() where T: Foo { ... }` + fn add_missing_named_generics( + &mut self, + bound: &mut TraitBound, + generics: &mut Vec, + ) { + let Some(the_trait) = self.lookup_trait_or_error(bound.trait_path.clone()) else { + return; + }; + + if the_trait.associated_types.len() > bound.trait_generics.named_args.len() { + for associated_type in the_trait.associated_types.clone() { + if !bound + .trait_generics + .named_args + .iter() + .any(|(name, _)| name.0.contents == *associated_type.name.as_ref()) + { + // This generic isn't contained in the bound's named arguments, + // implicitly add it to the function + let new_generic_id = self.interner.next_type_variable_id(); + let new_generic = TypeVariable::unbound(new_generic_id, Kind::Normal); + generics.push(new_generic.clone()); + + let span = bound.trait_path.span; + let typ = Type::NamedGeneric(new_generic, associated_type.name.clone()); + let typ = self.interner.push_quoted_type(typ); + let typ = UnresolvedTypeData::Resolved(typ).with_span(span); + let ident = Ident::new(associated_type.name.as_ref().clone(), span); + bound.trait_generics.named_args.push((ident, typ)); + } + } + } + } + + fn resolve_trait_constraint( &mut self, constraint: &UnresolvedTraitConstraint, + add_constraints_to_scope: bool, ) -> Option { let typ = self.resolve_type(constraint.typ.clone()); let trait_bound = self.resolve_trait_bound(&constraint.trait_bound)?; + + if add_constraints_to_scope { + self.add_trait_bound_to_scope( + constraint.trait_bound.trait_path.span, + &typ, + &trait_bound, + trait_bound.trait_id, + ); + } + Some(TraitConstraint { typ, trait_bound }) } @@ -800,10 +864,11 @@ impl<'context> Elaborator<'context> { let has_inline_attribute = has_no_predicates_attribute || should_fold; let is_pub_allowed = self.pub_allowed(func, in_contract); self.add_generics(&func.def.generics); + let mut generics = vecmap(&self.generics, |generic| generic.type_var.clone()); - let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause); + self.desugar_trait_constraints(&mut func.def.where_clause, &mut generics); + let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause, true); - let mut generics = vecmap(&self.generics, |generic| generic.type_var.clone()); let mut parameters = Vec::new(); let mut parameter_types = Vec::new(); let mut parameter_idents = Vec::new(); @@ -874,6 +939,9 @@ impl<'context> Elaborator<'context> { None }; + // Remove the traits assumed by `resolve_trait_constraints` from scope + self.remove_trait_constraints_from_scope(&trait_constraints); + let meta = FuncMeta { name: name_ident, kind: func.kind, @@ -1013,10 +1081,10 @@ impl<'context> Elaborator<'context> { } } - fn add_trait_constraints_to_scope(&mut self, func_meta: &FuncMeta) { - for constraint in &func_meta.trait_constraints { + fn add_trait_constraints_to_scope(&mut self, constraints: &[TraitConstraint], span: Span) { + for constraint in constraints { self.add_trait_bound_to_scope( - func_meta, + span, &constraint.typ, &constraint.trait_bound, constraint.trait_bound.trait_id, @@ -1030,7 +1098,7 @@ impl<'context> Elaborator<'context> { let self_type = self.self_type.clone().expect("Expected a self type if there's a current trait"); self.add_trait_bound_to_scope( - func_meta, + span, &self_type, &constraint.trait_bound, constraint.trait_bound.trait_id, @@ -1038,8 +1106,8 @@ impl<'context> Elaborator<'context> { } } - fn remove_trait_constraints_from_scope(&mut self, func_meta: &FuncMeta) { - for constraint in &func_meta.trait_constraints { + fn remove_trait_constraints_from_scope(&mut self, constraints: &[TraitConstraint]) { + for constraint in constraints { self.interner .remove_assumed_trait_implementations_for_trait(constraint.trait_bound.trait_id); } @@ -1052,7 +1120,7 @@ impl<'context> Elaborator<'context> { fn add_trait_bound_to_scope( &mut self, - func_meta: &FuncMeta, + span: Span, object: &Type, trait_bound: &ResolvedTraitBound, starting_trait_id: TraitId, @@ -1064,7 +1132,6 @@ impl<'context> Elaborator<'context> { if let Some(the_trait) = self.interner.try_get_trait(trait_id) { let trait_name = the_trait.name.to_string(); let typ = object.clone(); - let span = func_meta.location.span; self.push_err(TypeCheckError::UnneededTraitConstraint { trait_name, typ, span }); } } @@ -1081,12 +1148,7 @@ impl<'context> Elaborator<'context> { let parent_trait_bound = self.instantiate_parent_trait_bound(trait_bound, &parent_trait_bound); - self.add_trait_bound_to_scope( - func_meta, - object, - &parent_trait_bound, - starting_trait_id, - ); + self.add_trait_bound_to_scope(span, object, &parent_trait_bound, starting_trait_id); } } } @@ -1315,7 +1377,7 @@ impl<'context> Elaborator<'context> { if let Some(trait_id) = trait_impl.trait_id { self.generics = trait_impl.resolved_generics.clone(); - let where_clause = self.resolve_trait_constraints(&trait_impl.where_clause); + let where_clause = self.resolve_trait_constraints(&trait_impl.where_clause, false); self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause); diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index a10939dde1..52e045e732 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -40,7 +40,7 @@ impl<'context> Elaborator<'context> { ); let where_clause = - this.resolve_trait_constraints(&unresolved_trait.trait_def.where_clause); + this.resolve_trait_constraints(&unresolved_trait.trait_def.where_clause, false); // Each associated type in this trait is also an implicit generic for associated_type in &this.interner.get_trait(*trait_id).associated_types { diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index e01cda3a2e..92542dd404 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -379,11 +379,13 @@ impl<'context> Elaborator<'context> { resolved.push(NamedType { name, typ }); } - // Anything that hasn't been removed yet is missing + // Anything that hasn't been removed yet is missing, fill it in to avoid a panic. + // An error should already be issued earlier. for generic in required_args { - let item = item.item_name(self.interner); let name = generic.name.clone(); - self.push_err(TypeCheckError::MissingNamedTypeArg { item, span, name }); + let name = Ident::new(name.as_ref().clone(), span); + let typ = self.interner.next_type_variable(); + resolved.push(NamedType { name, typ }); } resolved @@ -1943,7 +1945,7 @@ pub(crate) fn bind_named_generics( args: &[NamedType], bindings: &mut TypeBindings, ) { - assert_eq!(params.len(), args.len()); + assert!(args.len() <= params.len()); for arg in args { let i = params @@ -1954,6 +1956,10 @@ pub(crate) fn bind_named_generics( let param = params.swap_remove(i); bind_generic(¶m, &arg.typ, bindings); } + + for unbound_param in params { + bind_generic(&unbound_param, &Type::Error, bindings); + } } fn bind_generic(param: &ResolvedGeneric, arg: &Type, bindings: &mut TypeBindings) { From ea59862a788f8332551a18d73e6ae3d74e27cfbf Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 10 Jan 2025 11:54:38 -0600 Subject: [PATCH 02/10] Try to get trait definitions working but fail --- compiler/noirc_frontend/src/elaborator/mod.rs | 67 +++++++++++++------ .../noirc_frontend/src/elaborator/traits.rs | 9 ++- 2 files changed, 54 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index a2b025116d..9fda483a16 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -315,7 +315,7 @@ impl<'context> Elaborator<'context> { self.define_function_metas(&mut items.functions, &mut items.impls, &mut items.trait_impls); - self.collect_traits(&items.traits); + self.collect_traits(&mut items.traits); // Before we resolve any function symbols we must go through our impls and // re-collect the methods within into their proper module. This cannot be @@ -354,6 +354,7 @@ impl<'context> Elaborator<'context> { self.current_trait = Some(trait_id); self.elaborate_functions(unresolved_trait.fns_with_default_impl); } + self.current_trait = None; for impls in items.impls.into_values() { @@ -746,31 +747,37 @@ impl<'context> Elaborator<'context> { .collect() } + /// Expands any traits in a where clause to mention all associated types if they were + /// ellided by the user. See `add_missing_named_generics` for more detail. + /// + /// Returns all newly created generics to be added to this function/trait/impl. fn desugar_trait_constraints( &mut self, where_clause: &mut [UnresolvedTraitConstraint], - generics: &mut Vec, - ) { - for constraint in where_clause { - self.add_missing_named_generics(&mut constraint.trait_bound, generics); - } + ) -> Vec { + where_clause + .iter_mut() + .flat_map(|constraint| self.add_missing_named_generics(&mut constraint.trait_bound)) + .collect() } /// For each associated type that isn't mentioned in a trait bound, this adds - /// the type as an implicit generic to the function. For example, this will - /// turn a function using a trait with 2 associated types: + /// the type as an implicit generic to the where clause and returns the newly + /// created generics in a vector to add to the function/trait/impl later. + /// For example, this will turn a function using a trait with 2 associated types: /// /// `fn foo() where T: Foo { ... }` /// /// into: + /// `fn foo() where T: Foo { ... }` + /// + /// with a vector of `` returned so that the caller can then modify the function to: /// `fn foo() where T: Foo { ... }` - fn add_missing_named_generics( - &mut self, - bound: &mut TraitBound, - generics: &mut Vec, - ) { + fn add_missing_named_generics(&mut self, bound: &mut TraitBound) -> Vec { + let mut added_generics = Vec::new(); + let Some(the_trait) = self.lookup_trait_or_error(bound.trait_path.clone()) else { - return; + return added_generics; }; if the_trait.associated_types.len() > bound.trait_generics.named_args.len() { @@ -782,20 +789,24 @@ impl<'context> Elaborator<'context> { .any(|(name, _)| name.0.contents == *associated_type.name.as_ref()) { // This generic isn't contained in the bound's named arguments, - // implicitly add it to the function + // so add it by creating a fresh type variable. let new_generic_id = self.interner.next_type_variable_id(); - let new_generic = TypeVariable::unbound(new_generic_id, Kind::Normal); - generics.push(new_generic.clone()); + let type_var = TypeVariable::unbound(new_generic_id, Kind::Normal); let span = bound.trait_path.span; - let typ = Type::NamedGeneric(new_generic, associated_type.name.clone()); + let no_name = Rc::new("_".to_string()); + let typ = Type::NamedGeneric(type_var.clone(), no_name.clone()); let typ = self.interner.push_quoted_type(typ); let typ = UnresolvedTypeData::Resolved(typ).with_span(span); let ident = Ident::new(associated_type.name.as_ref().clone(), span); + bound.trait_generics.named_args.push((ident, typ)); + added_generics.push(ResolvedGeneric { name: no_name, span, type_var }) } } } + + added_generics } fn resolve_trait_constraint( @@ -866,7 +877,9 @@ impl<'context> Elaborator<'context> { self.add_generics(&func.def.generics); let mut generics = vecmap(&self.generics, |generic| generic.type_var.clone()); - self.desugar_trait_constraints(&mut func.def.where_clause, &mut generics); + let new_generics = self.desugar_trait_constraints(&mut func.def.where_clause); + generics.extend(new_generics.into_iter().map(|generic| generic.type_var)); + let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause, true); let mut parameters = Vec::new(); @@ -1377,7 +1390,8 @@ impl<'context> Elaborator<'context> { if let Some(trait_id) = trait_impl.trait_id { self.generics = trait_impl.resolved_generics.clone(); - let where_clause = self.resolve_trait_constraints(&trait_impl.where_clause, false); + let where_clause = self.resolve_trait_constraints(&trait_impl.where_clause, true); + self.remove_trait_constraints_from_scope(&where_clause); self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause); @@ -1867,6 +1881,17 @@ impl<'context> Elaborator<'context> { self.add_generics(&trait_impl.generics); trait_impl.resolved_generics = self.generics.clone(); + let new_generics = self.desugar_trait_constraints(&mut trait_impl.where_clause); + for new_generic in new_generics { + trait_impl.resolved_generics.push(new_generic.clone()); + self.generics.push(new_generic); + } + + // We need to resolve the where clause before any associated types to be + // able to resolve trait as type syntax, eg. `` in case there + // is a where constraint for `T: Foo`. + let constraints = self.resolve_trait_constraints(&trait_impl.where_clause, true); + for (_, _, method) in trait_impl.methods.functions.iter_mut() { // Attach any trait constraints on the impl to the function method.def.where_clause.append(&mut trait_impl.where_clause.clone()); @@ -1890,6 +1915,8 @@ impl<'context> Elaborator<'context> { trait_impl.resolved_trait_generics = ordered_generics; self.interner.set_associated_types_for_impl(impl_id, named_generics); + self.remove_trait_constraints_from_scope(&constraints); + let self_type = self.resolve_type(unresolved_type); self.self_type = Some(self_type.clone()); trait_impl.methods.self_type = Some(self_type); diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 52e045e732..75162dcd08 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -21,7 +21,7 @@ use crate::{ use super::Elaborator; impl<'context> Elaborator<'context> { - pub fn collect_traits(&mut self, traits: &BTreeMap) { + pub fn collect_traits(&mut self, traits: &mut BTreeMap) { for (trait_id, unresolved_trait) in traits { self.local_module = unresolved_trait.module_id; @@ -39,8 +39,13 @@ impl<'context> Elaborator<'context> { &resolved_generics, ); + let new_generics = + this.desugar_trait_constraints(&mut unresolved_trait.trait_def.where_clause); + this.generics.extend(new_generics); + let where_clause = - this.resolve_trait_constraints(&unresolved_trait.trait_def.where_clause, false); + this.resolve_trait_constraints(&unresolved_trait.trait_def.where_clause, true); + this.remove_trait_constraints_from_scope(&where_clause); // Each associated type in this trait is also an implicit generic for associated_type in &this.interner.get_trait(*trait_id).associated_types { From f1ef945a1e1e9207d1e273526d597e9baa4a3e0d Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 10 Jan 2025 11:56:02 -0600 Subject: [PATCH 03/10] Add test --- .../associated_types_implicit/Nargo.toml | 6 ++ .../associated_types_implicit/Prover.toml | 1 + .../associated_types_implicit/src/main.nr | 58 +++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 test_programs/compile_success_empty/associated_types_implicit/Nargo.toml create mode 100644 test_programs/compile_success_empty/associated_types_implicit/Prover.toml create mode 100644 test_programs/compile_success_empty/associated_types_implicit/src/main.nr diff --git a/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml b/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml new file mode 100644 index 0000000000..6b54cbfca6 --- /dev/null +++ b/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "associated_types_implicit" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/compile_success_empty/associated_types_implicit/Prover.toml b/test_programs/compile_success_empty/associated_types_implicit/Prover.toml new file mode 100644 index 0000000000..cab679b4b6 --- /dev/null +++ b/test_programs/compile_success_empty/associated_types_implicit/Prover.toml @@ -0,0 +1 @@ +a = [[0, 1], [2, 3]] diff --git a/test_programs/compile_success_empty/associated_types_implicit/src/main.nr b/test_programs/compile_success_empty/associated_types_implicit/src/main.nr new file mode 100644 index 0000000000..9c1949d49f --- /dev/null +++ b/test_programs/compile_success_empty/associated_types_implicit/src/main.nr @@ -0,0 +1,58 @@ +trait Foo { + type Bar; + + fn foo(self) -> Self::Bar; +} + +impl Foo for u64 { + type Bar = u8; + + fn foo(self) -> Self::Bar { + self as u8 + } +} + +fn main() { + // This currently requires a type annotation to find the impl + let three: u64 = 3; + call_foo(three); + + let x: Option> = Option::some(Option::some(0)); + let x_foo = x.foo(); + assert_eq(x_foo, x_foo); // ensure we don't need an additional type annotation for Bar here + + // The `as u8` is still necessary even though we know the object type, + // otherwise we try to search for `u64: Foo`. + // It seems the `Bar = u8` constraint is still there & checked for, but + // the defaulting of the polymorphic integer occurs first. + assert_eq(x.foo(), 0 as u8); +} + +// Ensure we can use `::Bar: Eq` in a function's where clause +fn call_foo(x: T) where + T: Foo, + ::Bar: Eq +{ + let y = x.foo(); + assert_eq(y, y); +} + +// Ensure we can use `::Bar: Eq` in a trait impl's where clause +impl Foo for Option where + T: Foo, + ::Bar: Eq +{ + type Bar = ::Bar; + + fn foo(self) -> Self::Bar { + self.unwrap().foo() + } +} + +// Ensure we can use `::Bar: Eq` in a trait's where clause +// TODO: Not working, see issue #7024 +// trait Baz where +// T: Foo, +// ::Bar: Eq {} +// +// impl Baz for u32 {} From 4a59194a9848e1f6cbbc99ddcf8ffd322db3491d Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 10 Jan 2025 12:10:28 -0600 Subject: [PATCH 04/10] Edit docs --- docs/docs/noir/concepts/traits.md | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/docs/docs/noir/concepts/traits.md b/docs/docs/noir/concepts/traits.md index 1b232dcf8a..13818ecffa 100644 --- a/docs/docs/noir/concepts/traits.md +++ b/docs/docs/noir/concepts/traits.md @@ -252,36 +252,33 @@ impl MyTrait for Field { Since associated constants can also be used in a type position, its values are limited to only other expression kinds allowed in numeric generics. -Note that currently all associated types and constants must be explicitly specified in a trait constraint. -If we leave out any, we'll get an error that we're missing one: +When writing a trait constraint, you can specify all associated types and constants explicitly if +you wish: ```rust -// Error! Constraint is missing associated constant for `Bar` -fn foo(x: T) where T: MyTrait { +fn foo(x: T) where T: MyTrait { ... } ``` -Because all associated types and constants must be explicitly specified, they are essentially named generics, -although this is set to change in the future. Future versions of Noir will allow users to elide associated types -in trait constraints similar to Rust. When this is done, you may still refer to their value with the `::AssociatedType` -syntax: +Or you can also elide them since there should only be one `Foo` and `Bar` for a given implementation +of `MyTrait` for a type: ```rust -// Only valid in future versions of Noir: fn foo(x: T) where T: MyTrait { - let _: ::Foo = ...; + ... } ``` -The type as trait syntax is possible in Noir today but is less useful when each type must be explicitly specified anyway: +If you elide associated types, you can still refer to them via the type as trait syntax ``: ```rust -fn foo(x: T) where T: MyTrait { - // Works, but could just use F directly - let _: >::Foo = ...; - - let _: F = ...; +fn foo(x: T) where + T: MyTrait, + ::Foo: Default + Eq +{ + let foo_value: ::Foo = Default::default(); + assert_eq(foo_value, foo_value); } ``` From e54010cc8ccca7be2c83db11079efda387ad39b8 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 10 Jan 2025 12:13:54 -0600 Subject: [PATCH 05/10] Clippy --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 9fda483a16..c56b31fb9a 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -801,7 +801,7 @@ impl<'context> Elaborator<'context> { let ident = Ident::new(associated_type.name.as_ref().clone(), span); bound.trait_generics.named_args.push((ident, typ)); - added_generics.push(ResolvedGeneric { name: no_name, span, type_var }) + added_generics.push(ResolvedGeneric { name: no_name, span, type_var }); } } } From 7e941732d1e2e34ead6c5371e367ca3f7c3dfd15 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 10 Jan 2025 13:59:23 -0600 Subject: [PATCH 06/10] Update compiler/noirc_frontend/src/elaborator/mod.rs Co-authored-by: Ary Borenszweig --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index c56b31fb9a..02b05034ce 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -748,7 +748,7 @@ impl<'context> Elaborator<'context> { } /// Expands any traits in a where clause to mention all associated types if they were - /// ellided by the user. See `add_missing_named_generics` for more detail. + /// elided by the user. See `add_missing_named_generics` for more detail. /// /// Returns all newly created generics to be added to this function/trait/impl. fn desugar_trait_constraints( From cc15211219977b14e90526b75ac430dcbe648800 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 10 Jan 2025 15:04:50 -0600 Subject: [PATCH 07/10] Fix first regression --- compiler/noirc_frontend/src/elaborator/mod.rs | 5 ++- .../noirc_frontend/src/elaborator/types.rs | 45 ++++++++++++++++--- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index c56b31fb9a..a0910230dd 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1904,11 +1904,12 @@ impl<'context> Elaborator<'context> { let impl_id = self.interner.next_trait_impl_id(); self.current_trait_impl = Some(impl_id); - // Fetch trait constraints here + let path_span = trait_impl.trait_path.span; let (ordered_generics, named_generics) = trait_impl .trait_id .map(|trait_id| { - self.resolve_type_args(trait_generics, trait_id, trait_impl.trait_path.span) + // Check for missing generics & associated types for the trait being implemented + self.resolve_trait_args_from_trait_impl(trait_generics, trait_id, path_span) }) .unwrap_or_default(); diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 92542dd404..fae949c7df 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -310,11 +310,32 @@ impl<'context> Elaborator<'context> { } } + /// Identical to `resolve_type_args` but does not allow + /// associated types to be elided since trait impls must specify them. + pub(super) fn resolve_trait_args_from_trait_impl( + &mut self, + args: GenericTypeArgs, + item: TraitId, + span: Span, + ) -> (Vec, Vec) { + self.resolve_type_args_inner(args, item, span, false) + } + pub(super) fn resolve_type_args( + &mut self, + args: GenericTypeArgs, + item: impl Generic, + span: Span, + ) -> (Vec, Vec) { + self.resolve_type_args_inner(args, item, span, true) + } + + pub(super) fn resolve_type_args_inner( &mut self, mut args: GenericTypeArgs, item: impl Generic, span: Span, + allow_implicit_named_args: bool, ) -> (Vec, Vec) { let expected_kinds = item.generics(self.interner); @@ -336,7 +357,12 @@ impl<'context> Elaborator<'context> { let mut associated = Vec::new(); if item.accepts_named_type_args() { - associated = self.resolve_associated_type_args(args.named_args, item, span); + associated = self.resolve_associated_type_args( + args.named_args, + item, + span, + allow_implicit_named_args, + ); } else if !args.named_args.is_empty() { let item_kind = item.item_kind(); self.push_err(ResolverError::NamedTypeArgs { span, item_kind }); @@ -350,6 +376,7 @@ impl<'context> Elaborator<'context> { args: Vec<(Ident, UnresolvedType)>, item: impl Generic, span: Span, + allow_implicit_named_args: bool, ) -> Vec { let mut seen_args = HashMap::default(); let mut required_args = item.named_generics(self.interner); @@ -379,13 +406,19 @@ impl<'context> Elaborator<'context> { resolved.push(NamedType { name, typ }); } - // Anything that hasn't been removed yet is missing, fill it in to avoid a panic. - // An error should already be issued earlier. + // Anything that hasn't been removed yet is missing. + // Fill it in to avoid a panic if we allow named args to be elided, otherwise error. for generic in required_args { let name = generic.name.clone(); - let name = Ident::new(name.as_ref().clone(), span); - let typ = self.interner.next_type_variable(); - resolved.push(NamedType { name, typ }); + + if allow_implicit_named_args { + let name = Ident::new(name.as_ref().clone(), span); + let typ = self.interner.next_type_variable(); + resolved.push(NamedType { name, typ }); + } else { + let item = item.item_name(self.interner); + self.push_err(TypeCheckError::MissingNamedTypeArg { item, span, name }); + } } resolved From ec865ad1d4ada9445c4436bfe9bd26d56517dcfb Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 13 Jan 2025 11:53:25 -0600 Subject: [PATCH 08/10] Fix double trait error --- compiler/noirc_frontend/src/elaborator/mod.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index ae7a1a4f2c..57dc2c62e1 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -776,10 +776,16 @@ impl<'context> Elaborator<'context> { fn add_missing_named_generics(&mut self, bound: &mut TraitBound) -> Vec { let mut added_generics = Vec::new(); - let Some(the_trait) = self.lookup_trait_or_error(bound.trait_path.clone()) else { - return added_generics; + let Ok(item) = self.resolve_path_or_error(bound.trait_path.clone()) else { + return Vec::new(); }; + let PathResolutionItem::Trait(trait_id) = item else { + return Vec::new(); + }; + + let the_trait = self.get_trait_mut(trait_id); + if the_trait.associated_types.len() > bound.trait_generics.named_args.len() { for associated_type in the_trait.associated_types.clone() { if !bound From c7bae6a4d526634c9c65f28185d6594a54df399a Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 13 Jan 2025 12:06:52 -0600 Subject: [PATCH 09/10] Change name of implicit associated types --- compiler/noirc_frontend/src/elaborator/mod.rs | 45 ++++++++++--------- .../noirc_frontend/src/elaborator/traits.rs | 2 +- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 57dc2c62e1..5acd4524b2 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -734,16 +734,20 @@ impl<'context> Elaborator<'context> { None } + /// Resolve the given trait constraints and add them to scope as we go. + /// This second step is necessary to resolve subsequent constraints such + /// as `::Bar: Eq` which may lookup an impl which was assumed + /// by a previous constraint. + /// + /// If these constraints are unwanted afterward they should be manually + /// removed from the interner. fn resolve_trait_constraints( &mut self, where_clause: &[UnresolvedTraitConstraint], - add_constraints_to_scope: bool, ) -> Vec { where_clause .iter() - .filter_map(|constraint| { - self.resolve_trait_constraint(constraint, add_constraints_to_scope) - }) + .filter_map(|constraint| self.resolve_trait_constraint(constraint)) .collect() } @@ -787,7 +791,7 @@ impl<'context> Elaborator<'context> { let the_trait = self.get_trait_mut(trait_id); if the_trait.associated_types.len() > bound.trait_generics.named_args.len() { - for associated_type in the_trait.associated_types.clone() { + for associated_type in &the_trait.associated_types.clone() { if !bound .trait_generics .named_args @@ -800,14 +804,14 @@ impl<'context> Elaborator<'context> { let type_var = TypeVariable::unbound(new_generic_id, Kind::Normal); let span = bound.trait_path.span; - let no_name = Rc::new("_".to_string()); - let typ = Type::NamedGeneric(type_var.clone(), no_name.clone()); + let name = associated_type.name.clone(); + let typ = Type::NamedGeneric(type_var.clone(), name.clone()); let typ = self.interner.push_quoted_type(typ); let typ = UnresolvedTypeData::Resolved(typ).with_span(span); let ident = Ident::new(associated_type.name.as_ref().clone(), span); bound.trait_generics.named_args.push((ident, typ)); - added_generics.push(ResolvedGeneric { name: no_name, span, type_var }); + added_generics.push(ResolvedGeneric { name, span, type_var }); } } } @@ -815,22 +819,23 @@ impl<'context> Elaborator<'context> { added_generics } + /// Resolves a trait constraint and adds it to scope as an assumed impl. + /// This second step is necessary to resolve subsequent constraints such + /// as `::Bar: Eq` which may lookup an impl which was assumed + /// by a previous constraint. fn resolve_trait_constraint( &mut self, constraint: &UnresolvedTraitConstraint, - add_constraints_to_scope: bool, ) -> Option { let typ = self.resolve_type(constraint.typ.clone()); let trait_bound = self.resolve_trait_bound(&constraint.trait_bound)?; - if add_constraints_to_scope { - self.add_trait_bound_to_scope( - constraint.trait_bound.trait_path.span, - &typ, - &trait_bound, - trait_bound.trait_id, - ); - } + self.add_trait_bound_to_scope( + constraint.trait_bound.trait_path.span, + &typ, + &trait_bound, + trait_bound.trait_id, + ); Some(TraitConstraint { typ, trait_bound }) } @@ -886,7 +891,7 @@ impl<'context> Elaborator<'context> { let new_generics = self.desugar_trait_constraints(&mut func.def.where_clause); generics.extend(new_generics.into_iter().map(|generic| generic.type_var)); - let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause, true); + let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause); let mut parameters = Vec::new(); let mut parameter_types = Vec::new(); @@ -1396,7 +1401,7 @@ impl<'context> Elaborator<'context> { if let Some(trait_id) = trait_impl.trait_id { self.generics = trait_impl.resolved_generics.clone(); - let where_clause = self.resolve_trait_constraints(&trait_impl.where_clause, true); + let where_clause = self.resolve_trait_constraints(&trait_impl.where_clause); self.remove_trait_constraints_from_scope(&where_clause); self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause); @@ -1896,7 +1901,7 @@ impl<'context> Elaborator<'context> { // We need to resolve the where clause before any associated types to be // able to resolve trait as type syntax, eg. `` in case there // is a where constraint for `T: Foo`. - let constraints = self.resolve_trait_constraints(&trait_impl.where_clause, true); + let constraints = self.resolve_trait_constraints(&trait_impl.where_clause); for (_, _, method) in trait_impl.methods.functions.iter_mut() { // Attach any trait constraints on the impl to the function diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 75162dcd08..b47b35e230 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -44,7 +44,7 @@ impl<'context> Elaborator<'context> { this.generics.extend(new_generics); let where_clause = - this.resolve_trait_constraints(&unresolved_trait.trait_def.where_clause, true); + this.resolve_trait_constraints(&unresolved_trait.trait_def.where_clause); this.remove_trait_constraints_from_scope(&where_clause); // Each associated type in this trait is also an implicit generic From 31080b42561f800c894bce1d7c915af6b60866a4 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 13 Jan 2025 17:42:03 -0300 Subject: [PATCH 10/10] Check trait is visible when checking trait impl method is visible --- .../src/hir/resolution/visibility.rs | 21 +++++++++++++---- compiler/noirc_frontend/src/tests.rs | 23 +++++++++++++++++++ .../associated_types_implicit/src/main.nr | 12 ++++++---- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/resolution/visibility.rs b/compiler/noirc_frontend/src/hir/resolution/visibility.rs index 5e6dffa56e..557f799df8 100644 --- a/compiler/noirc_frontend/src/hir/resolution/visibility.rs +++ b/compiler/noirc_frontend/src/hir/resolution/visibility.rs @@ -138,18 +138,29 @@ pub fn method_call_is_visible( ItemVisibility::Public => true, ItemVisibility::PublicCrate | ItemVisibility::Private => { let func_meta = interner.function_meta(&func_id); - if let Some(struct_id) = func_meta.struct_id { - return struct_member_is_visible( - struct_id, + + if let Some(trait_id) = func_meta.trait_id { + return trait_member_is_visible( + trait_id, modifiers.visibility, current_module, def_maps, ); } - if let Some(trait_id) = func_meta.trait_id { + if let Some(trait_impl_id) = func_meta.trait_impl { + let trait_impl = interner.get_trait_implementation(trait_impl_id); return trait_member_is_visible( - trait_id, + trait_impl.borrow().trait_id, + modifiers.visibility, + current_module, + def_maps, + ); + } + + if let Some(struct_id) = func_meta.struct_id { + return struct_member_is_visible( + struct_id, modifiers.visibility, current_module, def_maps, diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c1b86b5dcf..637b15e719 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3955,3 +3955,26 @@ fn mutable_self_call() { "#; assert_no_errors(src); } + +#[test] +fn checks_visibility_of_trait_related_to_trait_impl_on_method_call() { + let src = r#" + mod moo { + pub struct Bar {} + } + + trait Foo { + fn foo(self); + } + + impl Foo for moo::Bar { + fn foo(self) {} + } + + fn main() { + let bar = moo::Bar {}; + bar.foo(); + } + "#; + assert_no_errors(src); +} diff --git a/test_programs/compile_success_empty/associated_types_implicit/src/main.nr b/test_programs/compile_success_empty/associated_types_implicit/src/main.nr index 9c1949d49f..d04cef5186 100644 --- a/test_programs/compile_success_empty/associated_types_implicit/src/main.nr +++ b/test_programs/compile_success_empty/associated_types_implicit/src/main.nr @@ -29,18 +29,20 @@ fn main() { } // Ensure we can use `::Bar: Eq` in a function's where clause -fn call_foo(x: T) where +fn call_foo(x: T) +where T: Foo, - ::Bar: Eq + ::Bar: Eq, { let y = x.foo(); assert_eq(y, y); } // Ensure we can use `::Bar: Eq` in a trait impl's where clause -impl Foo for Option where +impl Foo for Option +where T: Foo, - ::Bar: Eq + ::Bar: Eq, { type Bar = ::Bar; @@ -54,5 +56,5 @@ impl Foo for Option where // trait Baz where // T: Foo, // ::Bar: Eq {} -// +// // impl Baz for u32 {}