Auto merge of #12535 - samueltardieu:issue-12528, r=y21

`useless_asref`: do not lint `.as_ref().map(Arc::clone)`

This applies to `Arc`, `Rc`, and their weak variants. Using `.clone()` would be less idiomatic.

This follows the discussion in <https://github.com/rust-lang/rust-clippy/issues/12528#issuecomment-2014444305>.

changelog: [`useless_asref`]: do not lint `.as_ref().map(Arc::clone)` and similar

6 files changed, 69 insertions, 25 deletions

diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs
index 0ba8cbbe699..9d020092ccb 100644
--- a/clippy_lints/src/methods/map_clone.rs
+++ b/clippy_lints/src/methods/map_clone.rs
@@ -1,7 +1,7 @@
 use clippy_config::msrvs::{self, Msrv};
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::ty::{is_copy, is_type_diagnostic_item};
+use clippy_utils::ty::{is_copy, is_type_diagnostic_item, should_call_clone_as_function};
 use clippy_utils::{is_diag_trait_item, match_def_path, paths, peel_blocks};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
@@ -124,11 +124,7 @@ fn handle_path(
             && let ty::Ref(_, ty, Mutability::Not) = ty.kind()
             && let ty::FnDef(_, lst) = cx.typeck_results().expr_ty(arg).kind()
             && lst.iter().all(|l| l.as_type() == Some(*ty))
-            && !matches!(
-                ty.ty_adt_def()
-                    .and_then(|adt_def| cx.tcx.get_diagnostic_name(adt_def.did())),
-                Some(sym::Arc | sym::ArcWeak | sym::Rc | sym::RcWeak)
-            )
+            && !should_call_clone_as_function(cx, *ty)
         {
             lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs()));
         }
diff --git a/clippy_lints/src/methods/useless_asref.rs b/clippy_lints/src/methods/useless_asref.rs
index b8baad18cc8..474103fccd2 100644
--- a/clippy_lints/src/methods/useless_asref.rs
+++ b/clippy_lints/src/methods/useless_asref.rs
@@ -1,6 +1,6 @@
 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::ty::{should_call_clone_as_function, walk_ptrs_ty_depth};
 use clippy_utils::{
     get_parent_expr, is_diag_trait_item, match_def_path, path_to_local_id, paths, peel_blocks, strip_pat_refs,
 };
@@ -93,6 +93,8 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str,
             // And that it only has one argument.
             && let [arg] = args
             && is_calling_clone(cx, arg)
+            // And that we are not recommending recv.clone() over Arc::clone() or similar
+            && !should_call_clone_as_function(cx, rcv_ty)
         {
             lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name);
         }
diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs
index a2595580eaf..3924eb5a81c 100644
--- a/clippy_utils/src/ty.rs
+++ b/clippy_utils/src/ty.rs
@@ -159,6 +159,16 @@ pub fn get_type_diagnostic_name(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Symb
     }
 }
 
+/// Returns true if `ty` is a type on which calling `Clone` through a function instead of
+/// as a method, such as `Arc::clone()` is considered idiomatic. Lints should avoid suggesting to
+/// replace instances of `ty::Clone()` by `.clone()` for objects of those types.
+pub fn should_call_clone_as_function(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
+    matches!(
+        get_type_diagnostic_name(cx, ty),
+        Some(sym::Arc | sym::ArcWeak | sym::Rc | sym::RcWeak)
+    )
+}
+
 /// Returns true if ty has `iter` or `iter_mut` methods
 pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option<Symbol> {
     // FIXME: instead of this hard-coded list, we should check if `<adt>::iter`
diff --git a/tests/ui/useless_asref.fixed b/tests/ui/useless_asref.fixed
index c98f2928e03..ddbb9255b46 100644
--- a/tests/ui/useless_asref.fixed
+++ b/tests/ui/useless_asref.fixed
@@ -8,6 +8,8 @@
 )]
 
 use std::fmt::Debug;
