Skip to content

Commit

Permalink
fix: (LSP) only add cached files relevant to workspace (#5775)
Browse files Browse the repository at this point in the history
# Description

## Problem

Resolves #5774

## Summary

LSP caches open files so that features work relative to open,
potentially unsaved, file buffers. Before analyzing a crate or
processing a package, we add these open files to a FileManager. The
problem was that we were adding all open files, regardless of whether
they belonged to a package. Because we don't always call `check_crate`
on LSP features (for example when you hover we collect a packages files
but don't call `check_crate`) a packages file FileIDs would change if we
added new files that didn't exist before in that package.

It's... kind of hard to explain. But it also makes sense to not add
files that are not relevant to the package. I debugged it for hours
until I found the reason. With this fix LSP should work really well now
(it wasn't working that well in Aztec-Packages).

## Additional Context

None.

## Documentation

Check one:
- [x] No documentation needed.
- [ ] 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.
  • Loading branch information
asterite authored Aug 20, 2024
1 parent 423e601 commit 1958a79
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 10 deletions.
13 changes: 8 additions & 5 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,15 +387,18 @@ pub fn insert_all_files_for_workspace_into_file_manager(
workspace: &Workspace,
file_manager: &mut FileManager,
) {
// First add files we cached: these have the source code of files that are modified
// but not saved to disk yet, and we want to make sure all LSP features work well
// according to these unsaved buffers, not what's saved on disk.
// Source code for files we cached override those that are read from disk.
let mut overrides: HashMap<&Path, &str> = HashMap::new();
for (path, source) in &state.input_files {
let path = path.strip_prefix("file://").unwrap();
file_manager.add_file_with_source_canonical_path(Path::new(path), source.clone());
overrides.insert(Path::new(path), source);
}

nargo::insert_all_files_for_workspace_into_file_manager(workspace, file_manager);
nargo::insert_all_files_for_workspace_into_file_manager_with_overrides(
workspace,
file_manager,
&overrides,
);
}

#[test]
Expand Down
31 changes: 26 additions & 5 deletions tooling/nargo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub mod ops;
pub mod package;
pub mod workspace;

use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashMap};

use fm::{FileManager, FILE_EXTENSION};
use noirc_driver::{add_dep, prepare_crate, prepare_dependency};
Expand Down Expand Up @@ -45,9 +45,21 @@ pub fn prepare_dependencies(
pub fn insert_all_files_for_workspace_into_file_manager(
workspace: &workspace::Workspace,
file_manager: &mut FileManager,
) {
insert_all_files_for_workspace_into_file_manager_with_overrides(
workspace,
file_manager,
&HashMap::new(),
);
}

pub fn insert_all_files_for_workspace_into_file_manager_with_overrides(
workspace: &workspace::Workspace,
file_manager: &mut FileManager,
overrides: &HashMap<&std::path::Path, &str>,
) {
for package in workspace.clone().into_iter() {
insert_all_files_for_package_into_file_manager(package, file_manager);
insert_all_files_for_package_into_file_manager(package, file_manager, overrides);
}
}
// We will pre-populate the file manager with all the files in the package
Expand All @@ -59,6 +71,7 @@ pub fn insert_all_files_for_workspace_into_file_manager(
fn insert_all_files_for_package_into_file_manager(
package: &Package,
file_manager: &mut FileManager,
overrides: &HashMap<&std::path::Path, &str>,
) {
// Start off at the entry path and read all files in the parent directory.
let entry_path_parent = package
Expand All @@ -70,8 +83,12 @@ fn insert_all_files_for_package_into_file_manager(
let paths = get_all_noir_source_in_dir(entry_path_parent)
.expect("could not get all paths in the package");
for path in paths {
let source = std::fs::read_to_string(path.as_path())
.unwrap_or_else(|_| panic!("could not read file {:?} into string", path));
let source = if let Some(src) = overrides.get(path.as_path()) {
src.to_string()
} else {
std::fs::read_to_string(path.as_path())
.unwrap_or_else(|_| panic!("could not read file {:?} into string", path))
};
file_manager.add_file_with_source(path.as_path(), source);
}

Expand All @@ -87,7 +104,11 @@ fn insert_all_files_for_packages_dependencies_into_file_manager(
for (_, dep) in package.dependencies.iter() {
match dep {
Dependency::Local { package } | Dependency::Remote { package } => {
insert_all_files_for_package_into_file_manager(package, file_manager);
insert_all_files_for_package_into_file_manager(
package,
file_manager,
&HashMap::new(),
);
insert_all_files_for_packages_dependencies_into_file_manager(package, file_manager);
}
}
Expand Down

0 comments on commit 1958a79

Please sign in to comment.