Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Jan 24, 2023

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
Copy link
Collaborator

rustbot commented Jan 24, 2023

r? @oli-obk

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 24, 2023
@rust-log-analyzer

This comment has been minimized.

@dingxiangfei2009
Copy link
Contributor Author

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
Copy link
Member

est31 commented Jan 29, 2023

@bors try

@bors
Copy link
Contributor

bors commented Jan 29, 2023

⌛ Trying commit 003f3cd8d3cecb9b20f5105400499741c936e92a with merge ece70b22820429ebf0c6d388c32356a14c58fc2f...

@bors
Copy link
Contributor

bors commented Jan 29, 2023

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

@dingxiangfei2009
Copy link
Contributor Author

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

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk

This comment was marked as resolved.

@craterbot

This comment was marked as resolved.

@dingxiangfei2009

This comment was marked as resolved.

@craterbot

This comment was marked as resolved.

@oli-obk

This comment was marked as resolved.

@craterbot

This comment was marked as resolved.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 30, 2023

@craterbot run mode=build-and-test

@craterbot
Copy link
Collaborator

👌 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-107251 is now running

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

@craterbot
Copy link
Collaborator

🎉 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

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 31, 2023
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
…r=jieyouxu

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

Tracking issue rust-lang#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.

Related to rust-lang#103108.
Related crater runs: rust-lang#129466.
@bors
Copy link
Contributor

bors commented Sep 12, 2024

⌛ Testing commit b4b2b35 with merge d51203c...

@rust-log-analyzer
Copy link
Collaborator

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
Copy link
Contributor

bors commented Sep 13, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 13, 2024
@jieyouxu
Copy link
Member

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

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2024
@bors
Copy link
Contributor

bors commented Sep 13, 2024

⌛ Testing commit b4b2b35 with merge a5efa01...

@bors
Copy link
Contributor

bors commented Sep 13, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 13, 2024
@bors bors merged commit a5efa01 into rust-lang:master Sep 13, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 13, 2024
@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot added the perf-regression Performance regression. label Sep 13, 2024
@dingxiangfei2009 dingxiangfei2009 deleted the let-chain-rescope branch September 14, 2024 17:00
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2024
…er-run, r=<try>

[CRATER RUN DO NOT MERGE] Let chain lint crater run

Tracked by rust-lang#124085
Related to rust-lang#107251
cc `@jieyouxu` for review context
cc `@traviscross` for edition tracking

There is one unresolved issue that `cargo fix --edition` does not emit `if-let-rescope` lint. Details in rust-lang/cargo#14447.

Note that this patch is assuming that the feature gate `if_let_rescope` is always on just for this crater run.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2024
…er-run, r=<try>

[CRATER RUN DO NOT MERGE] Let chain lint crater run

Tracked by rust-lang#124085
Related to rust-lang#107251
cc `@jieyouxu` for review context
cc `@traviscross` for edition tracking

There is one unresolved issue that `cargo fix --edition` does not emit `if-let-rescope` lint. Details in rust-lang/cargo#14447.

Note that this patch is assuming that the feature gate `if_let_rescope` is always on just for this crater run.
@Mark-Simulacrum
Copy link
Member

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.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2024
…er-run, r=<try>

[CRATER RUN DO NOT MERGE] Let chain lint crater run

Tracked by rust-lang#124085
Related to rust-lang#107251
cc `@jieyouxu` for review context
cc `@traviscross` for edition tracking

There is one unresolved issue that `cargo fix --edition` does not emit `if-let-rescope` lint. Details in rust-lang/cargo#14447.

Note that this patch is assuming that the feature gate `if_let_rescope` is always on just for this crater run.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
…er-run, r=<try>

[CRATER RUN DO NOT MERGE] Let chain lint crater run

Tracked by rust-lang#124085
Related to rust-lang#107251
cc `@jieyouxu` for review context
cc `@traviscross` for edition tracking

There is one unresolved issue that `cargo fix --edition` does not emit `if-let-rescope` lint. Details in rust-lang/cargo#14447.

Note that this patch is assuming that the feature gate `if_let_rescope` is always on just for this crater run.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
…er-run, r=<try>

[CRATER RUN DO NOT MERGE] Let chain lint crater run

Tracked by rust-lang#124085
Related to rust-lang#107251
cc `@jieyouxu` for review context
cc `@traviscross` for edition tracking

There is one unresolved issue that `cargo fix --edition` does not emit `if-let-rescope` lint. Details in rust-lang/cargo#14447.

Note that this patch is assuming that the feature gate `if_let_rescope` is always on just for this crater run.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
…er-run, r=<try>

[CRATER RUN DO NOT MERGE] Let chain lint crater run

Tracked by rust-lang#124085
Related to rust-lang#107251
cc `@jieyouxu` for review context
cc `@traviscross` for edition tracking

There is one unresolved issue that `cargo fix --edition` does not emit `if-let-rescope` lint. Details in rust-lang/cargo#14447.

Note that this patch is assuming that the feature gate `if_let_rescope` is always on just for this crater run.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
…er-run, r=<try>

[CRATER RUN DO NOT MERGE] Let chain lint crater run

Tracked by rust-lang#124085
Related to rust-lang#107251
cc `@jieyouxu` for review context
cc `@traviscross` for edition tracking

There is one unresolved issue that `cargo fix --edition` does not emit `if-let-rescope` lint. Details in rust-lang/cargo#14447.

Note that this patch is assuming that the feature gate `if_let_rescope` is always on just for this crater run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.