Skip to content

Commit

Permalink
feat: visibility for modules (noir-lang#6165)
Browse files Browse the repository at this point in the history
# Description

## Problem

Part of noir-lang#4515

## Summary

## Additional Context

I made most of the stdlib modules public. These are all except the test
modules and some modules that I thought were just implementation details
of some other modules.

I'm adding very short documentation for each of these. I'm thinking that
once we get all visibility done it should be documented in its own
section, with examples.

## Documentation

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
  • Loading branch information
asterite and TomAFrench authored Sep 27, 2024
1 parent 877b806 commit fcdbcb9
Show file tree
Hide file tree
Showing 27 changed files with 260 additions and 185 deletions.
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/ast/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
}
Expand Down
9 changes: 8 additions & 1 deletion compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,7 @@ impl<'a> ModCollector<'a> {
let trait_id = match self.push_child_module(
context,
&name,
ItemVisibility::Public,
Location::new(name.span(), self.file_id),
Vec::new(),
Vec::new(),
Expand Down Expand Up @@ -612,6 +613,7 @@ impl<'a> ModCollector<'a> {
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(),
Expand Down Expand Up @@ -711,6 +713,7 @@ impl<'a> ModCollector<'a> {
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(),
Expand Down Expand Up @@ -761,6 +764,7 @@ impl<'a> ModCollector<'a> {
&mut self,
context: &mut Context,
mod_name: &Ident,
visibility: ItemVisibility,
mod_location: Location,
outer_attributes: Vec<SecondaryAttribute>,
inner_attributes: Vec<SecondaryAttribute>,
Expand All @@ -772,6 +776,7 @@ impl<'a> ModCollector<'a> {
&mut self.def_collector.def_map,
self.module_id,
mod_name,
visibility,
mod_location,
outer_attributes,
inner_attributes,
Expand Down Expand Up @@ -806,6 +811,7 @@ fn push_child_module(
def_map: &mut CrateDefMap,
parent: LocalModuleId,
mod_name: &Ident,
visibility: ItemVisibility,
mod_location: Location,
outer_attributes: Vec<SecondaryAttribute>,
inner_attributes: Vec<SecondaryAttribute>,
Expand Down Expand Up @@ -840,7 +846,7 @@ fn push_child_module(
// 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,
Expand Down Expand Up @@ -952,6 +958,7 @@ pub fn collect_struct(
def_map,
module_id,
&name,
ItemVisibility::Public,
location,
Vec::new(),
Vec::new(),
Expand Down
3 changes: 2 additions & 1 deletion compiler/noirc_frontend/src/hir/def_map/module_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_frontend/src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
18 changes: 13 additions & 5 deletions compiler/noirc_frontend/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
})
},
)
}
Expand Down
81 changes: 1 addition & 80 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ mod name_shadowing;
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
Expand Down Expand Up @@ -3092,29 +3093,6 @@ fn use_numeric_generic_in_trait_method() {
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
Expand Down Expand Up @@ -3143,60 +3121,3 @@ fn trait_unconstrained_methods_typechecked_correctly() {
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");
}
113 changes: 113 additions & 0 deletions compiler/noirc_frontend/src/tests/visibility.rs
Original file line number Diff line number Diff line change
@@ -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");
}
12 changes: 11 additions & 1 deletion docs/docs/noir/modules_packages_crates/modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
```
Loading

0 comments on commit fcdbcb9

Please sign in to comment.