Add lint assigning_clones.

[kpreid]

This lint reports when *a = b.clone(); can be replaced with a.clone_from(&b);, and the same for ToOwned::clone_into(). Fixes #1709.

This is my first attempt at writing code for clippy or rustc, and I could use help with these missing parts:

• Does not correctly verify that the call is a call to Clone::clone() rather than some other similarly named method. I need advice on how to ask rustc for this information; the lint isn't really correct without it.
• Does not detect function call syntax in addition to method call syntax. I will fix this after the previous item.
• Does not lint on assignment to a local variable or DerefMut coercion, only on assignment to *{some &mut T}, because it does not know whether the local variable might be uninitialized. I need advice on how to ask rustc for this information. (But if it's not feasible, then the lint will still work, just not apply to as many things.)

changelog: new lint [assigning_clones]

[rustbot]

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

[Noratrieb]

Note that the derive macro currently doesn't implement clone_from and it's unlikely to do so in the future: rust-lang/rust#98445 (comment)

[kpreid]

Note that the derive macro currently doesn't implement clone_from and it's unlikely to do so in the future

Indeed, that's why the note says “may be inefficient”. I could imagine the lint pass checking the implementing type to see whether it overrides clone_from or not to decide whether to emit the lint (or how to phrase it). I also thought about mentioning the derive's behavior in the lint documentation, but it seems not great to do that when it might, in fact, be wrong in the future without being updated. (But maybe that's worth doing anyway.)

However, my hope here is that the lint will raise awareness of the existence of clone_from, and authors of types will more often implement it when it is applicable, thus expanding the performance gains. The rewrite this lint proposes should not significantly decrease performance even if it gains none, so the cost is only in “churn” (people seeing a new lint and fixing/allowing it), which is not to be ignored! But discussing whether this lint should not exist at all should probably happen on the issue, not the PR, yes?

Another idea: there could be a configuration option (or splitting this lint into two lints) to have it lint only on types known to have clone_from() implementations.

[xFrednet]

Thank you for the PR. I left remarks regarding most todos. As suggested in a comment, it would probably be best to only lint cases, where a custom clone_from implementation exists. I'm not 100% sure how to do that right now, but will look it up, until the next review. I hope that this gives you enough hints to continue on the implementation for now.

You're welcome to reach out if you have any questions :)

[xFrednet]

I feel like perf is too harsh for this lint. In part because I find a = XYZ.clone() more readable. For instance, when I scan for the origin of a, I would find the example much faster than a.clone_from(XYZ). However, I do understand how this can affect performance. I see three options:

1. Keep the lint in perf. This will most likely cause several lint issues in the next release and will annoy users, to the point where they'll just allow the lint.
2. Have the lint in an allow-by-default category, like pedantic or restriction
3. Restrict the lint, to only detect cases, where the type actually has a custom implementation of clone_from()

I favor the third option, we can add a config value to detect all cases, but I'd guess that 99+% of the users will prefer this variant. I'll ask the other team members, what they think about it :)

---

Looking at the standard library, it looks like, most types that implement this function, mostly do so, to forward it to generic parameters, in case they have a custom implementation. COW is one of the examples where this is not the case.

[kpreid]

I personally like option 3 for now — focusing on giving definitely-useful advice. I haven't gotten back to working on this yet, but when I do I will look into that, and if doesn't seem feasible to do that then I'll change the category to pedantic.

[xFrednet]

We've discussed the topic in the meeting 2 weeks ago, but I missed writing a comment then 😅. Here is a link to the discussion:

https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Meeting.202023-05-02/near/355124756

It was suggested to add a clippy attribute that crates can use, to mark a struct for this lint. Also restricting the lint to custom clone_from suggestions was encouraged, IIRC

[xFrednet]

I would write this much shorter, similar to the description in the Clone trait:

Suggested change

	/// If cloning `bar` allocates memory (or other resources), then this code will allocate
	/// new memory for the clone of `bar`, then drop `foo`, then overwrite `foo` with the clone.
	/// Instead, `Clone::clone_from()`, or `ToOwned::clone_into()`, may be able to update
	/// `foo` in-place, reusing existing memory.
	///
	/// Note that this only provides any actual improvement if the type has explicitly implemented
	/// the `clone_from()` trait method, since the trait-provided implementation will just call
	/// `clone()`.
	/// Custom `clone_from()` implementations allow the objects to share resources
	/// and therefore avoid allocations.

[xFrednet]

Suggested change

	/// ### Example
	/// ```rust
	/// #[derive(Clone)]
	/// struct Thing;
	///
	/// pub fn assign_to_ref(a: &mut Thing, b: Thing) {
	/// *a = b.clone();
	/// }
	/// ```
	/// Use instead:
	/// ```rust
	/// #[derive(Clone)]
	/// struct Thing;
	///
	/// pub fn assign_to_ref(a: &mut Thing, b: Thing) {
	/// a.clone_from(&b);
	/// }
	/// ```
	/// ### Example
	/// ```rust
	/// # let src = "Cheesecake".to_string();
	/// let mut target = "Hello world".to_string();
	/// // [...]
	/// target = src.clone();
	/// ```
	/// Use instead:
	/// ```rust
	/// # let src = "Cheesecake".to_string();
	/// let mut target = "Hello world".to_string();
	/// // [...]
	/// target.clone_from(&src);
	/// ```

[xFrednet]

Tiny nit: You can check directly if args are empty in this pattern :)

