Reduce false positives of tail-expr-drop-order from consumed values (attempt #2) #131326

dingxiangfei2009 · 2024-10-06T12:42:04Z

Tracked by #123739.

Related to #129864 but not replacing, yet.

Related to #130836.

This is an implementation of the approach suggested in the Zulip stream. A new MIR statement BackwardsIncompatibleDrop is added to the MIR syntax. The lint now works by inspecting possibly live move paths before at the BackwardsIncompatibleDrop location and the actual drop under the current edition, which should be one before Edition 2024 in practice.

rustbot · 2024-10-06T12:42:13Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred in match lowering

cc @Nadrieril

dingxiangfei2009 · 2024-10-06T13:03:30Z

I still have two issues.

I am not sure if using tcx.ty_string_with_limit is the way to go, because it doesn't print fully qualified path names for ADTs yet. I still need to find out how one can configure PrettyPrinter::pretty_print_type for this.
I found it hard to reason about lints when futures are involved.

About the futures

Take the crate sqlx-macros-core @ 0.8.1 as an example. In src/database/mod.rs:57:71 the future awaited involves generics B, C and E, T on separate occasions. I suppose it would be much more helpful if we can refer to the code where those types are involved.

error: this value has significant drop implementation that will have a different drop order from that of Edition 2021, whose type `Pin<Box<dyn Future<Output = Result<<DB as Database>::Connection, Error>> + Send>>` drops `Ptr` while dropping
  --> src/database/mod.rs:57:71
   |
57 |                     miss.insert(DB::Connection::connect(database_url).await?)
   |                                 --------------------------------------^^^^^- this local binding may observe changes in drop order under Edition 2024, whose type `ControlFlow<std::result::Result<Infallible, sqlx_core::Error>, <DB as sqlx_core::database::Database>::Connection>` drops `B, C` while dropping
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see issue #123739 <https://github.com/rust-lang/rust/issues/123739>
   = note: requested on the command line with `-D tail-expr-drop-order`

error: this value has significant drop implementation that will have a different drop order from that of Edition 2021, whose type `Pin<Box<dyn Future<Output = Result<<DB as Database>::Connection, Error>> + Send>>` drops `Ptr` while dropping
  --> src/database/mod.rs:57:71
   |
57 |                     miss.insert(DB::Connection::connect(database_url).await?)
   |                                 --------------------------------------^^^^^- this local binding may observe changes in drop order under Edition 2024, whose type `std::result::Result<Infallible, sqlx_core::Error>` drops `E, T` while dropping
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see issue #123739 <https://github.com/rust-lang/rust/issues/123739>

dingxiangfei2009 · 2024-10-07T12:11:30Z

@rustbot labels +A-edition-2024

dingxiangfei2009 · 2024-10-07T12:54:45Z

@bors try

bors · 2024-10-07T12:55:56Z

⌛ Trying commit 8f09388 with merge 1c6d72d...

…t-2, r=<try> Reduce false positives of tail-expr-drop-order from consumed values (attempt #2) r? `@nikomatsakis` Related to rust-lang#129864 but not replacing, yet. Related to rust-lang#130836. This is an implementation of the approach suggested in the [Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/temporary.20drop.20order.20changes). A new MIR statement `BackwardsIncompatibleDrop` is added to the MIR syntax. The lint now works by inspecting possibly live move paths before at the `BackwardsIncompatibleDrop` location and the actual drop under the current edition, which should be one before Edition 2024 in practice.

bors · 2024-10-07T14:40:51Z

☀️ Try build successful - checks-actions
Build commit: 1c6d72d (1c6d72d356cc2668e49375eb9391c0b95cb18594)

dingxiangfei2009 · 2024-10-07T16:12:40Z

@bors try

…t-2, r=<try> Reduce false positives of tail-expr-drop-order from consumed values (attempt #2) r? `@nikomatsakis` Related to rust-lang#129864 but not replacing, yet. Related to rust-lang#130836. This is an implementation of the approach suggested in the [Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/temporary.20drop.20order.20changes). A new MIR statement `BackwardsIncompatibleDrop` is added to the MIR syntax. The lint now works by inspecting possibly live move paths before at the `BackwardsIncompatibleDrop` location and the actual drop under the current edition, which should be one before Edition 2024 in practice.

bors · 2024-10-07T16:13:53Z

⌛ Trying commit 80c5c8f with merge fa33670...

compiler/rustc_middle/src/mir/syntax.rs

bors · 2024-10-07T18:01:58Z

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

dingxiangfei2009 · 2024-10-07T18:23:03Z

@compiler-errors Shall we give it a crater run with run mode=check-only p=1 crates=https://gist.githubusercontent.com/compiler-errors/cc84423a4a9fbc5eb598383fe2555467/raw/a11fe3433fefddaab0038ee75cd2e0ab8852748a/crates start=master#0c227e91aa956d20861deb2bb25083d8a4d094e1 end=try#fa3367093b28d0fb18394b7fd734d540fcd30c47+rustflags=-Dtail_expr_drop_order? 🙇

nikomatsakis

Overall this seems much better than before! Minimally invasive, which is good.

nikomatsakis · 2024-10-07T19:18:48Z

compiler/rustc_hir_analysis/src/check/region.rs

            {
-                visitor.terminating_scopes.insert(tail_expr.hir_id.local_id);
+                // Note: we are unconditionally adding this information so that we can run


The word "unconditionally" is confusing here, since clearly this insert call is conditioned on several things (edition, lint level, etc). I think I know what you mean by it, perhaps rewrite to

// If this temporary scope will be changing once the codebase adopts Rust 2024, // and we are linting about possible semantic changes that would result, // then record this node-id in the field `backwards_incompatible_scope` // for future reference.

Applied. The original comment was added when I thought it was right to insert the linting info under all circumstances. Given that we only want to lint when edition migration is run, I changed the condition when the linting info is inserted.

compiler/rustc_middle/src/mir/syntax.rs

nikomatsakis · 2024-10-07T19:21:29Z

compiler/rustc_middle/src/thir.rs

+    /// Lifetime for temporaries as expected.
+    /// This should be `None` in a constant context.
+    pub temp_lifetime: Option<region::Scope>,
+    /// Backward-incompatible lifetime for future editions


I would say:

/// If `Some(lt)`, indicates that the lifetime of this temporary will change to `lt` in a future edition. /// If `None`, then no changes are expected, or lints are disabled.

compiler/rustc_middle/src/ty/rvalue_scopes.rs

dingxiangfei2009 · 2024-10-08T08:22:57Z

cc @traviscross for visibility

compiler/rustc_index/src/bit_set.rs

dingxiangfei2009 · 2024-10-08T17:32:17Z

@bors try

…t-2, r=<try> Reduce false positives of tail-expr-drop-order from consumed values (attempt #2) r? `@nikomatsakis` Tracked by rust-lang#123739. Related to rust-lang#129864 but not replacing, yet. Related to rust-lang#130836. This is an implementation of the approach suggested in the [Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/temporary.20drop.20order.20changes). A new MIR statement `BackwardsIncompatibleDrop` is added to the MIR syntax. The lint now works by inspecting possibly live move paths before at the `BackwardsIncompatibleDrop` location and the actual drop under the current edition, which should be one before Edition 2024 in practice.

bors · 2024-11-14T17:44:13Z

📌 Commit bfb48b7 has been approved by nikomatsakis

It is now in the queue for this repository.

ehuss · 2024-11-15T01:42:50Z

@bors r-

Looks like there are merge conflicts.

take 2 open up coroutines tweak the wordings the lint works up until 2021 We were missing one case, for ADTs, which was causing `Result` to yield incorrect results. only include field spans with significant types deduplicate and eliminate field spans switch to emit spans to impl Drops Co-authored-by: Niko Matsakis <[email protected]> collect drops instead of taking liveness diff apply some suggestions and add explantory notes small fix on the cache let the query recurse through coroutine new suggestion format with extracted variable name fine-tune the drop span and messages bugfix on runtime borrows tweak message wording filter out ecosystem types earlier apply suggestions clippy check lint level at session level further restrict applicability of the lint translate bid into nop for stable mir detect cycle in type structure

dingxiangfei2009 · 2024-11-20T12:54:32Z

@rustbot ready

Updates:

Switching to TypingEnv with correct typing mode designation at different stages

traviscross · 2024-11-20T15:19:22Z

Before this starts attracting new conflicts...

@bors r=nikomatsakis p=1

bors · 2024-11-20T15:19:25Z

📌 Commit 297b618 has been approved by nikomatsakis

It is now in the queue for this repository.

matthiaskrgr · 2024-11-20T18:14:39Z

@bors p=10

so this is put in front of my rollup

bors · 2024-11-20T18:51:57Z

⌛ Testing commit 297b618 with merge 3fee0f1...

bors · 2024-11-20T21:57:30Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 3fee0f1 to master...

rust-timer · 2024-11-20T23:15:04Z

Finished benchmarking commit (3fee0f1): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.5%	[0.2%, 2.3%]	15
Improvements ✅ (primary)	-0.7%	[-2.2%, -0.2%]	19
Improvements ✅ (secondary)	-0.6%	[-1.0%, -0.3%]	8
All ❌✅ (primary)	-0.7%	[-2.2%, 0.2%]	20

Max RSS (memory usage)

Results (primary -1.6%, secondary 1.6%)

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)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	2.5%	[1.1%, 6.2%]	24
Improvements ✅ (primary)	-2.2%	[-3.5%, -1.5%]	7
Improvements ✅ (secondary)	-1.9%	[-2.8%, -1.1%]	6
All ❌✅ (primary)	-1.6%	[-3.5%, 3.1%]	8

Cycles

Results (primary -2.4%, secondary 5.0%)

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)	5.0%	[5.0%, 5.0%]	1
Improvements ✅ (primary)	-2.4%	[-2.6%, -2.3%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.4%	[-2.6%, -2.3%]	4

Binary size

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

Bootstrap: 794.285s -> 797.74s (0.43%)
Artifact size: 336.01 MiB -> 335.91 MiB (-0.03%)

nnethercote · 2024-11-24T19:45:45Z

@dingxiangfei2009: I just stumbled across this PR because it caused a conflict with one of my changes. (Which is fine; the conflict was easy to fix.) It looks like you originally had a lot of separate commits and the squashed them together? As a result, the commit log message is incomprehensible. Next time, could you rewrite the commit log so it's more readable to someone not familiar with the PR? Thanks.

dingxiangfei2009 · 2024-11-24T19:48:42Z

Sorry! I will take care of the commit message in squashing. Most of them are just describing back-and-forth changes and they indeed need some sorting.

Kobzol · 2024-11-26T08:37:32Z

This was actually a small perf. win on primary benchmarks, and the largest secondary regression was just noise. Marking as triaged.

@rustbot label: +perf-regression-triaged

Fix required due to the following changes to Rust's internal API: - rust-lang/rust#132460 - rust-lang/rust#133212 - rust-lang/rust#131326 Resolves #3731 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses. --------- Co-authored-by: Zyad Hassan <[email protected]>

rustbot assigned nikomatsakis Oct 6, 2024

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 Oct 6, 2024

dingxiangfei2009 force-pushed the issue-130836-attempt-2 branch from 31f4a7f to 39b18f4 Compare October 6, 2024 12:49