Fixes a case where collapsible_match suggested a transformation that changes runtime behavior.#16878
Conversation
|
r? @dswij rustbot has assigned @dswij. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Lintcheck changes for c7cadb6
This comment will be updated if you push new changes |
|
Reminder, once the PR becomes ready for a review, use |
Co-authored-by: Samuel Tardieu <sam@rfc1149.net>
|
@rustbot ready |
|
Alright @profetia checking right now. |
|
Please keep beta-nominated PR at minimal size, so that they are easy to review. Those PRs are merged very late into the beta cycle, almost right before the stable release, so they must be very very readable. This one fixes a |
This is not a good idea. |
|
@samueltardieu @profetia should I push the new commits or not :) |
Not in this PR. |
|
@samueltardieu Alright, not doing it, kindly merge my PR then. |
|
Given that's it's beta-nominated and possibly considered for stable backporting, I want to let a second maintainer a chance to have a look to it first. Ping @rust-lang/clippy |
The `clippy::collapsible_match` lint [1] can make code harder to read
in certain cases [2], e.g.
CLIPPY P rust/libmacros.so - due to command line change
warning: this `if` can be collapsed into the outer `match`
--> rust/pin-init/internal/src/helpers.rs:91:17
|
91 | / if nesting == 1 {
92 | | impl_generics.push(tt.clone());
93 | | impl_generics.push(tt);
94 | | skip_until_comma = false;
95 | | }
| |_________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match
= note: `-W clippy::collapsible-match` implied by `-W clippy::all`
= help: to override `-W clippy::all` add `#[allow(clippy::collapsible_match)]`
help: collapse nested if block
|
90 ~ TokenTree::Punct(p) if skip_until_comma && p.as_char() == ','
91 ~ && nesting == 1 => {
92 | impl_generics.push(tt.clone());
93 | impl_generics.push(tt);
94 | skip_until_comma = false;
95 ~ }
|
The lint does not have much upside -- when the suggestion may be a good
one, it would still read fine when nested anyway. And it is the kind of
lint that may easily bias people to just apply the suggestion instead
of allowing it.
[ In addition, as Gary points out [3], the suggestion is also wrong [4] and
in the process of being fixed [5], possibly for Rust 1.97.0:
Link: https://lore.kernel.org/rust-for-linux/DI3YV94TH9I3.1SOHW51552497@garyguo.net/ [3]
Link: rust-lang/rust-clippy#16875 [4]
Link: rust-lang/rust-clippy#16878 [5]
- Miguel ]
Thus just let developers decide on their own.
Cc: stable@vger.kernel.org # Needed in 6.12.y and later (Rust is pinned in older LTSs).
Link: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match [1]
Link: https://lore.kernel.org/rust-for-linux/CANiq72nWYJna_hdFxjQCQZK6yJBrr1Mb86iKavivV0U0BgufeA@mail.gmail.com/ [2]
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://patch.msgid.link/20260426144201.227108-1-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Closes #16875
What was wrong
The lint suggested collapsing
pattern => { if test { body } }intopattern if test => { body }. These are not equivalent. In the original, oncepatternmatches, the match exits regardless oftest. In the suggestion, a failing guard allows fall-through to subsequent arms, which can match values that previously never reached them.Fix
The
if-guard collapsing is only safe when there are no non-wildcard arms between the current arm and the wildcard arm. The fix adds a check that all arms after the current one are "wild-like" (a bare_, a binding, orNone, all without guards) before suggesting the transformation.As a side effect, three
#[expect(clippy::collapsible_match)]suppressions inclippy_lints/src/methods/mod.rsare removed. They had been added to suppress this exact false positive and are no longer needed.Still lints correctly
When only wildcard arms follow, the transformation is safe and the lint still fires:
changelog: [
collapsible_match]: no longer suggests collapsingpattern => { if test { body } }into a match guard when non-wildcard arms follow, as this changes the semantics of the match.Summary Notes
Managed by
@rustbot—see help for details