Repository: rust-lang/rust-analyzer
Pull request #17019 (2024-04-05 22:52:04)

fix: VFS should not confuse paths with source roots that have the same prefix
Previously, the VFS would assign paths to the source root that had the longest string prefix match. This would break when we had source roots in subdirectories:

```
/foo
/foo/bar
```

Given a file `/foo/bar_baz.rs`, we would attribute it to the `/foo/bar` source root, which is wrong.

As a result, we would attribute paths to the wrong crate when a crate was in a subdirectory of another one. This is more common in larger monorepos, but could occur in any Rust project.

Fix this in the VFS, and add a test.

Veykril: Thanks!
@bors r+
bors: :pushpin: Commit 8885d3084bd0d6840ee385b0b2554bf1acfcb077 has been approved by `Veykril`

It is now in the [queue](https://bors.rust-lang.org/homu/queue/rust-analyzer) for this repository.

<!-- @bors r=Veykril 8885d3084bd0d6840ee385b0b2554bf1acfcb077 -->
<!-- homu: {"type":"Approved","sha":"8885d3084bd0d6840ee385b0b2554bf1acfcb077","approver":"Veykril","queue":"https://bors.rust-lang.org/homu/queue/rust-analyzer"} -->
bors: :hourglass: Testing commit 8885d3084bd0d6840ee385b0b2554bf1acfcb077 with merge 4d46cc53b276517ae8071f8ece5454a6ba2e06f1...
<!-- homu: {"type":"BuildStarted","head_sha":"8885d3084bd0d6840ee385b0b2554bf1acfcb077","merge_sha":"4d46cc53b276517ae8071f8ece5454a6ba2e06f1"} -->
bors: :broken_heart: Test failed - [checks-actions](https://github.com/rust-lang/rust-analyzer/actions/runs/8579593610/job/23514829511)
<!-- homu: {"type":"BuildFailed","builder_url":"https://github.com/rust-lang/rust-analyzer/actions/runs/8579593610/job/23514829511","builder_name":"checks-actions"} -->
Veykril: 🤔 why is this failing on merge only, judging from the commit parents this is a recent branch so there shouldn't be any semantic conflicts I think?
ShoyuVanilla: > 🤔 why is this failing on merge only, judging from the commit parents this is a recent branch so there shouldn't be any semantic conflicts I think?

According to CI log, the following tests fail on Windows

```
 failures:
    tests::ancestor_can_be_parent
    tests::ancestor_can_be_parent_2
    tests::basic_child_parent
    tests::basic_child_parent_with_unrelated_parents_sib
    tests::child_binds_ancestor_if_parent_nonlocal
    tests::non_locals_are_skipped
```
and it seems that CI runs Windows tests on merges, but not on PRs, so it fails on merge only.

https://github.com/rust-lang/rust-analyzer/blob/7a8374c162c64c17e865b98aad282d16b16e96d6/.github/workflows/ci.yaml#L93-L95

Wilfred: Aha, there is a `.parent()` method, so I've updated the PR to use that instead. Let me know what you think.

Patch:
diff --git a/crates/vfs/src/file_set.rs b/crates/vfs/src/file_set.rs
index 7eeb10d544..d7d283c3eb 100644
--- a/crates/vfs/src/file_set.rs
+++ b/crates/vfs/src/file_set.rs
@@ -132,6 +132,10 @@ impl FileSetConfig {
     ///
     /// `scratch_space` is used as a buffer and will be entirely replaced.
     fn classify(&self, path: &VfsPath, scratch_space: &mut Vec<u8>) -> usize {
+        // `path` is a file, but r-a only cares about the containing directory. We don't
+        // want `/foo/bar_baz.rs` to be attributed to source root directory `/foo/bar`.
+        let path = path.parent().unwrap_or_else(|| path.clone());
+
         scratch_space.clear();
         path.encode(scratch_space);
         let automaton = PrefixOf::new(scratch_space.as_slice());


Test patch:
diff --git a/crates/vfs/src/file_set/tests.rs b/crates/vfs/src/file_set/tests.rs
index 2146df185d..3cdb60dcb2 100644
--- a/crates/vfs/src/file_set/tests.rs
+++ b/crates/vfs/src/file_set/tests.rs
@@ -40,3 +40,26 @@ fn name_prefix() {
     let partition = file_set.partition(&vfs).into_iter().map(|it| it.len()).collect::<Vec<_>>();
     assert_eq!(partition, vec![1, 1, 0]);
 }
+
+/// Ensure that we don't consider `/foo/bar_baz.rs` to be in the
+/// `/foo/bar/` root.
+#[test]
+fn name_prefix_partially_matches() {
+    let mut file_set = FileSetConfig::builder();
+    file_set.add_file_set(vec![VfsPath::new_virtual_path("/foo".into())]);
+    file_set.add_file_set(vec![VfsPath::new_virtual_path("/foo/bar".into())]);
+    let file_set = file_set.build();
+
+    let mut vfs = Vfs::default();
+
+    // These two are both in /foo.
+    vfs.set_file_contents(VfsPath::new_virtual_path("/foo/lib.rs".into()), Some(Vec::new()));
+    vfs.set_file_contents(VfsPath::new_virtual_path("/foo/bar_baz.rs".into()), Some(Vec::new()));
+
+    // Only this file is in /foo/bar.
+    vfs.set_file_contents(VfsPath::new_virtual_path("/foo/bar/biz.rs".into()), Some(Vec::new()));
+
+    let partition = file_set.partition(&vfs).into_iter().map(|it| it.len()).collect::<Vec<_>>();
+
+    assert_eq!(partition, vec![2, 1, 0]);
+}