+use std::rc::{Rc, Weak as RcWeak};
+use std::sync::{Arc, Weak as ArcWeak};
 
 struct FakeAsRef;
 
@@ -180,6 +182,22 @@ mod issue12135 {
     }
 }
 
+fn issue_12528() {
+    struct Foo;
+
+    let opt = Some(Arc::new(Foo));
+    let _ = opt.as_ref().map(Arc::clone);
+
+    let opt = Some(Rc::new(Foo));
+    let _ = opt.as_ref().map(Rc::clone);
+
+    let opt = Some(Arc::downgrade(&Arc::new(Foo)));
+    let _ = opt.as_ref().map(ArcWeak::clone);
+
+    let opt = Some(Rc::downgrade(&Rc::new(Foo)));
+    let _ = opt.as_ref().map(RcWeak::clone);
+}
+
 fn main() {
     not_ok();
     ok();
diff --git a/tests/ui/useless_asref.rs b/tests/ui/useless_asref.rs
index f9d603f116d..b0405e930a2 100644
--- a/tests/ui/useless_asref.rs
+++ b/tests/ui/useless_asref.rs
@@ -8,6 +8,8 @@
 )]
 
 use std::fmt::Debug;
+use std::rc::{Rc, Weak as RcWeak};
+use std::sync::{Arc, Weak as ArcWeak};
 
 struct FakeAsRef;
 
@@ -180,6 +182,22 @@ mod issue12135 {
     }
 }
 
