From 9ca1a60f9310f9822414c62dfa410c85bfe1737b Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 13 Mar 2024 15:30:50 +0000 Subject: [PATCH] chore: allow setting namespace visibility on functions (#4510) # Description ## Problem\* Resolves ## Summary\* This PR allows setting non-public visibilities when adding functions to namespaces. This doesn't currently cause any changes as we never actually read this visibility but we start to do so in a later PR. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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. --- .../compute_note_hash_and_nullifier.rs | 4 +-- .../src/hir/def_collector/dc_mod.rs | 11 +++---- .../src/hir/def_map/item_scope.rs | 13 ++++++-- .../src/hir/def_map/module_data.rs | 30 +++++++++++-------- .../src/hir/resolution/impls.rs | 11 +++++-- .../src/hir/resolution/traits.rs | 11 +++++-- 6 files changed, 54 insertions(+), 26 deletions(-) diff --git a/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs b/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs index 4f8f3f19ab8..fd538dc578b 100644 --- a/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs +++ b/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs @@ -7,7 +7,7 @@ use noirc_frontend::{ }, macros_api::{FileId, HirContext, MacroError}, node_interner::FuncId, - parse_program, FunctionReturnType, NoirFunction, UnresolvedTypeData, + parse_program, FunctionReturnType, ItemVisibility, NoirFunction, UnresolvedTypeData, }; use crate::utils::hir_utils::fetch_struct_trait_impls; @@ -113,7 +113,7 @@ pub fn inject_compute_note_hash_and_nullifier( context.def_map_mut(crate_id).unwrap() .modules_mut()[module_id.0] .declare_function( - func.name_ident().clone(), func_id + func.name_ident().clone(), ItemVisibility::Public, func_id ).expect( "Failed to declare the autogenerated compute_note_hash_and_nullifier function, likely due to a duplicate definition. See https://github.com/AztecProtocol/aztec-packages/issues/4647." ); 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 ae99e61e534..5adb9eb5b7e 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -10,8 +10,8 @@ use crate::{ macros_api::MacroProcessor, node_interner::{FunctionModifiers, TraitId, TypeAliasId}, parser::{SortedModule, SortedSubModule}, - FunctionDefinition, Ident, LetStatement, ModuleDeclaration, NoirFunction, NoirStruct, - NoirTrait, NoirTraitImpl, NoirTypeAlias, TraitImplItem, TraitItem, TypeImpl, + FunctionDefinition, Ident, ItemVisibility, LetStatement, ModuleDeclaration, NoirFunction, + NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, TraitImplItem, TraitItem, TypeImpl, }; use super::{ @@ -232,6 +232,7 @@ impl<'a> ModCollector<'a> { let name = function.name_ident().clone(); let func_id = context.def_interner.push_empty_fn(); + let visibility = function.def.visibility; // First create dummy function in the DefInterner // So that we can get a FuncId @@ -248,7 +249,7 @@ impl<'a> ModCollector<'a> { // Add function to scope/ns of the module let result = self.def_collector.def_map.modules[self.module_id.0] - .declare_function(name, func_id); + .declare_function(name, visibility, func_id); if let Err((first_def, second_def)) = result { let error = DefCollectorErrorKind::Duplicate { @@ -407,7 +408,7 @@ impl<'a> ModCollector<'a> { let modifiers = FunctionModifiers { name: name.to_string(), - visibility: crate::ItemVisibility::Public, + visibility: ItemVisibility::Public, // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629 attributes: crate::token::Attributes::empty(), is_unconstrained: false, @@ -419,7 +420,7 @@ impl<'a> ModCollector<'a> { .push_function_definition(func_id, modifiers, trait_id.0, location); match self.def_collector.def_map.modules[trait_id.0.local_id.0] - .declare_function(name.clone(), func_id) + .declare_function(name.clone(), ItemVisibility::Public, func_id) { Ok(()) => { if let Some(body) = body { diff --git a/compiler/noirc_frontend/src/hir/def_map/item_scope.rs b/compiler/noirc_frontend/src/hir/def_map/item_scope.rs index 178b91e1e84..cd4eafdf669 100644 --- a/compiler/noirc_frontend/src/hir/def_map/item_scope.rs +++ b/compiler/noirc_frontend/src/hir/def_map/item_scope.rs @@ -19,10 +19,11 @@ impl ItemScope { pub fn add_definition( &mut self, name: Ident, + visibility: ItemVisibility, mod_def: ModuleDefId, trait_id: Option, ) -> Result<(), (Ident, Ident)> { - self.add_item_to_namespace(name, mod_def, trait_id, false)?; + self.add_item_to_namespace(name, visibility, mod_def, trait_id, false)?; self.defs.push(mod_def); Ok(()) } @@ -33,6 +34,7 @@ impl ItemScope { pub fn add_item_to_namespace( &mut self, name: Ident, + visibility: ItemVisibility, mod_def: ModuleDefId, trait_id: Option, is_prelude: bool, @@ -41,6 +43,11 @@ impl ItemScope { if let Entry::Occupied(mut o) = map.entry(name.clone()) { let trait_hashmap = o.get_mut(); if let Entry::Occupied(mut n) = trait_hashmap.entry(trait_id) { + // Generally we want to reject having two of the same ident in the same namespace. + // The exception to this is when we're explicitly importing something + // which exists in the Noir stdlib prelude. + // + // In this case we ignore the prelude and favour the explicit import. let is_prelude = std::mem::replace(&mut n.get_mut().2, is_prelude); let old_ident = o.key(); @@ -50,12 +57,12 @@ impl ItemScope { Err((old_ident.clone(), name)) } } else { - trait_hashmap.insert(trait_id, (mod_def, ItemVisibility::Public, is_prelude)); + trait_hashmap.insert(trait_id, (mod_def, visibility, is_prelude)); Ok(()) } } else { let mut trait_hashmap = HashMap::new(); - trait_hashmap.insert(trait_id, (mod_def, ItemVisibility::Public, is_prelude)); + trait_hashmap.insert(trait_id, (mod_def, visibility, is_prelude)); map.insert(name, trait_hashmap); Ok(()) } diff --git a/compiler/noirc_frontend/src/hir/def_map/module_data.rs b/compiler/noirc_frontend/src/hir/def_map/module_data.rs index 309618dd011..4dd38f0e3e5 100644 --- a/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/compiler/noirc_frontend/src/hir/def_map/module_data.rs @@ -4,7 +4,7 @@ use noirc_errors::Location; use crate::{ node_interner::{FuncId, GlobalId, StructId, TraitId, TypeAliasId}, - Ident, + Ident, ItemVisibility, }; use super::{ItemScope, LocalModuleId, ModuleDefId, ModuleId, PerNs}; @@ -48,18 +48,24 @@ impl ModuleData { fn declare( &mut self, name: Ident, + visibility: ItemVisibility, item_id: ModuleDefId, trait_id: Option, ) -> Result<(), (Ident, Ident)> { - self.scope.add_definition(name.clone(), item_id, trait_id)?; + self.scope.add_definition(name.clone(), visibility, item_id, trait_id)?; // definitions is a subset of self.scope so it is expected if self.scope.define_func_def // returns without error, so will self.definitions.define_func_def. - self.definitions.add_definition(name, item_id, trait_id) + self.definitions.add_definition(name, visibility, item_id, trait_id) } - pub fn declare_function(&mut self, name: Ident, id: FuncId) -> Result<(), (Ident, Ident)> { - self.declare(name, id.into(), None) + pub fn declare_function( + &mut self, + name: Ident, + visibility: ItemVisibility, + id: FuncId, + ) -> Result<(), (Ident, Ident)> { + self.declare(name, visibility, id.into(), None) } pub fn declare_trait_function( @@ -68,7 +74,7 @@ impl ModuleData { id: FuncId, trait_id: TraitId, ) -> Result<(), (Ident, Ident)> { - self.declare(name, id.into(), Some(trait_id)) + self.declare(name, ItemVisibility::Public, id.into(), Some(trait_id)) } pub fn remove_function(&mut self, name: &Ident) { @@ -77,11 +83,11 @@ impl ModuleData { } pub fn declare_global(&mut self, name: Ident, id: GlobalId) -> Result<(), (Ident, Ident)> { - self.declare(name, id.into(), None) + self.declare(name, ItemVisibility::Public, id.into(), None) } pub fn declare_struct(&mut self, name: Ident, id: StructId) -> Result<(), (Ident, Ident)> { - self.declare(name, ModuleDefId::TypeId(id), None) + self.declare(name, ItemVisibility::Public, ModuleDefId::TypeId(id), None) } pub fn declare_type_alias( @@ -89,11 +95,11 @@ impl ModuleData { name: Ident, id: TypeAliasId, ) -> Result<(), (Ident, Ident)> { - self.declare(name, id.into(), None) + self.declare(name, ItemVisibility::Public, id.into(), None) } pub fn declare_trait(&mut self, name: Ident, id: TraitId) -> Result<(), (Ident, Ident)> { - self.declare(name, ModuleDefId::TraitId(id), None) + self.declare(name, ItemVisibility::Public, ModuleDefId::TraitId(id), None) } pub fn declare_child_module( @@ -101,7 +107,7 @@ impl ModuleData { name: Ident, child_id: ModuleId, ) -> Result<(), (Ident, Ident)> { - self.declare(name, child_id.into(), None) + self.declare(name, ItemVisibility::Public, child_id.into(), None) } pub fn find_func_with_name(&self, name: &Ident) -> Option { @@ -114,7 +120,7 @@ impl ModuleData { id: ModuleDefId, is_prelude: bool, ) -> Result<(), (Ident, Ident)> { - self.scope.add_item_to_namespace(name, id, None, is_prelude) + self.scope.add_item_to_namespace(name, ItemVisibility::Public, id, None, is_prelude) } pub fn find_name(&self, name: &Ident) -> PerNs { diff --git a/compiler/noirc_frontend/src/hir/resolution/impls.rs b/compiler/noirc_frontend/src/hir/resolution/impls.rs index 4aa70f00cfc..72f6adc3770 100644 --- a/compiler/noirc_frontend/src/hir/resolution/impls.rs +++ b/compiler/noirc_frontend/src/hir/resolution/impls.rs @@ -13,7 +13,7 @@ use crate::{ Context, }, node_interner::{FuncId, NodeInterner}, - Type, + ItemVisibility, Type, }; use super::{ @@ -67,7 +67,14 @@ pub(crate) fn collect_impls( // be accessed with the `TypeName::method` syntax. We'll check later whether the // object types in each method overlap or not. If they do, we issue an error. // If not, that is specialization which is allowed. - if module.declare_function(method.name_ident().clone(), *method_id).is_err() { + if module + .declare_function( + method.name_ident().clone(), + ItemVisibility::Public, + *method_id, + ) + .is_err() + { module.remove_function(method.name_ident()); } } diff --git a/compiler/noirc_frontend/src/hir/resolution/traits.rs b/compiler/noirc_frontend/src/hir/resolution/traits.rs index 5d546954f0d..04da558a642 100644 --- a/compiler/noirc_frontend/src/hir/resolution/traits.rs +++ b/compiler/noirc_frontend/src/hir/resolution/traits.rs @@ -16,7 +16,7 @@ use crate::{ }, hir_def::traits::{TraitConstant, TraitFunction, TraitImpl, TraitType}, node_interner::{FuncId, NodeInterner, TraitId}, - Generics, Path, Shared, TraitItem, Type, TypeVariable, TypeVariableKind, + Generics, ItemVisibility, Path, Shared, TraitItem, Type, TypeVariable, TypeVariableKind, }; use super::{ @@ -301,7 +301,14 @@ fn collect_trait_impl( // be accessed with the `TypeName::method` syntax. We'll check later whether the // object types in each method overlap or not. If they do, we issue an error. // If not, that is specialization which is allowed. - if module.declare_function(method.name_ident().clone(), *method_id).is_err() { + if module + .declare_function( + method.name_ident().clone(), + ItemVisibility::Public, + *method_id, + ) + .is_err() + { module.remove_function(method.name_ident()); } }