Add block_scrutinee lint

[shivendra02467]

View all comments

This PR introduces a new late pass lint to catch scrutinees unnecessarily wrapped in blocks on older editions, preventing unintended behavior regarding temporary lifetimes.

Fixes #16827

changelog: new lint: [block_scrutinee] to warn on scrutinees wrapped in blocks in older editions

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[github-actions]

No changes for 3ff2380

[llogiq]

What is the behavior in the current edition? Would it make sense to check whether the block survives a lifetime check if the edition was updated?

[shivendra02467]

Hi @llogiq, thanks for taking a look!
To answer your questions:

1. Behavior in the 2024 edition:As of the 2024 edition, temporaries created in the tail expression of a block are dropped "before" the body executes (at the end of the block). Prior to 2024, they were not dropped until the end of the entire match/if let statement. This can cause a real bug like someone tried to fix a deadlock by doing if let Some(x) = { lock().foo() } { ... }. They assumed the lock would drop before the body. That works in 2024, but silently deadlocks in 2021.

2. Regarding a lifetime check:
   @Darksonn explicitly addressed this in the original issue (Warn on old editions when scrutinee is wrapped in a block #16827) and determined that an advanced lifetime check isn't necessary because removing the block (when it only contains a tail expression) "never" changes behavior prior to the 2024 edition:
   If the user meant to drop the temporaries (like the deadlock fix attempt), the block doesn't actually work in the 2021 edition anyway. The code is broken and they must rewrite it (eg. extracting the value to a separate let statement).
   If the user didnt mean to drop temporaries, the block is just unnecessary visual noise, and removing it makes the code less confusing without changing the drop semantics.

because of this, we should warn unconditionally on older editions to catch these false assumptions without needing deeper type/lifetime checking. Let me know if you think we should still try to narrow the lint down!

[Darksonn]

Please include a test that verifies that this does not trigger on the 2024 edition.

[Darksonn]

What is the behavior in the current edition?

The behavior of { expr } in the current edition is what you would expect. Temporaries in the expression are dropped right away when the block returns, and not after the end of the parent expression's scope.

Would it make sense to check whether the block survives a lifetime check if the edition was updated?

I don't think so. Nobody is going to write { expr } when they could have written expr without a good reason, and on older editions, the behavior is exactly identical, so there is no good reason to do it on older editions. Anyone writing { expr } did so with the intent of getting the 2024 edition behavior. If they don't get that behavior, we should tell them.

[shivendra02467]

@Darksonn Thanks for suggesting the improvements, I will update the PR to include them.

[llogiq]

Ok. I think the lint should suggest updating the edition in case the user actually wants the early drop – or moving the block into its own let stmt.

[shivendra02467]

Hi @llogiq, thanks for the review!

Ok. I think the lint should suggest updating the edition in case the user actually wants the early drop

I just added a help message for this in the new commit (508183e).

or moving the block into its own let stmt.

The suggestion to move the expression to a separate let statement was actually added in the previous commit after Alice's review!

Let me know if you need me to change anything else!

[shivendra02467]

Hi @llogiq, just sending a gentle ping on this when you have a moment! All previous feedback has been addressed and the latest tests are passing.

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

[shivendra02467]

@Jarcho Thanks for the review, All feedback implemented, please take a quick look to verify.
@rustbot ready

Requested changes are applied.