Auto merge of #11318 - Centri3:#11309, r=Manishearth

[`filter_map_bool_then`]: Don't ICE on late bound regions

Fixes #11309

Also lints `&NonCopy` now, since any `&` is `Copy`. That was accidental, but it seems that this is a consequence (or improvement!) of this fix.

r? `@Jarcho`

changelog: [`filter_map_bool_then`]: Don't ICE on late bound regions

clippy_lints/src/methods/filter_map_bool_then.rs

@@ -1,3 +1,4 @@
+use super::FILTER_MAP_BOOL_THEN;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::paths::BOOL_THEN;
 use clippy_utils::source::snippet_opt;
@@ -7,10 +8,9 @@ use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LintContext};
 use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty::Binder;
 use rustc_span::{sym, Span};
 
-use super::FILTER_MAP_BOOL_THEN;
-
 pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, arg: &Expr<'_>, call_span: Span) {
     if !in_external_macro(cx.sess(), expr.span)
         && is_trait_method(cx, expr, sym::Iterator)
@@ -21,7 +21,15 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, arg: &
         // `inputs` and `params` here as we need both the type and the span
         && let param_ty = closure.fn_decl.inputs[0]
         && let param = body.params[0]
-        && is_copy(cx, cx.typeck_results().node_type(param_ty.hir_id).peel_refs())
+        // Issue #11309
+        && let param_ty = cx.tcx.liberate_late_bound_regions(
+            closure.def_id.to_def_id(),
+            Binder::bind_with_vars(
+                cx.typeck_results().node_type(param_ty.hir_id),
+                cx.tcx.late_bound_vars(cx.tcx.hir().local_def_id_to_hir_id(closure.def_id)),
+            ),
+        )
+        && is_copy(cx, param_ty)
         && let ExprKind::MethodCall(_, recv, [then_arg], _) = value.kind
         && let ExprKind::Closure(then_closure) = then_arg.kind
         && let then_body = peel_blocks(cx.tcx.hir().body(then_closure.body).value)

tests/ui/filter_map_bool_then.fixed

@@ -4,6 +4,7 @@
     clippy::clone_on_copy,
     clippy::map_identity,
     clippy::unnecessary_lazy_evaluations,
+    clippy::unnecessary_filter_map,
     unused
 )]
 #![warn(clippy::filter_map_bool_then)]
@@ -29,9 +30,18 @@ fn main() {
         .copied()
         .filter(|&i| i != 1000)
         .filter(|&i| (i.clone() % 2 == 0)).map(|i| i + 1);
+    // Despite this is non-copy, `is_copy` still returns true (at least now) because it's `&NonCopy`,
+    // and any `&` is `Copy`. So since we can dereference it in `filter` (since it's then `&&NonCopy`),
+    // we can lint this and still get the same input type.
+    // See: <https://doc.rust-lang.org/std/primitive.reference.html#trait-implementations-1>
+    let v = vec![NonCopy, NonCopy];
+    v.clone().iter().filter(|&i| (i == &NonCopy)).map(|i| i);
     // Do not lint
     let v = vec![NonCopy, NonCopy];
-    v.clone().iter().filter_map(|i| (i == &NonCopy).then(|| i));
+    v.clone().into_iter().filter_map(|i| (i == NonCopy).then(|| i));
+    // `&mut` is `!Copy`.
+    let v = vec![NonCopy, NonCopy];
+    v.clone().iter_mut().filter_map(|i| (i == &mut NonCopy).then(|| i));
     external! {
         let v = vec![1, 2, 3, 4, 5, 6];
         v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
@@ -42,3 +52,7 @@ fn main() {
         v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
     }
 }
+
+fn issue11309<'a>(iter: impl Iterator<Item = (&'a str, &'a str)>) -> Vec<&'a str> {
+    iter.filter_map(|(_, s): (&str, _)| Some(s)).collect()
+}

tests/ui/filter_map_bool_then.rs

@@ -4,6 +4,7 @@
     clippy::clone_on_copy,
     clippy::map_identity,
     clippy::unnecessary_lazy_evaluations,
+    clippy::unnecessary_filter_map,
     unused
 )]
 #![warn(clippy::filter_map_bool_then)]
