Rescope temp lifetime in if-let into IfElse with migration lint

[dingxiangfei2009]

Tracking issue #124085

This PR shortens the temporary lifetime to cover only the pattern matching and consequent branch of a if let.

At the expression location, means that the lifetime is shortened from previously the deepest enclosing block or statement in Edition 2021. This warrants an Edition change.

Coming with the Edition change, this patch also implements an edition lint to warn about the change and a safe rewrite suggestion to preserve the 2021 semantics in most cases.

The state of lint

The rustc_lint works as follows. It checks a if HIR expression and collect a consecutive run of else if cascade as far as it can see. Whenever a significant dropper is found in the scrutinee in any one of the conditional, it emits a lint. A series and rewrite suggestion is emitted at the end of the probe so that it does not get too verbose and most importantly generates a non-self-intersecting set of spans for a valid rewrite.

The reason that we are doing this suggestion-coalescing gymnastic for now is because of the current limitation of rustfix, details of which can be found #53934. My personal view on the matter is that holistic solution is really necessary for a more elegant solution here. I believe that we will make more progress if we push through #53934.

The exact reason that the same suggestion as what you get from rustc_lint is not automatically machine applicable from the borrow checker as what you get from rustc_lint is due to the possible intersection between spans. We apologize for the inconvenience.

Related to #103108.
Related crater runs: #129466.

[rustbot]

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

[dingxiangfei2009]

cc @est31

I have tested this branch on several of my repositories that I am working on and nothing obvious has broken. Shall we have a crater run?

[est31]

@bors try

[bors]

⌛ Trying commit 003f3cd8d3cecb9b20f5105400499741c936e92a with merge ece70b22820429ebf0c6d388c32356a14c58fc2f...

[bors]

☀️ Try build successful - checks-actions
Build commit: ece70b22820429ebf0c6d388c32356a14c58fc2f (ece70b22820429ebf0c6d388c32356a14c58fc2f)

[dingxiangfei2009]

I have updated the failing tests to show the effect of this change.

[oli-obk]

@craterbot run mode=build-and-test

[craterbot]

👌 Experiment pr-107251 created and queued.
🤖 Automatically detected try build ece70b22820429ebf0c6d388c32356a14c58fc2f
⚠️ Try build based on commit 003f3cd8d3cecb9b20f5105400499741c936e92a, but latest commit is 92550dd. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-107251 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-107251 is completed!
📊 128 regressed and 99 fixed (253904 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[bors]

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

[dingxiangfei2009]

@rustbot ready

cc @jieyouxu

• I found a typo in explain_borrow text and fixed it.
• I have to downgrade the applicability level for notes from borrow checker, because the span will unfortunately intersect and there is currently impossible to coalesce multiple suggestions into one, like we have done for the rustc_lint changes. I suppose that we have to leave it as unresolved question for now. However, the suggestion itself should be very helpful enough for users to migrate. It is sad to see it being unable to automatically apply them, but the supposed impact according to the crater run before the commencement of this work suggested that the impact should be small.

[jieyouxu]

cc #129466 (comment) for the top-1000 crater run.

[jieyouxu]

Thanks, this mostly looks reasonable (I can't say for certain that it's entirely correct because unfortunately the logic is... complicated).

[jieyouxu]

Something I noticed is that having to handle when or when not to add parenthesis makes the logic very, very complicated. Could we just unconditionally add parentheses? Or will that cause issues with rustfix?

also cc @traviscross FYI for T-lang, due to limitations of mutually overlapping suggestions we cannot make the suggestions MachineApplicable, meaning that auto-migration is not feasible in this case. I believe this is cc #53934.

Note that after the diagnostics here is fixed, we probably want to check the impact in a full crater run, previous crater run was for top-1000 only.

[dingxiangfei2009]

@rustbot ready

• Suggestion is simplified. The strategy to employ multiple Subdiagnostic backfires, so those are flattened into one layer.

[jieyouxu]

Thanks, this looks reasonable to me now.

[jieyouxu]

Since this migration lint is still feature-gated, I'm okay with merging the implementation first and not block this on a crater run.

@bors r+ rollup=never (this is quite complicated and tricky)

[bors]

📌 Commit b4b2b35 has been approved by jieyouxu

It is now in the queue for this repository.

[bors]

⌛ Testing commit b4b2b35 with merge d51203c...

[rust-log-analyzer]

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

💔 Test failed - checks-actions

[jieyouxu]

@bors retry (failed to remove cargo.exe, cc #127883)

[bors]

⌛ Testing commit b4b2b35 with merge a5efa01...

[bors]

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing a5efa01 to master...

[rust-timer]

Finished benchmarking commit (a5efa01): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.2%, 1.3%]	19
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	1

Max RSS (memory usage)

Results (primary -1.6%, secondary -3.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-3.2%	[-3.7%, -2.8%]	3
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

Cycles

Results (secondary -7.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-7.5%	[-7.5%, -7.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 757.342s -> 759.189s (0.24%)
Artifact size: 341.24 MiB -> 341.20 MiB (-0.01%)

[Mark-Simulacrum]

Seems like the regression is primarily looking like a revert of #127313... @cjgillot do you have thoughts on what the delta here might be? I don't see a very obvious cause here. Regressions are minor enough that I'm not worried, but just checking if there's an easy explanation.