fix: filter_map_bool_then suggest wrongly when the closure cannot be decompose directly (rust-lang#14370)

Closes rust-lang#11617
Closes rust-lang#14368

Clippy gives wrong suggestions when the filter and then cannot be put
into closure directly. Since trying to transform these can be too
complicated, Clippy will simply warn but don't try to fix.

changelog: [`filter_map_bool_then`]: fix wrong suggestions when the
closure cannot be decompose directly

Author: blyxyas

clippy_lints/src/methods/filter_map_bool_then.rs

@@ -1,10 +1,14 @@
 use super::FILTER_MAP_BOOL_THEN;
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::SpanRangeExt;
 use clippy_utils::ty::is_copy;
-use clippy_utils::{is_from_proc_macro, is_trait_method, peel_blocks};
+use clippy_utils::{
+    CaptureKind, can_move_expr_to_closure, contains_return, is_from_proc_macro, is_trait_method, peel_blocks,
+};
+use rustc_ast::Mutability;
+use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind};
+use rustc_hir::{Expr, ExprKind, HirId, Param, Pat};
 use rustc_lint::{LateContext, LintContext};
 use rustc_middle::ty::Binder;
 use rustc_middle::ty::adjustment::Adjust;
@@ -44,17 +48,69 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, arg: &
         && let Some(filter) = recv.span.get_source_text(cx)
         && let Some(map) = then_body.span.get_source_text(cx)
     {
-        span_lint_and_sugg(
+        span_lint_and_then(
             cx,
             FILTER_MAP_BOOL_THEN,
             call_span,
             "usage of `bool::then` in `filter_map`",
-            "use `filter` then `map` instead",
-            format!(
-                "filter(|&{param_snippet}| {derefs}{filter}).map(|{param_snippet}| {map})",
-                derefs = "*".repeat(needed_derefs)
-            ),
-            Applicability::MachineApplicable,
+            |diag| {
+                if can_filter_and_then_move_to_closure(cx, &param, recv, then_body) {
+                    diag.span_suggestion(
+                        call_span,
+                        "use `filter` then `map` instead",
+                        format!(
+                            "filter(|&{param_snippet}| {derefs}{filter}).map(|{param_snippet}| {map})",
+                            derefs = "*".repeat(needed_derefs)
+                        ),
+                        Applicability::MachineApplicable,
+                    );
+                } else {
+                    diag.help("consider using `filter` then `map` instead");
+                }
+            },
         );
     }
 }
+
+/// Returns a set of all bindings found in the given pattern.
+fn find_bindings_from_pat(pat: &Pat<'_>) -> FxHashSet<HirId> {
+    let mut bindings = FxHashSet::default();
+    pat.walk(|p| {
+        if let rustc_hir::PatKind::Binding(_, hir_id, _, _) = p.kind {
+            bindings.insert(hir_id);
+        }
+        true
+    });
+    bindings
+}
+
+/// Returns true if we can take a closure parameter and have it in both the `filter` function and
+/// the`map` function. This is not the case if:
+///
+/// - The `filter` would contain an early return,
+/// - `filter` and `then` contain captures, and any of those are &mut
+fn can_filter_and_then_move_to_closure<'tcx>(
+    cx: &LateContext<'tcx>,
+    param: &Param<'tcx>,
+    filter: &'tcx Expr<'tcx>,
+    then: &'tcx Expr<'tcx>,
+) -> bool {
+    if contains_return(filter) {
+        return false;
+    }
+
+    let Some(filter_captures) = can_move_expr_to_closure(cx, filter) else {
+        return true;
+    };
+    let Some(then_captures) = can_move_expr_to_closure(cx, then) else {
+        return true;
+    };
+
+    let param_bindings = find_bindings_from_pat(param.pat);
+    filter_captures.iter().all(|(hir_id, filter_cap)| {
+        param_bindings.contains(hir_id)
+            || !then_captures
+                .get(hir_id)
+                .is_some_and(|then_cap| matches!(*filter_cap | *then_cap, CaptureKind::Ref(Mutability::Mut)))
+    })
+}

tests/ui/filter_map_bool_then_unfixable.rs