+fn issue_12528() {
+    struct Foo;
+
+    let opt = Some(Arc::new(Foo));
+    let _ = opt.as_ref().map(Arc::clone);
+
+    let opt = Some(Rc::new(Foo));
+    let _ = opt.as_ref().map(Rc::clone);
+
+    let opt = Some(Arc::downgrade(&Arc::new(Foo)));
+    let _ = opt.as_ref().map(ArcWeak::clone);
+
+    let opt = Some(Rc::downgrade(&Rc::new(Foo)));
+    let _ = opt.as_ref().map(RcWeak::clone);
+}
+
 fn main() {
     not_ok();
     ok();
diff --git a/tests/ui/useless_asref.stderr b/tests/ui/useless_asref.stderr
index c7d622ec2dc..5f495c39670 100644
--- a/tests/ui/useless_asref.stderr
+++ b/tests/ui/useless_asref.stderr
@@ -1,5 +1,5 @@
 error: this call to `as_ref` does nothing
-  --> tests/ui/useless_asref.rs:48:18
+  --> tests/ui/useless_asref.rs:50:18
    |
 LL |         foo_rstr(rstr.as_ref());
    |                  ^^^^^^^^^^^^^ help: try: `rstr`
@@ -11,103 +11,103 @@ LL | #![deny(clippy::useless_asref)]
    |         ^^^^^^^^^^^^^^^^^^^^^
 
 error: this call to `as_ref` does nothing
-  --> tests/ui/useless_asref.rs:50:20
+  --> tests/ui/useless_asref.rs:52:20
    |
 LL |         foo_rslice(rslice.as_ref());
    |                    ^^^^^^^^^^^^^^^ help: try: `rslice`
 
 error: this call to `as_mut` does nothing
-  --> tests/ui/useless_asref.rs:54:21
+  --> tests/ui/useless_asref.rs:56:21
    |
 LL |         foo_mrslice(mrslice.as_mut());
    |                     ^^^^^^^^^^^^^^^^ help: try: `mrslice`
 
 error: this call to `as_ref` does nothing
-  --> tests/ui/useless_asref.rs:56:20
+  --> tests/ui/useless_asref.rs:58:20
    |
 LL |         foo_rslice(mrslice.as_ref());
    |                    ^^^^^^^^^^^^^^^^ help: try: `mrslice`
 
 error: this call to `as_ref` does nothing
-  --> tests/ui/useless_asref.rs:63:20
+  --> tests/ui/useless_asref.rs:65:20
    |
 LL |         foo_rslice(rrrrrslice.as_ref());
    |                    ^^^^^^^^^^^^^^^^^^^ help: try: `rrrrrslice`
 
 error: this call to `as_ref` does nothing
-  --> tests/ui/useless_asref.rs:65:18
+  --> tests/ui/useless_asref.rs:67:18
    |
 LL |         foo_rstr(rrrrrstr.as_ref());
    |                  ^^^^^^^^^^^^^^^^^ help: try: `rrrrrstr`
 
 error: this call to `as_mut` does nothing
-  --> tests/ui/useless_asref.rs:70:21
+  --> tests/ui/useless_asref.rs:72:21
    |
 LL |         foo_mrslice(mrrrrrslice.as_mut());
    |                     ^^^^^^^^^^^^^^^^^^^^ help: try: `mrrrrrslice`
 
 error: this call to `as_ref` does nothing
-  --> tests/ui/useless_asref.rs:72:20
+  --> tests/ui/useless_asref.rs:74:20
    |
 LL |         foo_rslice(mrrrrrslice.as_ref());
    |                    ^^^^^^^^^^^^^^^^^^^^ help: try: `mrrrrrslice`
 
 error: this call to `as_ref` does nothing
-  --> tests/ui/useless_asref.rs:76:16
+  --> tests/ui/useless_asref.rs:78:16
    |
 LL |     foo_rrrrmr((&&&&MoreRef).as_ref());
    |                ^^^^^^^^^^^^^^^^^^^^^^ help: try: `(&&&&MoreRef)`
 
 error: this call to `as_mut` does nothing
-  --> tests/ui/useless_asref.rs:126:13
+  --> tests/ui/useless_asref.rs:128:13
    |
 LL |     foo_mrt(mrt.as_mut());
    |             ^^^^^^^^^^^^ help: try: `mrt`
 
 error: this call to `as_ref` does nothing
-  --> tests/ui/useless_asref.rs:128:12
+  --> tests/ui/useless_asref.rs:130:12
    |
 LL |     foo_rt(mrt.as_ref());
    |            ^^^^^^^^^^^^ help: try: `mrt`
 
 error: this call to `as_ref.map(...)` does nothing
-  --> tests/ui/useless_asref.rs:139:13
+  --> tests/ui/useless_asref.rs:141:13
    |
 LL |     let z = x.as_ref().map(String::clone);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
 
 error: this call to `as_ref.map(...)` does nothing
-  --> tests/ui/useless_asref.rs:141:13
+  --> tests/ui/useless_asref.rs:143:13
    |
 LL |     let z = x.as_ref().map(|z| z.clone());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
 
 error: this call to `as_ref.map(...)` does nothing
-  --> tests/ui/useless_asref.rs:143:13
+  --> tests/ui/useless_asref.rs:145:13
    |
 LL |     let z = x.as_ref().map(|z| String::clone(z));
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
 
 error: this call to `as_ref.map(...)` does nothing
-  --> tests/ui/useless_asref.rs:167:9
+  --> tests/ui/useless_asref.rs:169:9
    |
 LL |         x.field.as_ref().map(|v| v.clone());
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()`
 
 error: this call to `as_ref.map(...)` does nothing
-  --> tests/ui/useless_asref.rs:169:9
+  --> tests/ui/useless_asref.rs:171:9
    |
 LL |         x.field.as_ref().map(Clone::clone);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()`
 
 error: this call to `as_ref.map(...)` does nothing
-  --> tests/ui/useless_asref.rs:171:9
+  --> tests/ui/useless_asref.rs:173:9
    |
 LL |         x.field.as_ref().map(|v| Clone::clone(v));
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()`
 
 error: this call to `as_ref.map(...)` does nothing
-  --> tests/ui/useless_asref.rs:176:9
+  --> tests/ui/useless_asref.rs:178:9
    |
 LL |         Some(1).as_ref().map(|&x| x.clone());
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(1).clone()`