chore: greenlight Elaborator visibility

compiler/noirc_frontend/src/ast/traits.rs

@@ -199,7 +199,7 @@ impl Display for TraitItem {
                 let visibility = if *visibility == ItemVisibility::Private {
                     "".to_string()
                 } else {
-                    visibility.to_string()
+                    format!("{visibility} ")
                 };
                 let is_comptime = if *is_comptime { "comptime " } else { "" };
 

compiler/noirc_frontend/src/elaborator/expressions.rs

@@ -15,9 +15,7 @@ use crate::{
     hir::{
         comptime::{self, InterpreterError},
         def_collector::dc_crate::CompilationError,
-        resolution::{
-            errors::ResolverError, import::PathResolutionError, visibility::method_call_is_visible,
-        },
+        resolution::errors::ResolverError,
         type_check::{Source, TypeCheckError, generics::TraitGenerics},
     },
     hir_def::{
@@ -825,21 +823,6 @@ impl Elaborator<'_> {
         (id, typ)
     }
 
-    fn check_method_call_visibility(&mut self, func_id: FuncId, object_type: &Type, name: &Ident) {
-        if !method_call_is_visible(
-            self.self_type.as_ref(),
-            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/mod.rs

@@ -440,8 +440,10 @@ impl<'context> Elaborator<'context> {
 
         self.run_function_lints(&func_meta, &modifiers);
 
-        // Check arg and return-value visibility of standalone functions.
-        if self.should_check_function_visibility(&func_meta, &modifiers) {
+        // Check arg and return-value are not more private than the function they are in.
+        if self.should_check_function_args_and_return_are_not_more_private_than_function(
+            &func_meta, &modifiers,
+        ) {
             let name = Ident::new(
                 self.interner.definition_name(func_meta.name.id).to_string(),
                 func_meta.name.location,

compiler/noirc_frontend/src/elaborator/statements.rs

@@ -1,18 +1,15 @@
 use noirc_errors::Location;
 
 use crate::{
-    DataType, Type,
+    Type,
     ast::{
-        AssignStatement, Expression, ForLoopStatement, ForRange, Ident, IntegerBitSize,
-        ItemVisibility, LValue, LetStatement, Statement, StatementKind, WhileStatement,
+        AssignStatement, Expression, ForLoopStatement, ForRange, IntegerBitSize, LValue,
+        LetStatement, Statement, StatementKind, WhileStatement,
     },
     elaborator::PathResolutionTarget,
     hir::{
         def_collector::dc_crate::CompilationError,
-        resolution::{
-            errors::ResolverError, import::PathResolutionError,
-            visibility::struct_member_is_visible,
-        },
+        resolution::errors::ResolverError,
         type_check::{Source, TypeCheckError},
     },
     hir_def::{
@@ -682,24 +679,6 @@ impl Elaborator<'_> {
         None
     }
 
-    pub(super) fn check_struct_field_visibility(
-        &mut self,
-        struct_type: &DataType,
-        field_name: &str,
-        visibility: ItemVisibility,
-        location: Location,
-    ) {
-        if self.silence_field_visibility_errors > 0 {
-            return;
-        }
-
-        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(), location),
-            )));
-        }
-    }
-
     fn elaborate_comptime_statement(&mut self, statement: Statement) -> (HirStatement, Type) {
         let location = statement.location;
         let (hir_statement, _typ) =

compiler/noirc_frontend/src/elaborator/visibility.rs

@@ -3,24 +3,74 @@ use noirc_errors::Location;
 use crate::{
     DataType, Type,
     ast::{Ident, ItemVisibility},
-    hir::resolution::errors::ResolverError,
+    hir::resolution::{
+        errors::ResolverError,
+        import::PathResolutionError,
+        visibility::{method_call_is_visible, struct_member_is_visible},
+    },
     hir_def::function::FuncMeta,
-    node_interner::FunctionModifiers,
+    node_interner::{FuncId, FunctionModifiers},
 };
 
 use super::Elaborator;
 
 impl Elaborator<'_> {
-    /// Find the struct in the parent module so we can know its visibility
-    pub(super) fn find_struct_visibility(&self, struct_type: &DataType) -> Option<ItemVisibility> {
+    /// Checks whether calling the method `func_id` on an object of type `object_type` is allowed
+    /// from the current location. If not, a visibility error is pushed to the error list.
+    /// The passed `name` is used for error reporting.
+    pub(super) fn check_method_call_visibility(
+        &mut self,
+        func_id: FuncId,
+        object_type: &Type,
+        name: &Ident,
+    ) {
+        if !method_call_is_visible(
+            self.self_type.as_ref(),
+            object_type,
+            func_id,
+            self.module_id(),
+            self.interner,
+            self.def_maps,
+        ) {
+            self.push_err(ResolverError::PathResolutionError(PathResolutionError::Private(
+                name.clone(),
+            )));
+        }
+    }
+
+    /// Checks whether accessing the struct field `field_name` of type `struct_type`, that has
+    /// the given `visibility`, is allowed from the current location. If not, a visibility
+    /// error is pushed to the error list.
+    pub(super) fn check_struct_field_visibility(
+        &mut self,
+        struct_type: &DataType,
+        field_name: &str,
+        visibility: ItemVisibility,
+        location: Location,
+    ) {
+        if self.silence_field_visibility_errors > 0 {
+            return;
+        }
+
+        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(), location),
+            )));
+        }
+    }
+
+    /// Returns a struct's visibility.
+    pub(super) fn find_struct_visibility(&self, struct_type: &DataType) -> ItemVisibility {
         let parent_module_id = struct_type.id.parent_module_id(self.def_maps);
         let parent_module_data = self.get_module(parent_module_id);
         let per_ns = parent_module_data.find_name(&struct_type.name);
-        per_ns.types.map(|(_, vis, _)| vis)
+        let (_, visibility, _) =
+            per_ns.types.expect("Expected to find struct in its parent module");
+        visibility
     }
 
-    /// Check whether a functions return value and args should be checked for private type visibility.
-    pub(super) fn should_check_function_visibility(
+    /// Check whether a function's args and return value should be checked for private type visibility.
+    pub(super) fn should_check_function_args_and_return_are_not_more_private_than_function(
         &self,
         func_meta: &FuncMeta,
         modifiers: &FunctionModifiers,
@@ -33,13 +83,12 @@ impl Elaborator<'_> {
         if func_meta.trait_impl.is_some() {
             return false;
         }
-        // Public struct functions should not expose private types.
-        if let Some(struct_visibility) = func_meta.type_id.and_then(|id| {
-            let struct_def = self.get_type(id);
+        // Non-private struct functions should not expose private types.
+        if let Some(struct_id) = func_meta.type_id {
+            let struct_def = self.get_type(struct_id);
             let struct_def = struct_def.borrow();
-            self.find_struct_visibility(&struct_def)
-        }) {
-            return struct_visibility != ItemVisibility::Private;
+            let visibility = self.find_struct_visibility(&struct_def);
+            return visibility != ItemVisibility::Private;
         }
         // Standalone functions should be checked
         true
@@ -62,14 +111,13 @@ impl Elaborator<'_> {
                 // then it's either accessible (all good) or it's not, in which case a different
                 // error will happen somewhere else, but no need to error again here.
                 if struct_module_id.krate == self.crate_id {
-                    if let Some(aliased_visibility) = self.find_struct_visibility(&struct_type) {
-                        if aliased_visibility < visibility {
-                            self.push_err(ResolverError::TypeIsMorePrivateThenItem {
-                                typ: struct_type.name.to_string(),
-                                item: name.to_string(),
-                                location,
-                            });
-                        }
+                    let aliased_visibility = self.find_struct_visibility(&struct_type);
+                    if aliased_visibility < visibility {
+                        self.push_err(ResolverError::TypeIsMorePrivateThenItem {
+                            typ: struct_type.name.to_string(),
+                            item: name.to_string(),
+                            location,
+                        });
                     }
                 }
 

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

@@ -71,6 +71,7 @@ fn module_is_parent_of_struct_module(
     module_data.is_type && module_data.parent == Some(current)
 }
 
+/// Returns whether a struct member with the given visibility is visible from `current_module_id`.
 pub fn struct_member_is_visible(
     struct_id: TypeId,
     visibility: ItemVisibility,
@@ -80,6 +81,7 @@ pub fn struct_member_is_visible(
     type_member_is_visible(struct_id.module_id(), visibility, current_module_id, def_maps)
 }
 
+/// Returns whether a trait member with the given visibility is visible from `current_module_id`.
 pub fn trait_member_is_visible(
     trait_id: TraitId,
     visibility: ItemVisibility,
@@ -89,6 +91,7 @@ pub fn trait_member_is_visible(
     type_member_is_visible(trait_id.0, visibility, current_module_id, def_maps)
 }
 
+/// Returns whether a struct or trait member with the given visibility is visible from `current_module_id`.
 fn type_member_is_visible(
     type_module_id: ModuleId,
     visibility: ItemVisibility,
@@ -123,6 +126,11 @@ fn type_member_is_visible(
     }
 }
 
+/// Returns whether a method call `func_id` on an object of type `object_type` is visible from
+/// `current_module`.
+/// If there's a self type at the current location it must be passed as `self_type`. This is
+/// used for the case of calling, inside a generic trait impl, a private method on the same
+/// type as `self_type` regardless of its generic arguments (in this case the call is allowed).
 pub fn method_call_is_visible(
     self_type: Option<&Type>,
     object_type: &Type,
@@ -188,6 +196,7 @@ pub fn method_call_is_visible(
     }
 }
 
+/// Returns whether two types are the same disregarding their generic arguments.
 fn is_same_type_regardless_generics(type1: &Type, type2: &Type) -> bool {
     if type1 == type2 {
         return true;

compiler/noirc_frontend/src/shared/visibility.rs

@@ -1,7 +1,7 @@
 use serde::{Deserialize, Serialize};
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, Default)]
-/// Represents whether the parameter is public or known only to the prover.
+/// Represents whether a parameter or return value is public or known only to the prover.
 pub enum Visibility {
     Public,
     // Constants are not allowed in the ABI for main at the moment.