@@ -29,9 +30,18 @@ fn main() {
         .copied()
         .filter(|&i| i != 1000)
         .filter_map(|i| (i.clone() % 2 == 0).then(|| i + 1));
-    // Do not lint
+    // Despite this is non-copy, `is_copy` still returns true (at least now) because it's `&NonCopy`,
+    // and any `&` is `Copy`. So since we can dereference it in `filter` (since it's then `&&NonCopy`),
+    // we can lint this and still get the same input type.
+    // See: <https://doc.rust-lang.org/std/primitive.reference.html#trait-implementations-1>
     let v = vec![NonCopy, NonCopy];
     v.clone().iter().filter_map(|i| (i == &NonCopy).then(|| i));
+    // Do not lint
+    let v = vec![NonCopy, NonCopy];
+    v.clone().into_iter().filter_map(|i| (i == NonCopy).then(|| i));
+    // `&mut` is `!Copy`.
+    let v = vec![NonCopy, NonCopy];
+    v.clone().iter_mut().filter_map(|i| (i == &mut NonCopy).then(|| i));
     external! {
         let v = vec![1, 2, 3, 4, 5, 6];
         v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
@@ -42,3 +52,7 @@ fn main() {
         v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
     }
 }
+
+fn issue11309<'a>(iter: impl Iterator<Item = (&'a str, &'a str)>) -> Vec<&'a str> {
+    iter.filter_map(|(_, s): (&str, _)| Some(s)).collect()
+}

tests/ui/filter_map_bool_then.stderr

@@ -1,34 +1,40 @@
 error: usage of `bool::then` in `filter_map`
-  --> $DIR/filter_map_bool_then.rs:19:22
+  --> $DIR/filter_map_bool_then.rs:20:22
    |
 LL |     v.clone().iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i % 2 == 0)).map(|i| i + 1)`
    |
    = note: `-D clippy::filter-map-bool-then` implied by `-D warnings`
 
 error: usage of `bool::then` in `filter_map`
-  --> $DIR/filter_map_bool_then.rs:20:27
+  --> $DIR/filter_map_bool_then.rs:21:27
    |
 LL |     v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1));
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i % 2 == 0)).map(|i| i + 1)`
 
 error: usage of `bool::then` in `filter_map`
-  --> $DIR/filter_map_bool_then.rs:23:10
+  --> $DIR/filter_map_bool_then.rs:24:10
    |
 LL |         .filter_map(|i| -> Option<_> { (i % 2 == 0).then(|| i + 1) });
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i % 2 == 0)).map(|i| i + 1)`
 
 error: usage of `bool::then` in `filter_map`
-  --> $DIR/filter_map_bool_then.rs:27:10
+  --> $DIR/filter_map_bool_then.rs:28:10
    |
 LL |         .filter_map(|i| (i % 2 == 0).then(|| i + 1));
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i % 2 == 0)).map(|i| i + 1)`
 
 error: usage of `bool::then` in `filter_map`
-  --> $DIR/filter_map_bool_then.rs:31:10
+  --> $DIR/filter_map_bool_then.rs:32:10
    |
 LL |         .filter_map(|i| (i.clone() % 2 == 0).then(|| i + 1));
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i.clone() % 2 == 0)).map(|i| i + 1)`
 
-error: aborting due to 5 previous errors
+error: usage of `bool::then` in `filter_map`
+  --> $DIR/filter_map_bool_then.rs:38:22
+   |
+LL |     v.clone().iter().filter_map(|i| (i == &NonCopy).then(|| i));
+   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|&i| (i == &NonCopy)).map(|i| i)`
+
+error: aborting due to 6 previous errors