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.

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

Regression: Semantics::file_to_module_def sometimes returns no result on Windows DOS device paths #18894

Open
redsun82 opened this issue Jan 9, 2025 · 2 comments
Labels
C-bug Category: bug

Comments

@redsun82
Copy link

redsun82 commented Jan 9, 2025

When using rust-analyzer as a library for analysis, rust analyzer has started to flakily misbehave on Windows paths. This was first seen in github/codeql#18443, but I've managed to reduce the issue in this repository.

What I do is to load a project contained in a workspace using ra_ap_load-cargo::load_workspace_at with default settings, then starting from a path to a module in the crate do some dance to map it to a file ID in the VFS (going from PathBuf to Utf8PathBuf, then to AbsPathBuf, then to VfsPath), and then call Semantics::file_to_module_def. This should work, but in some cases fails on Windows if the initial path is a DOS device path (i.e. //?/ prefixed paths), which is what PathBuf::canonicalize will return on Windows.

This can be seen running this workflow.

Some things I've noticed

  • everything works on Linux
  • this used to work in version 0.0.248
  • failure is flaky, but quite frequent
  • a workaround is to use dunce for canonicalization, but that will not work for long paths (more than 256 chars in general)

rust-analyzer version: 0.0.257

rustc version: 1.83

editor or extension: none (using rust-anlyzer as a library)

relevant settings: (eg. client settings, or environment variables like CARGO, RUSTC, RUSTUP_HOME or CARGO_HOME)

repository link (if public, optional): https://github.com/redsun82/rust-analyzer-windows-issue

code snippet to reproduce: see https://github.com/redsun82/rust-analyzer-windows-issue/blob/main/tests/load.rs, failure can be seen on windows running cargo test multiple times

@redsun82 redsun82 added the C-bug Category: bug label Jan 9, 2025
@Veykril
Copy link
Member

Veykril commented Jan 9, 2025

Ths VFS assumes uncanonicalized paths (or rather, non DOS device paths on windows) so this failing doesn't suprise me, I am more surprised that this supposedly used to work as I don't recall us every changing that behavior recently

@redsun82
Copy link
Author

redsun82 commented Jan 9, 2025

@Veykril thanks for your reply: now that I looked into it a bit more closely after your message, I understand more what changed between 0.0.248 and 0.0.257.

With 0.0.248 the code sample I have will fail at the VFS level, in the path -> file ID mapping (i.e. vfs.file_id returns None). In our code we have a workaround for precisely that.

Now however (in at least 0.0.256 and 0.0.257 as far as I have tested), the path -> file ID mapping succeeds, and the failure comes later, not in the VFS layer which will happily give a file ID, of which however the HIR layer may not know anything about (and I intermittently get a failure from semantics.file_to_module_id).

We can make sure to use dunce::canonicalize instead of PathBuf::canonicalize. I do suspect that will not work with long paths...

redsun82 pushed a commit to github/codeql that referenced this issue Jan 9, 2025
Rust-analyzer turned out to be quite picky about paths, where
`//?/`-prefixed paths can lead to flaky failures. See

rust-lang/rust-analyzer#18894

for details.

This makes paths always be canonicalized with `dunce`. Previously,
`dunce` was used as a fallback, but that stopped working somewhere
after version 0.0.248 of rust-analyzer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

2 participants