From d3e098cf673f1eb4f9ddf52e2f71482817800020 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 15:26:29 -0300 Subject: [PATCH 01/13] Change `Methods::iter` to return `Option<&Type>` instead of `&Option` --- compiler/noirc_frontend/src/node_interner.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index a2b1d7fc8f..5476bf0d38 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -2268,12 +2268,9 @@ impl Methods { } /// Iterate through each method, starting with the direct methods - pub fn iter(&self) -> impl Iterator)> + '_ { - let trait_impl_methods = self.trait_impl_methods.iter().map(|m| (m.method, &m.typ)); - let direct = self.direct.iter().copied().map(|func_id| { - let typ: &Option = &None; - (func_id, typ) - }); + pub fn iter(&self) -> impl Iterator)> + '_ { + let trait_impl_methods = self.trait_impl_methods.iter().map(|m| (m.method, m.typ.as_ref())); + let direct = self.direct.iter().copied().map(|func_id| (func_id, None)); direct.chain(trait_impl_methods) } @@ -2302,7 +2299,7 @@ impl Methods { interner: &NodeInterner, ) -> Option { for method in &self.direct { - if Self::method_matches(typ, has_self_param, *method, &None, interner) { + if Self::method_matches(typ, has_self_param, *method, None, interner) { return Some(*method); } } @@ -2320,7 +2317,7 @@ impl Methods { for trait_impl_method in &self.trait_impl_methods { let method = trait_impl_method.method; - let method_type = &trait_impl_method.typ; + let method_type = trait_impl_method.typ.as_ref(); let trait_id = trait_impl_method.trait_id; if Self::method_matches(typ, has_self_param, method, method_type, interner) { @@ -2335,7 +2332,7 @@ impl Methods { typ: &Type, has_self_param: bool, method: FuncId, - method_type: &Option, + method_type: Option<&Type>, interner: &NodeInterner, ) -> bool { match interner.function_meta(&method).typ.instantiate(interner).0 { From 8c31ade818d67e0d6f5a5cf250ccd411c517ba42 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 15:28:24 -0300 Subject: [PATCH 02/13] Let `Methods::iter()` also return `Option` --- compiler/noirc_frontend/src/node_interner.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 5476bf0d38..f9b9c1e183 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -2268,9 +2268,10 @@ impl Methods { } /// Iterate through each method, starting with the direct methods - pub fn iter(&self) -> impl Iterator)> + '_ { - let trait_impl_methods = self.trait_impl_methods.iter().map(|m| (m.method, m.typ.as_ref())); - let direct = self.direct.iter().copied().map(|func_id| (func_id, None)); + pub fn iter(&self) -> impl Iterator, Option)> + '_ { + let trait_impl_methods = + self.trait_impl_methods.iter().map(|m| (m.method, m.typ.as_ref(), Some(m.trait_id))); + let direct = self.direct.iter().copied().map(|func_id| (func_id, None, None)); direct.chain(trait_impl_methods) } @@ -2283,7 +2284,7 @@ impl Methods { ) -> Option { // When adding methods we always check they do not overlap, so there should be // at most 1 matching method in this list. - for (method, method_type) in self.iter() { + for (method, method_type, _trait_id) in self.iter() { if Self::method_matches(typ, has_self_param, method, method_type, interner) { return Some(method); } From 8ba91b9ec884e2871e37e83ec95117ff2edf792c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 16:17:05 -0300 Subject: [PATCH 03/13] No need to have mutating methods for public fields --- .../noirc_frontend/src/elaborator/trait_impls.rs | 2 +- compiler/noirc_frontend/src/elaborator/traits.rs | 6 +++--- compiler/noirc_frontend/src/hir_def/traits.rs | 12 ------------ 3 files changed, 4 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 20f048bed0..7eca34d7a1 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -103,7 +103,7 @@ impl<'context> Elaborator<'context> { // Restore the methods that were taken before the for loop let the_trait = self.interner.get_trait_mut(trait_id); - the_trait.set_methods(methods); + the_trait.methods = methods; // Emit MethodNotInTrait error for methods in the impl block that // don't have a corresponding method signature defined in the trait diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index a10939dde1..5a9137e04a 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -54,14 +54,14 @@ impl<'context> Elaborator<'context> { } this.interner.update_trait(*trait_id, |trait_def| { - trait_def.set_trait_bounds(resolved_trait_bounds); - trait_def.set_where_clause(where_clause); + trait_def.trait_bounds = resolved_trait_bounds; + trait_def.where_clause = where_clause; }); let methods = this.resolve_trait_methods(*trait_id, unresolved_trait); this.interner.update_trait(*trait_id, |trait_def| { - trait_def.set_methods(methods); + trait_def.methods = methods; }); }); diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index 6fd3c4f7a2..2839bc2f91 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -148,18 +148,6 @@ impl PartialEq for Trait { } impl Trait { - pub fn set_methods(&mut self, methods: Vec) { - self.methods = methods; - } - - pub fn set_trait_bounds(&mut self, trait_bounds: Vec) { - self.trait_bounds = trait_bounds; - } - - pub fn set_where_clause(&mut self, where_clause: Vec) { - self.where_clause = where_clause; - } - pub fn find_method(&self, name: &str) -> Option { for (idx, method) in self.methods.iter().enumerate() { if &method.name == name { From 9661486371b87cf4dbe803c809ee206880aa6a86 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 16:18:03 -0300 Subject: [PATCH 04/13] Correctly track trait impl method visibility --- compiler/noirc_frontend/src/elaborator/mod.rs | 9 +++++++++ compiler/noirc_frontend/src/elaborator/traits.rs | 1 + compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs | 7 ++++--- compiler/noirc_frontend/src/hir_def/traits.rs | 3 ++- compiler/noirc_frontend/src/node_interner.rs | 1 + 5 files changed, 17 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index aeee417789..2fe73683c1 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1110,10 +1110,19 @@ impl<'context> Elaborator<'context> { self.check_parent_traits_are_implemented(&trait_impl); self.remove_trait_impl_assumed_trait_implementations(trait_impl.impl_id); + let trait_id = + trait_impl.trait_id.expect("Trait impl trait ID should have been set at this point"); + let trait_ = self.interner.get_trait(trait_id); + let trait_visibility = trait_.visibility; + for (module, function, _) in &trait_impl.methods.functions { self.local_module = *module; let errors = check_trait_impl_method_matches_declaration(self.interner, *function); self.errors.extend(errors.into_iter().map(|error| (error.into(), self.file))); + + // A trait impl method has the same visibility as its trait + let modifiers = self.interner.function_modifiers_mut(function); + modifiers.visibility = trait_visibility; } self.elaborate_functions(trait_impl.methods); diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 5a9137e04a..f7ace68560 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -56,6 +56,7 @@ impl<'context> Elaborator<'context> { this.interner.update_trait(*trait_id, |trait_def| { trait_def.trait_bounds = resolved_trait_bounds; trait_def.where_clause = where_clause; + trait_def.visibility = unresolved_trait.trait_def.visibility; }); let methods = this.resolve_trait_methods(*trait_id, unresolved_trait); 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 ec13cb2db1..ead6a801ba 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -1228,9 +1228,10 @@ pub(crate) fn collect_trait_impl_items( for item in std::mem::take(&mut trait_impl.items) { match item.item.kind { TraitImplItemKind::Function(mut impl_method) => { - // Regardless of what visibility was on the source code, treat it as public - // (a warning is produced during parsing for this) - impl_method.def.visibility = ItemVisibility::Public; + // Set the impl method visibility as temporarily private. + // Eventually when we find out what trait is this impl for we'll set it + // to the trait's visibility. + impl_method.def.visibility = ItemVisibility::Private; let func_id = interner.push_empty_fn(); let location = Location::new(impl_method.span(), file_id); diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index 2839bc2f91..949e6e5a51 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -1,7 +1,7 @@ use iter_extended::vecmap; use rustc_hash::FxHashMap as HashMap; -use crate::ast::{Ident, NoirFunction}; +use crate::ast::{Ident, ItemVisibility, NoirFunction}; use crate::hir::type_check::generics::TraitGenerics; use crate::ResolvedGeneric; use crate::{ @@ -66,6 +66,7 @@ pub struct Trait { pub name: Ident, pub generics: Generics, pub location: Location, + pub visibility: ItemVisibility, /// When resolving the types of Trait elements, all references to `Self` resolve /// to this TypeVariable. Then when we check if the types of trait impl elements diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index f9b9c1e183..f1bbb147d3 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -729,6 +729,7 @@ impl NodeInterner { crate_id: unresolved_trait.crate_id, location: Location::new(unresolved_trait.trait_def.span, unresolved_trait.file_id), generics, + visibility: ItemVisibility::Private, self_type_typevar: TypeVariable::unbound(self.next_type_variable_id(), Kind::Normal), methods: Vec::new(), method_ids: unresolved_trait.method_ids.clone(), From 7a2bd68e80594ebf4bca285749c2ea184f0ef6c8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 16:18:19 -0300 Subject: [PATCH 05/13] Fix small typo --- tooling/lsp/src/requests/completion.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index fd6ef60af8..5cfda32daa 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -245,7 +245,7 @@ impl<'a> NodeFinder<'a> { let mut idents: Vec = Vec::new(); // Find in which ident we are in, and in which part of it - // (it could be that we are completting in the middle of an ident) + // (it could be that we are completing in the middle of an ident) for segment in &path.segments { let ident = &segment.ident; From ad9ca2f64e6c9dadf9f505073a0a470a9e8e3822 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 16:18:35 -0300 Subject: [PATCH 06/13] Suggest all possible trait functions, but only visible ones --- compiler/noirc_frontend/src/node_interner.rs | 25 ++++---- tooling/lsp/src/requests/completion.rs | 65 +++++++++++--------- tooling/lsp/src/requests/completion/tests.rs | 21 +++++++ 3 files changed, 69 insertions(+), 42 deletions(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index f1bbb147d3..0ec033a399 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -2269,29 +2269,26 @@ impl Methods { } /// Iterate through each method, starting with the direct methods - pub fn iter(&self) -> impl Iterator, Option)> + '_ { + pub fn iter(&self) -> impl Iterator, Option)> { let trait_impl_methods = self.trait_impl_methods.iter().map(|m| (m.method, m.typ.as_ref(), Some(m.trait_id))); let direct = self.direct.iter().copied().map(|func_id| (func_id, None, None)); direct.chain(trait_impl_methods) } - /// Select the 1 matching method with an object type matching `typ` - pub fn find_matching_method( - &self, - typ: &Type, + pub fn find_matching_methods<'a>( + &'a self, + typ: &'a Type, has_self_param: bool, - interner: &NodeInterner, - ) -> Option { - // When adding methods we always check they do not overlap, so there should be - // at most 1 matching method in this list. - for (method, method_type, _trait_id) in self.iter() { + interner: &'a NodeInterner, + ) -> impl Iterator)> + 'a { + self.iter().filter_map(move |(method, method_type, trait_id)| { if Self::method_matches(typ, has_self_param, method, method_type, interner) { - return Some(method); + Some((method, trait_id)) + } else { + None } - } - - None + }) } pub fn find_direct_method( diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 5cfda32daa..a6c890c423 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -28,6 +28,7 @@ use noirc_frontend::{ def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId}, resolution::visibility::{ item_in_module_is_visible, method_call_is_visible, struct_member_is_visible, + trait_member_is_visible, }, }, hir_def::traits::Trait, @@ -658,39 +659,47 @@ impl<'a> NodeFinder<'a> { let has_self_param = matches!(function_kind, FunctionKind::SelfType(..)); for (name, methods) in methods_by_name { - let Some(func_id) = - methods.find_matching_method(typ, has_self_param, self.interner).or_else(|| { - // Also try to find a method assuming typ is `&mut typ`: - // we want to suggest methods that take `&mut self` even though a variable might not - // be mutable, so a user can know they need to mark it as mutable. - let typ = Type::MutableReference(Box::new(typ.clone())); - methods.find_matching_method(&typ, has_self_param, self.interner) - }) - else { + if !name_matches(name, prefix) { continue; - }; - - if let Some(struct_id) = struct_id { - let modifiers = self.interner.function_modifiers(&func_id); - let visibility = modifiers.visibility; - if !struct_member_is_visible(struct_id, visibility, self.module_id, self.def_maps) { - continue; - } } - if is_primitive - && !method_call_is_visible( - typ, - func_id, - self.module_id, - self.interner, - self.def_maps, - ) + for (func_id, trait_id) in + methods.find_matching_methods(typ, has_self_param, self.interner) { - continue; - } + if let Some(struct_id) = struct_id { + let modifiers = self.interner.function_modifiers(&func_id); + let visibility = modifiers.visibility; + if !struct_member_is_visible( + struct_id, + visibility, + self.module_id, + self.def_maps, + ) { + continue; + } + } + + if let Some(trait_id) = trait_id { + let modifiers = self.interner.function_modifiers(&func_id); + let visibility = modifiers.visibility; + if !trait_member_is_visible(trait_id, visibility, self.module_id, self.def_maps) + { + continue; + } + } + + if is_primitive + && !method_call_is_visible( + typ, + func_id, + self.module_id, + self.interner, + self.def_maps, + ) + { + continue; + } - if name_matches(name, prefix) { let completion_items = self.function_completion_items( name, func_id, diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 6515709076..c1d37f9150 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -2879,4 +2879,25 @@ fn main() { let items = get_completions(src).await; assert_eq!(items.len(), 1); } + + #[test] + async fn test_does_not_suggest_trait_function_not_visible() { + let src = r#" + mod moo { + trait Foo { + fn foobar(); + } + + impl Foo for Field { + fn foobar() {} + } + } + + fn main() { + Field::fooba>|< + } + + "#; + assert_completion(src, vec![]).await; + } } From 1df763c444f4897c8f826d00039b75f2383fb093 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 16:18:49 -0300 Subject: [PATCH 07/13] Don't show visibility for trait and trait impl functions --- tooling/lsp/src/requests/hover.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index 9da24fd1c8..ef1246d752 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -399,7 +399,10 @@ fn format_function(id: FuncId, args: &ProcessRequestCallbackArgs) -> String { } string.push_str(" "); - if func_modifiers.visibility != ItemVisibility::Private { + if func_modifiers.visibility != ItemVisibility::Private + && func_meta.trait_id.is_none() + && func_meta.trait_impl.is_none() + { string.push_str(&func_modifiers.visibility.to_string()); string.push(' '); } @@ -1154,7 +1157,7 @@ mod hover_tests { assert!(hover_text.starts_with( " two impl Bar for Foo - pub fn bar_stuff(self)" + fn bar_stuff(self)" )); } From c65b63aa126e052ab601bb5e05e7711a299812c8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 16:27:19 -0300 Subject: [PATCH 08/13] Test that multiple trait methods are suggested --- tooling/lsp/src/requests/completion/tests.rs | 30 ++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index c1d37f9150..e3fdddaa3a 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -2900,4 +2900,34 @@ fn main() { "#; assert_completion(src, vec![]).await; } + + #[test] + async fn test_suggests_multiple_trait_methods() { + let src = r#" + mod moo { + pub trait Foo { + fn foobar(); + } + + impl Foo for Field { + fn foobar() {} + } + + pub trait Bar { + fn foobar(); + } + + impl Bar for Field { + fn foobar() {} + } + } + + fn main() { + Field::fooba>|< + } + + "#; + let items = get_completions(src).await; + assert_eq!(items.len(), 2); + } } From 6202c67a73b0792d3769070f2b4eb2c4e2a3b788 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 16:56:59 -0300 Subject: [PATCH 09/13] Set trait impl methods visibility before elaborating functions --- compiler/noirc_frontend/src/elaborator/mod.rs | 31 +++++++++++++------ 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 2fe73683c1..e6abe5f652 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -346,6 +346,11 @@ impl<'context> Elaborator<'context> { &items.module_attributes, ); + // Make sure trait impl functions have the same visibility as their traits + for trait_impl in &items.trait_impls { + self.set_trait_impl_functions_visibility(trait_impl); + } + for functions in items.functions { self.elaborate_functions(functions); } @@ -1098,6 +1103,23 @@ impl<'context> Elaborator<'context> { } } + fn set_trait_impl_functions_visibility(&mut self, trait_impl: &UnresolvedTraitImpl) { + let trait_visibility = if let Some(trait_id) = trait_impl.trait_id { + let trait_ = self.interner.get_trait(trait_id); + Some(trait_.visibility) + } else { + None + }; + + for (_, function, _) in &trait_impl.methods.functions { + // A trait impl method has the same visibility as its trait + if let Some(trait_visibility) = trait_visibility { + let modifiers = self.interner.function_modifiers_mut(function); + modifiers.visibility = trait_visibility; + } + } + } + fn elaborate_trait_impl(&mut self, trait_impl: UnresolvedTraitImpl) { self.file = trait_impl.file_id; self.local_module = trait_impl.module_id; @@ -1110,19 +1132,10 @@ impl<'context> Elaborator<'context> { self.check_parent_traits_are_implemented(&trait_impl); self.remove_trait_impl_assumed_trait_implementations(trait_impl.impl_id); - let trait_id = - trait_impl.trait_id.expect("Trait impl trait ID should have been set at this point"); - let trait_ = self.interner.get_trait(trait_id); - let trait_visibility = trait_.visibility; - for (module, function, _) in &trait_impl.methods.functions { self.local_module = *module; let errors = check_trait_impl_method_matches_declaration(self.interner, *function); self.errors.extend(errors.into_iter().map(|error| (error.into(), self.file))); - - // A trait impl method has the same visibility as its trait - let modifiers = self.interner.function_modifiers_mut(function); - modifiers.visibility = trait_visibility; } self.elaborate_functions(trait_impl.methods); From 6ee2c24168fc8077918b83f8403766c8ba0e7c22 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 19:19:40 -0300 Subject: [PATCH 10/13] Update compiler/noirc_frontend/src/elaborator/mod.rs Co-authored-by: jfecher --- compiler/noirc_frontend/src/elaborator/mod.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index e6abe5f652..0d3414332e 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1104,12 +1104,8 @@ impl<'context> Elaborator<'context> { } fn set_trait_impl_functions_visibility(&mut self, trait_impl: &UnresolvedTraitImpl) { - let trait_visibility = if let Some(trait_id) = trait_impl.trait_id { - let trait_ = self.interner.get_trait(trait_id); - Some(trait_.visibility) - } else { - None - }; + let Some(trait_id) = trait_impl.trait_id else { return; }; + let trait_visibility = self.interner.get_trait(trait_id).visibility; for (_, function, _) in &trait_impl.methods.functions { // A trait impl method has the same visibility as its trait From b353e08de37d3818b0f176c3f1ee447912fbfcbf Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 19:22:54 -0300 Subject: [PATCH 11/13] fix --- compiler/noirc_frontend/src/elaborator/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 0d3414332e..cf87180bd6 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1104,15 +1104,15 @@ impl<'context> Elaborator<'context> { } fn set_trait_impl_functions_visibility(&mut self, trait_impl: &UnresolvedTraitImpl) { - let Some(trait_id) = trait_impl.trait_id else { return; }; + let Some(trait_id) = trait_impl.trait_id else { + return; + }; let trait_visibility = self.interner.get_trait(trait_id).visibility; for (_, function, _) in &trait_impl.methods.functions { // A trait impl method has the same visibility as its trait - if let Some(trait_visibility) = trait_visibility { - let modifiers = self.interner.function_modifiers_mut(function); - modifiers.visibility = trait_visibility; - } + let modifiers = self.interner.function_modifiers_mut(function); + modifiers.visibility = trait_visibility; } } From 1b990fcde07f4589a776b33646b3f3d70b9a297f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 19:22:58 -0300 Subject: [PATCH 12/13] Use mutable methods again --- .../noirc_frontend/src/elaborator/trait_impls.rs | 2 +- compiler/noirc_frontend/src/elaborator/traits.rs | 8 ++++---- compiler/noirc_frontend/src/hir_def/traits.rs | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 7eca34d7a1..20f048bed0 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -103,7 +103,7 @@ impl<'context> Elaborator<'context> { // Restore the methods that were taken before the for loop let the_trait = self.interner.get_trait_mut(trait_id); - the_trait.methods = methods; + the_trait.set_methods(methods); // Emit MethodNotInTrait error for methods in the impl block that // don't have a corresponding method signature defined in the trait diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index f7ace68560..09bd639e31 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -54,15 +54,15 @@ impl<'context> Elaborator<'context> { } this.interner.update_trait(*trait_id, |trait_def| { - trait_def.trait_bounds = resolved_trait_bounds; - trait_def.where_clause = where_clause; - trait_def.visibility = unresolved_trait.trait_def.visibility; + trait_def.set_trait_bounds(resolved_trait_bounds); + trait_def.set_where_clause(where_clause); + trait_def.set_visibility(unresolved_trait.trait_def.visibility); }); let methods = this.resolve_trait_methods(*trait_id, unresolved_trait); this.interner.update_trait(*trait_id, |trait_def| { - trait_def.methods = methods; + trait_def.set_methods(methods); }); }); diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index 949e6e5a51..ff0cac027b 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -149,6 +149,22 @@ impl PartialEq for Trait { } impl Trait { + pub fn set_methods(&mut self, methods: Vec) { + self.methods = methods; + } + + pub fn set_trait_bounds(&mut self, trait_bounds: Vec) { + self.trait_bounds = trait_bounds; + } + + pub fn set_where_clause(&mut self, where_clause: Vec) { + self.where_clause = where_clause; + } + + pub fn set_visibility(&mut self, visibility: ItemVisibility) { + self.visibility = visibility; + } + pub fn find_method(&self, name: &str) -> Option { for (idx, method) in self.methods.iter().enumerate() { if &method.name == name { From 12cee2e3ac5d5e4564a438992e8fac96d7afd760 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 10 Jan 2025 19:25:10 -0300 Subject: [PATCH 13/13] Set visibility in existing loop --- compiler/noirc_frontend/src/elaborator/mod.rs | 24 +++++-------------- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index cf87180bd6..0f59de6004 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -346,11 +346,6 @@ impl<'context> Elaborator<'context> { &items.module_attributes, ); - // Make sure trait impl functions have the same visibility as their traits - for trait_impl in &items.trait_impls { - self.set_trait_impl_functions_visibility(trait_impl); - } - for functions in items.functions { self.elaborate_functions(functions); } @@ -1103,19 +1098,6 @@ impl<'context> Elaborator<'context> { } } - fn set_trait_impl_functions_visibility(&mut self, trait_impl: &UnresolvedTraitImpl) { - let Some(trait_id) = trait_impl.trait_id else { - return; - }; - let trait_visibility = self.interner.get_trait(trait_id).visibility; - - for (_, function, _) in &trait_impl.methods.functions { - // A trait impl method has the same visibility as its trait - let modifiers = self.interner.function_modifiers_mut(function); - modifiers.visibility = trait_visibility; - } - } - fn elaborate_trait_impl(&mut self, trait_impl: UnresolvedTraitImpl) { self.file = trait_impl.file_id; self.local_module = trait_impl.module_id; @@ -1340,9 +1322,15 @@ impl<'context> Elaborator<'context> { let span = trait_impl.object_type.span; self.declare_methods_on_struct(Some(trait_id), &mut trait_impl.methods, span); + let trait_visibility = self.interner.get_trait(trait_id).visibility; + let methods = trait_impl.methods.function_ids(); for func_id in &methods { self.interner.set_function_trait(*func_id, self_type.clone(), trait_id); + + // A trait impl method has the same visibility as its trait + let modifiers = self.interner.function_modifiers_mut(func_id); + modifiers.visibility = trait_visibility; } let trait_generics = trait_impl.resolved_trait_generics.clone();