Skip to content

Commit

Permalink
Auto merge of rust-lang#12783 - shanretoo:fix-assigning-clones, r=bly…
Browse files Browse the repository at this point in the history 
…xyas

fix: use hir_with_context to produce correct snippets for assigning_clones

The `assigning_clones` lint is producing wrong output when the assignment is a macro call.
Since Applicability level `Unspecified` will never be changed inside `hir_with_applicability`, so it is safe here to replace `hir_with_applicability` with `hir_with_context` to generate snippets of the macro call instead of the expansion.

fixes rust-lang#12776

changelog: [`assigning_clones`]: use `hir_with_context` to produce correct snippets
  • Loading branch information
@bors
bors committed May 9, 2024
2 parents 5a28d8f + 99a42ba commit baf2a23
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 20 deletions.
26 changes: 17 additions & 9 deletions clippy_lints/src/assigning_clones.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rustc_middle::ty::{self, Instance, Mutability};
use rustc_session::impl_lint_pass;
use rustc_span::def_id::DefId;
use rustc_span::symbol::sym;
use rustc_span::ExpnKind;
use rustc_span::{ExpnKind, SyntaxContext};

declare_clippy_lint! {
/// ### What it does
Expand Down Expand Up @@ -68,7 +68,8 @@ impl_lint_pass!(AssigningClones => [ASSIGNING_CLONES]);
impl<'tcx> LateLintPass<'tcx> for AssigningClones {
fn check_expr(&mut self, cx: &LateContext<'tcx>, assign_expr: &'tcx Expr<'_>) {
// Do not fire the lint in macros
let expn_data = assign_expr.span().ctxt().outer_expn_data();
let ctxt = assign_expr.span().ctxt();
let expn_data = ctxt.outer_expn_data();
match expn_data.kind {
ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) | ExpnKind::Macro(..) => return,
ExpnKind::Root => {},
Expand All @@ -83,7 +84,7 @@ impl<'tcx> LateLintPass<'tcx> for AssigningClones {
};

if is_ok_to_suggest(cx, lhs, &call, &self.msrv) {
suggest(cx, assign_expr, lhs, &call);
suggest(cx, ctxt, assign_expr, lhs, &call);
}
}

Expand Down Expand Up @@ -221,14 +222,20 @@ fn is_ok_to_suggest<'tcx>(cx: &LateContext<'tcx>, lhs: &Expr<'tcx>, call: &CallC
implemented_fns.contains_key(&provided_fn.def_id)
}

fn suggest<'tcx>(cx: &LateContext<'tcx>, assign_expr: &Expr<'tcx>, lhs: &Expr<'tcx>, call: &CallCandidate<'tcx>) {
fn suggest<'tcx>(
cx: &LateContext<'tcx>,
ctxt: SyntaxContext,
assign_expr: &Expr<'tcx>,
lhs: &Expr<'tcx>,
call: &CallCandidate<'tcx>,
) {
span_lint_and_then(cx, ASSIGNING_CLONES, assign_expr.span, call.message(), |diag| {
let mut applicability = Applicability::Unspecified;

diag.span_suggestion(
assign_expr.span,
call.suggestion_msg(),
call.suggested_replacement(cx, lhs, &mut applicability),
call.suggested_replacement(cx, ctxt, lhs, &mut applicability),
applicability,
);
});
Expand Down Expand Up @@ -274,6 +281,7 @@ impl<'tcx> CallCandidate<'tcx> {
fn suggested_replacement(
&self,
cx: &LateContext<'tcx>,
ctxt: SyntaxContext,
lhs: &Expr<'tcx>,
applicability: &mut Applicability,
) -> String {
Expand All @@ -293,7 +301,7 @@ impl<'tcx> CallCandidate<'tcx> {
// Determine whether we need to reference the argument to clone_from().
let clone_receiver_type = cx.typeck_results().expr_ty(receiver);
let clone_receiver_adj_type = cx.typeck_results().expr_ty_adjusted(receiver);
let mut arg_sugg = Sugg::hir_with_applicability(cx, receiver, "_", applicability);
let mut arg_sugg = Sugg::hir_with_context(cx, receiver, ctxt, "_", applicability);
if clone_receiver_type != clone_receiver_adj_type {
// The receiver may have been a value type, so we need to add an `&` to
// be sure the argument to clone_from will be a reference.
Expand All @@ -311,7 +319,7 @@ impl<'tcx> CallCandidate<'tcx> {
Sugg::hir_with_applicability(cx, lhs, "_", applicability).mut_addr()
};
// The RHS had to be exactly correct before the call, there is no auto-deref for function calls.
let rhs_sugg = Sugg::hir_with_applicability(cx, self_arg, "_", applicability);
let rhs_sugg = Sugg::hir_with_context(cx, self_arg, ctxt, "_", applicability);

format!("Clone::clone_from({self_sugg}, {rhs_sugg})")
},
Expand Down Expand Up @@ -340,11 +348,11 @@ impl<'tcx> CallCandidate<'tcx> {

match self.kind {
CallKind::MethodCall { receiver } => {
let receiver_sugg = Sugg::hir_with_applicability(cx, receiver, "_", applicability);
let receiver_sugg = Sugg::hir_with_context(cx, receiver, ctxt, "_", applicability);
format!("{receiver_sugg}.clone_into({rhs_sugg})")
},
CallKind::FunctionCall { self_arg, .. } => {
let self_sugg = Sugg::hir_with_applicability(cx, self_arg, "_", applicability);
let self_sugg = Sugg::hir_with_context(cx, self_arg, ctxt, "_", applicability);
format!("ToOwned::clone_into({self_sugg}, {rhs_sugg})")
},
}
Expand Down
20 changes: 20 additions & 0 deletions tests/ui/assigning_clones.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ fn clone_method_rhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFr
mut_thing.clone_from(ref_thing + ref_thing);
}

fn clone_method_macro() {
let mut s = String::from("");
s.clone_from(&format!("{} {}", "hello", "world"));
}

fn clone_function_macro() {
let mut s = String::from("");
Clone::clone_from(&mut s, &format!("{} {}", "hello", "world"));
}

fn assign_to_init_mut_var(b: HasCloneFrom) -> HasCloneFrom {
let mut a = HasCloneFrom;
for _ in 1..10 {
Expand Down Expand Up @@ -214,6 +224,16 @@ fn owned_function_val(mut mut_thing: String, ref_str: &str) {
ToOwned::clone_into(ref_str, &mut mut_thing);
}

fn owned_method_macro() {
let mut s = String::from("");
format!("{} {}", "hello", "world").clone_into(&mut s);
}

fn owned_function_macro() {
let mut s = String::from("");
ToOwned::clone_into(&format!("{} {}", "hello", "world"), &mut s);
}

struct FakeToOwned;
impl FakeToOwned {
/// This looks just like `ToOwned::to_owned`
Expand Down
20 changes: 20 additions & 0 deletions tests/ui/assigning_clones.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ fn clone_method_rhs_complex(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFr
*mut_thing = (ref_thing + ref_thing).clone();
}

fn clone_method_macro() {
let mut s = String::from("");
s = format!("{} {}", "hello", "world").clone();
}

fn clone_function_macro() {
let mut s = String::from("");
s = Clone::clone(&format!("{} {}", "hello", "world"));
}

fn assign_to_init_mut_var(b: HasCloneFrom) -> HasCloneFrom {
let mut a = HasCloneFrom;
for _ in 1..10 {
Expand Down Expand Up @@ -214,6 +224,16 @@ fn owned_function_val(mut mut_thing: String, ref_str: &str) {
mut_thing = ToOwned::to_owned(ref_str);
}

fn owned_method_macro() {
let mut s = String::from("");
s = format!("{} {}", "hello", "world").to_owned();
}

fn owned_function_macro() {
let mut s = String::from("");
s = ToOwned::to_owned(&format!("{} {}", "hello", "world"));
}

struct FakeToOwned;
impl FakeToOwned {
/// This looks just like `ToOwned::to_owned`
Expand Down
46 changes: 35 additions & 11 deletions tests/ui/assigning_clones.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,64 +62,88 @@ LL | *mut_thing = (ref_thing + ref_thing).clone();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `mut_thing.clone_from(ref_thing + ref_thing)`

error: assigning the result of `Clone::clone()` may be inefficient
--> tests/ui/assigning_clones.rs:68:9
--> tests/ui/assigning_clones.rs:67:5
|
LL | s = format!("{} {}", "hello", "world").clone();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `s.clone_from(&format!("{} {}", "hello", "world"))`

error: assigning the result of `Clone::clone()` may be inefficient
--> tests/ui/assigning_clones.rs:72:5
|
LL | s = Clone::clone(&format!("{} {}", "hello", "world"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `Clone::clone_from(&mut s, &format!("{} {}", "hello", "world"))`

error: assigning the result of `Clone::clone()` may be inefficient
--> tests/ui/assigning_clones.rs:78:9
|
LL | a = b.clone();
| ^^^^^^^^^^^^^ help: use `clone_from()`: `a.clone_from(&b)`

error: assigning the result of `Clone::clone()` may be inefficient
--> tests/ui/assigning_clones.rs:139:5
--> tests/ui/assigning_clones.rs:149:5
|
LL | a = b.clone();
| ^^^^^^^^^^^^^ help: use `clone_from()`: `a.clone_from(&b)`

error: assigning the result of `Clone::clone()` may be inefficient
--> tests/ui/assigning_clones.rs:146:5
--> tests/ui/assigning_clones.rs:156:5
|
LL | a = b.clone();
| ^^^^^^^^^^^^^ help: use `clone_from()`: `a.clone_from(&b)`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:147:5
--> tests/ui/assigning_clones.rs:157:5
|
LL | a = c.to_owned();
| ^^^^^^^^^^^^^^^^ help: use `clone_into()`: `c.clone_into(&mut a)`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:177:5
--> tests/ui/assigning_clones.rs:187:5
|
LL | *mut_string = ref_str.to_owned();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(mut_string)`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:181:5
--> tests/ui/assigning_clones.rs:191:5
|
LL | mut_string = ref_str.to_owned();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(&mut mut_string)`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:202:5
--> tests/ui/assigning_clones.rs:212:5
|
LL | **mut_box_string = ref_str.to_owned();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(&mut (*mut_box_string))`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:206:5
--> tests/ui/assigning_clones.rs:216:5
|
LL | **mut_box_string = ref_str.to_owned();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ref_str.clone_into(&mut (*mut_box_string))`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:210:5
--> tests/ui/assigning_clones.rs:220:5
|
LL | *mut_thing = ToOwned::to_owned(ref_str);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(ref_str, mut_thing)`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:214:5
--> tests/ui/assigning_clones.rs:224:5
|
LL | mut_thing = ToOwned::to_owned(ref_str);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(ref_str, &mut mut_thing)`

error: aborting due to 20 previous errors
error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:229:5
|
LL | s = format!("{} {}", "hello", "world").to_owned();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `format!("{} {}", "hello", "world").clone_into(&mut s)`

error: assigning the result of `ToOwned::to_owned()` may be inefficient
--> tests/ui/assigning_clones.rs:234:5
|
LL | s = ToOwned::to_owned(&format!("{} {}", "hello", "world"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_into()`: `ToOwned::clone_into(&format!("{} {}", "hello", "world"), &mut s)`

error: aborting due to 24 previous errors

0 comments on commit baf2a23

Please sign in to comment.