diff --git a/compiler/noirc_frontend/src/ast/traits.rs b/compiler/noirc_frontend/src/ast/traits.rs index b370d7356e6..a9d4e29d98e 100644 --- a/compiler/noirc_frontend/src/ast/traits.rs +++ b/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 { "" }; diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index a4fcfa04a13..1c6c672c8c0 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/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, diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 5f9c2d07cfc..90aebbf16ba 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/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, diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index 37d06d237a8..9653fc95bee 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/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) = diff --git a/compiler/noirc_frontend/src/elaborator/visibility.rs b/compiler/noirc_frontend/src/elaborator/visibility.rs index 63f6b44dfee..123a9168d97 100644 --- a/compiler/noirc_frontend/src/elaborator/visibility.rs +++ b/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 { + /// 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, + }); } } diff --git a/compiler/noirc_frontend/src/hir/resolution/visibility.rs b/compiler/noirc_frontend/src/hir/resolution/visibility.rs index f3907fc9833..aaf80c9a1b8 100644 --- a/compiler/noirc_frontend/src/hir/resolution/visibility.rs +++ b/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; diff --git a/compiler/noirc_frontend/src/shared/visibility.rs b/compiler/noirc_frontend/src/shared/visibility.rs index 36abf7b198a..f7f3ab2bba6 100644 --- a/compiler/noirc_frontend/src/shared/visibility.rs +++ b/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.