From 850b2b17a5720d5fad9dbd3102f51ef7bfe36dd4 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 7 Jan 2025 11:31:47 -0300 Subject: [PATCH] feat!: turn TypeIsMorePrivateThenItem into an error (#6953) --- .../noirc_frontend/src/elaborator/traits.rs | 7 +- .../src/hir/def_collector/dc_mod.rs | 2 +- .../src/hir/resolution/errors.rs | 2 +- .../src/hir/resolution/visibility.rs | 77 ++++++++++++------- compiler/noirc_frontend/src/tests.rs | 15 ++++ .../noirc_frontend/src/tests/visibility.rs | 23 ++++++ docs/docs/noir/concepts/traits.md | 2 + 7 files changed, 97 insertions(+), 31 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index e1be45927ca..0b5b5cba0d2 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -127,6 +127,7 @@ impl<'context> Elaborator<'context> { parameters, return_type, where_clause, + unresolved_trait.trait_def.visibility, func_id, ); @@ -189,6 +190,7 @@ impl<'context> Elaborator<'context> { parameters: &[(Ident, UnresolvedType)], return_type: &FunctionReturnType, where_clause: &[UnresolvedTraitConstraint], + trait_visibility: ItemVisibility, func_id: FuncId, ) { let old_generic_count = self.generics.len(); @@ -205,8 +207,9 @@ impl<'context> Elaborator<'context> { where_clause, return_type, ); - // Trait functions are always public - def.visibility = ItemVisibility::Public; + + // Trait functions always have the same visibility as the trait they are in + def.visibility = trait_visibility; let mut function = NoirFunction { kind, def }; self.define_function_meta(&mut function, func_id, Some(trait_id)); diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index eff48ce22a6..426db7c8f84 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -487,7 +487,7 @@ impl<'a> ModCollector<'a> { let location = Location::new(name.span(), self.file_id); let modifiers = FunctionModifiers { name: name.to_string(), - visibility: ItemVisibility::Public, + visibility: trait_definition.visibility, // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629 attributes: crate::token::Attributes::empty(), is_unconstrained: *is_unconstrained, diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 8817904c6f5..a4b3c1b9c07 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -607,7 +607,7 @@ impl<'a> From<&'a ResolverError> for Diagnostic { }, ResolverError::UnsupportedNumericGenericType(err) => err.into(), ResolverError::TypeIsMorePrivateThenItem { typ, item, span } => { - Diagnostic::simple_warning( + Diagnostic::simple_error( format!("Type `{typ}` is more private than item `{item}`"), String::new(), *span, diff --git a/compiler/noirc_frontend/src/hir/resolution/visibility.rs b/compiler/noirc_frontend/src/hir/resolution/visibility.rs index c2fe887fe62..5e6dffa56e4 100644 --- a/compiler/noirc_frontend/src/hir/resolution/visibility.rs +++ b/compiler/noirc_frontend/src/hir/resolution/visibility.rs @@ -1,5 +1,5 @@ use crate::graph::CrateId; -use crate::node_interner::{FuncId, NodeInterner, StructId}; +use crate::node_interner::{FuncId, NodeInterner, StructId, TraitId}; use crate::Type; use std::collections::BTreeMap; @@ -79,26 +79,47 @@ pub fn struct_member_is_visible( visibility: ItemVisibility, current_module_id: ModuleId, def_maps: &BTreeMap, +) -> bool { + type_member_is_visible(struct_id.module_id(), visibility, current_module_id, def_maps) +} + +pub fn trait_member_is_visible( + trait_id: TraitId, + visibility: ItemVisibility, + current_module_id: ModuleId, + def_maps: &BTreeMap, +) -> bool { + type_member_is_visible(trait_id.0, visibility, current_module_id, def_maps) +} + +fn type_member_is_visible( + type_module_id: ModuleId, + visibility: ItemVisibility, + current_module_id: ModuleId, + def_maps: &BTreeMap, ) -> bool { match visibility { ItemVisibility::Public => true, ItemVisibility::PublicCrate => { - struct_id.parent_module_id(def_maps).krate == current_module_id.krate + let type_parent_module_id = + type_module_id.parent(def_maps).expect("Expected parent module to exist"); + type_parent_module_id.krate == current_module_id.krate } ItemVisibility::Private => { - let struct_parent_module_id = struct_id.parent_module_id(def_maps); - if struct_parent_module_id.krate != current_module_id.krate { + let type_parent_module_id = + type_module_id.parent(def_maps).expect("Expected parent module to exist"); + if type_parent_module_id.krate != current_module_id.krate { return false; } - if struct_parent_module_id.local_id == current_module_id.local_id { + if type_parent_module_id.local_id == current_module_id.local_id { return true; } let def_map = &def_maps[¤t_module_id.krate]; module_descendent_of_target( def_map, - struct_parent_module_id.local_id, + type_parent_module_id.local_id, current_module_id.local_id, ) } @@ -115,35 +136,37 @@ pub fn method_call_is_visible( 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::PublicCrate | ItemVisibility::Private => { + let func_meta = interner.function_meta(&func_id); + if let Some(struct_id) = func_meta.struct_id { + return struct_member_is_visible( + struct_id, + modifiers.visibility, + current_module, + def_maps, + ); } - } - ItemVisibility::Private => { + + if let Some(trait_id) = func_meta.trait_id { + return trait_member_is_visible( + trait_id, + modifiers.visibility, + current_module, + def_maps, + ); + } + if object_type.is_primitive() { let func_module = interner.function_module(func_id); - item_in_module_is_visible( + return item_in_module_is_visible( def_maps, current_module, 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 - } + ); } + + true } } } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index f29747431ea..3d2e71662e9 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2842,6 +2842,21 @@ fn trait_constraint_on_tuple_type() { assert_no_errors(src); } +#[test] +fn trait_constraint_on_tuple_type_pub_crate() { + let src = r#" + pub(crate) trait Foo { + fn foo(self, x: A) -> bool; + } + + pub fn bar(x: (T, U), y: V) -> bool where (T, U): Foo { + x.foo(y) + } + + fn main() {}"#; + assert_no_errors(src); +} + #[test] fn incorrect_generic_count_on_struct_impl() { let src = r#" diff --git a/compiler/noirc_frontend/src/tests/visibility.rs b/compiler/noirc_frontend/src/tests/visibility.rs index 824a1de4c37..917394316cf 100644 --- a/compiler/noirc_frontend/src/tests/visibility.rs +++ b/compiler/noirc_frontend/src/tests/visibility.rs @@ -229,6 +229,29 @@ fn errors_if_pub_trait_returns_private_struct() { assert_type_is_more_private_than_item_error(src, "Bar", "foo"); } +#[test] +fn does_not_error_if_trait_with_default_visibility_returns_struct_with_default_visibility() { + let src = r#" + struct Foo {} + + trait Bar { + fn bar(self) -> Foo; + } + + impl Bar for Foo { + fn bar(self) -> Foo { + self + } + } + + fn main() { + let foo = Foo {}; + let _ = foo.bar(); + } + "#; + assert_no_errors(src); +} + #[test] fn errors_if_trying_to_access_public_function_inside_private_module() { let src = r#" diff --git a/docs/docs/noir/concepts/traits.md b/docs/docs/noir/concepts/traits.md index b6c0a886eb0..1b232dcf8ab 100644 --- a/docs/docs/noir/concepts/traits.md +++ b/docs/docs/noir/concepts/traits.md @@ -582,3 +582,5 @@ pub trait Trait {} // This trait alias is now public pub trait Baz = Foo + Bar; ``` + +Trait methods have the same visibility as the trait they are in.