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] crater run on the tail expression drop order lint #129604

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Aug 26, 2024

This PR is intended for a crater run for the lint tail-expr-drop-order.

This need another patch on src/tools/cargo which is still in progress.

@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2024

r? @fmease

rustbot has assigned @fmease.
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 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 26, 2024
@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:20d3b4d4a2629cbf7865cdbf92fe47512a7c96658c24253a045ff38e8075cd7fb37ca6fcadfa6e6d093333943ad24f6fc4f163ec5b74fd940de9d5bb03eb4d3b:
------
##[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.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.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
---- [ui] tests/ui/drop/lint-tail-expr-drop-order-gated.rs#neither stdout ----

error in revision `neither`: 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-tail-expr-drop-order-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" "neither" "--check-cfg" "cfg(FALSE,neither,no_feature_gate,edition_less_than_2024)" "--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-tail-expr-drop-order-gated.neither" "-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-tail-expr-drop-order-gated.neither/auxiliary" "--edition=2021"
--- stderr -------------------------------
--- stderr -------------------------------
error: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021
   |
LL |     let x = LoudDropper;
LL |     let x = LoudDropper;
   |         - these values have significant drop implementation and will observe changes in drop order under Edition 2024
LL |     x.get() + LoudDropper.get()
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see issue #123739 <https://github.com/rust-lang/rust/issues/123739>
note: the lint level is defined here
---


---- [ui] tests/ui/drop/lint-tail-expr-drop-order-gated.rs#edition_less_than_2024 stdout ----

error in revision `edition_less_than_2024`: 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-tail-expr-drop-order-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" "edition_less_than_2024" "--check-cfg" "cfg(FALSE,neither,no_feature_gate,edition_less_than_2024)" "--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-tail-expr-drop-order-gated.edition_less_than_2024" "-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-tail-expr-drop-order-gated.edition_less_than_2024/auxiliary" "--edition=2021"
--- stderr -------------------------------
--- stderr -------------------------------
error: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021
   |
LL |     let x = LoudDropper;
LL |     let x = LoudDropper;
   |         - these values have significant drop implementation and will observe changes in drop order under Edition 2024
LL |     x.get() + LoudDropper.get()
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see issue #123739 <https://github.com/rust-lang/rust/issues/123739>
note: the lint level is defined here
---
---- [ui] tests/ui/drop/lint-tail-expr-drop-order-gated.rs#no_feature_gate stdout ----

error in revision `no_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-tail-expr-drop-order-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" "no_feature_gate" "--check-cfg" "cfg(FALSE,neither,no_feature_gate,edition_less_than_2024)" "--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-tail-expr-drop-order-gated.no_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-tail-expr-drop-order-gated.no_feature_gate/auxiliary" "-Z" "unstable-options" "--edition=2024"
--- stderr -------------------------------
--- stderr -------------------------------
error: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021
   |
LL |     let x = LoudDropper;
LL |     let x = LoudDropper;
   |         - these values have significant drop implementation and will observe changes in drop order under Edition 2024
LL |     x.get() + LoudDropper.get()
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see issue #123739 <https://github.com/rust-lang/rust/issues/123739>
note: the lint level is defined here

@jieyouxu
Copy link
Member

r? jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned fmease Aug 26, 2024
@jieyouxu
Copy link
Member

The tests may have to be temporarily ignored since you're changing lint's behavior.

@dingxiangfei2009
Copy link
Contributor Author

@jieyouxu Let's put this on ice briefly. @traviscross and I have come up with a plan to enable both the lint and the feature gate over in #129607. This will help us narrow down to a smaller set of crates to test for the lint and/or the feature separately.

@jieyouxu
Copy link
Member

That seems fine with me. Feel free to ping me if you are happy with a set of changes that you would like a crater run on.

@dingxiangfei2009
Copy link
Contributor Author

dingxiangfei2009 commented Aug 27, 2024

@jieyouxu @traviscross I need your advice here. Given that simulating the shorter tail expression temporary lifetime on Edition 2021 will stop rustc from bootstrapping itself beyond stage1, shall we move ahead and test the lint first? Just to get a feel of its verbosity and accuracy.

If this makes sense, I would propose to do checking only with +rustflags=-Dtail-expr-drop-order. We don't need to patch cargo in this PR since we are only running cargo check.

WDYT?

@traviscross
Copy link
Contributor

Sounds OK . You might want to see if this can be included in the crater rollup which is scheduled to run next:

@compiler-errors
Copy link
Member

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
…-order-crater-run, r=<try>

[DO NOT MERGE] crater run on the tail expression drop order lint

This PR is intended for a crater run *for the lint tail-expr-drop-order*.

This need another patch on `src/tools/cargo` which is still in progress.
@bors
Copy link
Contributor

bors commented Aug 27, 2024

⌛ Trying commit e694863 with merge 4d98531...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2024
[CRATER] Crater Rollup

This is a " crater rollup" of:
* rust-lang#126452
* rust-lang#128784
* rust-lang#129392
* rust-lang#129422
* rust-lang#129543
* rust-lang#129604

**What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build.

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2024
[CRATER] Crater Rollup

This is a " crater rollup" of:
* rust-lang#126452
* rust-lang#128784
* rust-lang#129392
* rust-lang#129422
* rust-lang#129543
* rust-lang#129604

**What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build.

r? `@ghost`
@bors
Copy link
Contributor

bors commented Aug 28, 2024

☀️ Try build successful - checks-actions
Build commit: 4d98531 (4d98531622e1718ae0bc3c7a1ab9cd8938428452)

@craterbot
Copy link
Collaborator

🚨 Error: experiment 'pr-129604' not found

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@compiler-errors
Copy link
Member

oops! crater never was started on this one anyways.

@craterbot mode=check-only start=master#ab869e094a907cc5d19b4080f22eccaf347f1f95 end=try#4d98531622e1718ae0bc3c7a1ab9cd8938428452+rustflags=-Dtail_expr_drop_order crates=https://gist.githubusercontent.com/compiler-errors/4a09d64cd15dc3dca50edeea26cc9938/raw/b4181c225709e120a11d91cce69d0d4da3e652d0/regressed.txt p=1

@craterbot
Copy link
Collaborator

🚨 Error: experiment 'pr-129604' not found

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@compiler-errors
Copy link
Member

@craterbot name=pr-129604-1 mode=check-only start=master#ab869e094a907cc5d19b4080f22eccaf347f1f95 end=try#4d98531622e1718ae0bc3c7a1ab9cd8938428452+rustflags=-Dtail_expr_drop_order crates=https://gist.githubusercontent.com/compiler-errors/4a09d64cd15dc3dca50edeea26cc9938/raw/b4181c225709e120a11d91cce69d0d4da3e652d0/regressed.txt p=1

@craterbot
Copy link
Collaborator

🚨 Error: experiment 'pr-129604-1' not found

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@compiler-errors
Copy link
Member

oops i needed a run

@craterbot run mode=check-only start=master#ab869e094a907cc5d19b4080f22eccaf347f1f95 end=try#4d98531622e1718ae0bc3c7a1ab9cd8938428452+rustflags=-Dtail_expr_drop_order crates=https://gist.githubusercontent.com/compiler-errors/4a09d64cd15dc3dca50edeea26cc9938/raw/b4181c225709e120a11d91cce69d0d4da3e652d0/regressed.txt p=1

@craterbot
Copy link
Collaborator

👌 Experiment pr-129604 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2024
@dingxiangfei2009
Copy link
Contributor Author

dingxiangfei2009 commented Aug 31, 2024

@jieyouxu @traviscross The lint seems to be working fine from the report, but as I have feared it has a lot of false positives.

For instance, the lint fires when a value with significant drop is moved/consumed at a tail expression, even though in fact it will not be dropped.

struct Droppy;
impl Droppy {
    fn consume(self) -> u8 {
        0 // dropping `self` here
    }
}
impl Drop for Droppy { .. }
fn droppy_is_consumed() -> u8 {
    let droppy = Droppy;
    droppy.consume()
}

There is a solution, which is to use rustc_hir_typeck::expr_use_visitor::ExprUseVisitor to mark nodes that are consumed to be ignored. The problem is, rust_hir_typeck crate already depends on rustc_lint. 😓

The question now is, should we leave the lint as Allow as it is now, or should we continue to polish it? If we are to polish it, for which I would recommend ExprUseVisitor aka. EUV, can we build a case for moving it into rustc_middle or somewhere else?

EDIT: I checked the ExprUseVisitor out. It depends on trait selection and FnCtxt which makes it a bit of challenge to move it.

@dingxiangfei2009
Copy link
Contributor Author

Another solution is to take a subset of visitor rules from ExprUseVisitor, tracking only a limit set of cases where consume happens. Also to avoid future divergence from the match ergonomics work, I will not consider consume cases in the match arms. This would allow us to prune away sufficient false positives. What would y'all think about this?

@jieyouxu
Copy link
Member

jieyouxu commented Sep 1, 2024

@dingxiangfei2009 I'm not super sure about this in terms of what the correct approach is. I would suggest that we discuss this in a new #t-compiler/help zulip thread.

@craterbot
Copy link
Collaborator

🚧 Experiment pr-129604 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-129604 is completed!
📊 70450 regressed and 0 fixed (98236 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 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants