Auto merge of rust-lang#12105 - GuillaumeGomez:useless-asf-extension, r=llogiq

bors · bors · commit e89968425495 · 2024-01-10T01:13:42.000Z
Extend `useless_asref` lint on `map(clone)`

If you have code like:

```rust
Some(String::new()).as_ref().map(Clone::clone)
```

the `as_ref` call is unneeded.

Interestingly enough, this lint and `map_clone` are starting to share a same "space" where both lints warn about different things for the same code. Not sure what's the policy about such cases though...

r? `@llogiq`

changelog: Extend `useless_asref` lint on `map(clone)`
diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs
@@ -5,6 +5,7 @@ use clippy_utils::ty::{is_copy, is_type_diagnostic_item};
 use clippy_utils::{is_diag_trait_item, match_def_path, paths, peel_blocks};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
+use rustc_hir::def_id::DefId;
 use rustc_lint::LateContext;
 use rustc_middle::mir::Mutability;
 use rustc_middle::ty;
@@ -14,11 +15,35 @@ use rustc_span::{sym, Span};
 
 use super::MAP_CLONE;
 
+// If this `map` is called on an `Option` or a `Result` and the previous call is `as_ref`, we don't
+// run this lint because it would overlap with `useless_asref` which provides a better suggestion
+// in this case.
+fn should_run_lint(cx: &LateContext<'_>, e: &hir::Expr<'_>, method_id: DefId) -> bool {
+    if is_diag_trait_item(cx, method_id, sym::Iterator) {
+        return true;
+    }
+    // We check if it's an `Option` or a `Result`.
+    if let Some(id) = cx.tcx.impl_of_method(method_id) {
+        let identity = cx.tcx.type_of(id).instantiate_identity();
+        if !is_type_diagnostic_item(cx, identity, sym::Option) && !is_type_diagnostic_item(cx, identity, sym::Result) {
+            return false;
+        }
+    } else {
+        return false;
+    }
+    // We check if the previous method call is `as_ref`.
+    if let hir::ExprKind::MethodCall(path1, receiver, _, _) = &e.kind
+        && let hir::ExprKind::MethodCall(path2, _, _, _) = &receiver.kind
+    {
+        return path2.ident.name != sym::as_ref || path1.ident.name != sym::map;
+    }
+
+    true
+}
+
 pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>, msrv: &Msrv) {
     if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
-        && (cx.tcx.impl_of_method(method_id).map_or(false, |id| {
-            is_type_diagnostic_item(cx, cx.tcx.type_of(id).instantiate_identity(), sym::Option)
-        }) || is_diag_trait_item(cx, method_id, sym::Iterator))
+        && should_run_lint(cx, e, method_id)
     {
         match arg.kind {
             hir::ExprKind::Closure(&hir::Closure { body, .. }) => {
diff --git a/clippy_lints/src/methods/useless_asref.rs b/clippy_lints/src/methods/useless_asref.rs
@@ -1,19 +1,52 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::walk_ptrs_ty_depth;
-use clippy_utils::{get_parent_expr, is_trait_method};
+use clippy_utils::{get_parent_expr, is_diag_trait_item, match_def_path, paths, peel_blocks};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
-use rustc_span::sym;
+use rustc_middle::ty::adjustment::Adjust;
+use rustc_middle::ty::{Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor};
+use rustc_span::{sym, Span};
+
+use core::ops::ControlFlow;
 
 use super::USELESS_ASREF;
 
+/// Returns the first type inside the `Option`/`Result` type passed as argument.
+fn get_enum_ty(enum_ty: Ty<'_>) -> Option<Ty<'_>> {
+    struct ContainsTyVisitor {
+        level: usize,
+    }
+
+    impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for ContainsTyVisitor {
+        type BreakTy = Ty<'tcx>;
+
+        fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
+            self.level += 1;
+            if self.level == 1 {
+                t.super_visit_with(self)
+            } else {
+                ControlFlow::Break(t)
+            }
+        }
+    }
+
+    match enum_ty.visit_with(&mut ContainsTyVisitor { level: 0 }) {
+        ControlFlow::Break(ty) => Some(ty),
+        ControlFlow::Continue(()) => None,
+    }
+}
+
 /// Checks for the `USELESS_ASREF` lint.
 pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, recvr: &hir::Expr<'_>) {
     // when we get here, we've already checked that the call name is "as_ref" or "as_mut"
     // check if the call is to the actual `AsRef` or `AsMut` trait
-    if is_trait_method(cx, expr, sym::AsRef) || is_trait_method(cx, expr, sym::AsMut) {
+    let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) else {
+        return;
+    };
+
+    if is_diag_trait_item(cx, def_id, sym::AsRef) || is_diag_trait_item(cx, def_id, sym::AsMut) {
         // check if the type after `as_ref` or `as_mut` is the same as before
         let rcv_ty = cx.typeck_results().expr_ty(recvr);
         let res_ty = cx.typeck_results().expr_ty(expr);
@@ -39,5 +72,89 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str,
                 applicability,
             );
         }
+    } else if match_def_path(cx, def_id, &["core", "option", "Option", call_name])
+        || match_def_path(cx, def_id, &["core", "result", "Result", call_name])
+    {
+        let rcv_ty = cx.typeck_results().expr_ty(recvr).peel_refs();
+        let res_ty = cx.typeck_results().expr_ty(expr).peel_refs();
+
+        if let Some(rcv_ty) = get_enum_ty(rcv_ty)
+            && let Some(res_ty) = get_enum_ty(res_ty)
+            // If the only thing the `as_mut`/`as_ref` call is doing is adding references and not
+            // changing the type, then we can move forward.
+            && rcv_ty.peel_refs() == res_ty.peel_refs()
+            && let Some(parent) = get_parent_expr(cx, expr)
+            && let hir::ExprKind::MethodCall(segment, _, args, _) = parent.kind
+            && segment.ident.span != expr.span
+            // We check that the called method name is `map`.
+            && segment.ident.name == sym::map
+            // And that it only has one argument.
+            && let [arg] = args
+            && is_calling_clone(cx, arg)
+        {
+            lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name);
+        }
+    }
+}
+
+fn check_qpath(cx: &LateContext<'_>, qpath: hir::QPath<'_>, hir_id: hir::HirId) -> bool {
+    // We check it's calling the `clone` method of the `Clone` trait.
+    if let Some(path_def_id) = cx.qpath_res(&qpath, hir_id).opt_def_id() {
+        match_def_path(cx, path_def_id, &paths::CLONE_TRAIT_METHOD)
+    } else {
+        false
     }
 }
