Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assigning_clones: makes code less readable and harder to maintain #12778

Closed
de-vri-es opened this issue May 8, 2024 · 4 comments · Fixed by #12779
Closed

assigning_clones: makes code less readable and harder to maintain #12778

de-vri-es opened this issue May 8, 2024 · 4 comments · Fixed by #12779

Comments

@de-vri-es
Copy link
Contributor

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
Copy link
Member

#[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
Copy link
Contributor Author

de-vri-es commented May 8, 2024

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
Copy link
Contributor

kpreid commented May 9, 2024

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
Copy link
Contributor Author

de-vri-es commented May 9, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants