feat: catch imports of private definitions in resolution #4491

TomAFrench · 2024-03-06T17:33:42Z

Description

Problem*

Resolves

Summary*

Currently Noir only defines import visibility for functions, while everything else is implicitly public. This PR is an experiment on adding proper import visibility to Noir so that we can.

I've modified ItemScope so that it can hold both public and private definitions and during as we traverse modules in resolution we check that dep:: paths don't enter any private modules. I've added pub mod syntax to allow this to be shown.

I've set the prelude to be pub mod so that we can compile most programs but all other modules within the stdlib are private to show how errors look like.

Additional Context

Documentation*

Check one:

No documentation needed.
Documentation included in this PR.
[Exceptional Case] Documentation to be submitted in a separate PR.
I'll document this closer to the time of this being merged.

PR Checklist*

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt on default settings.

TomAFrench · 2024-03-06T18:41:03Z

noir_stdlib/src/lib.nr

Note that we're breaking a bunch of Rust visibility rules here. The prelude is importing things from private modules.

compiler/noirc_frontend/src/hir/resolution/import.rs

TomAFrench · 2024-03-07T19:55:32Z

Heads up that I'm going to be chopping this PR up into chunks to get some less potentially controversial sections through quicker and reduce the diff. You're welcome to look and comment on it but reviews may become obsolete.

# Description ## Problem\* Resolves  ## Summary\* This is some groundwork pulled out from #4491 We're going to want to apply visibility modifiers to module declarations so this PR creates a proper struct for these so it's easier for us to add an extra field to hold the visibility. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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.

compiler/noirc_frontend/src/hir/resolution/import.rs

compiler/noirc_frontend/src/parser/parser/function.rs

Savio-Sou · 2024-03-11T13:40:24Z

Is this an extension of the open and internal keywords to general Noir programs?

TomAFrench · 2024-03-11T14:33:59Z

Nope this is an equivalent of https://doc.rust-lang.org/reference/visibility-and-privacy.html

# Description ## Problem\* Resolves  ## Summary\* This pulls out a renaming from #4491. We have the concept of `FunctionVisibility` which determines whether a function can be called from other modules in the same crate/other crates. If we're expanding the same concept of visibility to items other than functions we need a generic name for this which doesn't mention functions. I've then renamed it to `ModuleVisibility` to reflect that it shows how this item is visible in other modules but I'm open to suggestions. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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.

compiler/noirc_frontend/src/hir/resolution/import.rs

#4559) # Description ## Problem\* Resolves  ## Summary\* I just had a test pass when it should have failed in #4491 because despite compiling when it shouldn't have, it failed in execution (because I didn't provide a `Prover.toml` but the circuit took arguments). We're then masking potential test failures by not separating circuits which we expect to fail to compile vs fail to execute. I've created an `execution_failure` directory to hold all of the latter and updated the `compile_failure` tests to only compile the circuit rather than executing it. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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.

