Fix replace_box FP when the box is moved

[profetia]

Closes #15968

changelog: [replace_box] fix FP when the box is moved

r? samueltardieu As you wish

Summary Notes

• Beta-nomination by samueltardieu

Managed by @rustbot—see help for details

[samueltardieu]

I am a bit concerned by the fact that we would be accumulating sets of HirId for every body in the crate while we don't need them after a body has been analyzed.

Since bodies can be stacked, maybe you can use a stack of caches instead of a large cache. This should be very lightweight. Also, you can use the BodyId from cx.enclosing_body more freely, to get the body's LocalDefId as well as the Body itself.

To make myself clear, I tried the proposed changes in samueltardieu@56b815e. Feel free to cherry-pick this commit if you agree and squash it with yours.

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.

[profetia]

Looks good to me. Thank you!

[samueltardieu]

LGTM, but can you please add those tests as well inside issue15951()?

    fn embedded_body() {
        let mut x = Box::new(());
        let y = x;
        x = Box::new(());

        struct Foo {
            inner: String,
        }
        let mut x = Box::new(Foo { inner: String::new() });
        let y = x.inner;
        x = Box::new(Foo { inner: String::new() });
        //~^ replace_box
    }

    let mut x = Box::new(Foo { inner: String::new() });
    let in_closure = || {
        x = Box::new(Foo { inner: String::new() });
        //~^ replace_box
    };

and this one outside any function:

static R: fn(&mut Box<String>) = |x| *x = Box::new(String::new());
//~^ replace_box

This will cover most test cases, and this is important since we are probably going to backport this fix to Rust 1.92.

View changes since this review

[samueltardieu]

@rustbot label beta-nominated
@rustbot note Beta-nomination

The issue reported in #15968 affects Rust 1.92 which is the release in which this lint has been introduced. Backporting this fix would prevent the issue from appearing in stable Rust as the same time as the lint.

[samueltardieu]

@y21 Would you do a second review since the PR is beta nominated?

[y21]

LGTM as well now

View changes since this review

[github-actions]

Lintcheck changes for 098ded3

Lint	Added	Removed	Changed
clippy::replace_box	0	3	0

This comment will be updated if you push new changes

[samueltardieu]

Could you apply Jarcho's suggestion at https://github.com/rust-lang/rust-clippy/pull/15984/files#r2480316068 which is even better than a stack? This will clear the set when the uppermost level that used it is left so it is enough not to accumulate data for the whole crate.

View changes since this review

[profetia]

Updated. Thank you all!

[samueltardieu]

@profetia Before I merge this, can you add a test with the double Box?

That will also let a last chance for people to comment on.

[samueltardieu]

Also, it looks like we get false negatives now (see the lintcheck results). Could you have a look?

[Jarcho]

The current approach will have lots of false positives since it doesn't take temporal information into account. This needs a MIR analysis to work properly, but that transition is a much larger undertaking and it's not needed to get the lint out of the nursery.

A basic false negative would look like:

let mut x = Box::new(String::new());
x = Box::new(String::new());
x;

Any number of arbitrarily complex cases can be constructed that will be a huge pain to deal with when working with the HIR tree.

[samueltardieu]

The current approach will have lots of false positives since it doesn't take temporal information into account

Do you mean FN? If yes, I agree.

[Jarcho]

That is indeed what I meant.

[profetia]

I agree too. I've add the test case and comment to describe it

[samueltardieu]

Thanks.