Skip to content

fix: better way to do LSP file overrides#8702

Merged
jfecher merged 9 commits intomasterfrom
ab/lsp-file-overrides
May 29, 2025
Merged

fix: better way to do LSP file overrides#8702
jfecher merged 9 commits intomasterfrom
ab/lsp-file-overrides

Conversation

@asterite
Copy link
Collaborator

@asterite asterite commented May 28, 2025

Description

Problem

Resolves #8670

Summary

When you open a Noir file in LSP, and whenever you edit it, we cache that file's path and source code. Then when building a FileManager we pass these files as "overrides" so their contents are read from this cache instead of the filesystem. We do this so that even if you don't save a file, code that references that unsaved content works fine. This is how Rust Analyzer seem to work too (I'm not sure how they implemented it, though, but the exhibited behavior is that).

For overrides, what we used to do before this PR is traversing all files in the filesystem for the relevant packages and adding them to the FileManager, except that if there's an override for a path we'd use that override's source code. That works fine except when a file is deleted from the filesystem. In that case the override won't be added because we never find that path in the filesystem to be able to override it!

Initially I thought we could add the overrides at the end, at least those that we didn't find in the filesystem. The problem with that is that the override's FileID will end up being different than the one before the file was deleted, so things like "Go to definition", etc., which rely on FileIDs won't work well.

An alternative approach, which is what this PR does, is to first compute all the filenames that need to go into the FileManager. Those come from the filesystem, but also from the overrides. Then, to have a consistent FileID order, we can sort all of these filenames, and just then add them to the FileManager (considering overrides here too).

That said, this is only done if there are overrides specified, otherwise in a regular compilation the order will change on where the files are stored (their parent absolute path). When there are no overrides the order will be that given by the packages traversal and the filesystem directory entries.

Here's how it works in Rust when you delete a file but continue editing it. Notice how we can still jump to that file from another file, and even if you add a new definition to the deleted file, it will be suggested in other files:

lsp-rust

And now it works the same way in Noir LSP:

lsp-noir

Additional Context

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] 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.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 940d829 Previous: 9b73209 Ratio
test_report_noir-lang_noir_bigcurve_ 284 s 230 s 1.23

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@asterite
Copy link
Collaborator Author

Hmm... the order of the files will change depending on the absolute filename they have.

@asterite asterite requested a review from a team May 29, 2025 11:46
@jfecher jfecher added this pull request to the merge queue May 29, 2025
Merged via the queue into master with commit 89783b6 May 29, 2025
118 checks passed
@jfecher jfecher deleted the ab/lsp-file-overrides branch May 29, 2025 15:30
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 1, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(fmt): correctly format mixed secondary attributes and doc comments
(noir-lang/noir#8735)
fix!: require types for trait impl associated constants
(noir-lang/noir#8734)
fix!: Prevent returning references from if expressions
(noir-lang/noir#8731)
fix: cast signed to u1 follow-up
(noir-lang/noir#8730)
fix: cast signed to u1 (noir-lang/noir#8720)
fix: do not mutate arrays later copied inside other arrays
(noir-lang/noir#8701)
chore: box `AsTraitPath` and `TypePath` in `ExpressionKind`
(noir-lang/noir#8716)
fix: general solution for accessing associated constants
(noir-lang/noir#8417)
fix(ssa): Validate checked signed add/sub is followed by a truncate
(noir-lang/noir#8706)
chore: add pre- and post-check on `array_set` optimization pass
(noir-lang/noir#8714)
fix: merge expr bindings with instantiations bindings during
monomorphization (noir-lang/noir#8713)
fix: better way to do LSP file overrides
(noir-lang/noir#8702)
fix(fmt): correct indentation when formatting long struct patterns
(noir-lang/noir#8711)
fix(fuzz): Prevent breaking/continuing out from let blocks in the AST
fuzzer (noir-lang/noir#8708)
chore: remove override for zero length inputs
(noir-lang/noir#8709)
feat: Replace converging jmpif with empty block destinations with a
single jmp (noir-lang/noir#8613)
feat(cli): Add `nargo interpret` command
(noir-lang/noir#8700)
chore: more 1-tuple printing fixes
(noir-lang/noir#8699)
chore(fuzz): Fuzz the SSA parser
(noir-lang/noir#8647)
fix: (SSA parser) translate blocks in logical order rather than syntax
order (noir-lang/noir#8668)
fix(SSA): show and parse range_check's assert_message
(noir-lang/noir#8652)
chore: Show the step number in the SSA message
(noir-lang/noir#8698)
chore(docs): Add pointers to tests
(noir-lang/noir#8695)
chore(fuzz): Capture comptime print output
(noir-lang/noir#8635)
fix: nargo expand reexports correctly implemented
(noir-lang/noir#8693)
feat(cli): Show multiple SSA passes
(noir-lang/noir#8692)
chore(test): Add regression test for defunctionalize
(noir-lang/noir#8691)
chore: add an assertion when parsing SSA that all functions are
well-formed (noir-lang/noir#8671)
feat!: prevent compiling blake3 hashes which barretenberg cannot prove
(noir-lang/noir#8690)
fix(ssa): Remove the array cache from the function inserter
(noir-lang/noir#8607)
chore: bump external pinned commits
(noir-lang/noir#8684)
chore: reenable fuzzing tests in CI
(noir-lang/noir#8688)
fix: Handle `&mut function` in defunctionalize
(noir-lang/noir#8665)
fix(defunctionalize): Higher order functions (HOF) dynamic dispatch and
HOFs in arrays (noir-lang/noir#8672)
chore(ci): avoid running fuzzer tests in the merge queue
(noir-lang/noir#8664)
fix: Make casts in `comptime` consistent with runtime casts
(noir-lang/noir#8669)
fix: relax connectedness requirement on purity analysis pass
(noir-lang/noir#8667)
fix: always use `u32` for indexing arrays in SSA
(noir-lang/noir#8633)
chore: explicitly pull from `next` branch in aztec-packages
(noir-lang/noir#8660)
chore(fuzz): Build the dictionary from the SSA
(noir-lang/noir#8591)
chore(docs): Remove old versioned docs
(noir-lang/noir#8061)
chore: bump external pinned commits
(noir-lang/noir#8634)
fix(SSA): don't use string literal if byte is "form feed" ('\f')
(noir-lang/noir#8653)
chore(defunctionalize): Add regression test for missing lambda variants
panic (noir-lang/noir#8642)
chore(ci): Do not run ast_fuzzer orig vs. morph in ci
(noir-lang/noir#8646)
fix: disable underflow fix for fields
(noir-lang/noir#8631)
fix: revert "fix: error on unused generic in trait impl
(noir-lang/noir#8395)"
(noir-lang/noir#8636)
fix(ssa interpreter): Default to zero when we have an overflowing shl
(noir-lang/noir#8638)
fix: ensure that purity analysis pass explores all functions
(noir-lang/noir#8452)
feat: acir_formal_proofs (noir-lang/noir#8140)
fix: error on unused generic in trait impl
(noir-lang/noir#8395)
fix(expand): use re-exports for non-visibile items
(noir-lang/noir#8374)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LSP: Crash after switching branches where file doesn't exist

2 participants