* master: chore: separate tests for execution failures from compilation failures (#4559) feat: remove curly braces with fmt (#4529) fix: Make `nargo` the default binary for cargo run (#4554) fix: Evaluate operators in globals in types (#4537) chore: Add more `Hash` impls to stdlib (#4470) feat: Sync from aztec-packages (#4546) chore: Add `Instruction::DecrementRc` (#4525) feat: add `nargo compile --watch` command (#4464) fix: Substitute generics when checking the field count of a type (#4547) feat: optimize sha2 implementation (#4441)

# Description ## Problem\* Resolves  ## Summary\* Running tests within the Noir stdlib is a little complicated as nargo will not accept it as a regular crate as the stdlib uses features which are disallowed in other crates. This means that in a number of cases we're placing stdlib unit tests inside of the `test_programs` instead of just using the `#[test]` format inside of the stdlib itself. This is suboptimal as it separates the implementation from the tests in the repository and once #4491 is merged, it will be impossible to use this method to test any private functions within the stdlib. This PR then adds a new integration test which is equivalent to running `nargo test` on the stdlib and moves some tests over to the stdlib as an example. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.

compiler/noirc_frontend/src/hir/resolution/import.rs

TomAFrench · 2024-03-15T16:16:19Z

I think that this is close enough to be ready for reviews now.

TomAFrench · 2024-03-19T11:32:40Z

compiler/noirc_frontend/src/parser/parser.rs

+fn import_visibility() -> impl NoirParser<ItemVisibility> {
+    let is_pub_crate = (keyword(Keyword::Pub)
+        .then_ignore(just(Token::LeftParen))
+        .then_ignore(keyword(Keyword::Crate))
+        .then_ignore(just(Token::RightParen)))
+    .map(|_| ItemVisibility::PublicCrate);
+
+    let is_pub_super = (keyword(Keyword::Pub)
+        .then_ignore(just(Token::LeftParen))
+        .then_ignore(keyword(Keyword::Super))
+        .then_ignore(just(Token::RightParen)))
+    .map(|_| ItemVisibility::PublicSuper);
+
+    let is_pub = keyword(Keyword::Pub).map(|_| ItemVisibility::Public);
+
+    let is_private = empty().map(|_| ItemVisibility::Private);
+
+    choice((is_pub_crate, is_pub_super, is_pub, is_private))
+}


This is a bit confusing as we're overloading pub for both zk visibiilty and standard programming language visibility.

An alternative would be to rename this to export so we'd have export(super) and export(crate) to avoid this overloading.

It's a break from rust but a relatively weak one as it's just a keyword change.

a break from rust

pub for ZK is a break anyway; the benefits of more intuitive keywords almost always outweigh the costs.

@jfecher what are your thoughts on this?

It sounds like using pub is a compromise either way. pub for item visibility is much more common within Noir while pub for zk is only used on entry-point function parameters. Changing either would be breaking but changing pub visibility may be less so just because it is newer. Using export fn, export(crate) fn, etc would make Noir less rust-like which could be a stumbling block. I'm fine with both uses of pub for now but if there's ever a future scenario where we could want to apply pub (zk) to a function somehow then things would get confusing real fast.

compiler/noirc_frontend/src/ast/mod.rs

compiler/noirc_frontend/src/ast/statement.rs

compiler/noirc_frontend/src/hir/resolution/path_resolver.rs

compiler/noirc_frontend/src/lexer/token.rs

compiler/noirc_frontend/src/parser/parser.rs

Co-authored-by: jfecher <[email protected]>

…than a namespace

jfecher

LGTM

…on (#4622) # Description ## Problem\* Resolves  ## Summary\* This PR pulls out the actual changes to path resolution from #4491, omitting the changes which add visibility modifiers to modules. We now determine whether a function is private (and so a warning or error should be emitted) during def collection. This is necessary as to properly determine whether a function is public or private we need information on whether all the modules in the function's path relative to where it's being used allow its contents to be used from this module - information which doesn't exist in the typechecking pass. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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.

Savio-Sou · 2024-04-22T19:08:25Z

@TomAFrench would you like to supplement docs before merging the PR in?

Savio-Sou · 2024-04-22T19:14:57Z

Also will this be a breaking change in terms of libraries without pub will turn from public-by-default to private-by-default after the PR?

* master: (105 commits) chore: unbundle `check_array_is_initialized` (#5451) feat: Sync from aztec-packages (#5467) chore: bump bb to 0.45.1 (#5469) feat: prefix operator overload trait dispatch (#5423) feat: add CLI argument for debugging comptime blocks (#5192) chore: document EmbeddedCurvePoint (#5468) feat: lsp rename/find-all-references for struct members (#5443) feat(optimization): Deduplicate more instructions (#5457) fix: remove compile-time error for invalid indices (#5466) feat: lsp rename/find-all-references for local variables (#5439) feat: remove duplicated array reads at constant indices (#5445) fix: Account for the expected kind when resolving turbofish generics (#5448) fix: Fix issue with unresolved results (#5453) feat: apply `no_predicates` in stdlib (#5454) fix: prevent `no_predicates` from removing predicates in calling function (#5452) feat: lsp rename/find-all-references for globals (#5415) feat: remove redundant `EnableSideEffects` instructions (#5440) fix: Change panic to error in interpreter (#5446) feat: Add more slice methods to the stdlib (#5424) feat: Unquote multiple items from annotations (#5441) ...

TomAFrench · 2024-09-09T12:49:05Z

Closing as superceded by more recent work.

TomAFrench commented Mar 6, 2024

View reviewed changes

TomAFrench force-pushed the tf/def-collection-visiblity branch from 6dfc2f1 to 36b48f4 Compare March 6, 2024 20:40

TomAFrench commented Mar 6, 2024

View reviewed changes

compiler/noirc_frontend/src/hir/resolution/import.rs Show resolved Hide resolved

TomAFrench mentioned this pull request Mar 6, 2024

chore: generalise FunctionVisibility to ItemVisibility #4495

Merged

5 tasks

TomAFrench mentioned this pull request Mar 7, 2024

chore: add ModuleDeclaration struct #4512

Merged

5 tasks

TomAFrench force-pushed the tf/def-collection-visiblity branch from 83f5320 to b8e59ff Compare March 7, 2024 22:20

TomAFrench changed the base branch from master to tf/less-visibility-hardcoding March 7, 2024 22:20

TomAFrench force-pushed the tf/def-collection-visiblity branch from b8e59ff to 1e557bb Compare March 7, 2024 22:25

TomAFrench commented Mar 7, 2024

View reviewed changes

compiler/noirc_frontend/src/hir/resolution/import.rs Outdated Show resolved Hide resolved

TomAFrench commented Mar 7, 2024

View reviewed changes

compiler/noirc_frontend/src/parser/parser/function.rs Show resolved Hide resolved

Base automatically changed from tf/less-visibility-hardcoding to master March 13, 2024 15:39

feat: catch imports of private definitions in def collection

5aaa6fe

TomAFrench force-pushed the tf/def-collection-visiblity branch from 1e557bb to 5aaa6fe Compare March 13, 2024 15:42

TomAFrench mentioned this pull request Mar 14, 2024

feat: allow usage of noir #[test] syntax in stdlib #4553

Merged

5 tasks

TomAFrench added 2 commits March 14, 2024 18:28

feat: fully resolve private items while returning warning

166dc3f

chore: fix warning for unused result

04c7203

TomAFrench mentioned this pull request Mar 14, 2024

chore: separate tests for execution failures from compilation failures #4559

Merged

5 tasks

chore: cleanup

6285007

TomAFrench commented Mar 14, 2024

View reviewed changes

compiler/noirc_frontend/src/hir/resolution/import.rs Outdated Show resolved Hide resolved

TomAFrench added 5 commits March 14, 2024 20:48

chore: replace placeholder error messages

e6bb6b7

chore: remove private modules test

bb95692

chore: formatting

a08090c

chore: nit

bf3a540

TomAFrench added 2 commits March 15, 2024 12:19

chore: refactoring

16048c9

chore: refactor display of ItemVisibility

424fbee

Merge branch 'master' into tf/def-collection-visiblity

8633239

TomAFrench commented Mar 15, 2024

View reviewed changes

compiler/noirc_frontend/src/hir/resolution/import.rs Outdated Show resolved Hide resolved

TomAFrench marked this pull request as ready for review March 15, 2024 16:16

TomAFrench commented Mar 19, 2024

View reviewed changes

jfecher reviewed Mar 19, 2024

View reviewed changes

TomAFrench and others added 9 commits March 19, 2024 18:19

Update compiler/noirc_frontend/src/parser/parser.rs

0e21701

Co-authored-by: jfecher <[email protected]>

Update compiler/noirc_frontend/src/parser/parser.rs

352cfa6

Co-authored-by: jfecher <[email protected]>

Update compiler/noirc_frontend/src/lexer/token.rs

564a067

Co-authored-by: jfecher <[email protected]>

Update compiler/noirc_frontend/src/ast/statement.rs

5deda19

chore: remove duplicated code

72151b8

chore: simplify return types

83198ac

chore: make fn more private

b7d57bf

chore: make PathResolution actually contain a resolved path rather …

c389a55

…than a namespace

chore: cargo fmt

90ee97a

TomAFrench mentioned this pull request Mar 23, 2024

chore: check for references to private functions during path resolution #4622

Merged

5 tasks

jfecher approved these changes Mar 25, 2024

View reviewed changes

Merge branch 'master' into tf/def-collection-visiblity

32f5246

TomAFrench mentioned this pull request Apr 16, 2024

Epic: Add visibility system to hide implementation details internal to modules #4515

Open

TomAFrench and others added 2 commits June 19, 2024 14:07

Merge branch 'master' into tf/def-collection-visiblity

b2d1f77

TomAFrench closed this Sep 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: catch imports of private definitions in resolution #4491

feat: catch imports of private definitions in resolution #4491

TomAFrench commented Mar 6, 2024

TomAFrench Mar 6, 2024

TomAFrench commented Mar 7, 2024 •

edited

Loading

Savio-Sou commented Mar 11, 2024

TomAFrench commented Mar 11, 2024

TomAFrench commented Mar 15, 2024

TomAFrench Mar 19, 2024

TomAFrench Mar 19, 2024

Savio-Sou Mar 19, 2024 •

edited

Loading

TomAFrench Mar 25, 2024

jfecher Mar 25, 2024

jfecher left a comment

Savio-Sou commented Apr 22, 2024

Savio-Sou commented Apr 22, 2024

TomAFrench commented Sep 9, 2024

feat: catch imports of private definitions in resolution #4491

feat: catch imports of private definitions in resolution #4491

Conversation

TomAFrench commented Mar 6, 2024

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

TomAFrench Mar 6, 2024

Choose a reason for hiding this comment

TomAFrench commented Mar 7, 2024 • edited Loading

Savio-Sou commented Mar 11, 2024

TomAFrench commented Mar 11, 2024

TomAFrench commented Mar 15, 2024

TomAFrench Mar 19, 2024

Choose a reason for hiding this comment

TomAFrench Mar 19, 2024

Choose a reason for hiding this comment

Savio-Sou Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

TomAFrench Mar 25, 2024

Choose a reason for hiding this comment

jfecher Mar 25, 2024

Choose a reason for hiding this comment

jfecher left a comment

Choose a reason for hiding this comment

Savio-Sou commented Apr 22, 2024

Savio-Sou commented Apr 22, 2024

TomAFrench commented Sep 9, 2024

TomAFrench commented Mar 7, 2024 •

edited

Loading

Savio-Sou Mar 19, 2024 •

edited

Loading