fix(experimental elaborator): Avoid calling add_generics twice on t…

…rait methods (#5108)

# Description

## Problem\*

Resolves the single "Duplicate definitions of <generic> found" error.

## Summary\*

Since `resolve_trait_function` already calls `add_generics` indirectly
via `define_function_meta`, we were adding generics on trait methods
twice before. I've removed the first of the two to fix this.

## Additional Context

After this the only errors that remain are the 5 on these lines of the
stdlib:
https://github.com/noir-lang/noir/blob/master/noir_stdlib/src/eddsa.nr#L49-L53.
I believe these are the result of something from the turbofish changes
being missed (likely #5041) since
they were recently modified from
#5050.

## 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.

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -295,9 +295,11 @@ impl<'context> Elaborator<'context> {
             .expect("FuncMetas should be declared before a function is elaborated")
             .clone();
 
-        for (parameter, param2) in function.def.parameters.iter().zip(&func_meta.parameters.0) {
-            let definition_kind = DefinitionKind::Local(None);
-            self.elaborate_pattern(parameter.pattern.clone(), param2.1.clone(), definition_kind);
+        // The DefinitionIds for each parameter were already created in define_function_meta
+        // so we need to reintroduce the same IDs into scope here.
+        for parameter in &func_meta.parameter_idents {
+            let name = self.interner.definition_name(parameter.id).to_owned();
+            self.add_existing_variable_to_scope(name, parameter.clone());
         }
 
         self.generics = func_meta.all_generics.clone();
@@ -560,8 +562,9 @@ impl<'context> Elaborator<'context> {
         self.add_generics(&func.def.generics);
 
         let mut generics = vecmap(&self.generics, |(_, typevar, _)| typevar.clone());
-        let mut parameters = vec![];
-        let mut parameter_types = vec![];
+        let mut parameters = Vec::new();
+        let mut parameter_types = Vec::new();
+        let mut parameter_idents = Vec::new();
 
         for Param { visibility, pattern, typ, span: _ } in func.parameters().iter().cloned() {
             if visibility == Visibility::Public && !self.pub_allowed(func) {
@@ -579,7 +582,12 @@ impl<'context> Elaborator<'context> {
                 has_inline_attribute,
                 type_span,
             );
-            let pattern = self.elaborate_pattern(pattern, typ.clone(), DefinitionKind::Local(None));
+            let pattern = self.elaborate_pattern_and_store_ids(
+                pattern,
+                typ.clone(),
+                DefinitionKind::Local(None),
+                &mut parameter_idents,
+            );
 
             parameters.push((pattern, typ.clone(), visibility));
             parameter_types.push(typ);
@@ -648,6 +656,7 @@ impl<'context> Elaborator<'context> {
             all_generics: self.generics.clone(),
             trait_impl: self.current_trait_impl,
             parameters: parameters.into(),
+            parameter_idents,
             return_type: func.def.return_type.clone(),
             return_visibility: func.def.return_visibility,
             has_body: !func.def.body.is_empty(),

compiler/noirc_frontend/src/elaborator/patterns.rs

@@ -27,7 +27,19 @@ impl<'context> Elaborator<'context> {
         expected_type: Type,
         definition_kind: DefinitionKind,
     ) -> HirPattern {
-        self.elaborate_pattern_mut(pattern, expected_type, definition_kind, None)
+        self.elaborate_pattern_mut(pattern, expected_type, definition_kind, None, &mut Vec::new())
+    }
+
+    /// Equivalent to `elaborate_pattern`, this version just also
+    /// adds any new DefinitionIds that were created to the given Vec.
+    pub(super) fn elaborate_pattern_and_store_ids(
+        &mut self,
+        pattern: Pattern,
+        expected_type: Type,
+        definition_kind: DefinitionKind,
+        created_ids: &mut Vec<HirIdent>,
+    ) -> HirPattern {
+        self.elaborate_pattern_mut(pattern, expected_type, definition_kind, None, created_ids)
     }
 
     fn elaborate_pattern_mut(
@@ -36,6 +48,7 @@ impl<'context> Elaborator<'context> {
         expected_type: Type,
         definition: DefinitionKind,
         mutable: Option<Span>,
+        new_definitions: &mut Vec<HirIdent>,
     ) -> HirPattern {
         match pattern {
             Pattern::Identifier(name) => {
@@ -47,15 +60,21 @@ impl<'context> Elaborator<'context> {
                 };
                 let ident = self.add_variable_decl(name, mutable.is_some(), true, definition);
                 self.interner.push_definition_type(ident.id, expected_type);
+                new_definitions.push(ident.clone());
                 HirPattern::Identifier(ident)
             }
             Pattern::Mutable(pattern, span, _) => {
                 if let Some(first_mut) = mutable {
                     self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span });
                 }
 
-                let pattern =
-                    self.elaborate_pattern_mut(*pattern, expected_type, definition, Some(span));
+                let pattern = self.elaborate_pattern_mut(
+                    *pattern,
+                    expected_type,
+                    definition,
+                    Some(span),
+                    new_definitions,
+                );
                 let location = Location::new(span, self.file);
                 HirPattern::Mutable(Box::new(pattern), location)
             }
@@ -79,7 +98,13 @@ impl<'context> Elaborator<'context> {
 
                 let fields = vecmap(fields.into_iter().enumerate(), |(i, field)| {
                     let field_type = field_types.get(i).cloned().unwrap_or(Type::Error);
-                    self.elaborate_pattern_mut(field, field_type, definition.clone(), mutable)
+                    self.elaborate_pattern_mut(
+                        field,
+                        field_type,
+                        definition.clone(),
+                        mutable,
+                        new_definitions,
+                    )
                 });
                 let location = Location::new(span, self.file);
                 HirPattern::Tuple(fields, location)
@@ -91,10 +116,12 @@ impl<'context> Elaborator<'context> {
                 expected_type,
                 definition,
                 mutable,
+                new_definitions,
             ),
         }
     }
 
+    #[allow(clippy::too_many_arguments)]
     fn elaborate_struct_pattern(
         &mut self,
         name: Path,
@@ -103,6 +130,7 @@ impl<'context> Elaborator<'context> {
         expected_type: Type,
         definition: DefinitionKind,
         mutable: Option<Span>,
+        new_definitions: &mut Vec<HirIdent>,
     ) -> HirPattern {
         let error_identifier = |this: &mut Self| {
             // Must create a name here to return a HirPattern::Identifier. Allowing
@@ -140,6 +168,7 @@ impl<'context> Elaborator<'context> {
             expected_type.clone(),
             definition,
             mutable,
+            new_definitions,
         );
 
         HirPattern::Struct(expected_type, fields, location)
@@ -148,6 +177,7 @@ impl<'context> Elaborator<'context> {
     /// Resolve all the fields of a struct constructor expression.
     /// Ensures all fields are present, none are repeated, and all
     /// are part of the struct.
+    #[allow(clippy::too_many_arguments)]
     fn resolve_constructor_pattern_fields(
         &mut self,
         struct_type: Shared<StructType>,
@@ -156,15 +186,21 @@ impl<'context> Elaborator<'context> {
         expected_type: Type,
         definition: DefinitionKind,
         mutable: Option<Span>,
+        new_definitions: &mut Vec<HirIdent>,
     ) -> Vec<(Ident, HirPattern)> {
         let mut ret = Vec::with_capacity(fields.len());
         let mut seen_fields = HashSet::default();
         let mut unseen_fields = struct_type.borrow().field_names();
 
         for (field, pattern) in fields {
             let field_type = expected_type.get_field_type(&field.0.contents).unwrap_or(Type::Error);
-            let resolved =
-                self.elaborate_pattern_mut(pattern, field_type, definition.clone(), mutable);
+            let resolved = self.elaborate_pattern_mut(
+                pattern,
+                field_type,
+                definition.clone(),
+                mutable,
+                new_definitions,
+            );
 
             if unseen_fields.contains(&field) {
                 unseen_fields.remove(&field);
@@ -239,6 +275,18 @@ impl<'context> Elaborator<'context> {
         ident
     }
 
+    pub fn add_existing_variable_to_scope(&mut self, name: String, ident: HirIdent) {
+        let second_span = ident.location.span;
+        let resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true };
+
+        let old_value = self.scopes.get_mut_scope().add_key_value(name.clone(), resolver_meta);
+
+        if let Some(old_value) = old_value {
+            let first_span = old_value.ident.location.span;
+            self.push_err(ResolverError::DuplicateDefinition { name, first_span, second_span });
+        }
+    }
+
     pub fn add_global_variable_decl(
         &mut self,
         name: Ident,

compiler/noirc_frontend/src/elaborator/traits.rs

@@ -88,7 +88,6 @@ impl<'context> Elaborator<'context> {
                     let name_span = the_trait.name.span();
 
                     this.add_existing_generic("Self", name_span, self_typevar);
-                    this.add_generics(generics);
                     this.self_type = Some(self_type.clone());
 
                     let func_id = unresolved_trait.method_ids[&name.0.contents];
@@ -101,8 +100,10 @@ impl<'context> Elaborator<'context> {
                         func_id,
                     );
 
-                    let arguments = vecmap(parameters, |param| this.resolve_type(param.1.clone()));
-                    let return_type = this.resolve_type(return_type.get_type().into_owned());
+                    let func_meta = this.interner.function_meta(&func_id);
+
+                    let arguments = vecmap(&func_meta.parameters.0, |(_, typ, _)| typ.clone());
+                    let return_type = func_meta.return_type().clone();
 
                     let generics = vecmap(&this.generics, |(_, type_var, _)| type_var.clone());
 

compiler/noirc_frontend/src/hir/resolution/resolver.rs

@@ -1110,6 +1110,7 @@ impl<'a> Resolver<'a> {
             // This is only used by the elaborator
             all_generics: Vec::new(),
             is_trait_function: false,
+            parameter_idents: Vec::new(),
         }
     }
 

compiler/noirc_frontend/src/hir/type_check/mod.rs

@@ -552,6 +552,7 @@ pub mod test {
             is_trait_function: false,
             has_inline_attribute: false,
             all_generics: Vec::new(),
+            parameter_idents: Vec::new(),
         };
         interner.push_fn_meta(func_meta, func_id);
 

compiler/noirc_frontend/src/hir_def/function.rs

@@ -95,6 +95,10 @@ pub struct FuncMeta {
 
     pub parameters: Parameters,
 
+    /// The HirIdent of each identifier within the parameter list.
+    /// Note that this includes separate entries for each identifier in e.g. tuple patterns.
+    pub parameter_idents: Vec<HirIdent>,
+
     pub return_type: FunctionReturnType,
 
     pub return_visibility: Visibility,