Comments (14)
Oops, wrong button.
from rust-clippy.
As you recently noted that you are currently thinking on how to implement this, why not pool our neuronal resources and think in the open?
To create a good lint against unnecessary clones, we have a few problems ranging from very easy to near impossible:
'1. (ridiculously easy) Finding a .clone() call.
'2. (easy enough) Finding out the status of the cloned value pertaining to the current code (borrowed / mutably borrowed / owned)
'3. (easy enough) Finding out how the operation / call / ... takes the value (borrow / mutable borrow / by value)
'4. (medium) Finding out if the value is internally mutable
At this point we could already lint against clones of non internally mutable values just for borrowing them immutably. It's not much, but it's a start. There is one caveat however: We'd somehow need to be sure to rule out side effects on cloning or mutating the value (e.g. some global registry could be updated).
'5. (medium) Finding out if the original value is used elsewhere afterwards
At this point we could lint against clones for those by-value ops, with the aforementioned caveat.
'6. (hard) Finding out if the op/call mutates the value (if it is internally mutable or borrowed mutably)
At this point we could add cases where the value isn't mutated to our lint (for mutably borrowed or internally mutable values). The caveat against side effects from above applies here, too.
'7. (near impossible) Finding out if cloning or the checked operation on the value has any side effects.
If we can rule this out, the caveat no longer applies, and we have a lint that warns against unnecessary clones without any false positives.
There is still one problem left:
'8. (near impossible) Creating a good suggestion depending on how the value is available and is used – for example we may have to make lifetimes line up to ensure the original value can even be used.
Perhaps we should add an [E-impossible] label for such things?
from rust-clippy.
At least for '4, '6 and '7, blacklisting would give us a good bang for quite little buck. For those cases, '8 would probably also be tractable.
Perhaps we should ask the compiler to annotate types with "is internally mutable" flags (edit: On the other hand, it would probably suffice to look at the public fields and look for mutable interior there), method arguments with "is mutated during this method" and methods with "mutate global state". This would allow us to do all the above analysis (apart from '8, which I think is still hard) in a reliable way.
from rust-clippy.
Oh, that's not the type of lint I meant. I was focusing on moving and clones. Basically, in many cases folks clone things when the variable could be moved out.
For example:
fn main() {
let x = vec![];
foo(x.clone());
bar(x.clone()); // should have been just `x`
// x no longer used here
}
Such things can lead to massive perf improvements are actually quite a common pitfall since Rust often makes folks clone()
until it works.
I suspect the "unused variable" lint (which uses ExprUseVisitor) can be tweaked so that a clone is not counted as a "use" (this fixes the above case). But, this may not be perfect when it comes to branching (I need to think about this more).
The alternative is to set up a bit vector framework similar to live variables analysis with a couple more states, and feed it the control flow graph. This isn't too hard either, but needs me to sit down with a whiteboard.
from rust-clippy.
But what if too or bar modify the vec?
from rust-clippy.
We just have to take care not to count clone()
s on references. I would actually be very interested in how this:
The alternative is to set up a bit vector framework similar to live variables analysis with a couple more states, and feed it the control flow graph. This isn't too hard either, but needs me to sit down with a whiteboard.
would be done within the rustc framework.
from rust-clippy.
But what if too or bar modify the vec?
Well, you can't use the vec after the function ends anyway, so it doesn't matter if bar
modifies it. And foo
has to get a clone in any case.
from rust-clippy.
True that. In that case the analysis is actually easier than I thought.
from rust-clippy.
Yep. We don't care about mutation. This is about liveness/use.
from rust-clippy.
I came here to see if this lint was available, so I'm glad to see it being discussed.
I've got a bunch of Rust code ported from F#. In cases where ownership wasn't clear in the original code, I temporarily threw in a clone(). I recently fixed one situation where I was cloning stuff from a hierarchy that was gonna get dropped anyway. A lint in this area would be super helpful for this kind of situation.
from rust-clippy.
I'd also like to see something that looks for
let mut x = ...
...
x = y.clone();
and suggests changing it to
let mut x = ...
...
x.clone_from(y);
from rust-clippy.
We should be able to write this as a MIR lint. If anyone wants to take a crack at that, I'll happily mentor.
from rust-clippy.
Should this be closed now that #3355 is merged?
from rust-clippy.
Closing this, redundant_clone has a bit of history already :)
from rust-clippy.
Related Issues (20)
- needless_borrows_for_generic_arg seems to have introduced suggestions that break compilation for 1.80.0 HOT 1
- `macro_metavars_in_unsafe` does not lint "without a statement".
- Slow and biased rounding
- unnecessary_lazy_evaluations suggests running `unsafe` code eagerly HOT 6
- redundant_locals incorrectly emitted when binding in new local scope for early drop
- Print_* family of lints fail to match on `async_trait` functions with a return value
- indexing does not trigger missing_panics_doc HOT 4
- declare_interior_mutable_const fires for static pointer types
- Lint against Arc<impl Copy> HOT 1
- duplicated_attributes: false positive with apistos crate
- Clippy to fix warnings when they are converted to errors HOT 1
- Incorrectly printed suggestion for `map(<f>).unwrap_or(false)` on multiple lines HOT 1
- regression: unnecessary_lazy_evaluations not triggered on thiserror enum variant
- Add lints to encourage use of specialized assert macros
- `cast_possible_truncation` could check whether floating point values are manually truncated (round, floor, ceil...) prior to cast HOT 1
- Cargo clippy --fix not working with 1.80.1 HOT 2
- `module_inception` false positive for private sub-module HOT 4
- Lint use of functions with underscore-prefixed names HOT 2
- Flag unnecessary visiblity attributes on nested functions HOT 1
- Cargo clippy report warnings from macros when using quote_spanned
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rust-clippy.