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

Use correct working directory for non-workspace proc-macro execution #19151

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

infiniteregrets
Copy link

@infiniteregrets infiniteregrets commented Feb 12, 2025

No description provided.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2025
@bjorn3
Copy link
Member

bjorn3 commented Feb 13, 2025

The workspace root of the crate for which we are expanding macros should be used as current_dir. For dependencies from crates.io this matches the directory of the dependency itself. And same for local dependencies outside of the workspace I think.

@infiniteregrets
Copy link
Author

Hi @bjorn3 Isnt this the case when we use

let current_dir = env.get("CARGO_RUSTC_CURRENT_DIR").or_else(|| env.get("CARGO_MANIFEST_DIR"));

Unless I am missing something please correct me! Additonally, I was trying to figure how I can change the proc_macro_cwd to be exactly reflect what you just mentioned

@Veykril
Copy link
Member

Veykril commented Feb 14, 2025

CARGO_RUSTC_CURRENT_DIR is no longer being set in rust-analyzer (as the env var is also likely not going to be stabilized within cargo anymore). CARGO_MANIFEST_DIR I believe won't be right in the workspace case. Either way we shouldn't be extracting the working directory from the env vars.

@infiniteregrets
Copy link
Author

@Veykril I agree with you for not using env, I was playing around with the snippet here rust-analyzer/crates/rust-analyzer/src/reload.rs however I haven't had any luck extracting the current_dir for the crates when being set in the mapping based on the current implementation, what do you think should be done here?

@Veykril
Copy link
Member

Veykril commented Feb 14, 2025

Assuming bjorn's comment is correct we'll have to move the proc_macro_cwd field from CrateWorkspaceData into CrateData and populate that appropriately when creating the CrateGraph for a workspace.

@rustbot rustbot added has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 15, 2025
@rustbot

This comment has been minimized.

@rustbot rustbot removed has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 15, 2025
@infiniteregrets infiniteregrets force-pushed the infi/fix-proc-macro branch 2 times, most recently from c1a9c43 to a4771a2 Compare February 15, 2025 21:49
crates/base-db/src/input.rs Outdated Show resolved Hide resolved
crates/project-model/src/project_json.rs Outdated Show resolved Hide resolved
@@ -362,6 +364,8 @@ struct CrateData {
repository: Option<String>,
#[serde(default)]
build: Option<BuildData>,
#[serde(default)]
cwd: Option<Utf8PathBuf>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cwd: Option<Utf8PathBuf>,
proc_macro_cwd: Option<Utf8PathBuf>,

@davidbarsky for input here, unsure if we need to model this in the project json payload or not but I don't think we can derive the correct one from the data.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for rust-project.json payloads, we should be conservative and mimic what Cargo does, but I'm not 100% sure.

crates/project-model/src/workspace.rs Outdated Show resolved Hide resolved
crates/project-model/src/workspace.rs Outdated Show resolved Hide resolved
@infiniteregrets infiniteregrets force-pushed the infi/fix-proc-macro branch 2 times, most recently from 7ad8987 to 94cf6fa Compare February 16, 2025 16:31
@Veykril Veykril changed the title (revert c2258d8) Use cargo env instead of crate_workspace_data for procmacro expansion Use correct working directory for non-workspace proc-macro execution Feb 17, 2025
@Veykril
Copy link
Member

Veykril commented Feb 17, 2025

test outputs need updating

@infiniteregrets infiniteregrets force-pushed the infi/fix-proc-macro branch 2 times, most recently from 8bd4bc7 to 4b0895e Compare February 17, 2025 15:30
@infiniteregrets
Copy link
Author

@Veykril updated, lmk if theres anything else missing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants