Add assigning_clones lint

[Kobzol]

This PR is a "revival" of #10613 (with @kpreid's permission).

I tried to resolve most of the unresolved things from the mentioned PR:

1. The lint now checks properly if we indeed call the functions std::clone::Clone::clone or std::borrow::ToOwned::to_owned.
2. It now supports both method and function (UFCS) calls.
3. A heuristic has been added to decide if the lint should apply. It will only apply if the type on which the method is called has a custom implementation of clone_from/clone_into. Notably, it will not trigger for types that use #[derive(Clone)].
4. Deref handling has been (hopefully) a bit improved, but I'm not sure if it's ideal yet.

I also added a bunch of additional tests.

There are a few things that could be improved, but shouldn't be blockers:

1. When the right-hand side is a function call, it is transformed into e.g. ::std::clone::Clone::clone(...). It would be nice to either auto-import the Clone trait or use the original path and modify it (e.g. clone::Clone::clone -> clone::Clone::clone_from). I don't know how to modify the QPath to do that though.
2. The lint currently does not trigger when the left-hand side is a local variable without an initializer. This is overly conservative, since it could trigger when the variable has no initializer, but it has been already initialized at the moment of the function call, e.g.

let mut a;
...
a = Foo;
...
a = b.clone(); // Here the lint should trigger, but currently doesn't

These cases probably won't be super common, but it would be nice to make the lint more precise. I'm not sure how to do that though, I'd need access to some dataflow analytics or something like that.

changelog: new lint [assigning_clones]

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[xFrednet]

Hey, thank you for the picking up the issue and creating a new PR. I'm currently in the middle of my exams and will probably not have time to review this in a timely manner. Therefore, I'll reassign it, please excuse the delay thus far.

r? rust-lang/clippy

[xFrednet]

@giraffate has removed themself from the review rotation. So, I'm rerolling this again

r? rust-lang/clippy

[xFrednet]

Rustbot really knows how to pick them xD

I know that @blyxyas is active. Do you maybe have time to take over this review, queen? =^.^=

r? @blyxyas

[blyxyas]

Meow meow very good first iteration! (●ↀωↀ●)✧ Just a few notes, mainly to improve performance (those 3 CPU cycles are important! /s)

[Kobzol]

Meow meow very good first iteration!

It was actually a second iteration, thanks to earlier work by kpreid, which made my work a lot easier :)

Pushed a commit with review changes. Thanks for taking a look!

[blyxyas]

Before any more reviewing, could you fix dogfood?

[Kobzol]

Done. Yeah, the lint shouldn't fire in macros I suppose. I saw the in_external_macro function, but it seems to me that it shouldn't fire even in local macros? (which I implemented now). Or maybe it should be reported in macros, but the quick fix shouldn't be suggested?

[blyxyas]

It can lint in local macros definitions, don't worry about that. You can use in_external_macro.

[Kobzol]

I tried to use it, but then it was auto-fixing code inside a macro, which was broken.

[blyxyas]

?? Try this diff

diff --git a/clippy_lints/src/assigning_clones.rs b/clippy_lints/src/assigning_clones.rs
index 1a38f456d..eec842d3b 100644
--- a/clippy_lints/src/assigning_clones.rs
+++ b/clippy_lints/src/assigning_clones.rs
@@ -5,6 +5,7 @@ use clippy_utils::{is_trait_method, path_to_local};
 use rustc_errors::Applicability;
 use rustc_hir::{self as hir, Expr, ExprKind, Node};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::lint::in_external_macro;
 use rustc_middle::ty::{self, Instance, Mutability};
 use rustc_session::declare_lint_pass;
 use rustc_span::def_id::DefId;
@@ -59,6 +60,9 @@ impl<'tcx> LateLintPass<'tcx> for AssigningClones {
             ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) | ExpnKind::Macro(..) => return,
             ExpnKind::Root => {},
         }
+        if in_external_macro(cx.tcx.sess, assign_expr.span()) {
+            return;
+        }
 
         let ExprKind::Assign(lhs, rhs, _span) = assign_expr.kind else {
             return;

It worked perfectly for me, not a single error ฅ(•ㅅ•❀)ฅ meow

[Kobzol]

Oh, yeah, this will work, because before that I already do this:

let expn_data = assign_expr.span().ctxt().outer_expn_data();
match expn_data.kind {
    ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) | ExpnKind::Macro(..) => return,
    ExpnKind::Root => {},
}

which filters all macro invocations, so if we're inside a macro, we won't ever get to the in_external_macro(cx.tcx.sess, assign_expr.span()) line.

The issue that I was describing is that if I removed these 5 lines from the beginning, and only used in_external_macro, then this happens:

macro_rules! clone_inside {
    ($a:expr, $b: expr) => {
        $a = $b.clone();
    };
}

// turns into v

macro_rules! clone_inside {
    ($a:expr, $b: expr) => {
        a.clone_from(&b);
    };
}

It replaces the body within the macro, which is not ideal.

[bors]

☔ The latest upstream changes (presumably #12160) made this pull request unmergeable. Please resolve the merge conflicts.

[Kobzol]

Rebased to fix conflicts.

[blyxyas]

For a first iteration, this LGTM.
Today I'll check for that TODO, but this could be merged right now. Sorry for the delay, I kinda forgot about this PR for a while and getting a 1K PR back to speed isn't the easiest thing in the world 😅

[blyxyas]

Curious here how, even though we check that expr is from ToOwned, we cannot check for sym::to_owned_method

[Kobzol]

We could, I guess that I just wanted to make it a bit more consistent with the checking of the clone method.

[bors]

☔ The latest upstream changes (presumably #12010) made this pull request unmergeable. Please resolve the merge conflicts.

[Kobzol]

Rebased to fix conflicts.

[blyxyas]

LGTM, thanks! ❤️

[blyxyas]

@bors r+

[bors]

📌 Commit 24de0be has been approved by blyxyas

It is now in the queue for this repository.

[bors]

⌛ Testing commit 24de0be with merge e485a02...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing e485a02 to master...

[torokati44]

FYI: ruffle-rs/ruffle#15703

[Kobzol]

I have wondered lately if it should be enabled by default in perf, it's unclear whether it helps more often than being annoying.