fix: Canonicalize rust-project.json manifest path #14430
Conversation
rustbot
added
the
S-waiting-on-review
|
@bors r+ |
|
☀️ Test successful - checks-actions |
1 similar comment
|
☀️ Test successful - checks-actions |
|
👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request. |
|
This might have Possibly™️ broken our usage in Rustlings - see rust-lang/rustlings#1462 |
|
|
||
| /// Equivalent of [`Path::canonicalize`] for `ManifestPath`. | ||
| pub fn canonicalize(&self) -> Result<ManifestPath, std::io::Error> { | ||
| Ok((&**self).canonicalize()?.try_into().unwrap()) |
There was a problem hiding this comment.
So one thing I've learned from alexchrichton is that path canonicalization is a very fragile thing. It is not particularly well-defined operation, and, in practice, can create broken paths on windows.
The rule-of-thumb for robust software is to use user-supplied paths as is.
If the user gives you foo/bar/../baz/quux, you don't ask them why, but rather just relay the path as is to the operating systems. User believes that this particular path works, but any alias of this path might as well be broken.
Looking at #14168, I think I disagree that rust-analyzer needs any specific treatment for symlinks. If there's ./foo/rust-project.json, and it mentions ./src/lib.rs, then rust-analyzer should look into ./foo/src/lib.rs regardless of whether ./foo/rust-project.json is a real file, symlink, hardlink, or a mirage summoned by a fuse file system.
Can we special case meson? If it always puts rust-project.json into a specific build directory, we can probe that path. Messon is a fairly popular tool. But, from reading the docs, it seems that there's no standard build directory, and each project can use whatever name?
I think for Cargo.toml we do two-level search, I guess we can do the same for messon?
There was a problem hiding this comment.
I think, eg, make treats symlinks this way:
19:34:58|~/tmp
λ exa -l
lrwxrwxrwx 17 matklad 3 May 19:25 Makefile -> ./subdir/Makefile
drwxr-xr-x - matklad 3 May 19:34 subdir
19:35:00|~/tmp
λ bat Makefile
.PHONY: path
path:
realpath .
pwd
19:35:03|~/tmp
λ make path
realpath .
/home/matklad/tmp
pwd
/home/matklad/tmp
19:35:06|~/tmp
λ cd subdir
19:35:09|~/tmp/subdir
λ make path
realpath .
/home/matklad/tmp/subdir
pwd
/home/matklad/tmp/subdirThere was a problem hiding this comment.
Agreed, after having looked at this (and the issues it caused, where for windows we get nice NT paths for no reason after canonicalizing), I'm also on the ship of not handling symlinks in any way. It is causing too much of a headache.
I believe meson currently works without it again (by emitting absolute paths), so we should be able to just revert this as is.
There was a problem hiding this comment.
So we should be able to just revert this as is.
Let's rather do
fn canonicalize(&self) -> ! {
panic!("We explicitly do not provide canonicalization API, as that is almost always a wrong solution, see #14430")
}
Also, some prior art here: https://stackoverflow.com/questions/72898205/clang-tidy-10-and-compile-commands-json-does-not-support-relative-paths
TL;DR: clang requires absolute paths
There was a problem hiding this comment.
Heh, I actually had the same idea of keeping the method with a panic behavior 😄
I think requiring absolute paths everywhere in rust-project.json would be a good idea in general. The format is meant to be generated by tooling anyways so the tool can always generate absolute paths instead.
There was a problem hiding this comment.
Can we special case meson? If it always puts
rust-project.jsoninto a specific build directory, we can probe that path. Messon is a fairly popular tool. But, from reading the docs, it seems that there's no standard build directory, and each project can use whatever name?
Well, tbh there's not precisely a standard build directory for cargo either. ;) Even target/ is not enforced, so external tools interacting with cargo would have to know how cargo was invoked as well as the global cargo config file to detect where the build directory is. It's common for people to have a builddir-debug/ and builddir-release/ profile for meson build directories.
I believe meson currently works without it again (by emitting absolute paths), so we should be able to just revert this as is.
Yes, this bugfix was released in meson 1.1.0 (mesonbuild/meson@36e2c86) and backported to 1.0.2 (mesonbuild/meson@6a58ef7), so rust-analyzer should not need to guess or special-case meson as long as "use a modern version of the toolchain" is an acceptable answer.
There was a problem hiding this comment.
What we aim for here is to "just work" in 90% of the cases. To work in 100% of the cases, the user can always specify config explicitly. So just a default name for a build directory would be enough here, it doesn't need to be one true name. But I guess just two level search would solve this as well.
There was a problem hiding this comment.
I suspect in the majority of cases, given a project ./, checking for ./*/rust-project.json and accepting it if the glob only expands to a single name, will work well.
internal: Forbid canonicalization of paths and normalize all rust-project.json paths Closes #14728 cc #14430 - Removes canonicalization (and forbids it from being used in a sense, clippy could help here again with its lint in the future) - Normalizes all paths in rust-project.json which we weren't doing in some cases
Looked a bit more into this and I think we can do this after all, I don't see any place where this should break things
cc #14168 #14402 (comment)