assigning_clones: makes code less readable and harder to maintain

[de-vri-es]

Description

Following the assigning_clones lint makes code less readable.

There is a strong semantic hint in the = operator. It gives a visual indication that a value is being assigned to.

The clone_from() function does not have this visual hint. Using clone_from() makes code harder to read and as a consequence, harder to maintain.

Consider the two snippets:

self.foo = some_value.clone()

self.foo.clone_from(some_value);

The second snippet is much less clear. In isolation this may not seem like much, but in a real codebase this is bad.

Lints should not favor performance at the cost of maintainability. Especially considering that #[derive(Clone)] doesn't even implement clone_from() and the default clone_from() is implemented as *self = source.clone(): https://github.com/rust-lang/rust/blob/5ce96b1d0f6b5093955e7b6a70dfd877395c1d73/library/core/src/clone.rs#L169-L171

I suggest the lint is changed to be turned off by default.

Version

rustc 1.78.0 (9b00956e5 2024-04-29)
binary: rustc
commit-hash: 9b00956e56009bab2aa15d7bff10916599e3d6d6
commit-date: 2024-04-29
host: x86_64-unknown-linux-gnu
release: 1.78.0
LLVM version: 18.1.2

Additional Labels

No response

[Alexendoo]

#[derive(Clone)] doesn't even implement clone_from() and the default clone_from() is implemented as *self = source.clone()

There are some false positives but they're from manual implementations in wrapper types (#12709). Directly at least types without a manual implementation of clone_from are ignored

[de-vri-es]

Ah, that's cool. I didn't know that.

But I still think clippy is suggesting to do a premature optimization. In code where this level of performance difference is truly important, the difference should be bench-marked.

I don't think this trade-off between absolute maximum possible performance and code readability is a proper one for the default configuration of clippy.

I got this warning from a String, but I really don't think the potential performance improvement is worth making the code less readable.

[kpreid]

Especially considering that #[derive(Clone)] doesn't even implement clone_from() and the default clone_from() is implemented as *self = source.clone():

Note that there's a catch-22 here: using clone_from() has little benefit because very few Clone implementations override it, and very few Clone implementations override it because it is so rarely used. I hoped that by creating assigning_clones, this cycle would be broken.

[de-vri-es]

That's true. But even if this gives a runtime performance improvement, I don't think this lint should be enabled by default. It favors a small performance gain over a big readability loss.

This kind of optimization may save a call to alloc and free in practise. If this is important enough, the compiler should try to figure this out and re-use the allocation behind the scenes (I think many C++ compilers do this nowadays?).

I really feel that telling people not to use the = operator for assigning values is the wrong thing to do as a default.

/edit: Reading the zulip conversation the consensus also seems to be to put it in pedantic? https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Meeting.202023-05-02/near/355185136