From 5d0de2a640bded08c84f0a756a8c84cadcdcd952 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Sat, 30 Sep 2023 08:46:42 +0300 Subject: [PATCH 01/14] feat(traits): added checks for duplicate trait associated items (types, consts, functions) --- .../src/hir/def_collector/dc_mod.rs | 100 +++++++++++++++++- .../src/hir/def_collector/errors.rs | 8 ++ .../dup_trait_items_1/Nargo.toml | 7 ++ .../dup_trait_items_1/Prover.toml | 0 .../dup_trait_items_1/src/main.nr | 7 ++ .../dup_trait_items_2/Nargo.toml | 7 ++ .../dup_trait_items_2/Prover.toml | 0 .../dup_trait_items_2/src/main.nr | 7 ++ .../dup_trait_items_3/Nargo.toml | 7 ++ .../dup_trait_items_3/Prover.toml | 0 .../dup_trait_items_3/src/main.nr | 7 ++ .../dup_trait_items_4/Nargo.toml | 7 ++ .../dup_trait_items_4/Prover.toml | 0 .../dup_trait_items_4/src/main.nr | 7 ++ .../dup_trait_items_5/Nargo.toml | 7 ++ .../dup_trait_items_5/Prover.toml | 0 .../dup_trait_items_5/src/main.nr | 7 ++ .../Nargo.toml | 7 ++ .../Prover.toml | 0 .../src/main.nr | 26 +++++ 20 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/src/main.nr create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/src/main.nr create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/src/main.nr create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/src/main.nr create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/src/main.nr create mode 100644 tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/Nargo.toml create mode 100644 tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/Prover.toml create mode 100644 tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 189cfaa1569..521e3b5e1f2 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/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; @@ -316,6 +316,102 @@ impl<'a> ModCollector<'a> { errors } + fn check_for_duplicate_trait_items( + &mut self, + trait_items: &Vec, + ) -> 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, + generics: _, + parameters: _, + return_type: _, + where_clause: _, + body: _, + } => { + if let Some(prev_item) = functions_and_consts.insert(name, trait_item) { + let error = match prev_item { + TraitItem::Function { + name: prev_name, + generics: _, + parameters: _, + return_type: _, + where_clause: _, + body: _, + } => DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedFunction, + first_def: prev_name.clone(), + second_def: name.clone(), + }, + TraitItem::Constant { name: prev_name, typ: _, default_value: _ } => { + DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedItem, + first_def: prev_name.clone(), + second_def: name.clone(), + } + } + TraitItem::Type { name: _ } => { + unreachable!(); + } + }; + errors.push((error.into(), self.file_id)); + } + } + TraitItem::Constant { name, typ: _, default_value: _ } => { + if let Some(prev_item) = functions_and_consts.insert(name, trait_item) { + let error = match prev_item { + TraitItem::Function { + name: prev_name, + generics: _, + parameters: _, + return_type: _, + where_clause: _, + body: _, + } => DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedItem, + first_def: prev_name.clone(), + second_def: name.clone(), + }, + TraitItem::Constant { name: prev_name, typ: _, default_value: _ } => { + DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedConst, + first_def: prev_name.clone(), + second_def: name.clone(), + } + } + TraitItem::Type { name: _ } => { + 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 +446,8 @@ impl<'a> ModCollector<'a> { 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() }; diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index f959cdec598..2318794390c 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/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"), } } } diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/Nargo.toml new file mode 100644 index 00000000000..d8b44af47c2 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/Prover.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/src/main.nr b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_1/src/main.nr new file mode 100644 index 00000000000..9055d6e3998 --- /dev/null +++ b/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() { +} diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/Nargo.toml new file mode 100644 index 00000000000..b37256a1292 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/Prover.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/src/main.nr b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_2/src/main.nr new file mode 100644 index 00000000000..312b1e5a9d4 --- /dev/null +++ b/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() { +} diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/Nargo.toml new file mode 100644 index 00000000000..c9a0de11174 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/Prover.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/src/main.nr b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_3/src/main.nr new file mode 100644 index 00000000000..ca97a9a143d --- /dev/null +++ b/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() { +} diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/Nargo.toml new file mode 100644 index 00000000000..5e4af3a29ae --- /dev/null +++ b/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] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/Prover.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/src/main.nr b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_4/src/main.nr new file mode 100644 index 00000000000..da0532e39c1 --- /dev/null +++ b/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() { +} diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/Nargo.toml new file mode 100644 index 00000000000..2d8c9aad7ec --- /dev/null +++ b/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] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/Prover.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/src/main.nr b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_5/src/main.nr new file mode 100644 index 00000000000..4881a338a84 --- /dev/null +++ b/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() { +} diff --git a/tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/Nargo.toml b/tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/Nargo.toml new file mode 100644 index 00000000000..459cf96bab7 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/Prover.toml b/tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/src/main.nr b/tooling/nargo_cli/tests/execution_success/trait_allowed_item_name_matches/src/main.nr new file mode 100644 index 00000000000..7db61e854fc --- /dev/null +++ b/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() { +} From aa8d93f81ba4754f90bd51fb2218298633f8dec4 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Sat, 30 Sep 2023 13:39:31 +0300 Subject: [PATCH 02/14] fix(traits): avoid compiler panic, when a trait method override is found, with duplicated trait functions with default implementations --- .../src/hir/def_collector/dc_crate.rs | 6 +----- .../compile_failure/dup_trait_items_6/Nargo.toml | 7 +++++++ .../compile_failure/dup_trait_items_6/Prover.toml | 0 .../compile_failure/dup_trait_items_6/src/main.nr | 15 +++++++++++++++ 4 files changed, 23 insertions(+), 5 deletions(-) create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 07ad6f598bd..9ef82fcc758 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -794,11 +794,7 @@ fn resolve_trait_methods( .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 diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/Nargo.toml new file mode 100644 index 00000000000..5107c10b41d --- /dev/null +++ b/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] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/Prover.toml b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/src/main.nr b/tooling/nargo_cli/tests/compile_failure/dup_trait_items_6/src/main.nr new file mode 100644 index 00000000000..a1a731d943b --- /dev/null +++ b/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() { +} From 3c51b48647efc53a9cf97a344aa04ae370b34d46 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Sat, 30 Sep 2023 14:30:09 +0300 Subject: [PATCH 03/14] fix(traits): report duplicated functions in the trait implementation as `trait associated functions` in the error message, instead of just `functions` --- compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 9ef82fcc758..d8884dd500a 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -463,7 +463,7 @@ fn collect_trait_impl_methods( 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(), }; From 4598fa1f0fab5d8b28a62837d423c96679d0f457 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Mon, 2 Oct 2023 11:28:48 +0300 Subject: [PATCH 04/14] refactor(traits): simplify code via the use of `..`. No functional changes. --- .../src/hir/def_collector/dc_mod.rs | 37 ++++--------------- 1 file changed, 8 insertions(+), 29 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 521e3b5e1f2..fd1fdf69ee7 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -327,65 +327,44 @@ impl<'a> ModCollector<'a> { for trait_item in trait_items { match trait_item { - TraitItem::Function { - name, - generics: _, - parameters: _, - return_type: _, - where_clause: _, - body: _, - } => { + 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, - generics: _, - parameters: _, - return_type: _, - where_clause: _, - body: _, - } => DefCollectorErrorKind::Duplicate { + TraitItem::Function { name: prev_name, .. } => DefCollectorErrorKind::Duplicate { typ: DuplicateType::TraitAssociatedFunction, first_def: prev_name.clone(), second_def: name.clone(), }, - TraitItem::Constant { name: prev_name, typ: _, default_value: _ } => { + TraitItem::Constant { name: prev_name, .. } => { DefCollectorErrorKind::Duplicate { typ: DuplicateType::TraitAssociatedItem, first_def: prev_name.clone(), second_def: name.clone(), } } - TraitItem::Type { name: _ } => { + TraitItem::Type { .. } => { unreachable!(); } }; errors.push((error.into(), self.file_id)); } } - TraitItem::Constant { name, typ: _, default_value: _ } => { + 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, - generics: _, - parameters: _, - return_type: _, - where_clause: _, - body: _, - } => DefCollectorErrorKind::Duplicate { + TraitItem::Function { name: prev_name, .. } => DefCollectorErrorKind::Duplicate { typ: DuplicateType::TraitAssociatedItem, first_def: prev_name.clone(), second_def: name.clone(), }, - TraitItem::Constant { name: prev_name, typ: _, default_value: _ } => { + TraitItem::Constant { name: prev_name, .. } => { DefCollectorErrorKind::Duplicate { typ: DuplicateType::TraitAssociatedConst, first_def: prev_name.clone(), second_def: name.clone(), } } - TraitItem::Type { name: _ } => { + TraitItem::Type { .. } => { unreachable!(); } }; From 9dfa68e965648b4ae24babdbf007a2f6328b7faf Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Mon, 2 Oct 2023 11:49:50 +0300 Subject: [PATCH 05/14] chore(traits): apply cargo fmt changes --- .../src/hir/def_collector/dc_mod.rs | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index fd1fdf69ee7..e623664c147 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -330,11 +330,13 @@ impl<'a> ModCollector<'a> { 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::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, @@ -352,11 +354,13 @@ impl<'a> ModCollector<'a> { 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::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, From a6407cd3d04d361d696e1ab639e3d807e7489b33 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Wed, 4 Oct 2023 16:15:06 +0300 Subject: [PATCH 06/14] refactor(traits): duplicate check refactored to use the module system --- .../src/hir/def_collector/dc_mod.rs | 127 +++++++----------- 1 file changed, 45 insertions(+), 82 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index e623664c147..d899df8667c 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, vec}; +use std::vec; use fm::FileId; use noirc_errors::Location; @@ -6,7 +6,7 @@ use noirc_errors::Location; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, - node_interner::TraitId, + node_interner::{FuncId, TraitId, StmtId, TypeAliasId}, parser::SubModule, FunctionDefinition, Ident, LetStatement, NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, ParsedModule, TraitImplItem, TraitItem, TypeImpl, @@ -316,85 +316,6 @@ impl<'a> ModCollector<'a> { errors } - fn check_for_duplicate_trait_items( - &mut self, - trait_items: &Vec, - ) -> 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( @@ -429,7 +350,49 @@ impl<'a> ModCollector<'a> { errors.push((error.into(), self.file_id)); } - errors.append(&mut self.check_for_duplicate_trait_items(&trait_definition.items)); + for trait_item in &trait_definition.items { + match trait_item { + TraitItem::Function { name, .. } => { + if let Err((first_def, second_def)) = self.def_collector.def_map.modules + [id.0.local_id.0] + .declare_function(name.clone(), FuncId::dummy_id()) + { + let error = DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedFunction, + first_def, + second_def, + }; + errors.push((error.into(), self.file_id)); + } + } + TraitItem::Constant { name, .. } => { + if let Err((first_def, second_def)) = self.def_collector.def_map.modules + [id.0.local_id.0] + .declare_global(name.clone(), StmtId::dummy_id()) + { + let error = DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedConst, + first_def, + second_def, + }; + errors.push((error.into(), self.file_id)); + } + } + TraitItem::Type { name } => { + if let Err((first_def, second_def)) = self.def_collector.def_map.modules + [id.0.local_id.0] + .declare_type_alias(name.clone(), TypeAliasId::dummy_id()) + { + let error = DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedType, + first_def, + second_def, + }; + errors.push((error.into(), self.file_id)); + } + } + } + } // Add all functions that have a default implementation in the trait let mut unresolved_functions = From 68941cf15063f692a6a6fea905c421337720e5ff Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Wed, 4 Oct 2023 16:17:08 +0300 Subject: [PATCH 07/14] refactor(traits): removed DuplicateType::TraitAssociatedItem, because it's no longer used --- compiler/noirc_frontend/src/hir/def_collector/errors.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index 2318794390c..cda9fea6d08 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -21,7 +21,6 @@ pub enum DuplicateType { TraitAssociatedType, TraitAssociatedConst, TraitAssociatedFunction, - TraitAssociatedItem, } #[derive(Error, Debug, Clone)] @@ -86,7 +85,6 @@ impl fmt::Display for DuplicateType { 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"), } } } From e9991f6b7bdb57fea4ca61e57b8276453e9286d2 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Wed, 4 Oct 2023 16:49:11 +0300 Subject: [PATCH 08/14] fix(traits): when registering the trait impl function in the struct namespace, use also the proper crate id for the struct --- compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index d8884dd500a..ef99f330993 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -535,7 +535,7 @@ fn collect_trait_impl( if let Some(struct_type) = get_struct_type(&typ) { errors.extend(take_errors(trait_impl.file_id, resolver)); - let current_def_map = def_maps.get_mut(&crate_id).unwrap(); + let current_def_map = def_maps.get_mut(&struct_type.borrow().id.krate()).unwrap(); match add_method_to_struct_namespace( current_def_map, struct_type, From f16e710ad81b470b669f3783acfa2e6f5721e537 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Wed, 4 Oct 2023 16:51:39 +0300 Subject: [PATCH 09/14] refactor(traits): cargo fmt --- compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index d899df8667c..51bff62421a 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -6,7 +6,7 @@ use noirc_errors::Location; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, - node_interner::{FuncId, TraitId, StmtId, TypeAliasId}, + node_interner::{FuncId, StmtId, TraitId, TypeAliasId}, parser::SubModule, FunctionDefinition, Ident, LetStatement, NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, ParsedModule, TraitImplItem, TraitItem, TypeImpl, From 1251c7ebaacaf75e60ccd73bb645d1413589cc7f Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Thu, 5 Oct 2023 12:30:27 +0300 Subject: [PATCH 10/14] refactor(traits): merge the loop that checks for duplicate items in the trait with the next loop. No functional changes. --- .../src/hir/def_collector/dc_mod.rs | 60 +++++++++---------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index d7ab3defeb0..0b2aaffd6c9 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -353,9 +353,22 @@ impl<'a> ModCollector<'a> { errors.push((error.into(), self.file_id)); } + // Add all functions that have a default implementation in the trait + let mut unresolved_functions = UnresolvedFunctions { + file_id: self.file_id, + functions: Vec::new(), + trait_id: None, + }; for trait_item in &trait_definition.items { match trait_item { - TraitItem::Function { name, .. } => { + TraitItem::Function { + name, + generics, + parameters, + return_type, + where_clause, + body, + } => { if let Err((first_def, second_def)) = self.def_collector.def_map.modules [id.0.local_id.0] .declare_function(name.clone(), FuncId::dummy_id()) @@ -367,6 +380,20 @@ impl<'a> ModCollector<'a> { }; errors.push((error.into(), self.file_id)); } + // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629 + if let Some(body) = body { + let func_id = context.def_interner.push_empty_fn(); + + let impl_method = NoirFunction::normal(FunctionDefinition::normal( + name, + generics, + parameters, + body, + where_clause, + return_type, + )); + unresolved_functions.push_fn(self.module_id, func_id, impl_method); + } } TraitItem::Constant { name, .. } => { if let Err((first_def, second_def)) = self.def_collector.def_map.modules @@ -397,37 +424,6 @@ impl<'a> ModCollector<'a> { } } - // Add all functions that have a default implementation in the trait - let mut unresolved_functions = UnresolvedFunctions { - file_id: self.file_id, - functions: Vec::new(), - trait_id: None, - }; - for trait_item in &trait_definition.items { - // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629 - if let TraitItem::Function { - name, - generics, - parameters, - return_type, - where_clause, - body: Some(body), - } = trait_item - { - let func_id = context.def_interner.push_empty_fn(); - - let impl_method = NoirFunction::normal(FunctionDefinition::normal( - name, - generics, - parameters, - body, - where_clause, - return_type, - )); - unresolved_functions.push_fn(self.module_id, func_id, impl_method); - } - } - // And store the TraitId -> TraitType mapping somewhere it is reachable let unresolved = UnresolvedTrait { file_id: self.file_id, From d5eca9608e66b477092f726fa19dabab4f2955a9 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Thu, 5 Oct 2023 12:32:22 +0300 Subject: [PATCH 11/14] refactor(traits): always generate a FuncId for a trait function, regardless of whether it has a body. Push that id in the trait namespace as well. --- compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 0b2aaffd6c9..ff2a60a524a 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -6,7 +6,7 @@ use noirc_errors::Location; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, - node_interner::{FuncId, StmtId, TraitId, TypeAliasId}, + node_interner::{StmtId, TraitId, TypeAliasId}, parser::SubModule, FunctionDefinition, Ident, LetStatement, NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, ParsedModule, TraitImplItem, TraitItem, TypeImpl, @@ -369,9 +369,10 @@ impl<'a> ModCollector<'a> { where_clause, body, } => { + let func_id = context.def_interner.push_empty_fn(); if let Err((first_def, second_def)) = self.def_collector.def_map.modules [id.0.local_id.0] - .declare_function(name.clone(), FuncId::dummy_id()) + .declare_function(name.clone(), func_id) { let error = DefCollectorErrorKind::Duplicate { typ: DuplicateType::TraitAssociatedFunction, @@ -382,8 +383,6 @@ impl<'a> ModCollector<'a> { } // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629 if let Some(body) = body { - let func_id = context.def_interner.push_empty_fn(); - let impl_method = NoirFunction::normal(FunctionDefinition::normal( name, generics, From c79c35003e1c3ff0370c66829fb428392dc97ff4 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Thu, 5 Oct 2023 12:37:27 +0300 Subject: [PATCH 12/14] refactor(traits): when adding consts to the trait namespace, always call def_interner.push_empty_global to get a StmtId, instead of pushing a dummy id --- compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index ff2a60a524a..6b58cdd07df 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -6,7 +6,7 @@ use noirc_errors::Location; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, - node_interner::{StmtId, TraitId, TypeAliasId}, + node_interner::{TraitId, TypeAliasId}, parser::SubModule, FunctionDefinition, Ident, LetStatement, NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, ParsedModule, TraitImplItem, TraitItem, TypeImpl, @@ -395,9 +395,11 @@ impl<'a> ModCollector<'a> { } } TraitItem::Constant { name, .. } => { + let stmt_id = context.def_interner.push_empty_global(); + if let Err((first_def, second_def)) = self.def_collector.def_map.modules [id.0.local_id.0] - .declare_global(name.clone(), StmtId::dummy_id()) + .declare_global(name.clone(), stmt_id) { let error = DefCollectorErrorKind::Duplicate { typ: DuplicateType::TraitAssociatedConst, From b1d75e35c828473e575ce6314805964f82d76dd4 Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Thu, 5 Oct 2023 12:56:51 +0300 Subject: [PATCH 13/14] refactor(traits): added TODO comment for replacing the trait type alias dummy id with a real id --- compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 6b58cdd07df..4ff7bf62d07 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -410,6 +410,7 @@ impl<'a> ModCollector<'a> { } } TraitItem::Type { name } => { + // TODO(nickysn or alexvitkov): implement context.def_interner.push_empty_type_alias and get an id, instead of using TypeAliasId::dummy_id() if let Err((first_def, second_def)) = self.def_collector.def_map.modules [id.0.local_id.0] .declare_type_alias(name.clone(), TypeAliasId::dummy_id()) From e52c7f7381759f5ca78f25476b28739625054bec Mon Sep 17 00:00:00 2001 From: Nikolay Nikolov Date: Thu, 5 Oct 2023 12:59:12 +0300 Subject: [PATCH 14/14] refactor(traits): only push the trait function body to the unresolved function list if it's not duplicated --- .../src/hir/def_collector/dc_mod.rs | 48 +++++++++++-------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 4ff7bf62d07..cc3bfc97f79 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -370,28 +370,36 @@ impl<'a> ModCollector<'a> { body, } => { let func_id = context.def_interner.push_empty_fn(); - if let Err((first_def, second_def)) = self.def_collector.def_map.modules - [id.0.local_id.0] + match self.def_collector.def_map.modules[id.0.local_id.0] .declare_function(name.clone(), func_id) { - let error = DefCollectorErrorKind::Duplicate { - typ: DuplicateType::TraitAssociatedFunction, - first_def, - second_def, - }; - errors.push((error.into(), self.file_id)); - } - // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629 - if let Some(body) = body { - let impl_method = NoirFunction::normal(FunctionDefinition::normal( - name, - generics, - parameters, - body, - where_clause, - return_type, - )); - unresolved_functions.push_fn(self.module_id, func_id, impl_method); + Ok(()) => { + // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629 + if let Some(body) = body { + let impl_method = + NoirFunction::normal(FunctionDefinition::normal( + name, + generics, + parameters, + body, + where_clause, + return_type, + )); + unresolved_functions.push_fn( + self.module_id, + func_id, + impl_method, + ); + } + } + Err((first_def, second_def)) => { + let error = DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedFunction, + first_def, + second_def, + }; + errors.push((error.into(), self.file_id)); + } } } TraitItem::Constant { name, .. } => {