Add `assigning_clones` lint #12077

Kobzol · 2024-01-02T16:16:57Z

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

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

The lint now checks properly if we indeed call the functions std::clone::Clone::clone or std::borrow::ToOwned::to_owned.
It now supports both method and function (UFCS) calls.
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)].
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:

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.
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 · 2024-01-02T16:17:03Z

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

clippy_lints/src/assigning_clones.rs

xFrednet · 2024-01-10T20:28:48Z

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 · 2024-01-22T13:48:23Z

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

r? rust-lang/clippy

xFrednet · 2024-01-22T13:49:45Z

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)

clippy_lints/src/assigning_clones.rs

Kobzol · 2024-01-22T19:41:48Z

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 · 2024-01-22T22:55:54Z

Before any more reviewing, could you fix dogfood?

Kobzol · 2024-01-23T14:33:45Z

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 · 2024-01-23T23:11:48Z

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

Kobzol · 2024-01-24T07:04:25Z

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

blyxyas · 2024-01-24T23:41:53Z

?? 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 · 2024-01-25T09:52:40Z

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 · 2024-01-26T13:54:03Z

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

Kobzol · 2024-02-25T22:55:49Z

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 · 2024-01-25T23:53:54Z

clippy_lints/src/assigning_clones.rs

+            };
+            if is_trait_method(cx, expr, sym::Clone) && path.ident.name == sym::clone {
+                (TargetTrait::Clone, CallKind::MethodCall { receiver }, resolved_method)
+            } else if is_trait_method(cx, expr, sym::ToOwned) && path.ident.name.as_str() == "to_owned" {


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

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

clippy_lints/src/assigning_clones.rs

bors · 2024-03-01T12:19:28Z

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

Kobzol · 2024-03-01T15:37:32Z

Rebased to fix conflicts.

blyxyas

LGTM, thanks! ❤️

blyxyas · 2024-03-05T15:25:48Z

@bors r+

bors · 2024-03-05T15:25:53Z

📌 Commit 24de0be has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-03-05T15:26:59Z

⌛ Testing commit 24de0be with merge e485a02...

bors · 2024-03-05T15:35:48Z

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

torokati44 · 2024-03-21T22:24:15Z

FYI: ruffle-rs/ruffle#15703

Kobzol · 2024-03-21T22:27:58Z

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

rustbot assigned xFrednet Jan 2, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 2, 2024

kpreid approved these changes Jan 2, 2024

View reviewed changes

clippy_lints/src/assigning_clones.rs Outdated Show resolved Hide resolved

rustbot assigned giraffate and unassigned xFrednet Jan 10, 2024

rustbot assigned Centri3 and unassigned giraffate Jan 22, 2024

rustbot assigned blyxyas and unassigned Centri3 Jan 22, 2024

blyxyas requested changes Jan 22, 2024

View reviewed changes

Kobzol force-pushed the assigning-clones branch from 2bcb00d to a881d48 Compare February 25, 2024 22:55

Kobzol force-pushed the assigning-clones branch from a881d48 to cf2e79f Compare February 26, 2024 09:08

blyxyas approved these changes Feb 26, 2024

View reviewed changes

blyxyas reviewed Feb 26, 2024

View reviewed changes

clippy_lints/src/assigning_clones.rs Show resolved Hide resolved

Kobzol added 2 commits March 1, 2024 16:35

Add assigning_clones lint

0656d28

Update lints

e3cda2a

Kobzol added 5 commits March 1, 2024 16:36

Fix tests

0fcc33e

React to review

41f5ee1

Fix lint errors

f7356f2

Do not run the lint on macro-generated code

bc551b9

Fix tests

24de0be

Kobzol force-pushed the assigning-clones branch from cf2e79f to 24de0be Compare March 1, 2024 15:45

Kobzol requested a review from blyxyas March 2, 2024 17:15

blyxyas approved these changes Mar 5, 2024

View reviewed changes

bors merged commit e485a02 into rust-lang:master Mar 5, 2024

Kobzol deleted the assigning-clones branch March 5, 2024 15:36

This was referenced Mar 5, 2024

Respect MSRV (MAYBE_BOUND_IN_WHERE) #12388

Closed

Add unnecessary_ref_mut lint #12399

Closed

kpreid mentioned this pull request Mar 5, 2024

Lint to suggest clone_from and clone_into #1709

Closed

taiki-e mentioned this pull request Mar 8, 2024

assigning_clones: Wrong suggestion with deref #12437

Closed

y21 mentioned this pull request Mar 10, 2024

Use of a = b.clone() instead of a.clone_from(&b) #9512

Closed

gibfahn mentioned this pull request Mar 11, 2024

assigning_clones: after fixes were automatically applied the compiler reported errors within these files: – cannot borrow name as mutable because it is also borrowed as immutable #12460

Closed

taiki-e mentioned this pull request Mar 18, 2024

assigning_clones should respect MSRV #12502

Closed

Add assigning_clones lint #12077

Add assigning_clones lint #12077

Uh oh!

Conversation

Kobzol commented Jan 2, 2024

Uh oh!

rustbot commented Jan 2, 2024

Uh oh!

Uh oh!

xFrednet commented Jan 10, 2024

Uh oh!

xFrednet commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet commented Jan 22, 2024

Uh oh!

blyxyas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kobzol commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blyxyas commented Jan 22, 2024

Uh oh!

Kobzol commented Jan 23, 2024

Uh oh!

blyxyas commented Jan 23, 2024

Uh oh!

Kobzol commented Jan 24, 2024

Uh oh!

blyxyas commented Jan 24, 2024

Uh oh!

Kobzol commented Jan 25, 2024

Uh oh!

bors commented Jan 26, 2024

Uh oh!

Kobzol commented Feb 25, 2024

Uh oh!

blyxyas left a comment

Choose a reason for hiding this comment

Uh oh!

blyxyas Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

Kobzol Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bors commented Mar 1, 2024

Uh oh!

Kobzol commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blyxyas left a comment

Choose a reason for hiding this comment

Uh oh!

blyxyas commented Mar 5, 2024

Uh oh!

bors commented Mar 5, 2024

Uh oh!

bors commented Mar 5, 2024

Uh oh!

bors commented Mar 5, 2024

Uh oh!

torokati44 commented Mar 21, 2024

Uh oh!

Kobzol commented Mar 21, 2024

Uh oh!

Uh oh!

Add `assigning_clones` lint #12077

Add `assigning_clones` lint #12077

xFrednet commented Jan 22, 2024 •

edited

Loading

Kobzol commented Jan 22, 2024 •

edited

Loading

Kobzol commented Mar 1, 2024 •

edited

Loading