+
+fn is_calling_clone(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool {
+    match arg.kind {
+        hir::ExprKind::Closure(&hir::Closure { body, .. }) => {
+            // If it's a closure, we need to check what is called.
+            let closure_body = cx.tcx.hir().body(body);
+            let closure_expr = peel_blocks(closure_body.value);
+            match closure_expr.kind {
+                hir::ExprKind::MethodCall(method, obj, [], _) => {
+                    if method.ident.name == sym::clone
+                        && let Some(fn_id) = cx.typeck_results().type_dependent_def_id(closure_expr.hir_id)
+                        && let Some(trait_id) = cx.tcx.trait_of_item(fn_id)
+                        // We check it's the `Clone` trait.
+                        && cx.tcx.lang_items().clone_trait().map_or(false, |id| id == trait_id)
+                        // no autoderefs
+                        && !cx.typeck_results().expr_adjustments(obj).iter()
+                            .any(|a| matches!(a.kind, Adjust::Deref(Some(..))))
+                    {
+                        true
+                    } else {
+                        false
+                    }
+                },
+                hir::ExprKind::Call(call, [_]) => {
+                    if let hir::ExprKind::Path(qpath) = call.kind {
+                        check_qpath(cx, qpath, call.hir_id)
+                    } else {
+                        false
+                    }
+                },
+                _ => false,
+            }
+        },
+        hir::ExprKind::Path(qpath) => check_qpath(cx, qpath, arg.hir_id),
+        _ => false,
+    }
+}
+
+fn lint_as_ref_clone(cx: &LateContext<'_>, span: Span, recvr: &hir::Expr<'_>, call_name: &str) {
+    let mut applicability = Applicability::MachineApplicable;
+    span_lint_and_sugg(
+        cx,
+        USELESS_ASREF,
+        span,
+        &format!("this call to `{call_name}.map(...)` does nothing"),
+        "try",
+        format!(
+            "{}.clone()",
+            snippet_with_applicability(cx, recvr.span, "..", &mut applicability)
+        ),
+        applicability,
+    );
+}
diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed
@@ -5,6 +5,7 @@
     clippy::many_single_char_names,
     clippy::redundant_clone,
     clippy::redundant_closure,
+    clippy::useless_asref,
     clippy::useless_vec
 )]
 
