Prevent ICE when formatting item-only vec!{}#5879
Merged
ytmimi merged 2 commits intorust-lang:masterfrom Aug 17, 2023
tdanniels:issue_5735
Merged
Prevent ICE when formatting item-only vec!{}#5879ytmimi merged 2 commits intorust-lang:masterfrom tdanniels:issue_5735
vec!{}#5879ytmimi merged 2 commits intorust-lang:masterfrom
tdanniels:issue_5735
Conversation
vec!{}
Member
|
Thanks for another PR! r? @fee1-dead |
Contributor
Author
|
Rebased on to the latest |
ytmimi
approved these changes
Aug 17, 2023
Contributor
ytmimi
left a comment
There was a problem hiding this comment.
@tdanniels Thanks for the help on this one. I think the approach of passing the original style to locate the correct opener makes a lot of sense.
Happy to merge this after rebasing on the latest master.
Fixes 5735 Attempting to format invocations of macros which are considered "forced bracket macros" (currently only `vec!`), but are invoked with braces instead of brackets, and contain only items in their token trees, currently triggers an ICE in rustfmt. This is because the function that handles formatting macro invocations containing only items, `rewrite_macro_with_items`, assumes that the forced delimiter style of the macro being formatted is the same as the delimiter style in the macro's source text when attempting to locate the span after the macro's opening delimiter. This leads to the construction of an invalid span, triggering the ICE. The fix here is to pass the old delimiter style to `rewrite_macro_with_items` as well, so that it can successfully locate the span.
Contributor
Author
|
Thanks for the reviews everyone! @ytmimi it should be all up to date now. |
Contributor
|
You're very welcome! Again, really appreciate the help! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevent ICE when formatting item-only
vec!{}Fixes #5735
Attempting to format invocations of macros which are considered "forced bracket macros" (currently only
vec!), but are invoked with braces instead of brackets, and contain only items in their token trees, currently triggers an ICE in rustfmt. This is because the function that handles formatting macro invocations containing only items,rewrite_macro_with_items, assumes that the forced delimiter style of the macro being formatted is the same as the delimiter style in the macro's source text when attempting to locate the span after the macro's opening delimiter. This leads to the construction of an invalid span, triggering the ICE.The fix here is to pass the old delimiter style to
rewrite_macro_with_itemsas well, so that it can successfully locate the span.Hi rustfmt'ers! I'm back again with another ICE fix. Some notes for the reviewer:
will be formatted into
I understand that the team's stance is that braced macro invocations should generally be left unformatted. However, the current behaviour of
rustfmton bracedvec!invocations is to format them. That is, before this PR,rustfmtwill turnvec!{1, struct X {} }intovec![1, struct X {}];.My PR doesn't change this behaviour. It just extends a similar behaviour to
vec!invocations that contain only items. So with my PR, whereas formattingvec!{ struct X {} }would previously have triggered an ICE, it's now formatted intoI'm not entirely sure that this is the 'right' direction to go in, but the alternative of leaving braced, item-only
vec!invocations unformatted seemed about equally surprising to me from a user perspective. Any feedback here is appreciated, and I'm happy to tweak the implementation to whatever behaviour the team thinks is best.I've also included a small refactoring commit of some redundancy I noticed while working in
macros.rs. I'm happy to split that off into a separate PR if that's what people prefer.Clippy isn't happy that I've added another parameter to
rewrite_macro_with_items, pushing it over the threshold of 7 parameters. I didn't see a way to cleanly keep it below the threshold while still passing in the original delimiter style, but as usual any suggestions are appreciated.