fix: incorrect suggestions generated by manual_retain lint

Author: yuxqiu

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
@@ -157,6 +181,9 @@ fn string_retain() {
     // Do lint.
     s.retain(|c| c != 'o');
 
+    // Do not lint, because we need to rewrite the lambda
+    s = s.chars().filter(|c| *c != 'o').to_owned().collect();
+
     // Do not lint, because this expression is not assign.
     let mut bar: String = s.chars().filter(|&c| c != 'o').to_owned().collect();
 
@@ -171,6 +198,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 +278,27 @@ 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_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);
+}