Lint to remove redundant `clone()`s #3355

sinkuu · 2018-10-24T13:59:39Z

This PR adds lint redundant_clone. It suggests to remove redundant clone() that clones a owned value that will be dropped without any usage after that.

Real-world example: https://github.com/rust-lang/rust/compare/7b0735a..sinkuu:redundant_clone2

flip1995 · 2018-10-24T14:28:21Z

cc #17

sinkuu · 2018-10-25T02:45:40Z

cont_eval errors are apeearing in indexing_slicing.rs. This lint causes MIR to be optimized if it has't (since mir_validated is stolen sometimes, it uses optimized_mir instead), which may trigger const-evals. It should slowdown linting too.
EDIT: Compared cargo +nightly clippy and this branch, and the slowdown was around 10%.

How can I know if mir_validated is available? Checking for no_codegen || !should_codegen() didn't work...

sinkuu · 2018-10-25T05:50:12Z

Ah, working on optimized MIR was much faster -- using mir_validated increased the slowdown to 50%, when it works. Perhaps because less number of BBs we need to visit? I'll just update indexing_slicing.stderr.

oli-obk

Wow! This is absolutely great.

One general nit: all these large if_lets and the bodies of filter_map would probably be more readable if they were separate functions with a descriptive name.

About these perf percentages that you posted, can you run clippy with and without your lint on some large code bases like cargo and diesel and compared those numbers? If it really causes a large slowdown, we can see how to optimize the lint to do less work

clippy_lints/src/redundant_clone.rs

oli-obk · 2018-10-25T10:14:56Z

clippy_lints/src/redundant_clone.rs

+                    if let Some(snip) = snippet_opt(cx, span);
+                    if let Some(dot) = snip.rfind('.');
+                    then {
+                        let sugg_span = span.with_lo(


I'm always wary of building our own spans, but I don't see how to do it better right now. Maybe we can change something in rustc that will provide us with better spans?

oli-obk · 2018-10-25T10:17:29Z

clippy_lints/src/redundant_clone.rs

+
+impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
+    fn visit_statement(&mut self, block: mir::BasicBlock, statement: &mir::Statement<'tcx>, location: mir::Location) {
+        // Once flagged, skip remaining statements


it might be more performant to iterate manually and run the visitor directly on statements until we don't need it anymore

oli-obk · 2018-10-25T15:34:22Z

clippy_lints/src/redundant_clone.rs

+                if ps.len() != 1 {
+                    continue;
+                }
+                let pred_terminator = unwrap_or_continue!(&mir[ps[0]].terminator);


ah, this should never be None in MIR you get from the compiler. use the terminator method to get the value directly.

Manishearth · 2018-10-25T16:17:12Z

Ooh, I've wanted this lint forever! Thanks for writing this! You may be interested in the set of escape analysis based lints that help avoid clones, boxes, and vecs -- we have boxed_local that kind of does this but ideally we'd have a set of lints that handle similar things. There's a lot of code that trips up on things like this.

…

On Thu, Oct 25, 2018, 4:35 PM Oliver S̶c̶h̶n̶e̶i̶d̶e̶r Scherer < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In clippy_lints/src/redundant_clone.rs <#3355 (comment)> : > // _1 in MIR `{ _2 = &_1; _3 = deref(move _2); } -> { _4 = _3; to_path_buf(move _4); }` let referent = if from_deref { let ps = mir.predecessors_for(bb); + if ps.len() != 1 { + continue; + } + let pred_terminator = unwrap_or_continue!(&mir[ps[0]].terminator); ah, this should never be None in MIR you get from the compiler. use the terminator method to get the value directly. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3355 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSMgTHp96niQnba9rOcikjOeBoNyYks5uodpCgaJpZM4X3_fD> .

sinkuu · 2018-10-25T17:13:43Z

Result of hyperfine -p "touch path/to/lib.rs" -w 3 -i:

Package	master [s]	redundant_clone [s]	Slowdown [%]
diesel	25.632	26.161	2.06
cargo	13.215	14.441	9.28
regex	1.983	2.176	9.73
serde-json	0.8877	0.9575	7.86

oli-obk

I don't really see where the slowdown is coming from. I presume it's a one-time cost we pay for computing the MIR of all function bodies (which we didn't so far I guess).

Can you confirm this by inserting a return right after you obtain the MIR and rerun the perf runs?

If that is the reason, then I'm absolutely fine with this slowdown.

oli-obk · 2018-10-25T17:49:28Z

clippy_lints/src/redundant_clone.rs

+                };
+
+                if_chain! {
+                    if !in_macro(span);


Move this check all the way to the start of the loop and continue if we are in a macro.

sinkuu · 2018-10-25T19:24:38Z

Quite noisy, but the slowdown caused by this lint may not be significant.

Package	master [s]	Only `optimized_mir` [s]	Slowdown [%]
diesel	25.38	24.869	-2.01
cargo	12.897	14.467	12.17
regex	2.029	2.217	9.27
serde-json	0.7779	0.8556	9.99

oli-obk · 2018-10-25T20:27:20Z

bors r+

Awesome! Thanks so much for creating this lint. Both because it's awesome and because we now have a MIR lint

oli-obk · 2018-10-25T20:28:33Z

bors ping

mati865 · 2018-10-25T21:10:33Z

Maybe bors is mad at you?
I'm wondering if it will respond to unprivileged person.

bors ping

EDIT: it did!

bors · 2018-10-25T21:10:34Z

pong

flip1995 · 2018-10-25T21:11:50Z

it was down, now it is back up again 🎉

bors r=oli-obk

3355: Lint to remove redundant `clone()`s r=oli-obk a=sinkuu This PR adds lint `redundant_clone`. It suggests to remove redundant `clone()` that clones a owned value that will be dropped without any usage after that. Real-world example: rust-lang/rust@8ec22e7...sinkuu:redundant_clone2 Co-authored-by: Shotaro Yamada <[email protected]>

bors · 2018-10-25T22:23:50Z

Timed out

notriddle · 2018-10-25T23:02:04Z

My bad. An upgrade went wrong, and caused some trouble over the last couple days.

... wait, I thought all the official Rust stuff used bors-voyager. What happened?

Manishearth · 2018-10-26T04:09:04Z

That's just crates.io, the official rust stuff uses homu. I think we set up bors-ng because setting up homu requires some infra edits.

flip1995 · 2018-10-26T07:30:37Z

bors retry

3355: Lint to remove redundant `clone()`s r=oli-obk a=sinkuu This PR adds lint `redundant_clone`. It suggests to remove redundant `clone()` that clones a owned value that will be dropped without any usage after that. Real-world example: https://github.com/rust-lang/rust/compare/7b0735a..sinkuu:redundant_clone2 Co-authored-by: Shotaro Yamada <[email protected]>

sinkuu · 2018-10-26T07:56:25Z

Nothing will pass until #3359 (comment) is resolved:(

bors · 2018-10-26T07:57:21Z

Build failed

continuous-integration/travis-ci/push

matthiaskrgr · 2018-10-26T10:21:00Z

bors retry
Just needs a retry I think (not sure if I am privileged)

bors · 2018-10-26T10:21:01Z

🔒 Permission denied

Existing reviewers: click here to make matthiaskrgr a reviewer

flip1995 · 2018-10-26T10:32:52Z

bors retry

flip1995 · 2018-10-26T10:35:29Z

bors ping

bors · 2018-10-26T10:35:30Z

pong

bors · 2018-10-26T10:35:30Z

pong

flip1995 · 2018-10-26T10:36:29Z

It seems that if retry was written before another retry won't trigger bors again

bors r=oli-obk

3355: Lint to remove redundant `clone()`s r=oli-obk a=sinkuu This PR adds lint `redundant_clone`. It suggests to remove redundant `clone()` that clones a owned value that will be dropped without any usage after that. Real-world example: https://github.com/rust-lang/rust/compare/7b0735a..sinkuu:redundant_clone2 Co-authored-by: Shotaro Yamada <[email protected]>

bors · 2018-10-26T11:02:59Z

Build succeeded

sinkuu force-pushed the redundant_clone branch 2 times, most recently from a7433b0 to d3df19f Compare October 25, 2018 02:34

oli-obk suggested changes Oct 25, 2018

View reviewed changes

oli-obk reviewed Oct 25, 2018

View reviewed changes

sinkuu added 6 commits October 26, 2018 01:15

Add redundant_clone lint

3ca0895

Run update_lints

5285372

update_references indexing_slicing

105ae71

Implement visit_basic_block_data

24d3f5b

Use lint_root

9a150b4

Refactor

6d6ff88

sinkuu force-pushed the redundant_clone branch from bed5947 to 6d6ff88 Compare October 25, 2018 16:21

Use BasicBlockData::terminator

a828692

oli-obk reviewed Oct 25, 2018

View reviewed changes

Move in_macro check

9034b87

oli-obk approved these changes Oct 25, 2018

View reviewed changes

bors bot merged commit 9034b87 into rust-lang:master Oct 26, 2018

sinkuu deleted the redundant_clone branch October 30, 2018 03:52

jfirebaugh mentioned this pull request Feb 18, 2019

Catching unnecessary clones #17

Closed

jon-chuang mentioned this pull request Sep 29, 2022

refactor(connector): remove redundant clone risingwavelabs/risingwave#5625

Merged

1 task

Lint to remove redundant clone()s #3355

Lint to remove redundant clone()s #3355

Uh oh!

Conversation

sinkuu commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 commented Oct 24, 2018

Uh oh!

sinkuu commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sinkuu commented Oct 25, 2018

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oli-obk Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Oct 25, 2018 via email

Uh oh!

sinkuu commented Oct 25, 2018

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

oli-obk Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

sinkuu commented Oct 25, 2018

Uh oh!

oli-obk commented Oct 25, 2018

Uh oh!

oli-obk commented Oct 25, 2018

Uh oh!

mati865 commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors bot commented Oct 25, 2018

Uh oh!

flip1995 commented Oct 25, 2018

Uh oh!

bors bot commented Oct 25, 2018

Timed out

Uh oh!

notriddle commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Manishearth commented Oct 26, 2018

Uh oh!

flip1995 commented Oct 26, 2018

Uh oh!

sinkuu commented Oct 26, 2018

Uh oh!

bors bot commented Oct 26, 2018

Build failed

Uh oh!

matthiaskrgr commented Oct 26, 2018

Uh oh!

bors bot commented Oct 26, 2018

Uh oh!

flip1995 commented Oct 26, 2018

Uh oh!

flip1995 commented Oct 26, 2018

Uh oh!

bors bot commented Oct 26, 2018

Uh oh!

bors bot commented Oct 26, 2018

Uh oh!

flip1995 commented Oct 26, 2018

Uh oh!

bors bot commented Oct 26, 2018

Lint to remove redundant `clone()`s #3355

Lint to remove redundant `clone()`s #3355

sinkuu commented Oct 24, 2018 •

edited

Loading

sinkuu commented Oct 25, 2018 •

edited

Loading

mati865 commented Oct 25, 2018 •

edited

Loading

notriddle commented Oct 25, 2018 •

edited

Loading