Skip to content

Convert redundant_clone to an analysis pass take 2 #14599

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Apr 12, 2025

supersedes #11364
fixes #10940
fixes #10893
fixes #10870
fixes #10577
fixes #10545
fixes #10517

Don't worry about reviewing this as a whole, I'll be splitting it up into smaller chunks to be reviewed separately.

This comes with a separate dataflow framework than rustc's since many changes would be needed to track values correctly. As an example a mutable borrow of a pair of strings &mut (String, String) would need to cause both strings to hold different values and the only way to do that in rustc's framework is to track sub-statement positions. The new framework lets us easily do a preprocessing pass at the expense of not trivially supporting cursors.

Arena allocation is used heavily throughout. This simplifies some lifetimes both by using only a single lifetime parameter and not having to store as many owning values. It also avoids having a large number of small allocations that need to also be dropped.

No suggestions are give with the lint. This is unfortunate but they are non-trivial to add since removing the clone call is only correct in a subset of cases. Future uses of a projection may need to be changed; the clone call might be changed to a move of a different name (x.clone() -> y) or changed to a borrow. mem::take is needed in some cases as well.

The diagnostic output is currently a stand-in. I will be adding notes about where the values diverge and where the lifetimes of the pair of values end. Basically a message saying one value is modified, but the other isn't used after that point before it's dropped.

changelog: redundant_clone: Fix false positives where the clone result is stored in a local.
changelog: redundant_clone: Track the use of the clone throughout the function body rather than only checking for immediate consumption.
changelog: redundant_clone: Extended to analyze code in loops
changelog: redundant_clone: Track values inside fields rather than just tracking top level locals.

@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2025

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 12, 2025
@Jarcho Jarcho force-pushed the rclone branch 3 times, most recently from 6ec4a89 to 651d371 Compare April 12, 2025 10:48
@blyxyas
Copy link
Member

blyxyas commented Apr 23, 2025

Don't worry about reviewing this as a whole

Is this ready for review? Seems that it's still a single commit.

@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 23, 2025

I'm splitting it into multiple PRs right now.

@Alexendoo Alexendoo added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Apr 24, 2025
@Alexendoo Alexendoo removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label May 4, 2025
Copy link

Lintcheck changes for 85441ab

Lint Added Removed Changed
clippy::redundant_clone 31 7 5

This comment will be updated if you push new changes

@Jarcho
Copy link
Contributor Author

Jarcho commented Jul 26, 2025

Ping @oli-obk for a review of the MIR related additions.


This does not use rustc's dataflow framework. I would need a few non-trivial changes to make it work so it's going to be easier to keep this separate for now. The main difference are that blocks are pre-processed before the analysis is run. This was needed for redundant_clone in order to give operations a stable value (leaking a mutable reference needs type dependant number of new values for instance). This does make a ResultsCursor-like interface non-trivial, but it has also not been needed.

A few other differences:

  • The transfer function is part of the analysis rather than the domain type.
  • The transfer function knows the source and destination block ids.
  • The clone for the temporary is also handled by the analysis. This is also used for a horrible hack that I don't have a better solution for (multiple predecessor values need to be collapsed into a single value only after we have all of them).
  • The analysis always runs the blocks in order. This is needed to avoid some false-negatives in redundant clone without doing a final pass to extract the results.
  • Analysis constructors are responsible for allocating and initializing the entry sets and the temporary state.

Overall this can result in a little extra boilerplate, but it also comes with a lot of flexibility for how things are done.

The projection map works does more or less the same job as MovePaths (flattening the local projection trees), but doesn't filter by which paths appear in the body. I should probably make it do that as well, but the current type filter is good enough for redundant_clone. childless_projection should probably also be removed; it used to be more different initially.


The actual lint itself is split into three parts: finding the lint calls, resolving self reference in the clone calls to locals, and then checking if the clones are redundant. The first two parts are relatively straight forward. Resolving the references couold be done in a single pass rather than as an analysis at the cost of missing some rare cases, but it's cheap enough to run.

The final part is where all the complexity comes in. There are still a few things to fix with it and I need to document what's happening better. The general idea of how it works is by tracking from each clone call when one of the values might be modified/consumed, and the other value is then read.

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
4 participants