feat: visibility for impl functions (#6179)

# Description

## Problem

Part of #4515

## Summary

~After searching for a while I found that to make this work we only
needed to change one line~

## Additional Context

I didn't include tests for now, but I'll add them later because we first
need to figure out which `impl` functions should be marked as `pub` or
`pub(crate)` in the stdlib. I'm not familiar with the stdlib though I
probably can't do it. That said, if you open the standard library with a
nargo compiled from this PR you already get some warnings, so those
functions should at least not be private.


## Documentation

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** 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.

---------

Co-authored-by: Tom French <tom@tomfren.ch>

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -1125,7 +1125,7 @@ impl<'context> Elaborator<'context> {
                 // object types in each method overlap or not. If they do, we issue an error.
                 // If not, that is specialization which is allowed.
                 let name = method.name_ident().clone();
-                if module.declare_function(name, ItemVisibility::Public, *method_id).is_err() {
+                if module.declare_function(name, method.def.visibility, *method_id).is_err() {
                     let existing = module.find_func_with_name(method.name_ident()).expect(
                         "declare_function should only error if there is an existing function",
                     );

compiler/noirc_frontend/src/elaborator/scope.rs

@@ -90,7 +90,13 @@ impl<'context> Elaborator<'context> {
     }
 
     fn resolve_path_in_module(&mut self, path: Path, module_id: ModuleId) -> PathResolutionResult {
-        let resolver = StandardPathResolver::new(module_id);
+        let self_type_module_id = if let Some(Type::Struct(struct_type, _)) = &self.self_type {
+            Some(struct_type.borrow().id.module_id())
+        } else {
+            None
+        };
+
+        let resolver = StandardPathResolver::new(module_id, self_type_module_id);
 
         if !self.interner.lsp_mode {
             return resolver.resolve(

compiler/noirc_frontend/src/hir/comptime/tests.rs

@@ -28,7 +28,8 @@ fn interpret_helper(src: &str) -> Result<Value, InterpreterError> {
         location,
         Vec::new(),
         Vec::new(),
-        false,
+        false, // is contract
+        false, // is struct
     )));
     assert_eq!(root, module_id);
 

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

@@ -554,6 +554,7 @@ fn inject_prelude(
         if let Ok(PathResolution { module_def_id, error }) = path_resolver::resolve_path(
             &context.def_maps,
             ModuleId { krate: crate_id, local_id: crate_root },
+            None,
             path,
             &mut context.def_interner.usage_tracker,
             &mut None,
@@ -571,6 +572,7 @@ fn inject_prelude(
                     ImportDirective {
                         visibility: ItemVisibility::Private,
                         module_id: crate_root,
+                        self_type_module_id: None,
                         path: Path { segments, kind: PathKind::Plain, span: Span::default() },
                         alias: None,
                         is_prelude: true,

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

@@ -81,6 +81,7 @@ pub fn collect_defs(
         collector.def_collector.imports.push(ImportDirective {
             visibility: import.visibility,
             module_id: collector.module_id,
+            self_type_module_id: None,
             path: import.path,
             alias: import.alias,
             is_prelude: false,
@@ -385,7 +386,8 @@ impl<'a> ModCollector<'a> {
                 Vec::new(),
                 Vec::new(),
                 false,
-                false,
+                false, // is contract
+                false, // is struct
             ) {
                 Ok(module_id) => TraitId(ModuleId { krate, local_id: module_id.local_id }),
                 Err(error) => {
@@ -619,6 +621,7 @@ impl<'a> ModCollector<'a> {
                 submodule.contents.inner_attributes.clone(),
                 true,
                 submodule.is_contract,
+                false, // is struct
             ) {
                 Ok(child) => {
                     self.collect_attributes(
@@ -718,7 +721,8 @@ impl<'a> ModCollector<'a> {
             mod_decl.outer_attributes.clone(),
             ast.inner_attributes.clone(),
             true,
-            false,
+            false, // is contract
+            false, // is struct
         ) {
             Ok(child_mod_id) => {
                 self.collect_attributes(
@@ -770,6 +774,7 @@ impl<'a> ModCollector<'a> {
         inner_attributes: Vec<SecondaryAttribute>,
         add_to_parent_scope: bool,
         is_contract: bool,
+        is_struct: bool,
     ) -> Result<ModuleId, DefCollectorErrorKind> {
         push_child_module(
             &mut context.def_interner,
@@ -782,6 +787,7 @@ impl<'a> ModCollector<'a> {
             inner_attributes,
             add_to_parent_scope,
             is_contract,
+            is_struct,
         )
     }
 
@@ -817,6 +823,7 @@ fn push_child_module(
     inner_attributes: Vec<SecondaryAttribute>,
     add_to_parent_scope: bool,
     is_contract: bool,
+    is_struct: bool,
 ) -> Result<ModuleId, DefCollectorErrorKind> {
     // Note: the difference between `location` and `mod_location` is:
     // - `mod_location` will point to either the token "foo" in `mod foo { ... }`
@@ -826,8 +833,14 @@ fn push_child_module(
     // Eventually the location put in `ModuleData` is used for codelenses about `contract`s,
     // so we keep using `location` so that it continues to work as usual.
     let location = Location::new(mod_name.span(), mod_location.file);
-    let new_module =
-        ModuleData::new(Some(parent), location, outer_attributes, inner_attributes, is_contract);
+    let new_module = ModuleData::new(
+        Some(parent),
+        location,
+        outer_attributes,
+        inner_attributes,
+        is_contract,
+        is_struct,
+    );
 
     let module_id = def_map.modules.insert(new_module);
     let modules = &mut def_map.modules;
@@ -962,8 +975,9 @@ pub fn collect_struct(
         location,
         Vec::new(),
         Vec::new(),
-        false,
-        false,
+        false, // add to parent scope
+        false, // is contract
+        true,  // is struct
     ) {
         Ok(module_id) => {
             interner.new_struct(&unresolved, resolved_generics, krate, module_id.local_id, file_id)

compiler/noirc_frontend/src/hir/def_map/mod.rs

@@ -102,7 +102,8 @@ impl CrateDefMap {
             location,
             Vec::new(),
             ast.inner_attributes.clone(),
-            false,
+            false, // is contract
+            false, // is struct
         ));
 
         let def_map = CrateDefMap {

compiler/noirc_frontend/src/hir/def_map/module_data.rs

@@ -26,6 +26,9 @@ pub struct ModuleData {
     /// True if this module is a `contract Foo { ... }` module containing contract functions
     pub is_contract: bool,
 
+    /// True if this module is actually a struct
+    pub is_struct: bool,
+
     pub attributes: Vec<SecondaryAttribute>,
 }
 
@@ -36,6 +39,7 @@ impl ModuleData {
         outer_attributes: Vec<SecondaryAttribute>,
         inner_attributes: Vec<SecondaryAttribute>,
         is_contract: bool,
+        is_struct: bool,
     ) -> ModuleData {
         let mut attributes = outer_attributes;
         attributes.extend(inner_attributes);
@@ -47,6 +51,7 @@ impl ModuleData {
             definitions: ItemScope::default(),
             location,
             is_contract,
+            is_struct,
             attributes,
         }
     }

compiler/noirc_frontend/src/hir/resolution/import.rs

@@ -5,6 +5,7 @@ use crate::graph::CrateId;
 use crate::hir::def_collector::dc_crate::CompilationError;
 use crate::node_interner::ReferenceId;
 use crate::usage_tracker::UsageTracker;
+
 use std::collections::BTreeMap;
 
 use crate::ast::{Ident, ItemVisibility, Path, PathKind, PathSegment};
@@ -16,6 +17,7 @@ use super::errors::ResolverError;
 pub struct ImportDirective {
     pub visibility: ItemVisibility,
     pub module_id: LocalModuleId,
+    pub self_type_module_id: Option<ModuleId>,
     pub path: Path,
     pub alias: Option<Ident>,
     pub is_prelude: bool,
@@ -92,18 +94,15 @@ pub fn resolve_import(
     path_references: &mut Option<&mut Vec<ReferenceId>>,
 ) -> Result<ResolvedImport, PathResolutionError> {
     let module_scope = import_directive.module_id;
-    let NamespaceResolution {
-        module_id: resolved_module,
-        namespace: resolved_namespace,
-        mut error,
-    } = resolve_path_to_ns(
-        import_directive,
-        crate_id,
-        crate_id,
-        def_maps,
-        usage_tracker,
-        path_references,
-    )?;
+    let NamespaceResolution { module_id: resolved_module, namespace: resolved_namespace, error } =
+        resolve_path_to_ns(
+            import_directive,
+            crate_id,
+            crate_id,
+            def_maps,
+            usage_tracker,
+            path_references,
+        )?;
 
     let name = resolve_path_name(import_directive);
 
@@ -113,14 +112,16 @@ pub fn resolve_import(
         .map(|(_, visibility, _)| visibility)
         .expect("Found empty namespace");
 
-    error = error.or_else(|| {
-        if can_reference_module_id(
-            def_maps,
-            crate_id,
-            import_directive.module_id,
-            resolved_module,
-            visibility,
-        ) {
+    let error = error.or_else(|| {
+        if import_directive.self_type_module_id == Some(resolved_module)
+            || can_reference_module_id(
+                def_maps,
+                crate_id,
+                import_directive.module_id,
+                resolved_module,
+                visibility,
+            )
+        {
             None
         } else {
             Some(PathResolutionError::Private(name.clone()))
@@ -408,6 +409,7 @@ fn resolve_external_dep(
     let dep_directive = ImportDirective {
         visibility: ItemVisibility::Private,
         module_id: dep_module.local_id,
+        self_type_module_id: directive.self_type_module_id,
         path,
         alias: directive.alias.clone(),
         is_prelude: false,
@@ -442,11 +444,15 @@ pub fn can_reference_module_id(
         ItemVisibility::PublicCrate => same_crate,
         ItemVisibility::Private => {
             same_crate
-                && module_descendent_of_target(
+                && (module_descendent_of_target(
                     target_crate_def_map,
                     target_module.local_id,
                     current_module,
-                )
+                ) || module_is_parent_of_struct_module(
+                    target_crate_def_map,
+                    current_module,
+                    target_module.local_id,
+                ))
         }
     }
 }
@@ -466,3 +472,13 @@ fn module_descendent_of_target(
         .parent
         .map_or(false, |parent| module_descendent_of_target(def_map, target, parent))
 }
+
+/// Returns true if `target` is a struct and its parent is `current`.
+fn module_is_parent_of_struct_module(
+    def_map: &CrateDefMap,
+    current: LocalModuleId,
+    target: LocalModuleId,
+) -> bool {
+    let module_data = &def_map.modules[target.0];
+    module_data.is_struct && module_data.parent == Some(current)
+}

compiler/noirc_frontend/src/hir/resolution/path_resolver.rs

@@ -2,6 +2,7 @@ use super::import::{resolve_import, ImportDirective, PathResolution, PathResolut
 use crate::ast::{ItemVisibility, Path};
 use crate::node_interner::ReferenceId;
 use crate::usage_tracker::UsageTracker;
+
 use std::collections::BTreeMap;
 
 use crate::graph::CrateId;
@@ -28,11 +29,13 @@ pub trait PathResolver {
 pub struct StandardPathResolver {
     // Module that we are resolving the path in
     module_id: ModuleId,
+    // The module of the self type, if any (for example, the ModuleId of a struct)
+    self_type_module_id: Option<ModuleId>,
 }
 
 impl StandardPathResolver {
-    pub fn new(module_id: ModuleId) -> StandardPathResolver {
-        Self { module_id }
+    pub fn new(module_id: ModuleId, self_type_module_id: Option<ModuleId>) -> StandardPathResolver {
+        Self { module_id, self_type_module_id }
     }
 }
 
@@ -44,7 +47,14 @@ impl PathResolver for StandardPathResolver {
         usage_tracker: &mut UsageTracker,
         path_references: &mut Option<&mut Vec<ReferenceId>>,
     ) -> PathResolutionResult {
-        resolve_path(def_maps, self.module_id, path, usage_tracker, path_references)
+        resolve_path(
+            def_maps,
+            self.module_id,
+            self.self_type_module_id,
+            path,
+            usage_tracker,
+            path_references,
+        )
     }
 
     fn local_module_id(&self) -> LocalModuleId {
@@ -61,6 +71,7 @@ impl PathResolver for StandardPathResolver {
 pub fn resolve_path(
     def_maps: &BTreeMap<CrateId, CrateDefMap>,
     module_id: ModuleId,
+    self_type_module_id: Option<ModuleId>,
     path: Path,
     usage_tracker: &mut UsageTracker,
     path_references: &mut Option<&mut Vec<ReferenceId>>,
@@ -69,6 +80,7 @@ pub fn resolve_path(
     let import = ImportDirective {
         visibility: ItemVisibility::Private,
         module_id: module_id.local_id,
+        self_type_module_id,
         path,
         alias: None,
         is_prelude: false,

compiler/noirc_frontend/src/tests.rs

@@ -90,7 +90,8 @@ pub(crate) fn get_program(src: &str) -> (ParsedModule, Context, Vec<(Compilation
             location,
             Vec::new(),
             inner_attributes.clone(),
-            false,
+            false, // is contract
+            false, // is struct
         ));
 
         let def_map = CrateDefMap {

compiler/noirc_frontend/src/tests/turbofish.rs

@@ -29,11 +29,11 @@ fn turbofish_numeric_generic_nested_call() {
     }
 
     impl<T> Foo<T> {
-        fn static_method<let N: u32>() -> [u8; N] {
+        pub fn static_method<let N: u32>() -> [u8; N] {
             [0; N]
         }
 
-        fn impl_method<let N: u32>(self) -> [T; N] {
+        pub fn impl_method<let N: u32>(self) -> [T; N] {
             [self.a; N]
         }
     }
@@ -108,7 +108,7 @@ fn turbofish_in_middle_of_variable_unsupported_yet() {
     }
 
     impl <T> Foo<T> {
-        fn new(x: T) -> Self {
+        pub fn new(x: T) -> Self {
             Foo { x }
         }
     }