fix: VFS should not confuse paths with source roots that have the same prefix

[Wilfred]

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]

📌 Commit 8885d30 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8885d30 with merge 4d46cc5...

[bors]

💔 Test failed - 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.

rust-analyzer/.github/workflows/ci.yaml

Lines 93 to 95 in 7a8374c

	- name: Test
	if: matrix.os == 'ubuntu-latest' || github.event_name == 'push'
	run: cargo test ${{ env.USE_SYSROOT_ABI }} -- --nocapture --quiet

[Wilfred]

Yeah, I wondered whether I should be unconditionally appending / or use the platform-specific path separator.

There doesn't seem to be a .parent() method on VfsPath, it's apparently an opaque type.

@ShoyuVanilla huh, that seems unfortunate. Maybe PRs should also run Windows tests?

[Wilfred]

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

[ShoyuVanilla]

huh, that seems unfortunate. Maybe PRs should also run Windows tests?

Though I don't know about the r-a CI policy but in my naive opinion, running tests for each major platforms would be good for cases like filesystem or environment variables level changes like in this issue and running it wouldn't slow down CI much because each platform CI is done in different instances.

[Wilfred]

@Veykril I think this PR is good to merge now, it's using .parent() which should do the right thing on Windows too.

[Veykril]

Thanks!
@bors r+

[bors]

📌 Commit b03844d has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit b03844d with merge 773b4a5...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 773b4a5 to master...

[bill-myers]

Is this enough? Seems to me this will still result in /foo/bar_baz/quux.rs being attributed to /foo/bar rather than /foo.

The correct solution is of course to add a final slash to prefixes, and add a final slash to the string to search as well if matching the prefix itself is needed.

And of course normalize separators and UNC long paths beforehand if running on Windows.