feat: visibility for modules

compiler/noirc_frontend/src/ast/statement.rs

@@ -294,6 +294,7 @@ pub trait Recoverable {
 
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub struct ModuleDeclaration {
+    pub visibility: ItemVisibility,
     pub ident: Ident,
     pub outer_attributes: Vec<SecondaryAttribute>,
 }

compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -381,6 +381,7 @@
             let trait_id = match self.push_child_module(
                 context,
                 &name,
+                ItemVisibility::Public,
                 Location::new(name.span(), self.file_id),
                 Vec::new(),
                 Vec::new(),
@@ -612,6 +613,7 @@
             match self.push_child_module(
                 context,
                 &submodule.name,
+                submodule.visibility,
                 Location::new(submodule.name.span(), file_id),
                 submodule.outer_attributes.clone(),
                 submodule.contents.inner_attributes.clone(),
@@ -711,6 +713,7 @@
         match self.push_child_module(
             context,
             &mod_decl.ident,
+            mod_decl.visibility,
             Location::new(Span::empty(0), child_file_id),
             mod_decl.outer_attributes.clone(),
             ast.inner_attributes.clone(),
@@ -761,6 +764,7 @@
         &mut self,
         context: &mut Context,
         mod_name: &Ident,
+        visibility: ItemVisibility,
         mod_location: Location,
         outer_attributes: Vec<SecondaryAttribute>,
         inner_attributes: Vec<SecondaryAttribute>,
@@ -772,6 +776,7 @@
             &mut self.def_collector.def_map,
             self.module_id,
             mod_name,
+            visibility,
             mod_location,
             outer_attributes,
             inner_attributes,
@@ -806,6 +811,7 @@
     def_map: &mut CrateDefMap,
     parent: LocalModuleId,
     mod_name: &Ident,
+    visibility: ItemVisibility,
     mod_location: Location,
     outer_attributes: Vec<SecondaryAttribute>,
     inner_attributes: Vec<SecondaryAttribute>,
@@ -817,7 +823,7 @@
     //   if it's an inline module, or the first char of a the file if it's an external module.
     // - `location` will always point to the token "foo" in `mod foo` regardless of whether
     //   it's inline or external.
     // Eventually the location put in `ModuleData` is used for codelenses about `contract`s,
     // so we keep using `location` so that it continues to work as usual.
     let location = Location::new(mod_name.span(), mod_location.file);
     let new_module =
@@ -840,7 +846,7 @@
     // the struct name.
     if add_to_parent_scope {
         if let Err((first_def, second_def)) =
-            modules[parent.0].declare_child_module(mod_name.to_owned(), mod_id)
+            modules[parent.0].declare_child_module(mod_name.to_owned(), visibility, mod_id)
         {
             let err = DefCollectorErrorKind::Duplicate {
                 typ: DuplicateType::Module,
@@ -952,6 +958,7 @@
         def_map,
         module_id,
         &name,
+        ItemVisibility::Public,
         location,
         Vec::new(),
         Vec::new(),

compiler/noirc_frontend/src/hir/def_map/module_data.rs

@@ -135,9 +135,10 @@ impl ModuleData {
     pub fn declare_child_module(
         &mut self,
         name: Ident,
+        visibility: ItemVisibility,
         child_id: ModuleId,
     ) -> Result<(), (Ident, Ident)> {
-        self.declare(name, ItemVisibility::Public, child_id.into(), None)
+        self.declare(name, visibility, child_id.into(), None)
     }
 
     pub fn find_func_with_name(&self, name: &Ident) -> Option<FuncId> {

compiler/noirc_frontend/src/parser/mod.rs

@@ -366,6 +366,7 @@ pub enum ItemKind {
 /// These submodules always share the same file as some larger ParsedModule
 #[derive(Clone, Debug)]
 pub struct ParsedSubModule {
+    pub visibility: ItemVisibility,
     pub name: Ident,
     pub contents: ParsedModule,
     pub outer_attributes: Vec<SecondaryAttribute>,
@@ -375,6 +376,7 @@ pub struct ParsedSubModule {
 impl ParsedSubModule {
     pub fn into_sorted(self) -> SortedSubModule {
         SortedSubModule {
+            visibility: self.visibility,
             name: self.name,
             contents: self.contents.into_sorted(),
             outer_attributes: self.outer_attributes,
@@ -398,6 +400,7 @@ impl std::fmt::Display for SortedSubModule {
 #[derive(Clone)]
 pub struct SortedSubModule {
     pub name: Ident,
+    pub visibility: ItemVisibility,
     pub contents: SortedModule,
     pub outer_attributes: Vec<SecondaryAttribute>,
     pub is_contract: bool,

compiler/noirc_frontend/src/parser/parser.rs

@@ -262,14 +262,16 @@ fn submodule(
     module_parser: impl NoirParser<ParsedModule>,
 ) -> impl NoirParser<TopLevelStatementKind> {
     attributes()
+        .then(item_visibility())
         .then_ignore(keyword(Keyword::Mod))
         .then(ident())
         .then_ignore(just(Token::LeftBrace))
         .then(module_parser)
         .then_ignore(just(Token::RightBrace))
-        .validate(|((attributes, name), contents), span, emit| {
+        .validate(|(((attributes, visibility), name), contents), span, emit| {
             let attributes = validate_secondary_attributes(attributes, span, emit);
             TopLevelStatementKind::SubModule(ParsedSubModule {
+                visibility,
                 name,
                 contents,
                 outer_attributes: attributes,
@@ -283,14 +285,16 @@ fn contract(
     module_parser: impl NoirParser<ParsedModule>,
 ) -> impl NoirParser<TopLevelStatementKind> {
     attributes()
+        .then(item_visibility())
         .then_ignore(keyword(Keyword::Contract))
         .then(ident())
         .then_ignore(just(Token::LeftBrace))
         .then(module_parser)
         .then_ignore(just(Token::RightBrace))
-        .validate(|((attributes, name), contents), span, emit| {
+        .validate(|(((attributes, visibility), name), contents), span, emit| {
             let attributes = validate_secondary_attributes(attributes, span, emit);
             TopLevelStatementKind::SubModule(ParsedSubModule {
+                visibility,
                 name,
                 contents,
                 outer_attributes: attributes,
@@ -431,10 +435,14 @@ fn optional_type_annotation<'a>() -> impl NoirParser<UnresolvedType> + 'a {
 }
 
 fn module_declaration() -> impl NoirParser<TopLevelStatementKind> {
-    attributes().then_ignore(keyword(Keyword::Mod)).then(ident()).validate(
-        |(attributes, ident), span, emit| {
+    attributes().then(item_visibility()).then_ignore(keyword(Keyword::Mod)).then(ident()).validate(
+        |((attributes, visibility), ident), span, emit| {
             let attributes = validate_secondary_attributes(attributes, span, emit);
-            TopLevelStatementKind::Module(ModuleDeclaration { ident, outer_attributes: attributes })
+            TopLevelStatementKind::Module(ModuleDeclaration {
+                visibility,
+                ident,
+                outer_attributes: attributes,
+            })
         },
     )
 }

compiler/noirc_frontend/src/tests.rs

@@ -6,6 +6,7 @@
 mod references;
 mod turbofish;
 mod unused_items;
+mod visibility;
 
 // XXX: These tests repeat a lot of code
 // what we should do is have test cases which are passed to a test harness
@@ -2998,7 +2999,7 @@
 }
 
 #[test]
 fn arithmetic_generics_canonicalization_deduplication_regression() {
     let source = r#"
         struct ArrData<let N: u32> {
             a: [Field; N],
@@ -3092,29 +3093,6 @@
     assert_eq!(errors.len(), 0);
 }
 
-#[test]
-fn errors_once_on_unused_import_that_is_not_accessible() {
-    // Tests that we don't get an "unused import" here given that the import is not accessible
-    let src = r#"
-        mod moo {
-            struct Foo {}
-        }
-        use moo::Foo;
-        fn main() {
-            let _ = Foo {};
-        }
-    "#;
-
-    let errors = get_program_errors(src);
-    assert_eq!(errors.len(), 1);
-    assert!(matches!(
-        errors[0].0,
-        CompilationError::DefinitionError(DefCollectorErrorKind::PathResolutionError(
-            PathResolutionError::Private { .. }
-        ))
-    ));
-}
-
 #[test]
 fn trait_unconstrained_methods_typechecked_correctly() {
     // This test checks that we properly track whether a method has been declared as unconstrained on the trait definition
@@ -3143,60 +3121,3 @@
     println!("{errors:?}");
     assert_eq!(errors.len(), 0);
 }
-
-#[test]
-fn errors_if_type_alias_aliases_more_private_type() {
-    let src = r#"
-    struct Foo {}
-    pub type Bar = Foo;
-
-    pub fn no_unused_warnings(_b: Bar) {
-        let _ = Foo {};
-    }
-
-    fn main() {}
-    "#;
-
-    let errors = get_program_errors(src);
-    assert_eq!(errors.len(), 1);
-
-    let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem {
-        typ, item, ..
-    }) = &errors[0].0
-    else {
-        panic!("Expected an unused item error");
-    };
-
-    assert_eq!(typ, "Foo");
-    assert_eq!(item, "Bar");
-}
-
-#[test]
-fn errors_if_type_alias_aliases_more_private_type_in_generic() {
-    let src = r#"
-    pub struct Generic<T> { value: T }
-
-    struct Foo {}
-    pub type Bar = Generic<Foo>;
-
-    pub fn no_unused_warnings(_b: Bar) {
-        let _ = Foo {};
-        let _ = Generic { value: 1 };
-    }
-
-    fn main() {}
-    "#;
-
-    let errors = get_program_errors(src);
-    assert_eq!(errors.len(), 1);
-
-    let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem {
-        typ, item, ..
-    }) = &errors[0].0
-    else {
-        panic!("Expected an unused item error");
-    };
-
-    assert_eq!(typ, "Foo");
-    assert_eq!(item, "Bar");
-}

compiler/noirc_frontend/src/tests/visibility.rs

@@ -0,0 +1,113 @@
+use crate::{
+    hir::{
+        def_collector::{dc_crate::CompilationError, errors::DefCollectorErrorKind},
+        resolution::{errors::ResolverError, import::PathResolutionError},
+    },
+    tests::get_program_errors,
+};
+
+#[test]
+fn errors_once_on_unused_import_that_is_not_accessible() {
+    // Tests that we don't get an "unused import" here given that the import is not accessible
+    let src = r#"
+        mod moo {
+            struct Foo {}
+        }
+        use moo::Foo;
+        fn main() {
+            let _ = Foo {};
+        }
+    "#;
+
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+    assert!(matches!(
+        errors[0].0,
+        CompilationError::DefinitionError(DefCollectorErrorKind::PathResolutionError(
+            PathResolutionError::Private { .. }
+        ))
+    ));
+}
+
+#[test]
+fn errors_if_type_alias_aliases_more_private_type() {
+    let src = r#"
+    struct Foo {}
+    pub type Bar = Foo;
+
+    pub fn no_unused_warnings(_b: Bar) {
+        let _ = Foo {};
+    }
+
+    fn main() {}
+    "#;
+
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+
+    let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem {
+        typ, item, ..
+    }) = &errors[0].0
+    else {
+        panic!("Expected an unused item error");
+    };
+
+    assert_eq!(typ, "Foo");
+    assert_eq!(item, "Bar");
+}
+
+#[test]
+fn errors_if_type_alias_aliases_more_private_type_in_generic() {
+    let src = r#"
+    pub struct Generic<T> { value: T }
+
+    struct Foo {}
+    pub type Bar = Generic<Foo>;
+
+    pub fn no_unused_warnings(_b: Bar) {
+        let _ = Foo {};
+        let _ = Generic { value: 1 };
+    }
+
+    fn main() {}
+    "#;
+
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+
+    let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem {
+        typ, item, ..
+    }) = &errors[0].0
+    else {
+        panic!("Expected an unused item error");
+    };
+
+    assert_eq!(typ, "Foo");
+    assert_eq!(item, "Bar");
+}
+
+#[test]
+fn errors_if_trying_to_access_public_function_inside_private_module() {
+    let src = r#"
+    mod foo {
+        mod bar {
+            pub fn baz() {}
+        }
+    }
+    fn main() {
+        foo::bar::baz()
+    }
+    "#;
+
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 2); // There's a bug that duplicates this error
+
+    let CompilationError::ResolverError(ResolverError::PathResolutionError(
+        PathResolutionError::Private(ident),
+    )) = &errors[0].0
+    else {
+        panic!("Expected a private error");
+    };
+
+    assert_eq!(ident.to_string(), "bar");
+}

docs/docs/noir/modules_packages_crates/modules.md

@@ -208,4 +208,14 @@ fn main() {
 }
 ```
 
-In this example, the module `some_module` re-exports two public names defined in `foo`.
+In this example, the module `some_module` re-exports two public names defined in `foo`.
+
+### Visibility
+
+By default, like functions, modules are private to the module (or crate) the exist in. You can use `pub`
+to make the module public or `pub(crate)` to make it public to just its crate:
+
+```rust
+// This module is now public and can be seen by other crates.
+pub mod foo;
+```