@@ -63,6 +64,24 @@ fn main() {
     }
 
     let x = Some(String::new());
-    let y = x.as_ref().cloned();
+    let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
+    let y = x.cloned();
     //~^ ERROR: you are explicitly cloning with `.map()`
+    let y = x.cloned();
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    let y = x.cloned();
+    //~^ ERROR: you are explicitly cloning with `.map()`
+
+    // Testing with `Result` now.
+    let x: Result<String, ()> = Ok(String::new());
+    let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
+    let y = x.cloned();
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    let y = x.cloned();
+
+    // We ensure that no warning is emitted here because `useless_asref` is taking over.
+    let x = Some(String::new());
+    let y = x.as_ref().map(|x| String::clone(x));
+    let x: Result<String, ()> = Ok(String::new());
+    let y = x.as_ref().map(|x| String::clone(x));
 }
diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs
@@ -5,6 +5,7 @@
     clippy::many_single_char_names,
     clippy::redundant_clone,
     clippy::redundant_closure,
+    clippy::useless_asref,
     clippy::useless_vec
 )]
 
@@ -63,6 +64,24 @@ fn main() {
     }
 
     let x = Some(String::new());
-    let y = x.as_ref().map(|x| String::clone(x));
+    let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
+    let y = x.map(|x| String::clone(x));
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    let y = x.map(Clone::clone);
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    let y = x.map(String::clone);
+    //~^ ERROR: you are explicitly cloning with `.map()`
+
+    // Testing with `Result` now.
+    let x: Result<String, ()> = Ok(String::new());
+    let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
+    let y = x.map(|x| String::clone(x));
     //~^ ERROR: you are explicitly cloning with `.map()`
+    let y = x.map(|x| String::clone(x));
+
+    // We ensure that no warning is emitted here because `useless_asref` is taking over.
+    let x = Some(String::new());
+    let y = x.as_ref().map(|x| String::clone(x));
+    let x: Result<String, ()> = Ok(String::new());
+    let y = x.as_ref().map(|x| String::clone(x));
 }
diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr
@@ -1,5 +1,5 @@
 error: you are using an explicit closure for copying elements
-  --> $DIR/map_clone.rs:12:22
+  --> $DIR/map_clone.rs:13:22
    |
 LL |     let _: Vec<i8> = vec![5_i8; 6].iter().map(|x| *x).collect();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `vec![5_i8; 6].iter().copied()`
@@ -8,40 +8,64 @@ LL |     let _: Vec<i8> = vec![5_i8; 6].iter().map(|x| *x).collect();
    = help: to override `-D warnings` add `#[allow(clippy::map_clone)]`
 
 error: you are using an explicit closure for cloning elements
-  --> $DIR/map_clone.rs:13:26
+  --> $DIR/map_clone.rs:14:26
    |
 LL |     let _: Vec<String> = vec![String::new()].iter().map(|x| x.clone()).collect();
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()`
 
 error: you are using an explicit closure for copying elements
-  --> $DIR/map_clone.rs:14:23
+  --> $DIR/map_clone.rs:15:23
    |
 LL |     let _: Vec<u32> = vec![42, 43].iter().map(|&x| x).collect();
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `vec![42, 43].iter().copied()`
 
 error: you are using an explicit closure for copying elements
-  --> $DIR/map_clone.rs:16:26
+  --> $DIR/map_clone.rs:17:26
    |
 LL |     let _: Option<u64> = Some(&16).map(|b| *b);
    |                          ^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `Some(&16).copied()`
 
 error: you are using an explicit closure for copying elements
-  --> $DIR/map_clone.rs:17:25
+  --> $DIR/map_clone.rs:18:25
    |
 LL |     let _: Option<u8> = Some(&1).map(|x| x.clone());
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `Some(&1).copied()`
 
 error: you are needlessly cloning iterator elements
-  --> $DIR/map_clone.rs:28:29
+  --> $DIR/map_clone.rs:29:29
    |
 LL |     let _ = std::env::args().map(|v| v.clone());
    |                             ^^^^^^^^^^^^^^^^^^^ help: remove the `map` call
 
 error: you are explicitly cloning with `.map()`
-  --> $DIR/map_clone.rs:66:13
+  --> $DIR/map_clone.rs:68:13
    |
-LL |     let y = x.as_ref().map(|x| String::clone(x));
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.as_ref().cloned()`
+LL |     let y = x.map(|x| String::clone(x));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
 
