fix: allow new, subsequent rust-project.json-based workspaces to get proc macro expansion
#14427
Conversation
proc macro expansion.
rustbot
added
the
S-waiting-on-review
Correction: this is incorrect; I've reverted my changes for this. |
crates/rust-analyzer/src/reload.rs
Outdated
| // workspaces to remain in the low single digits. the `cloned_workspace` is needed for borrowck | ||
| // reasons. | ||
| let cloned_workspaces = workspaces.clone(); | ||
| let different_workspaces = cloned_workspaces |
There was a problem hiding this comment.
I can undo these changes and go back to the previous approach, since the reason I even did this proved to be wrong 😅
There was a problem hiding this comment.
We don't even need the checks for whether the new workspaces are rust-project.json ones or not, we need respawn the proc-macro servers in general when !same_workspaces. (Think of when someone adds a new top level cargo project)
There was a problem hiding this comment.
Oh, good point. Didn't consider that; will add!
crates/rust-analyzer/src/reload.rs
Outdated
| // workspaces to remain in the low single digits. the `cloned_workspace` is needed for borrowck | ||
| // reasons. | ||
| let cloned_workspaces = workspaces.clone(); | ||
| let different_workspaces = cloned_workspaces |
There was a problem hiding this comment.
We don't even need the checks for whether the new workspaces are rust-project.json ones or not, we need respawn the proc-macro servers in general when !same_workspaces. (Think of when someone adds a new top level cargo project)
crates/rust-analyzer/src/reload.rs
Outdated
| // | ||
| // The else branch is used to provide better diagnostics to users while procedural macros | ||
| // are still being built. | ||
| if same_workspaces || different_workspaces.iter().any(|ws| ws.is_json()) { |
There was a problem hiding this comment.
This also still doesn't fix the case where build scripts are disabled and rust-project.json are used without loading up workspaces after the start. I think I overcomplicated this part here, we should just replace this entire block of lines (477-490) with
if self.config.expand_proc_macros() {
self.fetch_proc_macros_queue.request_op(cause, proc_macro_paths);
}And set up the proc-macros errors when building up the crate graph, that is change
rust-analyzer/crates/base-db/src/input.rs
Line 20 in e7337fc
to
pub type ProcMacroPaths = FxHashMap<CrateId, Result<(Option<String>, AbsPathBuf), String>>; and fix up where we set those up.
There was a problem hiding this comment.
Currently we signal with Option::None that proc-macros haven't been build but this overcomplicates things by pushing the error generation to a later place, where we don't really have the information anymore.
|
I think I addressed your comments. Thanks for your help (and I'm sorry that most of my changes are to this part of the codebase! |
That is completely fine! By now I know this part of the codebase pretty well :) Changes look good to me 👍 |
…paces-to-have-proc-macros, r=Veykril fix: allow new, subsequent `rust-project.json`-based workspaces to get proc macro expansion As detailed in #14417 (comment), `rust-project.json` workspaces added after the initial `rust-project.json`-based workspace was already indexed by rust-analyzer would not receive procedural macro expansion despite `config.expand_proc_macros` returning true. To fix this issue: 1. I changed `reload.rs` to check which workspaces are newly added. 2. Spawned new procedural macro expansion servers based on the _new_ workspaces. 1. This is to prevent spawning duplicate procedural macro expansion servers for already existing workspaces. While the overall memory usage of duplicate procedural macro servers is minimal, this is more about the _principle_ of not leaking processes 😅. 3. Launched procedural macro expansion if any workspaces are `rust-project.json`-based _or_ `same_workspaces` is true. `same_workspaces` being true (and reachable) indicates that that build scripts have finished building (in Cargo-based projects), while the build scripts in `rust-project.json`-based projects have _already been built_ by the build system that produced the `rust-project.json`. I couldn't really think of structuring this code in a better way without engaging with #7444.
|
💔 Test failed - checks-actions |
|
@bors retry |
|
☀️ 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. |
davidbarsky
deleted the
davidbarsky/allow-subsequent-workspaces-to-have-proc-macros
branch
As detailed in #14417 (comment),
rust-project.jsonworkspaces added after the initialrust-project.json-based workspace was already indexed by rust-analyzer would not receive procedural macro expansion despiteconfig.expand_proc_macrosreturning true. To fix this issue:reload.rsto check which workspaces are newly added.rust-project.json-based orsame_workspacesis true.same_workspacesbeing true (and reachable) indicates that that build scripts have finished building (in Cargo-based projects), while the build scripts inrust-project.json-based projects have already been built by the build system that produced therust-project.json.I couldn't really think of structuring this code in a better way without engaging with #7444.