chore: allow setting namespace visibility on functions (#4510)

TomAFrench · web-flow · commit 9ca1a60f9310 · 2024-03-13T15:30:50.000Z
# 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.
diff --git 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
@@ -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
@@ -19,10 +19,11 @@ impl ItemScope {
     pub fn add_definition(
         &mut self,
         name: Ident,
+        visibility: ItemVisibility,
         mod_def: ModuleDefId,
         trait_id: Option<TraitId>,
     ) -> 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<TraitId>,
         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
@@ -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<TraitId>,
     ) -> 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,31 +83,31 @@ 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(
         &mut self,
         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(
         &mut self,
         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<FuncId> {
@@ -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
@@ -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
@@ -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());
                 }
             }
