Skip to content

fix: Canonicalize rust-project.json manifest path #14430

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 29, 2023
Merged

fix: Canonicalize rust-project.json manifest path #14430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/paths/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,11 @@ impl AbsPath {
AbsPathBuf::try_from(self.0.to_path_buf()).unwrap()
}

/// Equivalent of [`Path::canonicalize`] for `AbsPath`.
pub fn canonicalize(&self) -> Result<AbsPathBuf, std::io::Error> {
Ok(self.as_ref().canonicalize()?.try_into().unwrap())
}

/// Equivalent of [`Path::strip_prefix`] for `AbsPath`.
///
/// Returns a relative path.
Expand Down
5 changes: 5 additions & 0 deletions crates/project-model/src/manifest_path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ impl ManifestPath {
pub fn parent(&self) -> &AbsPath {
self.file.parent().unwrap()
}

/// Equivalent of [`Path::canonicalize`] for `ManifestPath`.
pub fn canonicalize(&self) -> Result<ManifestPath, std::io::Error> {
Ok((&**self).canonicalize()?.try_into().unwrap())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/subdir

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

impl ops::Deref for ManifestPath {
Expand Down
9 changes: 2 additions & 7 deletions crates/project-model/src/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use std::{collections::VecDeque, fmt, fs, process::Command, sync::Arc};

use anyhow::{bail, format_err, Context, Result};
use anyhow::{format_err, Context, Result};
use base_db::{
CrateDisplayName, CrateGraph, CrateId, CrateName, CrateOrigin, Dependency, Edition, Env,
FileId, LangCrateOrigin, ProcMacroPaths, TargetLayoutLoadResult,
Expand Down Expand Up @@ -154,12 +154,7 @@ impl ProjectWorkspace {
) -> Result<ProjectWorkspace> {
let res = match manifest {
ProjectManifest::ProjectJson(project_json) => {
let metadata = fs::symlink_metadata(&project_json).with_context(|| {
format!("Failed to read json file {}", project_json.display())
})?;
if metadata.is_symlink() {
bail!("The project-json may not currently point to a symlink");
}
let project_json = project_json.canonicalize()?;
let file = fs::read_to_string(&project_json).with_context(|| {
format!("Failed to read json file {}", project_json.display())
})?;
Expand Down