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

Don't insert semicolons inside of a macro_rules! arm body #4507

Merged
merged 2 commits into from
Nov 7, 2020

Conversation

Aaron1011
Copy link
Member

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.

@Aaron1011
Copy link
Member Author

#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.
@Aaron1011
Copy link
Member Author

@calebcartwright This PR is now ready for review

@Aaron1011
Copy link
Member Author

@calebcartwright: Are there any changes that you'd like me to make?

@calebcartwright
Copy link
Member

@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

@calebcartwright
Copy link
Member

I was initially surprised by the updates to OperationSetting though see now why that was done. I'd forgotten how... fun the macro def formatting code is 😄

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 (format_input_inner and friends) could be updated to accept the new param and then utils::format_snippet could just directly use format_input_inner instead of the public-facing function.

@Aaron1011
Copy link
Member Author

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.

Yeah. that was my conclusion as well.

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

That's in reference to OperationSetting, right?

Perhaps some additional internal functions (format_input_inner and friends) could be updated to accept the new param and then utils::format_snippet could just directly use format_input_inner instead of the public-facing function.

I'll try that

@calebcartwright
Copy link
Member

That's in reference to OperationSetting, right?

Correct

@Aaron1011
Copy link
Member Author

@calebcartwright Updated

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks!

@calebcartwright calebcartwright merged commit a64f7ad into rust-lang:master Nov 7, 2020
@Aaron1011
Copy link
Member Author

@calebcartwright: Could you make a new release containing this change? I need this to be able to make progress on some upstream issues.

@calebcartwright
Copy link
Member

Sure, will push for doing so in the next day or two

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
…aron1011

update rustfmt to v1.4.25

Contains changes from rust-lang/rustfmt#4507

r? `@Aaron1011`
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
…aron1011

update rustfmt to v1.4.25

Contains changes from rust-lang/rustfmt#4507

r? ``@Aaron1011``
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Nov 19, 2020
This pulls in rust-lang/rustfmt#4507,
allowing us to remove a semicolon in an internal libstd macro
@karyon
Copy link
Contributor

karyon commented Oct 26, 2021

backported in #4521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants