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

[CRATER RUN DO NOT MERGE] crater run to evaluate effectiveness of lint for if_let_rescope #129466

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Aug 23, 2024

Tracked by #124085
Related to #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.

@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2024

r? @lcnr

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

@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2024

⚠️ Warning ⚠️

  • These commits modify submodules.

@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 Aug 23, 2024
@dingxiangfei2009
Copy link
Contributor Author

dingxiangfei2009 commented Aug 23, 2024

r? @jieyouxu

Maybe cc or r ? @ehuss if more appropriate

@rustbot rustbot assigned jieyouxu and unassigned lcnr Aug 23, 2024
@@ -4,7 +4,7 @@
shallow = true
[submodule "src/tools/cargo"]
path = src/tools/cargo
url = https://github.com/rust-lang/cargo.git
url = https://github.com/dingxiangfei2009/cargo.git
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: maybe this also needs to be on a separate branch?

@jieyouxu
Copy link
Member

jieyouxu commented Aug 23, 2024

We can inject crate-level attrs thanks to #52355:

rustc -Z unstable-options -Z crate-attr='feature(if_let_rescope)'

This would satisfy one of the conditions for lint emission, which is the feature-gate. If we also do the Cargo.toml edition = 2021 -> edition = 2024 rewrite, then I think that would satisfy both conditions.

EDIT: this is a migration lint, it cannot only fire on Edition 2024.

@jieyouxu
Copy link
Member

But I'm not sure about cargo fix --edition not applying the migration lint fixes in the migration pass, so we'll want a cargo expert to help us take a look.

@jieyouxu
Copy link
Member

