Auto merge of rust-lang#12084 - yuxqiu:manual_retain, r=Alexendoo

fix: incorrect suggestions generated by `manual_retain` lint

fixes rust-lang#10393, fixes rust-lang#11457, fixes rust-lang#12081

rust-lang#10393: In the current implementation of `manual_retain`, if the argument to the closure is matched using tuple, they are all treated as the result of a call to `map.into_iter().filter(<f>)`. However, such tuple pattern matching can also occur in many different containers that stores tuples internally. The correct approach is to apply different lint policies depending on whether the receiver of `into_iter` is a map or not.

rust-lang#11457 and rust-lang#12081: In the current implementation of `manual_retain`, if the argument to the closure is `Binding`, the closure will be used directly in the `retain` method, which will result in incorrect suggestion because the first argument to the `retain` closure may be of a different type. In addition, if the argument to the closure is `Ref + Binding`, the lint will simply remove the `Ref` part and use the `Binding` part as the argument to the new closure, which will lead to bad suggestion for the same reason. The correct approach is to detect each of these cases and apply lint suggestions conservatively.

changelog: [`manual_retain`] refactor and add check for various patterns

Author: bors

clippy_lints/src/manual_retain.rs

@@ -11,6 +11,7 @@ use rustc_lint::{LateContext, LateLintPass};
 use rustc_semver::RustcVersion;
 use rustc_session::impl_lint_pass;
 use rustc_span::symbol::sym;
