feat(traits): added checks for duplicated trait associated items (types, consts, functions)

compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -302,7 +302,7 @@
         errors.extend(collect_impls(context, crate_id, &def_collector.collected_impls));
 
         // Bind trait impls to their trait. Collect trait functions, that have a
         // default implementation, which hasn't been overriden.
         errors.extend(collect_trait_impls(
             context,
             crate_id,
@@ -463,7 +463,7 @@
 
             if overrides.len() > 1 {
                 let error = DefCollectorErrorKind::Duplicate {
-                    typ: DuplicateType::Function,
+                    typ: DuplicateType::TraitAssociatedFunction,
                     first_def: overrides[0].2.name_ident().clone(),
                     second_def: overrides[1].2.name_ident().clone(),
                 };
@@ -794,11 +794,7 @@
                 .iter()
                 .filter(|(_, _, q)| q.name() == name.0.contents)
                 .collect();
-            let default_impl = if !default_impl_list.is_empty() {
-                if default_impl_list.len() > 1 {
-                    // TODO(nickysn): Add check for method duplicates in the trait and emit proper error messages. This is planned in a future PR.
-                    panic!("Too many functions with the same name!");
-                }
+            let default_impl = if default_impl_list.len() == 1 {
                 Some(Box::new(default_impl_list[0].2.clone()))
             } else {
                 None
@@ -1025,7 +1021,7 @@
     methods
 }
 
 // TODO(vitkov): Move this out of here and into type_check
 fn check_methods_signatures(
     resolver: &mut Resolver,
     impl_methods: &Vec<(FileId, FuncId)>,
@@ -1043,7 +1039,7 @@
         let meta = resolver.interner.function_meta(func_id);
         let func_name = resolver.interner.function_name(func_id).to_owned();
 
         let mut typecheck_errors = Vec::new();
 
         // `method` is None in the case where the impl block has a method that's not part of the trait.
         // If that's the case, a `MethodNotInTrait` error has already been thrown, and we can ignore
@@ -1059,7 +1055,7 @@
                     for (parameter_index, ((expected, actual), (hir_pattern, _, _))) in
                         method.arguments.iter().zip(&params).zip(&meta.parameters.0).enumerate()
                     {
                         expected.unify(actual, &mut typecheck_errors, || {
                             TypeCheckError::TraitMethodParameterTypeMismatch {
                                 method_name: func_name.to_string(),
                                 expected_typ: expected.to_string(),
@@ -1088,7 +1084,7 @@
             let resolved_return_type =
                 resolver.resolve_type(meta.return_type.get_type().into_owned());
 
             method.return_type.unify(&resolved_return_type, &mut typecheck_errors, || {
                 let ret_type_span =
                     meta.return_type.get_type().span.expect("return type must always have a span");
 
@@ -1099,7 +1095,7 @@
                 }
             });
 
             errors.extend(typecheck_errors.iter().cloned().map(|e| (e.into(), *file_id)));
         }
     }
 

compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -1,4 +1,4 @@
-use std::vec;
+use std::{collections::HashMap, vec};
 
 use fm::FileId;
 use noirc_errors::Location;
@@ -240,7 +240,7 @@
         types: Vec<NoirStruct>,
         krate: CrateId,
     ) -> Vec<(CompilationError, FileId)> {
         let mut definiton_errors = vec![];
         for struct_definition in types {
             let name = struct_definition.name.clone();
 
@@ -254,7 +254,7 @@
             let id = match self.push_child_module(&name, self.file_id, false, false) {
                 Ok(local_id) => context.def_interner.new_struct(&unresolved, krate, local_id),
                 Err(error) => {
                     definiton_errors.push((error.into(), self.file_id));
                     continue;
                 }
             };
@@ -269,13 +269,13 @@
                     first_def,
                     second_def,
                 };
                 definiton_errors.push((error.into(), self.file_id));
             }
 
             // And store the TypeId -> StructType mapping somewhere it is reachable
             self.def_collector.collected_types.insert(id, unresolved);
         }
         definiton_errors
     }
 
     /// Collect any type aliases definitions declared within the ast.
@@ -316,6 +316,85 @@
         errors
     }
 
+    fn check_for_duplicate_trait_items(
+        &mut self,
+        trait_items: &Vec<TraitItem>,
+    ) -> Vec<(CompilationError, FileId)> {
+        let mut errors: Vec<(CompilationError, FileId)> = vec![];
+
+        let mut functions_and_consts: HashMap<&Ident, &TraitItem> = HashMap::new();
+        let mut types: HashMap<&Ident, &TraitItem> = HashMap::new();
+
+        for trait_item in trait_items {
+            match trait_item {
+                TraitItem::Function { name, .. } => {
+                    if let Some(prev_item) = functions_and_consts.insert(name, trait_item) {
+                        let error = match prev_item {
+                            TraitItem::Function { name: prev_name, .. } => {
+                                DefCollectorErrorKind::Duplicate {
+                                    typ: DuplicateType::TraitAssociatedFunction,
+                                    first_def: prev_name.clone(),
+                                    second_def: name.clone(),
+                                }
+                            }
+                            TraitItem::Constant { name: prev_name, .. } => {
+                                DefCollectorErrorKind::Duplicate {
+                                    typ: DuplicateType::TraitAssociatedItem,
+                                    first_def: prev_name.clone(),
+                                    second_def: name.clone(),
+                                }
+                            }
+                            TraitItem::Type { .. } => {
+                                unreachable!();
+                            }
+                        };
+                        errors.push((error.into(), self.file_id));
+                    }
+                }
+                TraitItem::Constant { name, .. } => {
+                    if let Some(prev_item) = functions_and_consts.insert(name, trait_item) {
+                        let error = match prev_item {
+                            TraitItem::Function { name: prev_name, .. } => {
+                                DefCollectorErrorKind::Duplicate {
+                                    typ: DuplicateType::TraitAssociatedItem,
+                                    first_def: prev_name.clone(),
+                                    second_def: name.clone(),
+                                }
+                            }
+                            TraitItem::Constant { name: prev_name, .. } => {
+                                DefCollectorErrorKind::Duplicate {
+                                    typ: DuplicateType::TraitAssociatedConst,
+                                    first_def: prev_name.clone(),
+                                    second_def: name.clone(),
+                                }
+                            }
+                            TraitItem::Type { .. } => {
+                                unreachable!();
+                            }
+                        };
+                        errors.push((error.into(), self.file_id));
+                    }
+                }
+                TraitItem::Type { name } => {
+                    if let Some(prev_item) = types.insert(name, trait_item) {
+                        if let TraitItem::Type { name: prev_name } = prev_item {
+                            let error = DefCollectorErrorKind::Duplicate {
+                                typ: DuplicateType::TraitAssociatedType,
+                                first_def: prev_name.clone(),
+                                second_def: name.clone(),
+                            };
+                            errors.push((error.into(), self.file_id));
+                        } else {
+                            unreachable!();
+                        }
+                    }
+                }
+            }
+        }
+
+        errors
+    }
+
     /// Collect any traits definitions declared within the ast.
     /// Returns a vector of errors if any traits were already defined.
     fn collect_traits(
@@ -350,6 +429,8 @@
                 errors.push((error.into(), self.file_id));
             }
 
+            errors.append(&mut self.check_for_duplicate_trait_items(&trait_definition.items));
+
             // Add all functions that have a default implementation in the trait
             let mut unresolved_functions =
                 UnresolvedFunctions { file_id: self.file_id, functions: Vec::new() };

compiler/noirc_frontend/src/hir/def_collector/errors.rs

@@ -18,6 +18,10 @@ pub enum DuplicateType {
     Import,
     Trait,
     TraitImplementation,
+    TraitAssociatedType,
+    TraitAssociatedConst,
+    TraitAssociatedFunction,
+    TraitAssociatedItem,
 }
 
 #[derive(Error, Debug, Clone)]
@@ -79,6 +83,10 @@ impl fmt::Display for DuplicateType {
             DuplicateType::Trait => write!(f, "trait definition"),
             DuplicateType::TraitImplementation => write!(f, "trait implementation"),
             DuplicateType::Import => write!(f, "import"),
+            DuplicateType::TraitAssociatedType => write!(f, "trait associated type"),
+            DuplicateType::TraitAssociatedConst => write!(f, "trait associated constant"),
+            DuplicateType::TraitAssociatedFunction => write!(f, "trait associated function"),
+            DuplicateType::TraitAssociatedItem => write!(f, "trait associated item"),
         }
     }
 }

tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "dup_trait_items_1"
+type = "bin"
+authors = [""]
+compiler_version = "0.15.0"
+
+[dependencies]

tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/src/main.nr

@@ -0,0 +1,7 @@
+trait MyTrait {
+  fn SomeFunc();
+  fn SomeFunc();
+}
+
+fn main() {
+}

tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "dup_trait_items_2"
+type = "bin"
+authors = [""]
+compiler_version = "0.15.0"
+
+[dependencies]

tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/src/main.nr

@@ -0,0 +1,7 @@
+trait MyTrait {
+  let SomeConst: u32;
+  let SomeConst: Field;
+}
+
+fn main() {
+}

tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "dup_trait_items_3"
+type = "bin"
+authors = [""]
+compiler_version = "0.15.0"
+
+[dependencies]

tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/src/main.nr

@@ -0,0 +1,7 @@
+trait MyTrait {
+  type SomeType;
+  type SomeType;
+}
+
+fn main() {
+}

tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "dup_trait_items_4"
+type = "bin"
+authors = [""]
+compiler_version = "0.15.0"
+
+[dependencies]

tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/src/main.nr

@@ -0,0 +1,7 @@
+trait MyTrait {
+  let MyItem: u32;
+  fn MyItem();
+}
+
+fn main() {
+}

tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "dup_trait_items_5"
+type = "bin"
+authors = [""]
+compiler_version = "0.15.0"
+
+[dependencies]

tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/src/main.nr

@@ -0,0 +1,7 @@
+trait MyTrait {
+  fn MyItem();
+  let MyItem: u32;
+}
+
+fn main() {
+}

tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "dup_trait_items_6"
+type = "bin"
+authors = [""]
+compiler_version = "0.15.0"
+
+[dependencies]

tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/src/main.nr

@@ -0,0 +1,15 @@
+trait MyTrait {
+  fn SomeFunc() { };
+  fn SomeFunc() { };
+}
+
+struct MyStruct {
+}
+
+impl MyTrait for MyStruct {
+  fn SomeFunc() {
+  }
+}
+
+fn main() {
+}

tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "trait_allowed_item_name_matches"
+type = "bin"
+authors = [""]
+compiler_version = "0.15.0"
+
+[dependencies]

tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/src/main.nr

@@ -0,0 +1,26 @@
+trait Trait1 {
+  // types and consts with the same name are allowed
+  type Tralala;
+  let Tralala: u32;
+}
+
+trait Trait2 {
+  // consts and types with the same name are allowed
+  let Tralala: u32;
+  type Tralala;
+}
+
+trait Trait3 {
+  // types and functions with the same name are allowed
+  type Tralala;
+  fn Tralala();
+}
+
+trait Trait4 {
+  // functions and types with the same name are allowed
+  fn Tralala();
+  type Tralala;
+}
+
+fn main() {
+}