Skip to content

Commit 41f5ee1

Browse files
KobzolKobzol
committed
React to review
1 parent 0fcc33e commit 41f5ee1

File tree

3 files changed

+35
-31
lines changed

3 files changed

+35
-31
lines changed

‎clippy_lints/src/assigning_clones.rs

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -76,33 +76,41 @@ fn extract_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<
7676
let (target, kind, resolved_method) = match expr.kind {
7777
ExprKind::MethodCall(path, receiver, [], _span) => {
7878
let args = cx.typeck_results().node_args(expr.hir_id);
79-
let resolved_method = Instance::resolve(cx.tcx, cx.param_env, fn_def_id, args);
79+
80+
// If we could not resolve the method, don't apply the lint
81+
let Ok(Some(resolved_method)) = Instance::resolve(cx.tcx, cx.param_env, fn_def_id, args) else {
82+
return None;
83+
};
8084
if is_trait_method(cx, expr, sym::Clone) && path.ident.name == sym::clone {
8185
(TargetTrait::Clone, CallKind::MethodCall { receiver }, resolved_method)
82-
} else if is_trait_method(cx, expr, sym::ToOwned) && path.ident.name == sym!(to_owned) {
86+
} else if is_trait_method(cx, expr, sym::ToOwned) && path.ident.name.as_str() == "to_owned" {
8387
(TargetTrait::ToOwned, CallKind::MethodCall { receiver }, resolved_method)
8488
} else {
8589
return None;
8690
}
8791
},
88-
ExprKind::Call(function, args) if args.len() == 1 => {
92+
ExprKind::Call(function, [arg]) => {
8993
let kind = cx.typeck_results().node_type(function.hir_id).kind();
90-
let resolved_method = match kind {
94+
95+
// If we could not resolve the method, don't apply the lint
96+
let Ok(Some(resolved_method)) = (match kind {
9197
ty::FnDef(_, args) => Instance::resolve(cx.tcx, cx.param_env, fn_def_id, args),
92-
_ => return None,
98+
_ => Ok(None),
99+
}) else {
100+
return None;
93101
};
94102
if cx.tcx.is_diagnostic_item(sym::to_owned_method, fn_def_id) {
95103
(
96104
TargetTrait::ToOwned,
97-
CallKind::FunctionCall { self_arg: &args[0] },
105+
CallKind::FunctionCall { self_arg: arg },
98106
resolved_method,
99107
)
100108
} else if let Some(trait_did) = cx.tcx.trait_of_item(fn_def_id)
101109
&& cx.tcx.is_diagnostic_item(sym::Clone, trait_did)
102110
{
103111
(
104112
TargetTrait::Clone,
105-
CallKind::FunctionCall { self_arg: &args[0] },
113+
CallKind::FunctionCall { self_arg: arg },
106114
resolved_method,
107115
)
108116
} else {
@@ -111,10 +119,6 @@ fn extract_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<
111119
},
112120
_ => return None,
113121
};
114-
let Ok(Some(resolved_method)) = resolved_method else {
115-
// If we could not resolve the method, don't apply the lint
116-
return None;
117-
};
118122

119123
Some(CallCandidate {
120124
target,
@@ -144,7 +148,7 @@ fn is_ok_to_suggest<'tcx>(cx: &LateContext<'tcx>, lhs: &Expr<'tcx>, call: &CallC
144148
};
145149

146150
// If the method implementation comes from #[derive(Clone)], then don't suggest the lint.
147-
// Automatically generated Clone impls do not override `clone_from`.
151+
// Automatically generated Clone impls do not currently override `clone_from`.
148152
// See e.g. https://github.com/rust-lang/rust/pull/98445#issuecomment-1190681305 for more details.
149153
if cx.tcx.is_builtin_derived(impl_block) {
150154
return false;
@@ -160,7 +164,7 @@ fn is_ok_to_suggest<'tcx>(cx: &LateContext<'tcx>, lhs: &Expr<'tcx>, call: &CallC
160164
TargetTrait::ToOwned => cx.tcx.get_diagnostic_item(sym::ToOwned).and_then(|to_owned| {
161165
cx.tcx
162166
.provided_trait_methods(to_owned)
163-
.find(|item| item.name == sym!(clone_into))
167+
.find(|item| item.name.as_str() == "clone_into")
164168
}),
165169
};
166170
let Some(provided_fn) = provided_fn else {
@@ -213,13 +217,15 @@ struct CallCandidate<'tcx> {
213217
}
214218

215219
impl<'tcx> CallCandidate<'tcx> {
220+
#[inline]
216221
fn message(&self) -> &'static str {
217222
match self.target {
218223
TargetTrait::Clone => "assigning the result of `Clone::clone()` may be inefficient",
219224
TargetTrait::ToOwned => "assigning the result of `ToOwned::to_owned()` may be inefficient",
220225
}
221226
}
222227

228+
#[inline]
223229
fn suggestion_msg(&self) -> &'static str {
224230
match self.target {
225231
TargetTrait::Clone => "use `clone_from()`",
@@ -269,9 +275,7 @@ impl<'tcx> CallCandidate<'tcx> {
269275
// The RHS had to be exactly correct before the call, there is no auto-deref for function calls.
270276
let rhs_sugg = Sugg::hir_with_applicability(cx, self_arg, "_", applicability);
271277

272-
// TODO: how to get rid of the full path? Modify the function call function's (q)path? Or
273-
// auto-import it the trait?
274-
format!("::std::clone::Clone::clone_from({self_sugg}, {rhs_sugg})")
278+
format!("Clone::clone_from({self_sugg}, {rhs_sugg})")
275279
},
276280
}
277281
},
@@ -303,7 +307,7 @@ impl<'tcx> CallCandidate<'tcx> {
303307
},
304308
CallKind::FunctionCall { self_arg, .. } => {
305309
let self_sugg = Sugg::hir_with_applicability(cx, self_arg, "_", applicability);
306-
format!("::std::borrow::ToOwned::clone_into({self_sugg}, {rhs_sugg})")
310+
format!("ToOwned::clone_into({self_sugg}, {rhs_sugg})")
307311
},
308312
}
309313
},

‎tests/ui/assigning_clones.fixed

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,23 +34,23 @@ fn clone_method_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) {
3434
}
3535

3636
fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
37-
::std::clone::Clone::clone_from(mut_thing, ref_thing);
37+
Clone::clone_from(mut_thing, ref_thing);
3838
}
3939

4040
fn clone_function_lhs_val(mut mut_thing: HasCloneFrom, ref_thing: &HasCloneFrom) {
41-
::std::clone::Clone::clone_from(&mut mut_thing, ref_thing);
41+
Clone::clone_from(&mut mut_thing, ref_thing);
4242
}
4343

4444
fn clone_function_through_trait(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
45-
::std::clone::Clone::clone_from(mut_thing, ref_thing);
45+
Clone::clone_from(mut_thing, ref_thing);
4646
}
4747

4848
fn clone_function_through_type(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
49-
::std::clone::Clone::clone_from(mut_thing, ref_thing);
49+
Clone::clone_from(mut_thing, ref_thing);
5050
}
5151

5252
fn clone_function_fully_qualified(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
53-
::std::clone::Clone::clone_from(mut_thing, ref_thing);
53+
Clone::clone_from(mut_thing, ref_thing);
5454
}
5555

5656
fn clone_method_lhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
@@ -164,11 +164,11 @@ fn owned_method_deref(mut_box_string: &mut HasDeref, ref_str: &str) {
164164
}
165165

166166
fn owned_function_mut_ref(mut_thing: &mut String, ref_str: &str) {
167-
::std::borrow::ToOwned::clone_into(ref_str, mut_thing);
167+
ToOwned::clone_into(ref_str, mut_thing);
168168
}
169169

170170
fn owned_function_val(mut mut_thing: String, ref_str: &str) {
171-
::std::borrow::ToOwned::clone_into(ref_str, &mut mut_thing);
171+
ToOwned::clone_into(ref_str, &mut mut_thing);
172172
}
173173

174174
struct FakeToOwned;

‎tests/ui/assigning_clones.stderr

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,31 +23,31 @@ error: assigning the result of `Clone::clone()` may be inefficient
2323
--> $DIR/assigning_clones.rs:37:5
2424
|
2525
LL | *mut_thing = Clone::clone(ref_thing);
26-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(mut_thing, ref_thing)`
26+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(mut_thing, ref_thing)`
2727

2828
error: assigning the result of `Clone::clone()` may be inefficient
2929
--> $DIR/assigning_clones.rs:41:5
3030
|
3131
LL | mut_thing = Clone::clone(ref_thing);
32-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(&mut mut_thing, ref_thing)`
32+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(&mut mut_thing, ref_thing)`
3333

3434
error: assigning the result of `Clone::clone()` may be inefficient
3535
--> $DIR/assigning_clones.rs:45:5
3636
|
3737
LL | *mut_thing = Clone::clone(ref_thing);
38-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(mut_thing, ref_thing)`
38+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(mut_thing, ref_thing)`
3939

4040
error: assigning the result of `Clone::clone()` may be inefficient
4141
--> $DIR/assigning_clones.rs:49:5
4242
|
4343
LL | *mut_thing = HasCloneFrom::clone(ref_thing);
44-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(mut_thing, ref_thing)`
44+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(mut_thing, ref_thing)`
4545

4646
error: assigning the result of `Clone::clone()` may be inefficient
4747
--> $DIR/assigning_clones.rs:53:5
4848
|
4949
LL | *mut_thing = <HasCloneFrom as Clone>::clone(ref_thing);
50-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `::std::clone::Clone::clone_from(mut_thing, ref_thing)`
50+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(mut_thing, ref_thing)`
5151

5252
error: assigning the result of `Clone::clone()` may be inefficient
5353
--> $DIR/assigning_clones.rs:58:5
@@ -95,13 +95,13 @@ error: assigning the result of `ToOwned::to_owned()` may be inefficient
9595
--> $DIR/assigning_clones.rs:167:5
9696
|
9797
LL | *mut_thing = ToOwned::to_owned(ref_str);
98-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `::std::borrow::ToOwned::clone_into(ref_str, mut_thing)`
98+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(ref_str, mut_thing)`
9999

100100
error: assigning the result of `ToOwned::to_owned()` may be inefficient
101101
--> $DIR/assigning_clones.rs:171:5
102102
|
103103
LL | mut_thing = ToOwned::to_owned(ref_str);
104-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `::std::borrow::ToOwned::clone_into(ref_str, &mut mut_thing)`
104+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(ref_str, &mut mut_thing)`
105105

106106
error: aborting due to 17 previous errors
107107

0 commit comments

Comments
 (0)