Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(lsp): Ensure lsp does not crawl past the root specified #2322

Merged
merged 5 commits into from
Aug 15, 2023
Merged

fix(lsp): Ensure lsp does not crawl past the root specified #2322

merged 5 commits into from
Aug 15, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented Aug 15, 2023

Description

Problem*

Resolves #2261

Summary*

This reworks the crawling for Nargo.toml files. When searching for the program_dir, we still search to the root using find_package_root; however, find_package_manifest requires a root where we stop the crawling. Additionally, we now read the <program_dir>/Nargo.toml exactly instead of traversing.

These changes allowed me to fix a problem with the LSP where it would crawl past the root_uri and find a Nargo.toml outside of the directory you have opened.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

@phated phated requested review from TomAFrench and kobyhallx August 15, 2023 15:14
TomAFrench
TomAFrench reviewed Aug 15, 2023
View reviewed changes
crates/nargo_toml/src/lib.rs Outdated Show resolved Hide resolved
crates/lsp/src/lib.rs Show resolved Hide resolved
crates/nargo_toml/src/lib.rs Outdated Show resolved Hide resolved
crates/nargo_toml/src/lib.rs Show resolved Hide resolved
phated and others added 2 commits August 15, 2023 09:11
@phated
pr review
b0dc9d2
@phated @TomAFrench
Update crates/nargo_toml/src/lib.rs
dcb8b03 
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
TomAFrench
TomAFrench reviewed Aug 15, 2023
View reviewed changes
Copy link
Member

@TomAFrench TomAFrench left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I just want to check that we're not creating an additional need for us to make paths absolute in order to find the package manifest that we haven't accounted for elsewhere.

crates/nargo_toml/src/lib.rs Outdated Show resolved Hide resolved
crates/nargo_toml/src/lib.rs Outdated Show resolved Hide resolved
crates/nargo_toml/src/lib.rs Show resolved Hide resolved
TomAFrench
TomAFrench requested changes Aug 15, 2023
View reviewed changes
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently panics if you do nargo --program-dir ./foo compile but if you do nargo --program-dir /absolute/path/to/foo compile then we succeed.

We should normalize the paths we're passing into these functions to avoid this

@TomAFrench TomAFrench self-requested a review August 15, 2023 16:27
@TomAFrench
Copy link
Member

Oof, looks like this is on master actually.

@phated
Copy link
Contributor Author

phated commented Aug 15, 2023

Oof, looks like this is on master actually.

Yeah, I didn't change the way that program-dir was searched. I don't think it ever supported relative paths

@TomAFrench TomAFrench mentioned this pull request Aug 15, 2023
Merged
5 tasks
@TomAFrench
Copy link
Member

I've addresses this panic in #2324

@TomAFrench
Merge branch 'master' into phated/nargo-toml-crawl
7045f0f 
* master:
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
@TomAFrench TomAFrench enabled auto-merge August 15, 2023 17:52
@TomAFrench TomAFrench added this pull request to the merge queue Aug 15, 2023
Merged via the queue into master with commit d69e372 Aug 15, 2023
@TomAFrench TomAFrench deleted the phated/nargo-toml-crawl branch August 15, 2023 18:18
@kevaundray kevaundray mentioned this pull request Aug 15, 2023
Merged
TomAFrench added a commit that referenced this pull request Aug 15, 2023
@TomAFrench
Merge branch 'master' into intrinsic-die
70aaf04 
* master: (26 commits)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  ...
TomAFrench added a commit that referenced this pull request Aug 16, 2023
@TomAFrench
Merge branch 'master' into assert-eq-builtin
9f60f3c 
* master: (34 commits)
  chore: move orphaned integration tests to new directory (#2331)
  chore(noir): Release 0.10.1 (#2328)
  feat(ssa): Switch mem2reg pass to be per function rather than per block (#2243)
  feat(ssa): Perform dead instruction elimination on intrinsic functions (#2276)
  feat: Add full call stacks to runtime errors (#2310)
  chore(ci): fix mismatched input name to publish workflow (#2327)
  chore: add README for integration test structure (#2277)
  feat: Improved error message for unexpected return type (#2302)
  feat(stdlib): Implement `str` `as_bytes` and `into_bytes` function (#2298)
  chore(ci): automatically convert changelog entries to sentence case (#2325)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobyhallx kobyhallx kobyhallx left review comments

@TomAFrench TomAFrench TomAFrench approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Searching for Nargo.toml should not crawl past root
3 participants
@phated @TomAFrench @kobyhallx