Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions clippy_lints/src/matches/manual_filter.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use clippy_utils::as_some_expr;
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath};
use clippy_utils::source::snippet_with_context;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_copy;
use clippy_utils::visitors::contains_unsafe_block;
use clippy_utils::{as_some_expr, span_contains_comment};
use rustc_errors::Applicability;
use rustc_hir::LangItem::OptionNone;
use rustc_hir::{Arm, Expr, ExprKind, HirId, Pat, PatKind};
Expand Down Expand Up @@ -118,7 +118,13 @@ pub(crate) fn check_and_then_method<'tcx>(
call_span,
"manual implementation of `Option::filter`",
|diag| {
let mut applicability = Applicability::MachineApplicable;
// A comment inside the replaced `and_then` closure (for example, one explaining the
// predicate) would be dropped by the rewrite, so don't auto-apply the fix then (#17376).
let mut applicability = if span_contains_comment(cx, call_span) {
Applicability::MaybeIncorrect
} else {
Applicability::MachineApplicable
};

let mut cond_snip =
Sugg::hir_with_context(cx, some_expr.expr, expr_span_ctxt, "..", &mut applicability);
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/manual_filter_unfixable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//@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() {}
Comment on lines +1 to +15

@Gri-ffin Gri-ffin Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

16 changes: 16 additions & 0 deletions tests/ui/manual_filter_unfixable.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error: manual implementation of `Option::filter`
--> tests/ui/manual_filter_unfixable.rs:8:9
|
LL | opt.and_then(|x| {
| _________^
LL | |
LL | | // keep this explanation
LL | | if x < 10 { Some(x) } else { None }
LL | | });
| |______^ help: try: `filter(|&x| x < 10)`
|
= note: `-D clippy::manual-filter` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_filter)]`

error: aborting due to 1 previous error

Loading