@rustbot blocked (on figuring out why cargo fix --edition doesn't apply the migration lint)

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2024
@jieyouxu jieyouxu added A-edition-2024 Area: The 2024 edition S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Aug 23, 2024
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cargo submodule change has some extra changes to it which I would not recommend including. I would suggest just cherry-picking commit b2608729b3ef3540a8020a06a3503eff9d0dbdd2 from my fork (applied to a branch from the current submodule's commit).

compiler/rustc_lint/src/if_let_rescope.rs Outdated Show resolved Hide resolved
@jieyouxu jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Aug 24, 2024
@dingxiangfei2009 dingxiangfei2009 force-pushed the let-chain-lint-crater-run branch from 0a7118a to 6aa732a Compare August 24, 2024 17:57
@rust-log-analyzer

This comment has been minimized.

@dingxiangfei2009
Copy link
Contributor Author

Looks like it is working locally.

Shall we give it a crater run? Just check is okay.

cc @jieyouxu

@jieyouxu
Copy link
Member

Looks like it is working locally.

We could, but it has to build on the try-job.

@bors try

@bors
Copy link
Contributor

bors commented Aug 25, 2024

⌛ Trying commit 6aa732a with merge c469850...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 25, 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.
@ehuss
Copy link
Contributor

ehuss commented Sep 17, 2024

@dingxiangfei2009, it looks like your cargo branch is outdated. You'll probably need to rebase it on latest master of the cargo repo.

(In general, using the tip of master of rust-lang/cargo means you are using things that haven't landed in rust-lang/rust, yet, which may lead to problems. Using the SHA of the submodule in rust-lang/rust's cargo submodule as your base can avoid that, but is more awkward to do.)

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 17, 2024

💔 Test failed - checks-actions

@dingxiangfei2009 dingxiangfei2009 force-pushed the let-chain-lint-crater-run branch from 912b738 to 81fac33 Compare September 17, 2024 21:51
@dingxiangfei2009
Copy link
Contributor Author

@ehuss Thanks for the tip! I rebased this onto the new master and cherry-picked the commit into the cargo submodule. I am going to give it a try again.

@dingxiangfei2009
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 17, 2024

⌛ Trying commit 81fac33 with merge ccf408f...

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.
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:fb97a1d6377f6cf2227825318ca4bbde3889e0c420746f5a03ba46a07e9a862725c26a09b9fc49a0d129ebd75935d3f6cd19acf41cc4267a6846fd4aa574b12c:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---

6    |                                       |
7    |                                       creates a temporary value which is freed while still in use
8    |
- note: lifetime for temporaries generated in `if let`s have been shorted in Edition 2024
-   --> $DIR/if-let-rescope-suggestions.rs:23:65
+ note: lifetimes for temporaries generated in `if let`s have been shortened in Edition 2024 so that they are dropped here instead
11    |
11    |
12 LL |     do_something(if let Some(value) = Droppy.get_ref() { value } else { &0 });
+    |                                                                ^
14 help: consider using a `let` binding to create a longer lived value
15    |
15    |
16 LL ~     let binding = Droppy;
18    |
19 help: consider rewriting the `if` into `match` which preserves the extended lifetime
20    |
20    |
- LL |     do_something(match Droppy.get_ref()  { Some(value) => { value } _ => { &0 }});
-    |                  ~~~~~                   ++++++++++++++++           ~~~~       +
+ LL |     do_something({ match Droppy.get_ref()  { Some(value) => { value } _ => { &0 }}});
23 
24 error: aborting due to 1 previous error
25 



The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/drop/if-let-rescope-suggestions/if-let-rescope-suggestions.stderr
diff of fixed:

21 
22 fn main() {
23     let binding = Droppy;
-     do_something(match binding.get_ref()  { Some(value) => { value } _ => { &0 }});
+     do_something({ match binding.get_ref()  { Some(value) => { value } _ => { &0 }}});
25     //~^ ERROR: temporary value dropped while borrowed
27 


The actual fixed differed from the expected fixed.
The actual fixed differed from the expected fixed.
Actual fixed saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/drop/if-let-rescope-suggestions/if-let-rescope-suggestions.fixed
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args drop/if-let-rescope-suggestions.rs`

error: 2 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/drop/if-let-rescope-suggestions.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/drop/if-let-rescope-suggestions" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/drop/if-let-rescope-suggestions/auxiliary" "--edition=2024" "-Z" "validate-mir" "-Zunstable-options"
--- stderr -------------------------------
error[E0716]: temporary value dropped while borrowed
##[error]  --> /checkout/tests/ui/drop/if-let-rescope-suggestions.rs:23:39
   |
   |
LL |     do_something(if let Some(value) = Droppy.get_ref() { value } else { &0 });
   |                                       |
   |                                       creates a temporary value which is freed while still in use
   |
   |
note: lifetimes for temporaries generated in `if let`s have been shortened in Edition 2024 so that they are dropped here instead
   |
   |
LL |     do_something(if let Some(value) = Droppy.get_ref() { value } else { &0 });
help: consider using a `let` binding to create a longer lived value
   |
   |
LL ~     let binding = Droppy;
LL ~     do_something(if let Some(value) = binding.get_ref() { value } else { &0 });
help: consider rewriting the `if` into `match` which preserves the extended lifetime
   |
   |
LL |     do_something({ match Droppy.get_ref()  { Some(value) => { value } _ => { &0 }}});

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0716`.
For more information about this error, try `rustc --explain E0716`.
------------------------------------------


---- [ui] tests/ui/drop/lint-if-let-rescope-gated.rs#without_feature_gate stdout ----

error in revision `without_feature_gate`: test compilation failed although it shouldn't!
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/drop/lint-if-let-rescope-gated.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--cfg" "without_feature_gate" "--check-cfg" "cfg(FALSE,with_feature_gate,without_feature_gate)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/drop/lint-if-let-rescope-gated.without_feature_gate" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/drop/lint-if-let-rescope-gated.without_feature_gate/auxiliary" "--edition=2021"
--- stderr -------------------------------
error: `if let` assigns a shorter lifetime since Edition 2024
##[error]  --> /checkout/tests/ui/drop/lint-if-let-rescope-gated.rs:27:8
   |
   |
LL |     if let Some(_value) = Droppy.get() {
   |                           |
   |                           |
   |                           this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
   = warning: this changes meaning in Rust 2024
   = note: for more information, see issue #124085 <https://github.com/rust-lang/rust/issues/124085>
help: the value is now dropped here in Edition 2024
  --> /checkout/tests/ui/drop/lint-if-let-rescope-gated.rs:31:5
  --> /checkout/tests/ui/drop/lint-if-let-rescope-gated.rs:31:5
   |
LL |     } else {
   |     ^
note: the lint level is defined here
  --> /checkout/tests/ui/drop/lint-if-let-rescope-gated.rs:11:9
   |
LL | #![deny(if_let_rescope)]
help: a `match` with a single arm can preserve the drop order up to Edition 2021
   |
   |
LL ~     match Droppy.get() { Some(_value) => {
LL |         //[with_feature_gate]~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
LL |         //[with_feature_gate]~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
LL |         //[with_feature_gate]~| WARN: this changes meaning in Rust 2024
LL ~     } _ => {
LL |         //[with_feature_gate]~^ HELP: the value is now dropped here in Edition 2024
LL ~     }}

error: aborting due to 1 previous error
------------------------------------------

@bors
Copy link
Contributor

bors commented Sep 17, 2024

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

@dingxiangfei2009
Copy link
Contributor Author

Finally!

@jieyouxu Shall we do one more crater run? Many thanks! 🙇

@jieyouxu
Copy link
Member

@craterbot run start=master#f609b7e0586f81fefb3523e3e17adf779ac416be end=try#ccf408f4326a858c00dd845a64a86b16f360a801 mode=check-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-129466-2 created and queued.
🔍 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2024
@jieyouxu
Copy link
Member

I think that incantation is correct since we hijacked cargo check to do cargo fix --edition

@dingxiangfei2009 dingxiangfei2009 changed the title [CRATER RUN DO NOT MERGE] Let chain lint crater run [CRATER RUN DO NOT MERGE] crater run to evaluate effectiveness of lint for if_let_rescope Sep 25, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-129466-2 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-129466-2 is completed!
📊 3783 regressed and 38667 fixed (514738 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 Sep 28, 2024
@dingxiangfei2009
Copy link
Contributor Author

We narrow down to the 3k crates in the entire ecosystem, most of which are not if let related and pertaining to other migration that is still in development.

The minority of them are false positives from while let which we don't need to lint, and a few triggering a corner case involving if lets surrounded by pairs of round brackets. rustc currently include the brackets into the span, which may or may not be reasonable, but we will handle them nicely in #131035 once it is merged. Thus, I would say we can conclude the crater run experiments and prepare for stabilization.

cc @traviscross

Open questions: what should we do about the brackets included in the span? I believe we should not, but maybe other parts of rustc depends on this contract, ie. rustfmt?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 30, 2024
…ope-lint, r=jieyouxu

Preserve brackets around if-lets and skip while-lets

r? `@jieyouxu`

Tracked by rust-lang#124085

Fresh out of rust-lang#129466, we have discovered 9 crates that the lint did not successfully migrate because the span of `if let` includes the surrounding brackets `(..)` like the following, which surprised me a bit.

```rust
if (if let .. { .. } else { .. }) {
// ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// the span somehow includes the surrounding brackets
}
```

There is one crate that failed the migration because some suggestion spans cross the macro expansion boundaries. Surely there is no way to patch them with `match` rewrite. To handle this case, we will instead require all spans to be tested for admissibility as suggestion spans.

Besides, there are 4 false negative cases discovered with desugared-`while let`. We don't need to lint them, because the `else` branch surely contains exactly one statement because the drop order is not changed whatsoever in this case.

```rust
while let Some(value) = droppy().get() {
..
}
// is desugared into
loop {
    if let Some(value) = droppy().get() {
        ..
    } else {
        break;
        // here can be nothing observable in this block
    }
}
```

I believe this is the one and only false positive that I have found. I think we have finally nailed all the corner cases this time.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2024
Rollup merge of rust-lang#131035 - dingxiangfei2009:tweak-if-let-rescope-lint, r=jieyouxu

Preserve brackets around if-lets and skip while-lets

r? `@jieyouxu`

Tracked by rust-lang#124085

Fresh out of rust-lang#129466, we have discovered 9 crates that the lint did not successfully migrate because the span of `if let` includes the surrounding brackets `(..)` like the following, which surprised me a bit.

```rust
if (if let .. { .. } else { .. }) {
// ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// the span somehow includes the surrounding brackets
}
```

There is one crate that failed the migration because some suggestion spans cross the macro expansion boundaries. Surely there is no way to patch them with `match` rewrite. To handle this case, we will instead require all spans to be tested for admissibility as suggestion spans.

Besides, there are 4 false negative cases discovered with desugared-`while let`. We don't need to lint them, because the `else` branch surely contains exactly one statement because the drop order is not changed whatsoever in this case.

```rust
while let Some(value) = droppy().get() {
..
}
// is desugared into
loop {
    if let Some(value) = droppy().get() {
        ..
    } else {
        break;
        // here can be nothing observable in this block
    }
}
```

I believe this is the one and only false positive that I have found. I think we have finally nailed all the corner cases this time.
@dingxiangfei2009 dingxiangfei2009 deleted the let-chain-lint-crater-run branch October 31, 2024 21:13
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 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants