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

[DO NOT MERGE] Force edition 2024 lint into action on crater #14447

Closed

Conversation

dingxiangfei2009
Copy link

@dingxiangfei2009 dingxiangfei2009 commented Aug 23, 2024

This is to initiate a discussion on enabling crater to apply some lints as a trial while checking crates and make sure that lints marked as machine applicable are not rendering code invalid after the lint application.

Help is needed for this PR. rust-lang/rust#107251 is introducing a new machine applicable lint. The intention of this PR is to force application of if-let-rescope lint during the cargo fix --edition ... invocation. With this commit, it seems that the in the migration pass the lint is still suppressed but the second cargo check did activate the said lint. Here is the log.

Copying to /tmp/fixit
Running `cargo fix --edition`
   Migrating Cargo.toml from 2021 edition to 2024
       Dirty testfixer v0.1.0 (/tmp/fixit): the target configuration changed
    Checking testfixer v0.1.0 (/tmp/fixit)
     Running `/mnt/dev/rust/build/x86_64-unknown-linux-gnu/stage1-tools-bin/cargo /home/xxx/.rustup/toolchains/devrust-stage1/bin/rustc --crate-name testfixer --edition=2021 src/main.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=420 --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 --test --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=225fa3b10800db73 -C extra-filename=-225fa3b10800db73 --out-dir /home/xxx/.cargo-target/debug/deps -C incremental=/home/xxx/.cargo-target/debug/incremental -L dependency=/home/xxx/.cargo-target/debug/deps`
     Running `/mnt/dev/rust/build/x86_64-unknown-linux-gnu/stage1-tools-bin/cargo /home/xxx/.rustup/toolchains/devrust-stage1/bin/rustc --crate-name testfixer --edition=2021 src/main.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=420 --crate-type bin --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=72e3d0a692bea088 -C extra-filename=-72e3d0a692bea088 --out-dir /home/xxx/.cargo-target/debug/deps -C incremental=/home/xxx/.cargo-target/debug/incremental -L dependency=/home/xxx/.cargo-target/debug/deps`
   Migrating src/main.rs from 2021 edition to 2024
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.06s
Running `cargo check` to verify 2024
       Dirty testfixer v0.1.0 (/tmp/fixit): the target configuration changed
    Checking testfixer v0.1.0 (/tmp/fixit)
     Running `/home/xxx/.rustup/toolchains/devrust-stage1/bin/rustc --crate-name testfixer --edition=2024 -Z unstable-options src/main.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=420 --crate-type bin --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=72e3d0a692bea088 -C extra-filename=-72e3d0a692bea088 --out-dir /home/xxx/.cargo-target/debug/deps -C incremental=/home/xxx/.cargo-target/debug/incremental -L dependency=/home/xxx/.cargo-target/debug/deps`
warning: `if let` assigns a shorter lifetime since Edition 2024
  --> src/main.rs:17:8
   |
17 |     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
  --> src/main.rs:18:5
   |
18 |     } else {
   |     ^
   = note: `#[warn(if_let_rescope)]` on by default
help: rewrite this `if let` into a `match` with a single arm to preserve the drop order up to Edition 2021
   |
17 ~     match droppy().get()  { Some(_value) => {
18 ~     } _ => {
19 ~     }}
   |

warning: `testfixer` (bin "testfixer") generated 1 warning (run `cargo fix --bin "testfixer"` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04

So it is really close to working, but preferably the lint should be applied in the Migrating phase already so that the check shows up successful.

@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (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

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-manifest Area: Cargo.toml issues A-rebuild-detection Area: rebuild detection and fingerprinting Command-check Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2024
@weihanglo
Copy link
Member

Thanks for the pull request. Just note that in Cargo a feature discussion usually happens in an issue. We leave pull requests for implementation specific discussions. See https://doc.crates.io/contrib/process/working-on-cargo.html#before-hacking-on-cargo

@traviscross
Copy link

traviscross commented Aug 23, 2024

Just note that in Cargo a feature discussion usually happens in an issue. We leave pull requests for implementation specific discussions.

As a clarification, he's not necessarily asking for a Cargo feature here. He's asking for help on making this branch, which would never be merged, do what he needs it to do for an edition-related crater run for rust-lang/rust#107251.

Hacking up Cargo on a branch in this way is how these edition-related crater runs have been done in the past. See, e.g., rust-lang/rust#125384.

I asked him to file this as a draft "do not merge" PR so there would be a place to hang that discussion of how to implement this properly.

This branch would be for the crater run in:

@weihanglo
Copy link
Member

Thanks for the clarification! Yeah I realised that immediately after posting that comment 😬

The discussion is now happening here
https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Simulate.20Edition.202024.20lints.20in.20edition.20migration

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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 29, 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 1, 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 5, 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.
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 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.
@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-cfg-expr Area: Platform cfg expressions A-cli-help Area: built-in command-line help labels Sep 17, 2024
@rustbot rustbot added A-crate-dependencies Area: [dependencies] of any kind A-documenting-cargo-itself Area: Cargo's documentation A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-interacts-with-crates.io Area: interaction with registries A-registries Area: registries A-semver Area: semver specifications, version matching, etc. A-testing-cargo-itself Area: cargo's tests A-workspaces Area: workspaces Command-add Command-login Command-metadata Command-new Command-owner Command-package Command-publish Command-read-manifest Command-remove Command-rustc Command-search Command-update Command-yank labels Sep 17, 2024
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
Copy link
Contributor

bors commented Nov 25, 2024

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

@ehuss
Copy link
Contributor

ehuss commented Nov 25, 2024

I'm going to close as I do not think this is needed anymore, thanks!

@ehuss ehuss closed this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-crate-dependencies Area: [dependencies] of any kind A-documenting-cargo-itself Area: Cargo's documentation A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues A-rebuild-detection Area: rebuild detection and fingerprinting A-registries Area: registries A-semver Area: semver specifications, version matching, etc. A-testing-cargo-itself Area: cargo's tests A-workspaces Area: workspaces Command-add Command-check Command-fix Command-login Command-metadata Command-new Command-owner Command-package Command-publish Command-read-manifest Command-remove Command-rustc Command-search Command-update Command-yank S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants