Skip to content

Commit 12f7c17

Browse files
borsbors
committed
Auto merge of rust-lang#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 <rust-lang/rust-clippy#12528 (comment)>. changelog: [`useless_asref`]: do not lint `.as_ref().map(Arc::clone)` and similar
2 parents db41621 + fed2f28 commit 12f7c17

File tree

6 files changed

+69
-25
lines changed

6 files changed

+69
-25
lines changed

‎clippy_lints/src/methods/map_clone.rs‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use clippy_config::msrvs::{self, Msrv};
22
use clippy_utils::diagnostics::span_lint_and_sugg;
33
use clippy_utils::source::snippet_with_applicability;
4-
use clippy_utils::ty::{is_copy, is_type_diagnostic_item};
4+
use clippy_utils::ty::{is_copy, is_type_diagnostic_item, should_call_clone_as_function};
55
use clippy_utils::{is_diag_trait_item, match_def_path, paths, peel_blocks};
66
use rustc_errors::Applicability;
77
use rustc_hir as hir;
@@ -124,11 +124,7 @@ fn handle_path(
124124
&& let ty::Ref(_, ty, Mutability::Not) = ty.kind()
125125
&& let ty::FnDef(_, lst) = cx.typeck_results().expr_ty(arg).kind()
126126
&& lst.iter().all(|l| l.as_type() == Some(*ty))
127-
&& !matches!(
128-
ty.ty_adt_def()
129-
.and_then(|adt_def| cx.tcx.get_diagnostic_name(adt_def.did())),
130-
Some(sym::Arc | sym::ArcWeak | sym::Rc | sym::RcWeak)
131-
)
127+
&& !should_call_clone_as_function(cx, *ty)
132128
{
133129
lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs()));
134130
}

‎clippy_lints/src/methods/useless_asref.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
22
use clippy_utils::source::snippet_with_applicability;
3-
use clippy_utils::ty::walk_ptrs_ty_depth;
3+
use clippy_utils::ty::{should_call_clone_as_function, walk_ptrs_ty_depth};
44
use clippy_utils::{
55
get_parent_expr, is_diag_trait_item, match_def_path, path_to_local_id, paths, peel_blocks, strip_pat_refs,
66
};
@@ -93,6 +93,8 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str,
9393
// And that it only has one argument.
9494
&& let [arg] = args
9595
&& is_calling_clone(cx, arg)
96+
// And that we are not recommending recv.clone() over Arc::clone() or similar
97+
&& !should_call_clone_as_function(cx, rcv_ty)
9698
{
9799
lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name);
98100
}

‎clippy_utils/src/ty.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,16 @@ pub fn get_type_diagnostic_name(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Symb
159159
}
160160
}
161161

162+
/// Returns true if `ty` is a type on which calling `Clone` through a function instead of
163+
/// as a method, such as `Arc::clone()` is considered idiomatic. Lints should avoid suggesting to
164+
/// replace instances of `ty::Clone()` by `.clone()` for objects of those types.
165+
pub fn should_call_clone_as_function(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
166+
matches!(
167+
get_type_diagnostic_name(cx, ty),
168+
Some(sym::Arc | sym::ArcWeak | sym::Rc | sym::RcWeak)
169+
)
170+
}
171+
162172
/// Returns true if ty has `iter` or `iter_mut` methods
163173
pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option<Symbol> {
164174
// FIXME: instead of this hard-coded list, we should check if `<adt>::iter`

‎tests/ui/useless_asref.fixed‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
)]
99

1010
use std::fmt::Debug;
11+
use std::rc::{Rc, Weak as RcWeak};
12+
use std::sync::{Arc, Weak as ArcWeak};
1113

1214
struct FakeAsRef;
1315

@@ -180,6 +182,22 @@ mod issue12135 {
180182
}
181183
}
182184

185+
fn issue_12528() {
186+
struct Foo;
187+
188+
let opt = Some(Arc::new(Foo));
189+
let _ = opt.as_ref().map(Arc::clone);
190+
191+
let opt = Some(Rc::new(Foo));
192+
let _ = opt.as_ref().map(Rc::clone);
193+
194+
let opt = Some(Arc::downgrade(&Arc::new(Foo)));
195+
let _ = opt.as_ref().map(ArcWeak::clone);
196+
197+
let opt = Some(Rc::downgrade(&Rc::new(Foo)));
198+
let _ = opt.as_ref().map(RcWeak::clone);
199+
}
200+
183201
fn main() {
184202
not_ok();
185203
ok();

‎tests/ui/useless_asref.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
)]
99

1010
use std::fmt::Debug;
11+
use std::rc::{Rc, Weak as RcWeak};
12+
use std::sync::{Arc, Weak as ArcWeak};
1113

1214
struct FakeAsRef;
1315

@@ -180,6 +182,22 @@ mod issue12135 {
180182
}
181183
}
182184

185+
fn issue_12528() {
186+
struct Foo;
187+
188+
let opt = Some(Arc::new(Foo));
189+
let _ = opt.as_ref().map(Arc::clone);
190+
191+
let opt = Some(Rc::new(Foo));
192+
let _ = opt.as_ref().map(Rc::clone);
193+
194+
let opt = Some(Arc::downgrade(&Arc::new(Foo)));
195+
let _ = opt.as_ref().map(ArcWeak::clone);
196+
197+
let opt = Some(Rc::downgrade(&Rc::new(Foo)));
198+
let _ = opt.as_ref().map(RcWeak::clone);
199+
}
200+
183201
fn main() {
184202
not_ok();
185203
ok();

‎tests/ui/useless_asref.stderr‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: this call to `as_ref` does nothing
2-
--> tests/ui/useless_asref.rs:48:18
2+
--> tests/ui/useless_asref.rs:50:18
33
|
44
LL | foo_rstr(rstr.as_ref());
55
| ^^^^^^^^^^^^^ help: try: `rstr`
@@ -11,103 +11,103 @@ LL | #![deny(clippy::useless_asref)]
1111
| ^^^^^^^^^^^^^^^^^^^^^
1212

1313
error: this call to `as_ref` does nothing
14-
--> tests/ui/useless_asref.rs:50:20
14+
--> tests/ui/useless_asref.rs:52:20
1515
|
1616
LL | foo_rslice(rslice.as_ref());
1717
| ^^^^^^^^^^^^^^^ help: try: `rslice`
1818

1919
error: this call to `as_mut` does nothing
20-
--> tests/ui/useless_asref.rs:54:21
20+
--> tests/ui/useless_asref.rs:56:21
2121
|
2222
LL | foo_mrslice(mrslice.as_mut());
2323
| ^^^^^^^^^^^^^^^^ help: try: `mrslice`
2424

2525
error: this call to `as_ref` does nothing
26-
--> tests/ui/useless_asref.rs:56:20
26+
--> tests/ui/useless_asref.rs:58:20
2727
|
2828
LL | foo_rslice(mrslice.as_ref());
2929
| ^^^^^^^^^^^^^^^^ help: try: `mrslice`
3030

3131
error: this call to `as_ref` does nothing
32-
--> tests/ui/useless_asref.rs:63:20
32+
--> tests/ui/useless_asref.rs:65:20
3333
|
3434
LL | foo_rslice(rrrrrslice.as_ref());
3535
| ^^^^^^^^^^^^^^^^^^^ help: try: `rrrrrslice`
3636

3737
error: this call to `as_ref` does nothing
38-
--> tests/ui/useless_asref.rs:65:18
38+
--> tests/ui/useless_asref.rs:67:18
3939
|
4040
LL | foo_rstr(rrrrrstr.as_ref());
4141
| ^^^^^^^^^^^^^^^^^ help: try: `rrrrrstr`
4242

4343
error: this call to `as_mut` does nothing
44-
--> tests/ui/useless_asref.rs:70:21
44+
--> tests/ui/useless_asref.rs:72:21
4545
|
4646
LL | foo_mrslice(mrrrrrslice.as_mut());
4747
| ^^^^^^^^^^^^^^^^^^^^ help: try: `mrrrrrslice`
4848

4949
error: this call to `as_ref` does nothing
50-
--> tests/ui/useless_asref.rs:72:20
50+
--> tests/ui/useless_asref.rs:74:20
5151
|
5252
LL | foo_rslice(mrrrrrslice.as_ref());
5353
| ^^^^^^^^^^^^^^^^^^^^ help: try: `mrrrrrslice`
5454

5555
error: this call to `as_ref` does nothing
56-
--> tests/ui/useless_asref.rs:76:16
56+
--> tests/ui/useless_asref.rs:78:16
5757
|
5858
LL | foo_rrrrmr((&&&&MoreRef).as_ref());
5959
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `(&&&&MoreRef)`
6060

6161
error: this call to `as_mut` does nothing
62-
--> tests/ui/useless_asref.rs:126:13
62+
--> tests/ui/useless_asref.rs:128:13
6363
|
6464
LL | foo_mrt(mrt.as_mut());
6565
| ^^^^^^^^^^^^ help: try: `mrt`
6666

6767
error: this call to `as_ref` does nothing
68-
--> tests/ui/useless_asref.rs:128:12
68+
--> tests/ui/useless_asref.rs:130:12
6969
|
7070
LL | foo_rt(mrt.as_ref());
7171
| ^^^^^^^^^^^^ help: try: `mrt`
7272

7373
error: this call to `as_ref.map(...)` does nothing
74-
--> tests/ui/useless_asref.rs:139:13
74+
--> tests/ui/useless_asref.rs:141:13
7575
|
7676
LL | let z = x.as_ref().map(String::clone);
7777
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
7878

7979
error: this call to `as_ref.map(...)` does nothing
80-
--> tests/ui/useless_asref.rs:141:13
80+
--> tests/ui/useless_asref.rs:143:13
8181
|
8282
LL | let z = x.as_ref().map(|z| z.clone());
8383
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
8484

8585
error: this call to `as_ref.map(...)` does nothing
86-
--> tests/ui/useless_asref.rs:143:13
86+
--> tests/ui/useless_asref.rs:145:13
8787
|
8888
LL | let z = x.as_ref().map(|z| String::clone(z));
8989
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
9090

9191
error: this call to `as_ref.map(...)` does nothing
92-
--> tests/ui/useless_asref.rs:167:9
92+
--> tests/ui/useless_asref.rs:169:9
9393
|
9494
LL | x.field.as_ref().map(|v| v.clone());
9595
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()`
9696

9797
error: this call to `as_ref.map(...)` does nothing
98-
--> tests/ui/useless_asref.rs:169:9
98+
--> tests/ui/useless_asref.rs:171:9
9999
|
100100
LL | x.field.as_ref().map(Clone::clone);
101101
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()`
102102

103103
error: this call to `as_ref.map(...)` does nothing
104-
--> tests/ui/useless_asref.rs:171:9
104+
--> tests/ui/useless_asref.rs:173:9
105105
|
106106
LL | x.field.as_ref().map(|v| Clone::clone(v));
107107
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()`
108108

109109
error: this call to `as_ref.map(...)` does nothing
110-
--> tests/ui/useless_asref.rs:176:9
110+
--> tests/ui/useless_asref.rs:178:9
111111
|
112112
LL | Some(1).as_ref().map(|&x| x.clone());
113113
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(1).clone()`

0 commit comments

Comments
 (0)