+use rustc_span::Span;
 
 const ACCEPTABLE_METHODS: [&[&str]; 5] = [
     &paths::BINARYHEAP_ITER,
@@ -28,6 +29,7 @@ const ACCEPTABLE_TYPES: [(rustc_span::Symbol, Option<RustcVersion>); 7] = [
     (sym::Vec, None),
     (sym::VecDeque, None),
 ];
+const MAP_TYPES: [rustc_span::Symbol; 2] = [sym::BTreeMap, sym::HashMap];
 
 declare_clippy_lint! {
     /// ### What it does
@@ -44,6 +46,7 @@ declare_clippy_lint! {
     /// ```no_run
     /// let mut vec = vec![0, 1, 2];
     /// vec.retain(|x| x % 2 == 0);
+    /// vec.retain(|x| x % 2 == 0);
     /// ```
     #[clippy::version = "1.64.0"]
     pub MANUAL_RETAIN,
@@ -74,9 +77,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualRetain {
             && let Some(collect_def_id) = cx.typeck_results().type_dependent_def_id(collect_expr.hir_id)
             && cx.tcx.is_diagnostic_item(sym::iterator_collect_fn, collect_def_id)
         {
-            check_into_iter(cx, parent_expr, left_expr, target_expr, &self.msrv);
-            check_iter(cx, parent_expr, left_expr, target_expr, &self.msrv);
-            check_to_owned(cx, parent_expr, left_expr, target_expr, &self.msrv);
+            check_into_iter(cx, left_expr, target_expr, parent_expr.span, &self.msrv);
+            check_iter(cx, left_expr, target_expr, parent_expr.span, &self.msrv);
+            check_to_owned(cx, left_expr, target_expr, parent_expr.span, &self.msrv);
         }
     }
 
@@ -85,9 +88,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualRetain {
 
 fn check_into_iter(
     cx: &LateContext<'_>,
-    parent_expr: &hir::Expr<'_>,
     left_expr: &hir::Expr<'_>,
     target_expr: &hir::Expr<'_>,
+    parent_expr_span: Span,
     msrv: &Msrv,
 ) {
     if let hir::ExprKind::MethodCall(_, into_iter_expr, [_], _) = &target_expr.kind
@@ -98,16 +101,39 @@ fn check_into_iter(
         && Some(into_iter_def_id) == cx.tcx.lang_items().into_iter_fn()
         && match_acceptable_type(cx, left_expr, msrv)
         && SpanlessEq::new(cx).eq_expr(left_expr, struct_expr)
+        && let hir::ExprKind::MethodCall(_, _, [closure_expr], _) = target_expr.kind
+        && let hir::ExprKind::Closure(closure) = closure_expr.kind
+        && let filter_body = cx.tcx.hir().body(closure.body)
+        && let [filter_params] = filter_body.params
     {
-        suggest(cx, parent_expr, left_expr, target_expr);
+        if match_map_type(cx, left_expr) {
+            if let hir::PatKind::Tuple([key_pat, value_pat], _) = filter_params.pat.kind {
+                if let Some(sugg) = make_sugg(cx, key_pat, value_pat, left_expr, filter_body) {
+                    make_span_lint_and_sugg(cx, parent_expr_span, sugg);
+                }
+            }
+            // Cannot lint other cases because `retain` requires two parameters
+        } else {
+            // Can always move because `retain` and `filter` have the same bound on the predicate
+            // for other types
+            make_span_lint_and_sugg(
+                cx,
+                parent_expr_span,
+                format!(
+                    "{}.retain({})",
+                    snippet(cx, left_expr.span, ".."),
+                    snippet(cx, closure_expr.span, "..")
+                ),
+            );
+        }
     }
 }
 
 fn check_iter(
     cx: &LateContext<'_>,
-    parent_expr: &hir::Expr<'_>,
     left_expr: &hir::Expr<'_>,
     target_expr: &hir::Expr<'_>,
+    parent_expr_span: Span,
     msrv: &Msrv,
 ) {
     if let hir::ExprKind::MethodCall(_, filter_expr, [], _) = &target_expr.kind
@@ -122,16 +148,50 @@ fn check_iter(
         && match_acceptable_def_path(cx, iter_expr_def_id)
         && match_acceptable_type(cx, left_expr, msrv)
         && SpanlessEq::new(cx).eq_expr(left_expr, struct_expr)
+        && let hir::ExprKind::MethodCall(_, _, [closure_expr], _) = filter_expr.kind
+        && let hir::ExprKind::Closure(closure) = closure_expr.kind
+        && let filter_body = cx.tcx.hir().body(closure.body)
+        && let [filter_params] = filter_body.params
     {
-        suggest(cx, parent_expr, left_expr, filter_expr);
+        match filter_params.pat.kind {
+            // hir::PatKind::Binding(_, _, _, None) => {
+            //     // Be conservative now. Do nothing here.
+            //     // TODO: Ideally, we can rewrite the lambda by stripping one level of reference
+            // },
+            hir::PatKind::Tuple([_, _], _) => {
+                // the `&&` reference for the `filter` method will be auto derefed to `ref`
+                // so, we can directly use the lambda
+                // https://doc.rust-lang.org/reference/patterns.html#binding-modes
+                make_span_lint_and_sugg(
+                    cx,
+                    parent_expr_span,
+                    format!(
+                        "{}.retain({})",
+                        snippet(cx, left_expr.span, ".."),
+                        snippet(cx, closure_expr.span, "..")
+                    ),
+                );
+            },
+            hir::PatKind::Ref(pat, _) => make_span_lint_and_sugg(
+                cx,
+                parent_expr_span,
+                format!(
+                    "{}.retain(|{}| {})",
+                    snippet(cx, left_expr.span, ".."),
+                    snippet(cx, pat.span, ".."),
+                    snippet(cx, filter_body.value.span, "..")
+                ),
+            ),
+            _ => {},
+        }
     }
 }
 
 fn check_to_owned(
     cx: &LateContext<'_>,
-    parent_expr: &hir::Expr<'_>,
     left_expr: &hir::Expr<'_>,
     target_expr: &hir::Expr<'_>,
+    parent_expr_span: Span,
     msrv: &Msrv,
 ) {
     if msrv.meets(msrvs::STRING_RETAIN)
@@ -147,43 +207,25 @@ fn check_to_owned(
         && let ty = cx.typeck_results().expr_ty(str_expr).peel_refs()
         && is_type_lang_item(cx, ty, hir::LangItem::String)
         && SpanlessEq::new(cx).eq_expr(left_expr, str_expr)
-    {
-        suggest(cx, parent_expr, left_expr, filter_expr);
-    }
-}
-
-fn suggest(cx: &LateContext<'_>, parent_expr: &hir::Expr<'_>, left_expr: &hir::Expr<'_>, filter_expr: &hir::Expr<'_>) {
-    if let hir::ExprKind::MethodCall(_, _, [closure], _) = filter_expr.kind
-        && let hir::ExprKind::Closure(&hir::Closure { body, .. }) = closure.kind
-        && let filter_body = cx.tcx.hir().body(body)
+        && let hir::ExprKind::MethodCall(_, _, [closure_expr], _) = filter_expr.kind
+        && let hir::ExprKind::Closure(closure) = closure_expr.kind
+        && let filter_body = cx.tcx.hir().body(closure.body)
         && let [filter_params] = filter_body.params
-        && let Some(sugg) = match filter_params.pat.kind {
-            hir::PatKind::Binding(_, _, filter_param_ident, None) => Some(format!(
-                "{}.retain(|{filter_param_ident}| {})",
-                snippet(cx, left_expr.span, ".."),
-                snippet(cx, filter_body.value.span, "..")
-            )),
-            hir::PatKind::Tuple([key_pat, value_pat], _) => make_sugg(cx, key_pat, value_pat, left_expr, filter_body),
-            hir::PatKind::Ref(pat, _) => match pat.kind {
-                hir::PatKind::Binding(_, _, filter_param_ident, None) => Some(format!(
-                    "{}.retain(|{filter_param_ident}| {})",
+    {
+        if let hir::PatKind::Ref(pat, _) = filter_params.pat.kind {
+            make_span_lint_and_sugg(
+                cx,
+                parent_expr_span,
+                format!(
+                    "{}.retain(|{}| {})",
                     snippet(cx, left_expr.span, ".."),
+                    snippet(cx, pat.span, ".."),
                     snippet(cx, filter_body.value.span, "..")
-                )),
-                _ => None,
-            },
-            _ => None,
+                ),
+            );
         }
-    {
-        span_lint_and_sugg(
-            cx,
-            MANUAL_RETAIN,
-            parent_expr.span,
-            "this expression can be written more simply using `.retain()`",
-            "consider calling `.retain()` instead",
-            sugg,
-            Applicability::MachineApplicable,
-        );
+        // Be conservative now. Do nothing for the `Binding` case.
+        // TODO: Ideally, we can rewrite the lambda by stripping one level of reference
     }
 }
 
@@ -229,3 +271,20 @@ fn match_acceptable_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>, msrv: &Msrv
             && acceptable_msrv.map_or(true, |acceptable_msrv| msrv.meets(acceptable_msrv))
     })
 }
+
+fn match_map_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool {
+    let expr_ty = cx.typeck_results().expr_ty(expr).peel_refs();
+    MAP_TYPES.iter().any(|ty| is_type_diagnostic_item(cx, expr_ty, *ty))
+}
+
+fn make_span_lint_and_sugg(cx: &LateContext<'_>, span: Span, sugg: String) {
+    span_lint_and_sugg(
+        cx,
+        MANUAL_RETAIN,
+        span,
+        "this expression can be written more simply using `.retain()`",
+        "consider calling `.retain()` instead",
+        sugg,
+        Applicability::MachineApplicable,
+    );
+}

tests/ui/manual_retain.fixed

@@ -14,6 +14,9 @@ fn main() {
     _msrv_153();
     _msrv_126();
     _msrv_118();
+
+    issue_10393();
+    issue_12081();
 }
 
 fn binary_heap_retain() {
@@ -23,6 +26,11 @@ fn binary_heap_retain() {
     binary_heap.retain(|x| x % 2 == 0);
     binary_heap.retain(|x| x % 2 == 0);
 
+    // Do lint, because we use pattern matching
+    let mut tuples = BinaryHeap::from([(0, 1), (1, 2), (2, 3)]);
+    tuples.retain(|(ref x, ref y)| *x == 0);
+    tuples.retain(|(x, y)| *x == 0);
+
     // Do not lint, because type conversion is performed
     binary_heap = binary_heap
         .into_iter()
@@ -55,6 +63,9 @@ fn btree_map_retain() {
     btree_map.retain(|_, &mut v| v % 2 == 0);
     btree_map.retain(|k, &mut v| (k % 2 == 0) && (v % 2 == 0));
 
+    // Do not lint, because the parameters are not matched in tuple pattern
+    btree_map = btree_map.into_iter().filter(|t| t.0 % 2 == 0).collect();
+
     // Do not lint.
     btree_map = btree_map
         .into_iter()
@@ -76,6 +87,11 @@ fn btree_set_retain() {
     btree_set.retain(|x| x % 2 == 0);
     btree_set.retain(|x| x % 2 == 0);
 
+    // Do lint, because we use pattern matching
+    let mut tuples = BTreeSet::from([(0, 1), (1, 2), (2, 3)]);
+    tuples.retain(|(ref x, ref y)| *x == 0);
+    tuples.retain(|(x, y)| *x == 0);
+
     // Do not lint, because type conversion is performed
     btree_set = btree_set
         .iter()
@@ -108,6 +124,9 @@ fn hash_map_retain() {
     hash_map.retain(|_, &mut v| v % 2 == 0);
     hash_map.retain(|k, &mut v| (k % 2 == 0) && (v % 2 == 0));
 
+    // Do not lint, because the parameters are not matched in tuple pattern
+    hash_map = hash_map.into_iter().filter(|t| t.0 % 2 == 0).collect();
+
     // Do not lint.
     hash_map = hash_map
         .into_iter()
@@ -128,6 +147,11 @@ fn hash_set_retain() {
     hash_set.retain(|x| x % 2 == 0);
     hash_set.retain(|x| x % 2 == 0);
 
+    // Do lint, because we use pattern matching
+    let mut tuples = HashSet::from([(0, 1), (1, 2), (2, 3)]);
+    tuples.retain(|(ref x, ref y)| *x == 0);
+    tuples.retain(|(x, y)| *x == 0);
+
     // Do not lint, because type conversion is performed
     hash_set = hash_set.into_iter().filter(|x| x % 2 == 0).collect::<HashSet<i8>>();
     hash_set = hash_set
@@ -171,6 +195,11 @@ fn vec_retain() {
     vec.retain(|x| x % 2 == 0);
     vec.retain(|x| x % 2 == 0);
 
+    // Do lint, because we use pattern matching
+    let mut tuples = vec![(0, 1), (1, 2), (2, 3)];
+    tuples.retain(|(ref x, ref y)| *x == 0);
+    tuples.retain(|(x, y)| *x == 0);
+
     // Do not lint, because type conversion is performed
     vec = vec.into_iter().filter(|x| x % 2 == 0).collect::<Vec<i8>>();
     vec = vec.iter().filter(|&x| x % 2 == 0).copied().collect::<Vec<i8>>();
@@ -246,3 +275,37 @@ fn _msrv_118() {
     let mut hash_map: HashMap<i8, i8> = (0..8).map(|x| (x, x * 10)).collect();
     hash_map = hash_map.into_iter().filter(|(k, _)| k % 2 == 0).collect();
 }
+
+fn issue_10393() {
+    // Do lint
+    let mut vec = vec![(0, 1), (1, 2), (2, 3)];
+    vec.retain(|(x, y)| *x == 0);
+
+    // Do lint
+    let mut tuples = vec![(true, -2), (false, 3)];
+    tuples.retain(|(_, n)| *n > 0);
+}
+
+fn issue_11457() {
+    // Do not lint, as we need to modify the closure
+    let mut vals = vec![1, 2, 3, 4];
+    vals = vals.iter().filter(|v| **v != 1).cloned().collect();
+
+    // Do not lint, as we need to modify the closure
+    let mut s = String::from("foobar");
+    s = s.chars().filter(|c| *c != 'o').to_owned().collect();
+}
+
+fn issue_12081() {
+    let mut vec = vec![0, 1, 2];
+
+    // Do lint
+    vec.retain(|&x| x == 0);
+    vec.retain(|&x| x == 0);
+    vec.retain(|&x| x == 0);
+
+    // Do lint
+    vec.retain(|x| *x == 0);
+    vec.retain(|x| *x == 0);
+    vec.retain(|x| *x == 0);
+}