-
Notifications
You must be signed in to change notification settings - Fork 888
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
Don't insert semicolons inside of a macro_rules!
arm body
#4507
Conversation
aea9629
to
a2ad4cc
Compare
#4496 should be merged first, so that the tests pass |
Currently, rustfmt inserts semicolons for certain trailing expressions (`return`, `continue`, and `break`). However, this should not be done in the body of a `macro_rules!` arm, since the macro might be used in expression position (where a trailing semicolon will be invalid). Currently, this rewriting has no effect due to rust-lang/rust#33953 If this issue is fixed, then this rewriting will prevent some macros from being used in expression position. This commit prevents rustfmt from inserting semicolons for trailing expressions in `macro_rules!` arms. The user is free to either include or omit the semicolon - rustfmt will not modify either case.
a2ad4cc
to
38efc96
Compare
@calebcartwright This PR is now ready for review |
@calebcartwright: Are there any changes that you'd like me to make? |
@Aaron1011 - sorry i haven't had a chance to look at this one yet. My rustfmt bandwidth is a bit limited this week, and largely focused on addressing the broken nightly toolstate and nested tuple panic on stable issues ATM. Will try to take a look within the next few days |
I was initially surprised by the updates to I feel like we should have a less painful way to track/set some visitor context when formatting macros, but that'd almost certainly require some major refactoring that we don't need to attempt to tackle here. I'm 👍 with the objective given the likely upstream changes as well as the general approach, although I do want to see this tweaked a bit to keep things encapsulated internally within the rustfmt lib vs. exposing the macro def context through the public API (we do actually have some folks using rustfmt as a lib). This should avoid leaking any internals, and also give us flexibility to potentially refactor down the road without introducing breaking changes to the interface. Perhaps some additional internal functions ( |
Yeah. that was my conclusion as well.
That's in reference to
I'll try that |
Correct |
@calebcartwright Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@calebcartwright: Could you make a new release containing this change? I need this to be able to make progress on some upstream issues. |
Sure, will push for doing so in the next day or two |
…aron1011 update rustfmt to v1.4.25 Contains changes from rust-lang/rustfmt#4507 r? `@Aaron1011`
…aron1011 update rustfmt to v1.4.25 Contains changes from rust-lang/rustfmt#4507 r? ``@Aaron1011``
This pulls in rust-lang/rustfmt#4507, allowing us to remove a semicolon in an internal libstd macro
backported in #4521 |
Currently, rustfmt inserts semicolons for certain trailing expressions
(
return
,continue
, andbreak
). However, this should not be done inthe body of a
macro_rules!
arm, since the macro might be used inexpression position (where a trailing semicolon will be invalid).
Currently, this rewriting has no effect due to rust-lang/rust#33953
If this issue is fixed, then this rewriting will prevent some macros
from being used in expression position.
This commit prevents rustfmt from inserting semicolons for trailing
expressions in
macro_rules!
arms. The user is free to either includeor omit the semicolon - rustfmt will not modify either case.