Skip to content

Conversation

sinkuu
Copy link
Contributor

@sinkuu sinkuu commented Sep 6, 2019

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

  1. _2 is the sole borrow of _1 at the statement (*)
  2. _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

  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 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.

@oli-obk oli-obk self-assigned this Sep 6, 2019
@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 6, 2019
@bors
Copy link
Contributor

bors commented Sep 14, 2019

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

@sinkuu sinkuu force-pushed the redundant_clone_fix branch 2 times, most recently from 53fe3f9 to 64cea46 Compare September 16, 2019 16:32
tmandry added a commit to tmandry/rust that referenced this pull request Sep 18, 2019
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.
@sinkuu sinkuu force-pushed the redundant_clone_fix branch 2 times, most recently from 7ef97d7 to a435739 Compare September 19, 2019 13:30
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, totally forgot about this.

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

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

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why CI is failing, maybe a rustup?

@sinkuu sinkuu force-pushed the redundant_clone_fix branch from 6ae4a0a to 0d6a077 Compare September 30, 2019 09:15
@sinkuu sinkuu force-pushed the redundant_clone_fix branch from 0d6a077 to 4cded6d Compare October 2, 2019 23:28
@sinkuu
Copy link
Contributor Author

sinkuu commented Oct 3, 2019

CI passed.

@llogiq
Copy link
Contributor

llogiq commented Oct 3, 2019

Looks good. Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2019

📌 Commit 4cded6d has been approved by llogiq

@bors
Copy link
Contributor

bors commented Oct 3, 2019

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

bors added a commit that referenced this pull request Oct 3, 2019
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
Copy link
Contributor

bors commented Oct 3, 2019

💔 Test failed - status-appveyor

@sinkuu
Copy link
Contributor Author

sinkuu commented Oct 3, 2019

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

llogiq commented Oct 3, 2019

Yeah, it unfortunately is.

phansch added a commit to phansch/rust-clippy that referenced this pull request Oct 4, 2019
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 added a commit that referenced this pull request Oct 4, 2019
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
bors added a commit that referenced this pull request Oct 4, 2019
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
@bors bors merged commit 4cded6d into rust-lang:master Oct 4, 2019
@sinkuu sinkuu deleted the redundant_clone_fix branch October 4, 2019 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants