Special case matches! macro
#5554
Conversation
ytmimi
force-pushed
the
special_case_matches_macro
branch
2 times, most recently
from
5e5c766 to
e834841
Compare
| matches!( | ||
| something, | ||
| Some(_) | ||
| if method( |
There was a problem hiding this comment.
The indentation here looks weird in my opinion. The indentation makes sense in match context because a match might contain multiple arms but the matches! macro does not.
There was a problem hiding this comment.
I appreciate the feedback. My goal with this PR was to make matches! formatting consistent with match expression formatting, and this particular formatting is a result of reusing rewrite_guard to not have to reimplement guard formatting.
There was a problem hiding this comment.
@calebcartwright maybe t-style would want to chime in on the guard formatting here
There was a problem hiding this comment.
Has this been discussed somewhere or yet. If so, what was the conclusion?
The first step in being able to format the `matches!` macro is to parse it into ast nodes that we know how to rewrite.
The guard rewrite rules vary based on how the pattern was rewritten. That logic was previously written in `rewrite_match_arm`, but now it's fully encapsulates in `rewrite_guard`.
These traits will make it easier to implement rewriting matches! in terms of `overflow::rewrite_with_*` methods.
Now that we're able to parse the TokenStream of `matches!` calls into a `Matches` struct and then convert that `Matches` struct into an iterable which implments `IntoOverflowableItem` we're able to implement `matches!` rewriting in terms of `overflow::rewrite_with_*` calls.
After special casing `matches!`, the formatting more closely resembles `match` expressions. These changes cause breaking formatting changes to that need to be version gated.
The unit tests cover version One and Two formatting
Since rustfmt uses version two formatting the self tests were failing due to the new special case handling for `matches!`. various `matches!` calls in the codebase were updated to address this issue.
ytmimi
force-pushed
the
special_case_matches_macro
branch
from
e834841 to
0c5552d
Compare
|
I feel like we discussed this a tad offline some time back, but probably worth posting here too that I think a big consideration around this is whether it needs to be version gated. A question i've had teed up to the style team for a while is if/how macro calls should have style guide prescriptions or whether it's going to be something completely deferred to rustfmt. In either case, I suspect it's highly likely this will need to be gated, and could perhaps become the new default as part of the 2024 style edition |
| } else if macro_name == "matches!" && context.config.version() == Version::Two { | ||
| if let success @ Some(..) = format_matches(context, shape, ¯o_name, mac) { | ||
| return success; | ||
| } |
There was a problem hiding this comment.
@calebcartwright per your comment #5554 (comment), I just wanted to let you know that the changes are version gated.
I don't think there's a rush to get this in, and we can definitely hold off until style_edition lands to gate this around the 2024 style edition.
|
note to self, add a test case for #5860 |
|
Sorry for the bump, but there hasn't been any public news on this for almost a year now. Have there been any discussions or anything about whether to include this in the 2024 edition or what the next steps are here? |
This isn't something that the team has discussed in quite some time so I don't have any updated to share from rustfmt. To the best of my knowledge the style-team has had some discussions on the 2024 edition, and these are the tracking issues for rustfmt. |
Closes #4462
Closes #5547
Closes #5709
Closes #5860
Related / duplicate issues #4885 #5176
This PR adds support to parse and format the arguments of the
matches!macro. Nowmatches!formatting more closely resemblesmatchexpressions. This update introduces breaking formatting changes and is only supported when formatting with version two.r? @calebcartwright
I've done my best to logically group commits and I think this PR would be best reviewed 1 commit at a time.