fix: visibility for impl methods (#6261)

# Description

## Problem

Part of #4515

Apparently in #6179 I only made it work for calls like `foo::Bar::baz()`
but no checks were done for method calls (where `self` is involved).

## Summary

This PR now warns when calling private or `pub(crate)` methods, either
on structs or primitive types, when they are not accessible from the
current module.

We also now don't suggest non-accessible methods from LSP.

## Additional Context


I also removed an unused global that was introduced some time ago.

## 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.

compiler/noirc_frontend/src/elaborator/expressions.rs

@@ -13,7 +13,9 @@ use crate::{
     },
     hir::{
         comptime::{self, InterpreterError},
-        resolution::errors::ResolverError,
+        resolution::{
+            errors::ResolverError, import::PathResolutionError, visibility::method_call_is_visible,
+        },
         type_check::{generics::TraitGenerics, TypeCheckError},
     },
     hir_def::{
@@ -449,6 +451,8 @@ impl<'context> Elaborator<'context> {
                 let method_call =
                     HirMethodCallExpression { method, object, arguments, location, generics };
 
+                self.check_method_call_visibility(func_id, &object_type, &method_call.method);
+
                 // Desugar the method call into a normal, resolved function call
                 // so that the backend doesn't need to worry about methods
                 // TODO: update object_type here?
@@ -487,6 +491,20 @@ impl<'context> Elaborator<'context> {
         }
     }
 
+    fn check_method_call_visibility(&mut self, func_id: FuncId, object_type: &Type, name: &Ident) {
+        if !method_call_is_visible(
+            object_type,
+            func_id,
+            self.module_id(),
+            self.interner,
+            self.def_maps,
+        ) {
+            self.push_err(ResolverError::PathResolutionError(PathResolutionError::Private(
+                name.clone(),
+            )));
+        }
+    }
+
     fn elaborate_constructor(
         &mut self,
         constructor: ConstructorExpression,

compiler/noirc_frontend/src/elaborator/statements.rs

@@ -8,7 +8,8 @@ use crate::{
     },
     hir::{
         resolution::{
-            errors::ResolverError, import::PathResolutionError, visibility::struct_field_is_visible,
+            errors::ResolverError, import::PathResolutionError,
+            visibility::struct_member_is_visible,
         },
         type_check::{Source, TypeCheckError},
     },
@@ -508,7 +509,7 @@ impl<'context> Elaborator<'context> {
         visibility: ItemVisibility,
         span: Span,
     ) {
-        if !struct_field_is_visible(struct_type, visibility, self.module_id(), self.def_maps) {
+        if !struct_member_is_visible(struct_type.id, visibility, self.module_id(), self.def_maps) {
             self.push_err(ResolverError::PathResolutionError(PathResolutionError::Private(
                 Ident::new(field_name.to_string(), span),
             )));

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

@@ -1,10 +1,11 @@
 use crate::graph::CrateId;
-use crate::StructType;
+use crate::node_interner::{FuncId, NodeInterner, StructId};
+use crate::Type;
 
 use std::collections::BTreeMap;
 
 use crate::ast::ItemVisibility;
-use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId};
+use crate::hir::def_map::{CrateDefMap, DefMaps, LocalModuleId, ModuleId};
 
 // Returns false if the given private function is being called from a non-child module, or
 // if the given pub(crate) function is being called from another crate. Otherwise returns true.
@@ -64,19 +65,19 @@ fn module_is_parent_of_struct_module(
     module_data.is_struct && module_data.parent == Some(current)
 }
 
-pub fn struct_field_is_visible(
-    struct_type: &StructType,
+pub fn struct_member_is_visible(
+    struct_id: StructId,
     visibility: ItemVisibility,
     current_module_id: ModuleId,
     def_maps: &BTreeMap<CrateId, CrateDefMap>,
 ) -> bool {
     match visibility {
         ItemVisibility::Public => true,
         ItemVisibility::PublicCrate => {
-            struct_type.id.parent_module_id(def_maps).krate == current_module_id.krate
+            struct_id.parent_module_id(def_maps).krate == current_module_id.krate
         }
         ItemVisibility::Private => {
-            let struct_parent_module_id = struct_type.id.parent_module_id(def_maps);
+            let struct_parent_module_id = struct_id.parent_module_id(def_maps);
             if struct_parent_module_id.krate != current_module_id.krate {
                 return false;
             }
@@ -94,3 +95,47 @@ pub fn struct_field_is_visible(
         }
     }
 }
+
+pub fn method_call_is_visible(
+    object_type: &Type,
+    func_id: FuncId,
+    current_module: ModuleId,
+    interner: &NodeInterner,
+    def_maps: &DefMaps,
+) -> bool {
+    let modifiers = interner.function_modifiers(&func_id);
+    match modifiers.visibility {
+        ItemVisibility::Public => true,
+        ItemVisibility::PublicCrate => {
+            if object_type.is_primitive() {
+                current_module.krate.is_stdlib()
+            } else {
+                interner.function_module(func_id).krate == current_module.krate
+            }
+        }
+        ItemVisibility::Private => {
+            if object_type.is_primitive() {
+                let func_module = interner.function_module(func_id);
+                can_reference_module_id(
+                    def_maps,
+                    current_module.krate,
+                    current_module.local_id,
+                    func_module,
+                    modifiers.visibility,
+                )
+            } else {
+                let func_meta = interner.function_meta(&func_id);
+                if let Some(struct_id) = func_meta.struct_id {
+                    struct_member_is_visible(
+                        struct_id,
+                        modifiers.visibility,
+                        current_module,
+                        def_maps,
+                    )
+                } else {
+                    true
+                }
+            }
+        }
+    }
+}

compiler/noirc_frontend/src/hir_def/types.rs

@@ -1041,6 +1041,34 @@ impl Type {
         }
     }
 
+    pub fn is_primitive(&self) -> bool {
+        match self.follow_bindings() {
+            Type::FieldElement
+            | Type::Array(_, _)
+            | Type::Slice(_)
+            | Type::Integer(..)
+            | Type::Bool
+            | Type::String(_)
+            | Type::FmtString(_, _)
+            | Type::Unit
+            | Type::Function(..)
+            | Type::Tuple(..) => true,
+            Type::Alias(alias_type, generics) => {
+                alias_type.borrow().get_type(&generics).is_primitive()
+            }
+            Type::MutableReference(typ) => typ.is_primitive(),
+            Type::Struct(..)
+            | Type::TypeVariable(..)
+            | Type::TraitAsType(..)
+            | Type::NamedGeneric(..)
+            | Type::Forall(..)
+            | Type::Constant(..)
+            | Type::Quoted(..)
+            | Type::InfixExpr(..)
+            | Type::Error => false,
+        }
+    }
+
     pub fn find_numeric_type_vars(&self, found_names: &mut Vec<String>) {
         // Return whether the named generic has a Kind::Numeric and save its name
         let named_generic_is_numeric = |typ: &Type, found_names: &mut Vec<String>| {

compiler/noirc_frontend/src/tests/visibility.rs

@@ -106,6 +106,61 @@ fn errors_if_trying_to_access_public_function_inside_private_module() {
     assert_eq!(ident.to_string(), "bar");
 }
 
+#[test]
+fn warns_if_calling_private_struct_method() {
+    let src = r#"
+    mod moo {
+        pub struct Foo {}
+
+        impl Foo {
+            fn bar(self) {
+                let _ = self;
+            }
+        }
+    }
+
+    pub fn method(foo: moo::Foo) {
+        foo.bar()
+    }
+
+    fn main() {}
+    "#;
+
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+
+    let CompilationError::ResolverError(ResolverError::PathResolutionError(
+        PathResolutionError::Private(ident),
+    )) = &errors[0].0
+    else {
+        panic!("Expected a private error");
+    };
+
+    assert_eq!(ident.to_string(), "bar");
+}
+
+#[test]
+fn does_not_warn_if_calling_pub_crate_struct_method_from_same_crate() {
+    let src = r#"
+    mod moo {
+        pub struct Foo {}
+
+        impl Foo {
+            pub(crate) fn bar(self) {
+                let _ = self;
+            }
+        }
+    }
+
+    pub fn method(foo: moo::Foo) {
+        foo.bar()
+    }
+
+    fn main() {}
+    "#;
+    assert_no_errors(src);
+}
+
 #[test]
 fn does_not_error_if_calling_private_struct_function_from_same_struct() {
     let src = r#"