Skip to content

feat: Add action to expand a declarative macro once, inline. Fixes #13598 #13810

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

Merged
merged 2 commits into from
Jan 9, 2023

Conversation

tfpk
Copy link
Contributor

@tfpk tfpk commented Dec 21, 2022

This commit adds a new r-a method, expandMacroInline, which expands the macro that's currently selected. See #13598 for the most applicable issue; though I suspect it'll resolve part of #5949 and make #11888 significantly easier).

The macro works like this:

rust-analyser-feature

I have 2 questions before this PR can be merged:

  1. Should we rustfmt the output? The advantage of doing this is neater code. The disadvantages are we'd have to format the whole expr/stmt/block (since there's no point just formatting one part, especially over multiple lines), and maybe it moves the code around more in weird ways. My suggestion here is to start off by not doing any formatting; and if it appears useful we can decide to do formatting in a later release.
  2. Is it worth solving the $crate hygiene issue now? -- I think this PR is usable as of right now for some use-cases; but it is annoying that many common macros (i.e. println!(), format!()) can't be expanded further unless the user guesses the correct $crate value. The trouble with solving that issue is that I think it's complicated and imperfect. If we do solve it; we'd also need to either change the existing expandMacro/expandMacroInline commands; provide some option to allow/disallow $crate expanding; or come to some other compromise.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 21, 2022
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

will take a proper look later but there is no need for this to be a custom command, it would make more sense to implement this as an assist instead
not formatting is okay right now, likewise not fixing the $crate replacement

@tfpk
Copy link
Contributor Author

tfpk commented Dec 21, 2022

I'll push a commit to make this an assist (thanks -- didn't know that was an option). Is it worth having a command too to do a recursive inline?

I'll start working on replacing $crate as well and can do that as a separate PR.

@tfpk
Copy link
Contributor Author

tfpk commented Dec 23, 2022

Just an update - the assist is almost done (just finishing off the tests). There's no point in reviewing most of this because it'll get reverted by changing it to be an assist. Thanks for your patience, and suggestions :)

@tfpk tfpk force-pushed the tfpk/macro-inline branch from cf79357 to 16654ce Compare December 23, 2022 12:22
@tfpk tfpk changed the title Add command to expand a declarative macro once, inline. Fixes #13598 Add action to expand a declarative macro once, inline. Fixes #13598 Dec 23, 2022
@tfpk
Copy link
Contributor Author

tfpk commented Dec 23, 2022

Okay @Veykril I think this is now ready for review. Thanks again :)

Comment on lines 37 to 49
let tok = ctx.token_at_offset().right_biased()?;

let mut anc = tok.parent_ancestors();
let (_name, expanded, unexpanded) = loop {
let node = anc.next()?;
if let Some(mac) = ast::MacroCall::cast(node.clone()) {
break (
mac.path()?.segment()?.name_ref()?.to_string(),
ctx.sema.expand(&mac)?.clone_for_update(),
node,
);
}
};
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
let tok = ctx.token_at_offset().right_biased()?;
let mut anc = tok.parent_ancestors();
let (_name, expanded, unexpanded) = loop {
let node = anc.next()?;
if let Some(mac) = ast::MacroCall::cast(node.clone()) {
break (
mac.path()?.segment()?.name_ref()?.to_string(),
ctx.sema.expand(&mac)?.clone_for_update(),
node,
);
}
};
let unexpanded = ctx.find_node_at_offset::<ast::MacroCall>()?;
let expanded = ctx.sema.expand(&mac)?.clone_for_update();

@Veykril
Copy link
Member

Veykril commented Jan 9, 2023

small nit, otherwise lgtm
@bors delegate+

@bors
Copy link
Contributor

bors commented Jan 9, 2023

✌️ @tfpk can now approve this pull request

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2023
@Veykril
Copy link
Member

Veykril commented Jan 9, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2023

📌 Commit 769273c has been approved by Veykril

It is now in the queue for this repository.

@tfpk
Copy link
Contributor Author

tfpk commented Jan 9, 2023

@Veykril is the CI failure above a known issue? A segfault in vscode doesn't seem like my issue, but wondering if we need to report or something?

@lnicola
Copy link
Member

lnicola commented Jan 9, 2023

CC microsoft/vscode#101069

@bors
Copy link
Contributor

bors commented Jan 9, 2023

⌛ Testing commit 769273c with merge 336608a...

@bors
Copy link
Contributor

bors commented Jan 9, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 336608a to master...

@bors bors merged commit 336608a into rust-lang:master Jan 9, 2023
@lnicola lnicola changed the title Add action to expand a declarative macro once, inline. Fixes #13598 feat: Add action to expand a declarative macro once, inline. Fixes #13598 Jan 9, 2023
@tfpk
Copy link
Contributor Author

tfpk commented Jan 9, 2023

Is it worth adding this to the change log?

@Veykril
Copy link
Member

Veykril commented Jan 9, 2023

This will appear on the next release changelog I believe

@lnicola
Copy link
Member

lnicola commented Jan 9, 2023

Yeah, it will be featured automatically next Monday (as it was merged today, after the release).

@tfpk
Copy link
Contributor Author

tfpk commented Jan 9, 2023

Ah, sorry! Didn't realise this happened after the release. Thanks so much:)

@jonas-schievink
Copy link
Contributor

The gif should ideally have been updated to show that this is an assist, not a command

@lnicola
Copy link
Member

lnicola commented Jan 16, 2023

I wanted to, but I have this little issue on my system:

inline-macro.mp4

@tfpk
Copy link
Contributor Author

tfpk commented Jan 16, 2023

I can update the gif in the next half hour if that helps?

@tfpk
Copy link
Contributor Author

tfpk commented Jan 16, 2023

rust-analyser-inline-action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants