Fix false-positive of redundant_clone and move to clippy::perf #4509

sinkuu · 2019-09-06T07:56:57Z

This PR introduces dataflow analysis to redundant_clone lint to filter out borrowed variables, which had been incorrectly detected.

Depends on rust-lang/rust#64207.

changelog: Moved redundant_clone lint to perf group

What this lint catches

`clone`/`to_owned`

let s = String::new();
let t = s.clone();

// MIR
_1 = String::new();
_2 = &_1;
_3 = clone(_2); // (*)

We can turn this clone call into a move if

_2 is the sole borrow of _1 at the statement (*)
_1 is not used hereafter

`Deref` + type-specific `to_owned` method

let s = std::path::PathBuf::new();
let t = s.to_path_buf();

// MIR
_1 = PathBuf::new();
_2 = &1;
_3 = call deref(_2);
_4 = _3;                         // Copies borrow
StorageDead(_2);
_5 = Path::to_path_buf(_4); // (*)

We can turn this to_path_buf call into a move if

_3 _4 are the sole borrow of _1 at (*)
_1 is not used hereafter

What this PR introduces

MaybeStorageLive that determines whether a local lives at a particular location
PossibleBorrowerVisitor that constructs TransitiveRelation of possible borrows, e.g. visiting _2 = &1; _3 = &_2: will result in _3 -> _2 -> _1 relation. Then _3 and _2 will be counted as possible borrowers of _1 in the sole-borrow analysis above.

bors · 2019-09-14T06:42:15Z

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

clippy_lints/src/redundant_clone.rs

Make rustc_mir::dataflow module pub (for clippy) I'm working on fixing false-positives of a MIR-based clippy lint (rust-lang/rust-clippy#4509), and in need of the dataflow infrastructure.

oli-obk

Sorry, totally forgot about this.

The impl lgtm. Just a nit and if you have the time some docs would be great.

clippy_lints/src/redundant_clone.rs

oli-obk

not sure why CI is failing, maybe a rustup?

Co-Authored-By: ecstatic-morse <ecstaticmorse@gmail.com>

sinkuu · 2019-10-03T00:03:26Z

CI passed.

llogiq · 2019-10-03T08:41:14Z

Looks good. Thank you! @bors r+

bors · 2019-10-03T08:41:15Z

📌 Commit 4cded6d has been approved by llogiq

bors · 2019-10-03T08:41:24Z

⌛ Testing commit 4cded6d with merge 01e0187...

Fix false-positive of redundant_clone and move to clippy::perf This PR introduces dataflow analysis to `redundant_clone` lint to filter out borrowed variables, which had been incorrectly detected. Depends on rust-lang/rust#64207. changelog: Moved `redundant_clone` lint to `perf` group # What this lint catches ## `clone`/`to_owned` ```rust let s = String::new(); let t = s.clone(); ``` ```rust // MIR _1 = String::new(); _2 = &_1; _3 = clone(_2); // (*) ``` We can turn this `clone` call into a move if 1. `_2` is the sole borrow of `_1` at the statement `(*)` 2. `_1` is not used hereafter ## `Deref` + type-specific `to_owned` method ```rust let s = std::path::PathBuf::new(); let t = s.to_path_buf(); ``` ```rust // MIR _1 = PathBuf::new(); _2 = &1; _3 = call deref(_2); _4 = _3; // Copies borrow StorageDead(_2); _5 = Path::to_path_buf(_4); // (*) ``` We can turn this `to_path_buf` call into a move if 1. `_3` `_4` are the sole borrow of `_1` at `(*)` 2. `_1` is not used hereafter # What this PR introduces 1. `MaybeStorageLive` that determines whether a local lives at a particular location 2. `PossibleBorrowerVisitor` that constructs [`TransitiveRelation`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/transitive_relation/struct.TransitiveRelation.html) of possible borrows, e.g. visiting `_2 = &1; _3 = &_2:` will result in `_3 -> _2 -> _1` relation. Then `_3` and `_2` will be counted as possible borrowers of `_1` in the sole-borrow analysis above.

bors · 2019-10-03T08:53:57Z

💔 Test failed - status-appveyor

sinkuu · 2019-10-03T09:19:20Z

Appveyor setup seems to be broken...

     Running `target\debug\clippy_dev.exe fmt --check`
error: no such subcommand: `+nightly`
error: no such subcommand: `+nightly`
error: no such subcommand: `+nightly`
Error: file `+nightly` does not exist

llogiq · 2019-10-03T16:01:44Z

Yeah, it unfortunately is.

Fix false-positive of redundant_clone and move to clippy::perf This PR introduces dataflow analysis to `redundant_clone` lint to filter out borrowed variables, which had been incorrectly detected. Depends on rust-lang/rust#64207. changelog: Moved `redundant_clone` lint to `perf` group # What this lint catches ## `clone`/`to_owned` ```rust let s = String::new(); let t = s.clone(); ``` ```rust // MIR _1 = String::new(); _2 = &_1; _3 = clone(_2); // (*) ``` We can turn this `clone` call into a move if 1. `_2` is the sole borrow of `_1` at the statement `(*)` 2. `_1` is not used hereafter ## `Deref` + type-specific `to_owned` method ```rust let s = std::path::PathBuf::new(); let t = s.to_path_buf(); ``` ```rust // MIR _1 = PathBuf::new(); _2 = &1; _3 = call deref(_2); _4 = _3; // Copies borrow StorageDead(_2); _5 = Path::to_path_buf(_4); // (*) ``` We can turn this `to_path_buf` call into a move if 1. `_3` `_4` are the sole borrow of `_1` at `(*)` 2. `_1` is not used hereafter # What this PR introduces 1. `MaybeStorageLive` that determines whether a local lives at a particular location 2. `PossibleBorrowerVisitor` that constructs [`TransitiveRelation`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/transitive_relation/struct.TransitiveRelation.html) of possible borrows, e.g. visiting `_2 = &1; _3 = &_2:` will result in `_3 -> _2 -> _1` relation. Then `_3` and `_2` will be counted as possible borrowers of `_1` in the sole-borrow analysis above.

@ghost

Rollup of 2 pull requests Successful merges: - #4509 (Fix false-positive of redundant_clone and move to clippy::perf) - #4614 (Allow casts from the result of `abs` to unsigned) Failed merges: r? @ghost

@ghost

Rollup of 2 pull requests Successful merges: - #4509 (Fix false-positive of redundant_clone and move to clippy::perf) - #4614 (Allow casts from the result of `abs` to unsigned) Failed merges: changelog: none r? @ghost

oli-obk self-assigned this Sep 6, 2019

JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 6, 2019

sinkuu force-pushed the redundant_clone_fix branch 2 times, most recently from 53fe3f9 to 64cea46 Compare September 16, 2019 16:32

ecstatic-morse reviewed Sep 18, 2019

View reviewed changes

clippy_lints/src/redundant_clone.rs Outdated Show resolved Hide resolved

sinkuu mentioned this pull request Sep 18, 2019

Make rustc_mir::dataflow module pub (for clippy) rust-lang/rust#64207

Merged

sinkuu force-pushed the redundant_clone_fix branch 2 times, most recently from 7ef97d7 to a435739 Compare September 19, 2019 13:30

oli-obk reviewed Sep 28, 2019

View reviewed changes

clippy_lints/src/redundant_clone.rs Show resolved Hide resolved

clippy_lints/src/redundant_clone.rs Outdated Show resolved Hide resolved

sinkuu force-pushed the redundant_clone_fix branch from 107c161 to 6ae4a0a Compare September 30, 2019 07:38

oli-obk approved these changes Sep 30, 2019

View reviewed changes

sinkuu force-pushed the redundant_clone_fix branch from 6ae4a0a to 0d6a077 Compare September 30, 2019 09:15

sinkuu and others added 7 commits October 3, 2019 08:10

Fix false-positive of redundant_clone and move to clippy::perf

301ef6b

Add run-rustfix

667223c

Test fixes

555f5a4

Apply suggestion

a3f403a

Co-Authored-By: ecstatic-morse <ecstaticmorse@gmail.com>

Resolve reviews

1cee3fe

Add comments

866729f

extern rustc_index

4cded6d

sinkuu force-pushed the redundant_clone_fix branch from 0d6a077 to 4cded6d Compare October 2, 2019 23:28

phansch mentioned this pull request Oct 4, 2019

Rollup of 2 pull requests #4625

Merged

bors merged commit 4cded6d into rust-lang:master Oct 4, 2019

sinkuu deleted the redundant_clone_fix branch October 4, 2019 07:36

flip1995 mentioned this pull request Oct 5, 2019

redundant_clone false positive #4633

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix false-positive of redundant_clone and move to clippy::perf #4509

Fix false-positive of redundant_clone and move to clippy::perf #4509

Uh oh!

sinkuu commented Sep 6, 2019 •

edited

Loading

Uh oh!

bors commented Sep 14, 2019

Uh oh!

Uh oh!

oli-obk left a comment

Uh oh!

Uh oh!

Uh oh!

oli-obk left a comment

Uh oh!

sinkuu commented Oct 3, 2019

Uh oh!

llogiq commented Oct 3, 2019

Uh oh!

bors commented Oct 3, 2019

Uh oh!

bors commented Oct 3, 2019

Uh oh!

bors commented Oct 3, 2019

Uh oh!

sinkuu commented Oct 3, 2019

Uh oh!

llogiq commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix false-positive of redundant_clone and move to clippy::perf #4509

Fix false-positive of redundant_clone and move to clippy::perf #4509

Uh oh!

Conversation

sinkuu commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this lint catches

clone/to_owned

Deref + type-specific to_owned method

What this PR introduces

Uh oh!

bors commented Sep 14, 2019

Uh oh!

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

sinkuu commented Oct 3, 2019

Uh oh!

llogiq commented Oct 3, 2019

Uh oh!

bors commented Oct 3, 2019

Uh oh!

bors commented Oct 3, 2019

Uh oh!

bors commented Oct 3, 2019

Uh oh!

sinkuu commented Oct 3, 2019

Uh oh!

llogiq commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sinkuu commented Sep 6, 2019 •

edited

Loading

`clone`/`to_owned`

`Deref` + type-specific `to_owned` method