Suggested change

	let ExprKind::MethodCall(method_name, clone_receiver, args, _span) = clone_call.kind else { return };
	let ExprKind::MethodCall(method_name, clone_receiver, [], _span) = clone_call.kind else { return };

[xFrednet]

Now to answer your first todo, from the PR description. Usually, we check if a type implements the trait in question, and then we check the function name. I believe, in this case, it would be:

if is_trait_method(cx, clone_call, sym::Clone) ...

[xFrednet]

Finding the origin of a variable can be a bit difficult, when it comes to patterns, function parameters etc. Though, in this case, we only want to find the let <var> statement if it's a local value. This is a rough sketch of how to check for this:

if let Some(id) = path_to_local(expr) {
    if let Some(Node::Stmt(stmt)) = cx.tcx.hir().find(id) {
        // if stmt is let, check for init expr
    }
}

clippy_utils::expr_or_init could also be interesting to look at :)

[xFrednet]

You can use type checking, to get the type of the clone call. If it's an ADT, you can use the DefId to check for known types :)

See:

• Checking for a specific type
• Diagnostic items

[xFrednet]

Nice usage of hir_with_applicability 👍

You can resolve this directly :)

[xFrednet]

The rewrite this lint proposes should not significantly decrease performance even if it gains none, so the cost is only in “churn” (people seeing a new lint and fixing/allowing it), which is not to be ignored!

In some cases, this can actually be quite a big crunch. Which I why I suggested restricting the lint above. I think this is also worth a discussion with others, which is why I'm nominating this for the next meeting.

@rustbot label +I-nominated

But discussing whether this lint should not exist at all should probably happen on the issue, not the PR, yes?

Ideally, yes. Though it's good to ask in PRs as well. We get so many notifications that we sadly cannot check every issue. These questions get noticed much faster in PRs. If you have a question in an issue, it's recommended to ping someone from the team, or ask on zulip for feedback :)

[bors]

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

[xFrednet]

Hey @kpreid, this is a ping from triage. Do you need any further assistance with this PR? :)

[kpreid]

Do you need any further assistance with this PR? :)

Not specifically; I just have not gotten back to it yet. I may have further questions when I do.

[xFrednet]

Hey @kpreid, I'm closing this PR due to inactivity. If you want to continue this, you can add a comment for me to reopen this PR or simply create a new one. Thank you for the time you already put into this lint!