chore!: Make file manager read-only to the compiler #3760

kevaundray · 2023-12-10T21:23:55Z

Description

This is a PR that moves us towards having the compiler see the FileManager as an immutable object. How it gets populated will now be upto the caller.

One issue we have so far is that caller, can be in the wasm context or the native context, so we have file_reader being a component that we conditionally compile depending on the context.

Ideally we move to a case where both package source resolution and dependency source resolution is done beforehand, and the compiler assumes that FileManager has both the source for its current crate and all of the crates dependencies.

This separation of concerns makes it easier to incrementally compile parts of the compiler in the future. For example, if the FileManager has not changed, then we know that the compilation result should not change,and we do not need to recompile.

Further, lets say we add a pass that solely does parsing of the root files in the File Manager, If a particular parsed AST has not changed, then we do not need to parse that file again.

Problem*

Resolves

Summary*

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 and/or cargo fmt on default settings.

…s already been populated

kevaundray · 2023-12-10T21:26:41Z

One difficulty with this PR is how to handle things in the browser context, where there are no files.

I think initially we can support the usecase of a single file in the browser, since this doesn't break existing playgrounds (This allows us to mitigate the file system issue). For node, we do have the file system and we should try to not break that workflow.

tooling/nargo/src/lib.rs

kevaundray · 2023-12-10T22:05:05Z

The remarkable errors are due to how we special handle the stdlib -- we may need to move this logic outside of the file manager (which makes sense since the file manager should only be concerned with files and not need to know about the stdlib)

…urce

…de their source code

kevaundray · 2023-12-10T22:41:17Z

Last set of errors should be related to the file manager not having the source code for its dependencies (since the method we added only allows us to add code for the current package)

compiler/noirc_driver/src/lib.rs

…les into the file manager anymore

… too

tooling/nargo/src/lib.rs

kevaundray · 2023-12-10T23:30:47Z

As of this commit, the native non-wasm version is passing tests. The tests in Test-ubuntu that are failing are related to compiler::wasm.

compiler/noirc_driver/src/stdlib.rs

…laceholders like panic! and format! do

compiler/wasm/src/compile.rs

tooling/nargo/src/lib.rs

TomAFrench · 2023-12-13T15:49:38Z

I think this doesn't cause any major problems beyond the expected need to modify code interacting with noir_wasm. I'm good to merge this once followup issues have been made.

kevaundray · 2023-12-13T17:18:24Z

I think this doesn't cause any major problems beyond the expected need to modify code interacting with noir_wasm. I'm good to merge this once followup issues have been made.

@alexghr pinging you to confirm this is okay too?

kevaundray · 2023-12-13T17:18:38Z

I'll also quickly put together a rough PR for a different API which aligns more with what we have in nargo that we can merge in the future

kevaundray · 2023-12-13T18:02:18Z

(non-breaking) PR is here for a different API that is more context-centric #3798

Co-authored-by: Tom French <[email protected]>

alexghr · 2023-12-13T18:15:40Z

Yep, looks good to me! I think we just have to be aware of having to update noir-compiler in aztec-packages at the same time as the noir subrepo is updated in it.

