fix(redundant_clone): Make visit_local_usage analyse loop bodies instead of giving up on them

[CommanderStorm]

View all comments

Fixes #16096

redundant_clone missed every clone() in a loop body:

#![warn(clippy::redundant_clone)]
#![expect(dead_code)]

fn foo(){
    loop {
        let a = String::new();
        let _b = a.clone(); // not linted :(
    }
}

visit_local_usage returned None whenever location.block was in a cycle, and redundant_clone passes the clone's own block.
The match in the issue is a bit of a red-herring.
The arm ending in return linted only because it leaves the cycle.

The bailout can't just be dropped, or issue13900::regression false-positives.
In #13900 a's only read precedes location in the clone's own block, and V::visit_place hides it even though the cycle re-executes it.

What separates the two cases is where the local is declared.
One declared inside the body is StorageDead-ed before the back edge and StorageLive-d again next iteration, so it is a distinct variable each time round.
One declared outside is not...

reachable_while_storage_live walks forward from location, stops a path once every tracked local is StorageDead, and returns None only if location.block is reached again while one is still live.

readonly_write_lock and used_exactly_once pass START_BLOCK, where the old bailout was unreachable since bb0 is never a branch target, therefore I don't think this specific issue exists there.
At least, I cannot come up with syntax that is not already caught by previous clippy.

changelog: [redundant_clone]: detect redundant clones inside loop bodies without aliases

[rustbot]

Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews.

In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[github-actions]

Lintcheck changes for 7724e6c

Lint	Added	Removed	Changed
clippy::redundant_clone	13	0	1

This comment will be updated if you push new changes

[CommanderStorm]

May be trivial for some, but in case it is not:

Blocks visited in reverse postorder, which is execution order.
Example A -> {B, C}, B -> D, C -> D (a diamond).

Callers take the first recorded location, so iterating blocks by index would be wrong.

View changes since the review

[CommanderStorm]

I think pop order here is irelevant due to symetry.

Also could not find a case where the order is relevant for the linting decision.
Not sure, does not seem to matter. 🤷🏻‍♂️

View changes since the review

[Jarcho]

In general for fixpoint analysis it matters only for perf reasons. The ideal order for most forward analyses would be to always check blocks in a reverse-postorder manner. For this case the maximum distance to the top of the lattice is the number of locals (either one or two) so there's not any real risk of checking a block too many times.

[CommanderStorm]

#14599 has a few interesting testcases

The cases which are incorrectly linting and #14599 does not lint are all unrelated to this change and already fail on main.

testcases that I am ignoring becauese does not compile under current rust (not sure about polonius, have not tried)

I am not, because that would not compile with error[E0382]: use of moved value: x

let mut x = black_box(String::new());
for _ in 0..10 {
    let _y = x.clone(); //~ redundant_clone
    black_box(&mut x);
}

I am not, because that would not compile with error[E0382]: borrow of moved value: y

let mut x = black_box(String::new());
for _ in 0..10 {
    let y = black_box(String::new());
    x = y.clone(); //~ redundant_clone
    black_box(&y);
}

let mut x = black_box(String::new());
for _ in 0..10 {
    let y = black_box(String::new());
    x = y.clone(); //~ redundant_clone
    black_box(&y);
}
black_box(&x);

🤔 The loop-cases which I AM missing (=not linting) are all closely related to aliasing

let mut x = black_box(String::new());
for _ in 0..10 {
    let y = x; //~ redundant_clone
    black_box(y);
    x = black_box(String::new());
}

let mut x = black_box(String::new());
for _ in 0..10 {
    let y = x.clone(); //~ redundant_clone
    black_box(y);
    x = black_box(String::new());
}

let mut x = black_box(String::new());
let mut y = black_box(String::new());
for _ in 0..10 {
    y = x.clone(); //~ redundant_clone
    x = black_box(String::new());
}
black_box(&x);

let mut x = black_box(String::new());
let mut y = x.clone(); //~ redundant_clone
for _ in 0..10 {
    black_box(y);
    x = black_box(String::new());
    y = x.clone(); //~ redundant_clone
}

[CommanderStorm]

seems that the aliasing lifeness issues are closely related, but would bloat the PR again, so lets likely better push that to a second PR.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[CommanderStorm]

@rustbot ready

[Gri-ffin]

Why is N limited to 7 here, from what I understand the old implementation has no restriction? I can't seem to find a reason looking at the code itself.

View changes since the review

[CommanderStorm]

N is currently well below this (1 or 2) as it is the size of locals.
Previous code did not have this limitation, but also did not use this and I am not sure what kind of lint would.. 7 is kind of a lot for this helper fn, given what locals is.

If we want more than 7 locals, we need to increase from an u8 bitset to something larger (u16/.../u128). Should be an easy fix, if we need to.
The assert just makes sure that if N > 7 that this is an error, given that this is likely 2-3 steps away from where N is actually specified via the size of locals.

Actually, my original impl was to use an DenseBitSet, but perf wise Jarcho is right, u8 is better..
See #17495 (comment) for context

[CommanderStorm]

if you want to have a code reason why this is one of the invariants of this impl:

https://github.com/CommanderStorm/rust-clippy/blob/8aa93d6f3d3b025f02a1041b26ecfac3f200925c/clippy_utils/src/mir/mod.rs#L102

[Jarcho]

If a caller ends up needing more than seven locals they shouldn't be using this function in the first place. The implementation does not scale well with the number of locals and fixing that requires preprocessing the body in a way that isn't worth doing when only checking one or two locals (which is all we need anyway).

[Jarcho]

The implementation looks to be good.

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[CommanderStorm]

@rustbot review