feat!: turn TypeIsMorePrivateThenItem into an error (#6953)

Author: asterite

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));

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,

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,

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<CrateId, CrateDefMap>,
+) -> 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<CrateId, CrateDefMap>,
+) -> 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<CrateId, CrateDefMap>,
 ) -> 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[&current_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
         }
     }
 }

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<A> {
+            fn foo(self, x: A) -> bool;
+        }
+
+        pub fn bar<T, U, V>(x: (T, U), y: V) -> bool where (T, U): Foo<V> {
+            x.foo(y)
+        }
+
+        fn main() {}"#;
+    assert_no_errors(src);
+}
+
 #[test]
 fn incorrect_generic_count_on_struct_impl() {
     let src = r#"

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

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.