* master: (25 commits) chore!: Make file manager read-only to the compiler (#3760) feat: Add some traits to the stdlib (#3796) fix: Stop issuing unused variable warnings for variables in trait definitions (#3797) fix(lsp): package resolution on save (#3794) chore: clippy fix (#3793) feat: Remove experimental feature warning for traits (#3783) fix: Allow trait method references from the trait name (#3774) chore: adds a new option only-acir (#3683) feat(lsp): add goto definition for structs (#3718) chore: disable code lens feature of lsp (#3789) chore: moving ordering to category jsons and frontmatters (#3777) chore(ci): use `actions/setup-node` for caching yarn dependencies (#2730) fix: remove `noirc_driver/aztec` feature flag in docker (#3784) chore: remove aztec compile time feature flag (#3596) chore: move debugger tests in submodule (#3780) feat: simplify explicit equality assertions to assert equality directly (#3708) fix(ssa): Handle array arguments to side effectual constrain statements (#3740) feat: add `prelude.nr` (#3693) chore: removing old docs (#3778) feat: avoid overflow checks on boolean multiplication (#3745) ...

# Description ## Problem\* Resolves #3760 (comment) ## Summary\* This PR removes the source-resolver package now that it's no longer used. ## 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.

* master: (148 commits) feat: add support for writing tracing debug info to file (#3790) chore: clippy fix (#3803) chore(ci): enforce timeouts on build and test workflows (#3804) chore!: remove unused `source-resolver` package (#3791) chore!: Make file manager read-only to the compiler (#3760) feat: Add some traits to the stdlib (#3796) fix: Stop issuing unused variable warnings for variables in trait definitions (#3797) fix(lsp): package resolution on save (#3794) chore: clippy fix (#3793) feat: Remove experimental feature warning for traits (#3783) fix: Allow trait method references from the trait name (#3774) chore: adds a new option only-acir (#3683) feat(lsp): add goto definition for structs (#3718) chore: disable code lens feature of lsp (#3789) chore: moving ordering to category jsons and frontmatters (#3777) chore(ci): use `actions/setup-node` for caching yarn dependencies (#2730) fix: remove `noirc_driver/aztec` feature flag in docker (#3784) chore: remove aztec compile time feature flag (#3596) chore: move debugger tests in submodule (#3780) feat: simplify explicit equality assertions to assert equality directly (#3708) ...

🤖 I have created a release *beep* *boop* --- <details><summary>0.21.0</summary> ## [0.21.0](v0.20.0...v0.21.0) (2023-12-15) ### ⚠ BREAKING CHANGES * remove unused `source-resolver` package ([#3791](#3791)) * Make file manager read-only to the compiler ([#3760](#3760)) ### Features * Add `prelude.nr` ([#3693](#3693)) ([5f0f81f](5f0f81f)) * Add some traits to the stdlib ([#3796](#3796)) ([8e11352](8e11352)) * Add support for writing tracing debug info to file ([#3790](#3790)) ([98a5004](98a5004)) * Allow passing custom foreign call handlers when creating proofs in NoirJS ([#3764](#3764)) ([6076e08](6076e08)) * Allow underscores in integer literals ([#3746](#3746)) ([2c06a64](2c06a64)) * Avoid overflow checks on boolean multiplication ([#3745](#3745)) ([9b5b686](9b5b686)) * Aztec-packages ([#3754](#3754)) ([c043265](c043265)) * Dockerfile to test cargo and JS packages ([#3684](#3684)) ([513d619](513d619)) * Docs landing page with a playground ([#3667](#3667)) ([9a95fbe](9a95fbe)) * Enhance test information output ([#3696](#3696)) ([468fbbc](468fbbc)) * Implement print without newline ([#3650](#3650)) ([9827dfe](9827dfe)) * **lsp:** Add goto definition for locals ([#3705](#3705)) ([9dd465c](9dd465c)) * **lsp:** Add goto definition for structs ([#3718](#3718)) ([a576c5b](a576c5b)) * Optimize out unnecessary truncation instructions ([#3717](#3717)) ([c9c72ae](c9c72ae)) * Remove experimental feature warning for traits ([#3783](#3783)) ([cb52242](cb52242)) * Reorganizing docs to fit diataxis framework ([#3711](#3711)) ([54a1ed5](54a1ed5)) * Simplify explicit equality assertions to assert equality directly ([#3708](#3708)) ([2fc46e2](2fc46e2)) * Speed up transformation of debug messages ([#3815](#3815)) ([2a8af1e](2a8af1e)) ### Bug Fixes * `try_unify` no longer binds types on failure ([#3697](#3697)) ([f03e581](f03e581)) * Add missing assertion to test ([#3765](#3765)) ([bcbe116](bcbe116)) * Add negative integer literals ([#3690](#3690)) ([8b3a68f](8b3a68f)) * Allow trait method references from the trait name ([#3774](#3774)) ([cfa34d4](cfa34d4)) * Deserialize odd length hex literals ([#3747](#3747)) ([4000fb2](4000fb2)) * **docs:** Trigger `update-docs` workflow when the `release-please` PR gets merged and not on every merge to master ([#3677](#3677)) ([9a3d1d2](9a3d1d2)) * Initialise strings as u8 array ([#3682](#3682)) ([8da40b7](8da40b7)) * **lsp:** Package resolution on save ([#3794](#3794)) ([14f2fff](14f2fff)) * Parse negative integer literals ([#3698](#3698)) ([463ab06](463ab06)) * Pub is required on return for entry points ([#3616](#3616)) ([7f1d796](7f1d796)) * Remove `noirc_driver/aztec` feature flag in docker ([#3784](#3784)) ([a48d562](a48d562)) * Remove include-keys option ([#3692](#3692)) ([95d7ce2](95d7ce2)) * Revert chnage to modify version in workspace file for acvm dependencies ([#3673](#3673)) ([0696f75](0696f75)) * Sequence update-lockfile workflow so it gets modified after the ACVM version in the root has been changed ([#3676](#3676)) ([c00cd85](c00cd85)) * **ssa:** Handle array arguments to side effectual constrain statements ([#3740](#3740)) ([028d65e](028d65e)) * Stop cloning Traits! ([#3736](#3736)) ([fcff412](fcff412)) * Stop issuing unused variable warnings for variables in trait definitions ([#3797](#3797)) ([0bb44c3](0bb44c3)) * Unsigned integers cannot be negated ([#3688](#3688)) ([f904ae1](f904ae1)) ### Miscellaneous Chores * Make file manager read-only to the compiler ([#3760](#3760)) ([e3dcc21](e3dcc21)) * Remove unused `source-resolver` package ([#3791](#3791)) ([57d2505](57d2505)) </details> <details><summary>0.37.1</summary> ## [0.37.1](v0.37.0...v0.37.1) (2023-12-15) ### Features * Aztec-packages ([#3754](#3754)) ([c043265](c043265)) * Speed up transformation of debug messages ([#3815](#3815)) ([2a8af1e](2a8af1e)) ### Bug Fixes * Deserialize odd length hex literals ([#3747](#3747)) ([4000fb2](4000fb2)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

@alexghr

…ger` for consistency with native interface (#3891) # Description Merging the work done here: AztecProtocol/aztec-packages#3696 #3781 #3760 Plus some extras to make the API nicer. ## Problem Closes(?) #3695 ## Summary Makes noir_wasm easier to work with, including dependency resolution and bundling. This package can be used from both node and the browser with identical API leveraging a virtual filesystem. Uses webpack for bundling, which is done in two steps: 1) rust -> wasm (cjs/esm) 2) TS + wasm (cjs/esm) -> universal package for web Tests have been migrated to mocha and playwright. ## Additional Context ~~I really want to test it [here](https://github.com/signorecello/noir-playground) before merging, but it's in a state in which it can be reviewed before we commit to an API.~~ Done: signorecello/noir-playground#32 Even though the initial memFS-backed FileManager developed by @alexghr is still here, it is not used for the web version due to import problems. The way it works now, webpack uses `memfs` directly it to alias the node `fs` module (which seems to be its intended use case) and allows us to use the nodejs `fs` API everywhere. ## Documentation Documentation is required for usage, but should basically be: ```typescript // Node.js import { compile, createFileManager } from '@noir-lang/noir_wasm'; // Rename!! const fm = createFileManager(myProjectPath); const myCompiledCode = await compile(fm); ``` ```typescript // Browser import { compile, createFileManager } from '@noir-lang/noir_wasm'; // Rename!! const fm = createFileManager('/'); for (const path of files) { await fm.writeFile(path, await getFileAsStream(path)); } const myCompiledCode = await compile(fm); ``` Check one: - [ ] 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. --------- Co-authored-by: sirasistant <[email protected]> Co-authored-by: Tom French <[email protected]> Co-authored-by: Tom French <[email protected]>

kevaundray added 4 commits December 10, 2023 21:07

make find_module be an immutable reference

7e0a3d1

add method in nargo that pre-populates the FileManager

b2ff05b

change add_file to name_to_id -- we assume that the file manager ha…

5592348

…s already been populated

cargo

d5fd4e0

kevaundray commented Dec 10, 2023

View reviewed changes

tooling/nargo/src/lib.rs Outdated Show resolved Hide resolved

Update tooling/nargo/src/lib.rs

2f137b7

kevaundray added 7 commits December 10, 2023 22:32

add a method in file manager that allows us to add a file with its so…

41ae81f

…urce

add deprecation TODO to file_reader

c7f576c

add stdlib file in noirc_driver that returns the stdlib paths alongsi…

a2460e6

…de their source code

add the contents of the stdlib whenever we call prepare_crate

270a0f4

cargo

52d33a5

cargo fmt

a38c27c

move tempfile to dev dependencies

6a5bf33

TomAFrench reviewed Dec 10, 2023

View reviewed changes

compiler/noirc_driver/src/lib.rs Outdated Show resolved Hide resolved

kevaundray added 6 commits December 10, 2023 22:53

add files into file manager in test since find_module does not add fi…

f72ce99

…les into the file manager anymore

insert all files for this packages dependencies into the file manager…

b30ad54

… too

cargo fmt

15a94ee

remove un-needed fully qualified path

4599361

Add note on stdlib

f20109e

cargo fmt

4413350

TomAFrench reviewed Dec 10, 2023

View reviewed changes

tooling/nargo/src/lib.rs Outdated Show resolved Hide resolved

kevaundray commented Dec 10, 2023

View reviewed changes

compiler/noirc_driver/src/stdlib.rs Show resolved Hide resolved

kevaundray added 3 commits December 11, 2023 00:22

add comment to process_dep_graph and fix setup_test_context

d6a24e8

replace expect with unwrap_or_else: expect doesn't allow you to add p…

552a4dd

…laceholders like panic! and format! do

try: add a PathToFileSourceMap object

3f8551b

kevaundray commented Dec 11, 2023

View reviewed changes

compiler/wasm/src/compile.rs Show resolved Hide resolved

chore: add simple doc comments for add_file_with_source methods

1e9ac97

TomAFrench reviewed Dec 13, 2023

View reviewed changes

tooling/nargo/src/lib.rs Outdated Show resolved Hide resolved

kevaundray and others added 2 commits December 13, 2023 18:06

Merge remote-tracking branch 'origin/master' into kw/make-fm-read-only

cc92c07

Update tooling/nargo/src/lib.rs

9aa0bb7

Co-authored-by: Tom French <[email protected]>

kevaundray mentioned this pull request Dec 13, 2023

Move module specific code out of the file manager #3799

Closed

alexghr approved these changes Dec 13, 2023

View reviewed changes

kevaundray added 2 commits December 13, 2023 18:23

read entry_path and get all files in its parent

3e26e3c

clippy

0b53332

TomAFrench approved these changes Dec 13, 2023

View reviewed changes

TomAFrench enabled auto-merge December 13, 2023 19:10

TomAFrench added this pull request to the merge queue Dec 13, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 13, 2023

kevaundray added this pull request to the merge queue Dec 14, 2023

Merged via the queue into master with commit e3dcc21 Dec 14, 2023
36 checks passed

kevaundray deleted the kw/make-fm-read-only branch December 14, 2023 01:29

kevaundray mentioned this pull request Dec 14, 2023

chore: Release Noir(0.21.0) #3672

Closed

Thunkar mentioned this pull request Dec 14, 2023

feat: update to latest noir and update noir compiler AztecProtocol/aztec-packages#3696

Merged

kevaundray mentioned this pull request Dec 15, 2023

chore: Release Noir(0.21.0) #3824

Merged

Thunkar mentioned this pull request Dec 20, 2023

feat: Add dependency resolver for noir_wasm and implement FileManager for consistency with native interface #3891

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: Make file manager read-only to the compiler #3760

chore!: Make file manager read-only to the compiler #3760

kevaundray commented Dec 10, 2023

kevaundray commented Dec 10, 2023

kevaundray commented Dec 10, 2023 •

edited

Loading

kevaundray commented Dec 10, 2023

kevaundray commented Dec 10, 2023

TomAFrench commented Dec 13, 2023

kevaundray commented Dec 13, 2023

kevaundray commented Dec 13, 2023

kevaundray commented Dec 13, 2023

alexghr commented Dec 13, 2023

chore!: Make file manager read-only to the compiler #3760

chore!: Make file manager read-only to the compiler #3760

Conversation

kevaundray commented Dec 10, 2023

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

kevaundray commented Dec 10, 2023

kevaundray commented Dec 10, 2023 • edited Loading

kevaundray commented Dec 10, 2023

kevaundray commented Dec 10, 2023

TomAFrench commented Dec 13, 2023

kevaundray commented Dec 13, 2023

kevaundray commented Dec 13, 2023

kevaundray commented Dec 13, 2023

alexghr commented Dec 13, 2023

kevaundray commented Dec 10, 2023 •

edited

Loading