significant_drop_in_scrutinee: Trigger lint only if lifetime allows early significant drop

[lrh2000]

I want to argue that the following code snippet should not trigger significant_drop_in_scrutinee (#8987). The iterator holds a reference to the locked data, so it is expected that the mutex guard must be alive until the entire loop is finished.

use std::sync::Mutex;

fn main() {
    let mutex_vec = Mutex::new(vec![1, 2, 3]);
    for number in mutex_vec.lock().unwrap().iter() {
        dbg!(number);
    }
}

However, the lint should be triggered when we clone the vector. In this case, the iterator does not hold any reference to the locked data.

-     for number in mutex_vec.lock().unwrap().iter() {
+     for number in mutex_vec.lock().unwrap().clone().iter() {

Unfortunately, it seems that regions on the types of local variables are mostly erased (ReErased) in the late lint pass. So it is hard to tell if the final expression has a lifetime relevant to the value with a significant drop.

In this PR, I try to make a best-effort guess based on the function signatures. To avoid false positives, no lint is issued if the result is uncertain. I'm not sure if this is acceptable or not, so any comments are welcome.

Fixes #8987

changelog: [significant_drop_in_scrutinee]: Trigger lint only if lifetime allows early significant drop.

r? @flip1995

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) some time within the next two weeks.

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

[bors]

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

[flip1995]

I think those are really valuable changes, but won't get to reviewing those for quite a long time, so re-assigning

r? clippy

[blyxyas]

Hi @lrh2000, thanks for the contribution! I've looked at it and nothing triggers my alerts, but we'd prefer if you could split this PR into multiple others. This would help both in the review & merging process and with bisecting future possible bugs.

Thanksies for your contribution! ❤️

[lrh2000]

Hi @lrh2000, thanks for the contribution! I've looked at it and nothing triggers my alerts, but we'd prefer if you could split this PR into multiple others. This would help both in the review & merging process and with bisecting future possible bugs.

Thanksies for your contribution! ❤️

Hi @blyxyas, thank you for your comment and advice. I am willing to do that. I plan to use three PRs to cover those three changelogs. (Maybe we can have more, like one PR for each commit, but I think there's a tradeoff.)

changelog: [significant_drop_in_scrutinee]: Fix false positives due to false drops of place expressions.
changelog: [significant_drop_in_scrutinee]: Trigger lint only if lifetime allows early significant drop.
changelog: [significant_drop_in_scrutinee]: Trigger lint also for scrutinees in while let and if let.

So this PR will only cover the second changelog. I'll temporarily mark it as a draft until the first PR (#12764) is merged. Since all these changes are in one file, they must be merged one at a time, or else there will be a bunch of conflicts to resolve (and break unit tests that depend on multiple fixes or features).

[bors]

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

[lrh2000]

OK, now this PR contains only one commit "Trigger lint only if lifetime allows early significant drop". This commit should be atomic enough to be reviewed separately.

I've updated the PR description accordingly.

[bors]

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

[blyxyas]

LGTM, just a mini-nit

[blyxyas]

I think that changing this to just an usize would be simpler

[lrh2000]

Actually, it does have some effect on the error messages, as shown in the diff.

But IMO either the old or the new error messages are fine, so it might be better to remove the Option<_> to avoid unnecessary confusion.

[lrh2000]

The above force-push is a pure rebase to resolve conflicts. No changes are made to the code.

See below for new changes:

• diff addresses the above comment;
• diff removes an erroneously added empty qemu.log file.. (sorry)

[blyxyas]

@bors r+

[bors]

📌 Commit 6641f9f has been approved by blyxyas

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6641f9f with merge 5aae5f6...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 5aae5f6 to master...