manual_filter: don't eat comments in the and_then suggestion#17377
manual_filter: don't eat comments in the and_then suggestion#17377costajohnt wants to merge 1 commit into
manual_filter: don't eat comments in the and_then suggestion#17377Conversation
The `opt.and_then(|x| { /* comment */ if .. { Some(x) } else { None } })`
to `filter` rewrite drops any comment inside the closure. Mark the
suggestion `MaybeIncorrect` when the replaced span contains a comment, so
`clippy --fix` no longer applies it silently. This matches the existing
`span_contains_comment` handling in manual_ok_err / manual_unwrap_or /
manual_flatten.
rustbot
added
the
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @llogiq (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
|
If refusing to automatically apply the fix is more consistent with existing behavior - that would work as well. The desired part is not eating comments. |
|
That's what this PR does. When the replaced span contains a comment the suggestion drops to MaybeIncorrect, so |
| //@no-rustfix: the suggestion drops the closure's comment, so it is not machine-applicable | ||
|
|
||
| #![warn(clippy::manual_filter)] | ||
|
|
||
| fn issue17376(opt: Option<u32>) { | ||
| // Rewriting to `filter` would drop the comment below, so the suggestion must not be | ||
| // machine-applicable here. Otherwise `clippy --fix` would silently eat the comment (#17376). | ||
| opt.and_then(|x| { | ||
| //~^ manual_filter | ||
| // keep this explanation | ||
| if x < 10 { Some(x) } else { None } | ||
| }); | ||
| } | ||
|
|
||
| fn main() {} |
There was a problem hiding this comment.
Wouldn't //@no-rustfix here defeat the purpose of what you are trying to achieve? If you disable rustfix, the test would still pass even if the suggestion accidentally remains MachineApplicable
There was a problem hiding this comment.
Good catch, but rustfix wouldn't actually guard that here. clippy's UI tests run with RustfixMode::Everything, which applies every suggestion regardless of applicability, so MaybeIncorrect gets applied too. The .fixed would come out identical either way (comment dropped), so it can't tell MachineApplicable from MaybeIncorrect, and turning rustfix on would just bless a .fixed showing the comment eaten.
So //@no-rustfix here, same as manual_unwrap_or_default_unfixable.rs. The comment-free path is still rustfix-tested over in manual_filter.rs. You're right that a regression back to MachineApplicable wouldn't be caught, but I don't think the harness exposes applicability anywhere to assert on. Open to it if you know a way.
5 participants
Fixes #17376.
clippy::manual_filterrewritesopt.and_then(|x| { /* comment */ if cond { Some(x) } else { None } })toopt.filter(|&x| cond), dropping any comment inside the closure. Under--fixthis silently deletes the comment.Mark the suggestion
MaybeIncorrectwhen the replaced span contains a comment, soclippy --fixno longer applies it automatically. This mirrors the existingspan_contains_commenthandling inmanual_ok_err,manual_unwrap_or, andmanual_flatten.The commented case lives in a new
manual_filter_unfixable.rs(//@no-rustfix), since the UI-test harness applies every suggestion to.fixedregardless of applicability.Happy to instead preserve the comment in the rewrite (the issue's "desired output", the way
some_filterkeeps the block) if you'd prefer that over the applicability downgrade.changelog: [
manual_filter]: don't silently drop comments in theand_thensuggestion under--fix