-error: aborting due to 7 previous errors
+error: you are explicitly cloning with `.map()`
+  --> $DIR/map_clone.rs:70:13
+   |
+LL |     let y = x.map(Clone::clone);
+   |             ^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
+
+error: you are explicitly cloning with `.map()`
+  --> $DIR/map_clone.rs:72:13
+   |
+LL |     let y = x.map(String::clone);
+   |             ^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
+
+error: you are explicitly cloning with `.map()`
+  --> $DIR/map_clone.rs:78:13
+   |
+LL |     let y = x.map(|x| String::clone(x));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
+
+error: you are explicitly cloning with `.map()`
+  --> $DIR/map_clone.rs:80:13
+   |
+LL |     let y = x.map(|x| String::clone(x));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
+
+error: aborting due to 11 previous errors
 
diff --git a/tests/ui/useless_asref.fixed b/tests/ui/useless_asref.fixed
@@ -2,7 +2,9 @@
 #![allow(
     clippy::explicit_auto_deref,
     clippy::uninlined_format_args,
-    clippy::needless_pass_by_ref_mut
+    clippy::map_clone,
+    clippy::needless_pass_by_ref_mut,
+    clippy::redundant_closure
 )]
 
 use std::fmt::Debug;
@@ -134,10 +136,12 @@ fn generic_ok<U: AsMut<T> + AsRef<T> + ?Sized, T: Debug + ?Sized>(mru: &mut U) {
 
 fn foo() {
     let x = Some(String::new());
-    let y = x.as_ref().cloned();
-    //~^ ERROR: you are explicitly cloning with `.map()`
-    let y = x.as_ref().cloned();
-    //~^ ERROR: you are explicitly cloning with `.map()`
+    let z = x.clone();
+    //~^ ERROR: this call to `as_ref.map(...)` does nothing
+    let z = x.clone();
+    //~^ ERROR: this call to `as_ref.map(...)` does nothing
+    let z = x.clone();
+    //~^ ERROR: this call to `as_ref.map(...)` does nothing
 }
 
 fn main() {
diff --git a/tests/ui/useless_asref.rs b/tests/ui/useless_asref.rs
@@ -2,7 +2,9 @@
 #![allow(
     clippy::explicit_auto_deref,
     clippy::uninlined_format_args,
-    clippy::needless_pass_by_ref_mut
+    clippy::map_clone,
+    clippy::needless_pass_by_ref_mut,
+    clippy::redundant_closure
 )]
 
 use std::fmt::Debug;
@@ -134,10 +136,12 @@ fn generic_ok<U: AsMut<T> + AsRef<T> + ?Sized, T: Debug + ?Sized>(mru: &mut U) {
 
 fn foo() {
     let x = Some(String::new());
-    let y = x.as_ref().map(Clone::clone);
-    //~^ ERROR: you are explicitly cloning with `.map()`
-    let y = x.as_ref().map(String::clone);
-    //~^ ERROR: you are explicitly cloning with `.map()`
+    let z = x.as_ref().map(String::clone);
+    //~^ ERROR: this call to `as_ref.map(...)` does nothing
+    let z = x.as_ref().map(|z| z.clone());
+    //~^ ERROR: this call to `as_ref.map(...)` does nothing
+    let z = x.as_ref().map(|z| String::clone(z));
+    //~^ ERROR: this call to `as_ref.map(...)` does nothing
 }
 
 fn main() {
diff --git a/tests/ui/useless_asref.stderr b/tests/ui/useless_asref.stderr