@@ -0,0 +1,63 @@
+#![allow(clippy::question_mark, unused)]
+#![warn(clippy::filter_map_bool_then)]
+//@no-rustfix
+
+fn issue11617() {
+    let mut x: Vec<usize> = vec![0; 10];
+    let _ = (0..x.len()).zip(x.clone().iter()).filter_map(|(i, v)| {
+        //~^ filter_map_bool_then
+        (x[i] != *v).then(|| {
+            x[i] = i;
+            i
+        })
+    });
+}
+
+mod issue14368 {
+
+    fn do_something(_: ()) -> bool {
+        true
+    }
+
+    fn option_with_early_return(x: &[Option<bool>]) {
+        let _ = x.iter().filter_map(|&x| x?.then(|| do_something(())));
+        //~^ filter_map_bool_then
+        let _ = x
+            .iter()
+            .filter_map(|&x| if let Some(x) = x { x } else { return None }.then(|| do_something(())));
+        //~^ filter_map_bool_then
+        let _ = x.iter().filter_map(|&x| {
+            //~^ filter_map_bool_then
+            match x {
+                Some(x) => x,
+                None => return None,
+            }
+            .then(|| do_something(()))
+        });
+    }
+
+    #[derive(Copy, Clone)]
+    enum Foo {
+        One(bool),
+        Two,
+        Three(Option<i32>),
+    }
+
+    fn nested_type_with_early_return(x: &[Foo]) {
+        let _ = x.iter().filter_map(|&x| {
+            //~^ filter_map_bool_then
+            match x {
+                Foo::One(x) => x,
+                Foo::Two => return None,
+                Foo::Three(inner) => {
+                    if inner? == 0 {
+                        return Some(false);
+                    } else {
+                        true
+                    }
+                },
+            }
+            .then(|| do_something(()))
+        });
+    }
+}

tests/ui/filter_map_bool_then_unfixable.stderr

@@ -0,0 +1,65 @@
+error: usage of `bool::then` in `filter_map`
+  --> tests/ui/filter_map_bool_then_unfixable.rs:7:48
+   |
+LL |       let _ = (0..x.len()).zip(x.clone().iter()).filter_map(|(i, v)| {
+   |  ________________________________________________^
+LL | |
+LL | |         (x[i] != *v).then(|| {
+LL | |             x[i] = i;
+LL | |             i
+LL | |         })
+LL | |     });
+   | |______^
+   |
+   = help: consider using `filter` then `map` instead
+   = note: `-D clippy::filter-map-bool-then` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::filter_map_bool_then)]`
+
+error: usage of `bool::then` in `filter_map`
+  --> tests/ui/filter_map_bool_then_unfixable.rs:23:26
+   |
+LL |         let _ = x.iter().filter_map(|&x| x?.then(|| do_something(())));
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using `filter` then `map` instead
+
+error: usage of `bool::then` in `filter_map`
+  --> tests/ui/filter_map_bool_then_unfixable.rs:27:14
+   |
+LL |             .filter_map(|&x| if let Some(x) = x { x } else { return None }.then(|| do_something(())));
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using `filter` then `map` instead
+
+error: usage of `bool::then` in `filter_map`
+  --> tests/ui/filter_map_bool_then_unfixable.rs:29:26
+   |
+LL |           let _ = x.iter().filter_map(|&x| {
+   |  __________________________^
+LL | |
+LL | |             match x {
+LL | |                 Some(x) => x,
+...  |
+LL | |             .then(|| do_something(()))
+LL | |         });
+   | |__________^
+   |
+   = help: consider using `filter` then `map` instead
+
+error: usage of `bool::then` in `filter_map`
+  --> tests/ui/filter_map_bool_then_unfixable.rs:47:26
+   |
+LL |           let _ = x.iter().filter_map(|&x| {
+   |  __________________________^
+LL | |
+LL | |             match x {
+LL | |                 Foo::One(x) => x,
+...  |
+LL | |             .then(|| do_something(()))
+LL | |         });
+   | |__________^
+   |
+   = help: consider using `filter` then `map` instead
+
+error: aborting due to 5 previous errors
+