Skip to content

Canonicalize rust-project.json path for determining the project root #14168

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

Closed
wants to merge 1 commit into from
Closed

Canonicalize rust-project.json path for determining the project root #14168

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 @@ -179,6 +179,11 @@ impl AbsPath {
self.0.ends_with(&suffix.0)
}

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

// region:delegate-methods

// Note that we deliberately don't implement `Deref<Target = Path>` here.
Expand Down
3 changes: 2 additions & 1 deletion crates/project-model/src/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,14 @@ impl ProjectWorkspace {
) -> Result<ProjectWorkspace> {
let res = match manifest {
ProjectManifest::ProjectJson(project_json) => {
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())
})?;
let data = serde_json::from_str(&file).with_context(|| {
format!("Failed to deserialize json file {}", project_json.display())
})?;
let project_location = project_json.parent().to_path_buf();
let project_location = project_json.parent().unwrap().to_path_buf();
Copy link
Author

Choose a reason for hiding this comment

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

Could potentially move the canonicalization to ManifestPath to avoid the unwrap() here, or do the canonicalization in every case inside ProjectJson::new(), or do the canonicalization already when creating the ProjectManifest. Many options :)

Copy link
Author

Choose a reason for hiding this comment

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

Or alternatively the rust-project.json could have absolute paths generated by meson for root_module and others but not sure that's allowed?

Copy link
Member

Choose a reason for hiding this comment

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

Absolute paths should work fine for root_module and friends. Not sure if canonicalizing is the right behavior we want here or not.

Copy link
Author

Choose a reason for hiding this comment

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

As relative paths can be used in rust-project.json it would make sense to at least resolve any symlinks for getting the real path of the file.

I'll also submit a PR to meson for writing absolute paths in there though.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't resolving symlinks when loading rust-project.json break when the editor doesn't resolve symlinks before making lsp requests?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know how that works in detail, but with nvim/coc it works fine this way

let project_json = ProjectJson::new(&project_location, data);
ProjectWorkspace::load_inline(
project_json,
Expand Down