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.

nikomatsakis

OK, left some comments, but also a few questions.

compiler/rustc_middle/src/mir/syntax.rs

compiler/rustc_mir_transform/messages.ftl

compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs

compiler/rustc_ty_utils/src/needs_drop.rs

bors · 2024-10-23T16:04:26Z

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

dingxiangfei2009 · 2024-10-30T18:20:05Z

@rustbot ready

The new query is reworked so that we don't run this manual recursion into coroutine. The documentation gives a warning, too, on at which stage the query can be used.
The message is reworked and the drop span is corrected. Specifically, the language is clearer by avoiding use of the indefinitive determiners in the message.

There are merge conflicts but I would prefer to get feedback first for the general framework and outlook of the lint messages.

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 <nikomat@amazon.com>

rustbot · 2024-11-06T09:20:58Z

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

compiler/rustc_lint_defs/src/builtin.rs

celinval

Since the new statement has no semantic, can we just handle it as a Nop in StableMir? Thanks!

celinval · 2024-11-08T01:57:21Z

compiler/rustc_smir/src/rustc_smir/convert/mir.rs

+            mir::StatementKind::BackwardIncompatibleDropHint { place, reason } => {
+                stable_mir::mir::StatementKind::BackwardIncompatibleDropHint {
+                    place: place.stable(tables),
+                    reason: reason.stable(tables),
+                }
+            }


Since this has no semantics, can we translate it to Nop for now?

Suggested change

mir::StatementKind::BackwardIncompatibleDropHint { place, reason } => {

stable_mir::mir::StatementKind::BackwardIncompatibleDropHint {

place: place.stable(tables),

reason: reason.stable(tables),

}

}

mir::StatementKind::BackwardIncompatibleDropHint { .. } => stable_mir::mir::StatementKind::Nop,

celinval · 2024-11-08T01:57:53Z

compiler/rustc_smir/src/rustc_smir/convert/mir.rs

+impl<'tcx> Stable<'tcx> for mir::BackwardIncompatibleDropReason {
+    type T = stable_mir::mir::BackwardIncompatibleDropReason;
+
+    fn stable(&self, _tables: &mut Tables<'_>) -> Self::T {
+        match self {
+            mir::BackwardIncompatibleDropReason::Edition2024 => {
+                stable_mir::mir::BackwardIncompatibleDropReason::Edition2024
+            }
+        }
+    }
+}
+


then we can cleanup all the other code that was added to StableMIR.

Suggested change

impl<'tcx> Stable<'tcx> for mir::BackwardIncompatibleDropReason {

type T = stable_mir::mir::BackwardIncompatibleDropReason;

fn stable(&self, _tables: &mut Tables<'_>) -> Self::T {

match self {

mir::BackwardIncompatibleDropReason::Edition2024 => {

stable_mir::mir::BackwardIncompatibleDropReason::Edition2024

}

}

}

}

celinval · 2024-11-08T01:59:29Z

compiler/stable_mir/src/mir/body.rs

@@ -448,9 +448,15 @@ pub enum StatementKind {
    Coverage(Coverage),
    Intrinsic(NonDivergingIntrinsic),
    ConstEvalCounter,
+    BackwardIncompatibleDropHint { place: Place, reason: BackwardIncompatibleDropReason },


Suggested change

BackwardIncompatibleDropHint { place: Place, reason: BackwardIncompatibleDropReason },

/// This has no semantics. E.g.: BackwardIncompatibleDropHint is translated to Nop.

celinval · 2024-11-08T01:59:39Z

compiler/stable_mir/src/mir/body.rs

+#[derive(Clone, Debug, Eq, PartialEq, Serialize)]
+pub enum BackwardIncompatibleDropReason {
+    Edition2024,
+}
+


Suggested change

#[derive(Clone, Debug, Eq, PartialEq, Serialize)]

pub enum BackwardIncompatibleDropReason {

Edition2024,

}

celinval · 2024-11-08T01:59:53Z

compiler/stable_mir/src/mir/pretty.rs

+        StatementKind::BackwardIncompatibleDropHint { place, reason: _ } => {
+            writeln!(writer, "backwards incompatible drop({place:?});")
+        }


Suggested change

StatementKind::BackwardIncompatibleDropHint { place, reason: _ } => {

writeln!(writer, "backwards incompatible drop({place:?});")

}

celinval · 2024-11-08T02:00:14Z

compiler/stable_mir/src/mir/visit.rs

+            StatementKind::BackwardIncompatibleDropHint { .. }
+            | StatementKind::ConstEvalCounter
+            | StatementKind::Nop => {}


Suggested change

StatementKind::BackwardIncompatibleDropHint { .. }

| StatementKind::ConstEvalCounter

| StatementKind::Nop => {}

StatementKind::ConstEvalCounter | StatementKind::Nop => {}

bors · 2024-11-08T11:09:50Z

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

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

This comment has been minimized.

Sign in to view

dingxiangfei2009 force-pushed the issue-130836-attempt-2 branch from 39b18f4 to e6641cf Compare October 6, 2024 19:13

This comment has been minimized.

Sign in to view

dingxiangfei2009 force-pushed the issue-130836-attempt-2 branch from e6641cf to 0c227e9 Compare October 6, 2024 22:41

This comment has been minimized.

Sign in to view

rustbot added the A-edition-2024 Area: The 2024 edition label Oct 7, 2024

This comment has been minimized.

Sign in to view

RalfJung reviewed Oct 7, 2024

View reviewed changes

compiler/rustc_middle/src/mir/syntax.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

nikomatsakis reviewed Oct 7, 2024

View reviewed changes

dingxiangfei2009 commented Oct 8, 2024

View reviewed changes

compiler/rustc_index/src/bit_set.rs Outdated Show resolved Hide resolved

dingxiangfei2009 force-pushed the issue-130836-attempt-2 branch 2 times, most recently from bb15b7c to 2f5657b Compare October 20, 2024 22:37

nikomatsakis reviewed Oct 21, 2024

View reviewed changes

dingxiangfei2009 force-pushed the issue-130836-attempt-2 branch from 55a1a8b to 4cc5482 Compare October 24, 2024 17:41

dingxiangfei2009 mentioned this pull request Oct 31, 2024

tail_expr_drop_order lint adds too much noise to rustfix #132323

Open

dingxiangfei2009 and others added 11 commits November 6, 2024 04:52

collect drops instead of taking liveness diff

ef77c45

apply some suggestions and add explantory notes

dfc8b11

small fix on the cache

f7baf29

let the query recurse through coroutine

fffd2df

new suggestion format with extracted variable name

f238ed6

fine-tune the drop span and messages

6045458

bugfix on runtime borrows

537de9c

tweak message wording

b2466f8

filter out ecosystem types earlier

bbdfef5

apply suggestions

b66ccf0

dingxiangfei2009 force-pushed the issue-130836-attempt-2 branch from 509f9d0 to b66ccf0 Compare November 5, 2024 21:55

This comment has been minimized.

Sign in to view

clippy

180bbd8

check lint level at session level

56fea8d

dingxiangfei2009 force-pushed the issue-130836-attempt-2 branch from f6b0503 to 56fea8d Compare November 6, 2024 09:36

dingxiangfei2009 commented Nov 6, 2024

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Show resolved Hide resolved

traviscross added the F-shorter_tail_lifetimes `#![feature(shorter_tail_lifetimes)]` label Nov 7, 2024

further restrict applicability of the lint

04216e4

ehuss mentioned this pull request Nov 8, 2024

Crater 2024 edition #132712

Open

celinval reviewed Nov 8, 2024

View reviewed changes

	BackwardIncompatibleDropHint { place: Place, reason: BackwardIncompatibleDropReason },
	/// This has no semantics. E.g.: BackwardIncompatibleDropHint is translated to Nop.

	StatementKind::BackwardIncompatibleDropHint { place, reason: _ } => {
	writeln!(writer, "backwards incompatible drop({place:?});")
	}

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

Are you sure you want to change the base?

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

Conversation

dingxiangfei2009 commented Oct 6, 2024 • edited Loading

rustbot commented Oct 6, 2024

dingxiangfei2009 commented Oct 6, 2024

About the futures

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dingxiangfei2009 commented Oct 7, 2024

This comment has been minimized.

dingxiangfei2009 commented Oct 7, 2024

bors commented Oct 7, 2024

bors commented Oct 7, 2024

dingxiangfei2009 commented Oct 7, 2024

bors commented Oct 7, 2024

This comment has been minimized.

bors commented Oct 7, 2024

dingxiangfei2009 commented Oct 7, 2024 • edited Loading

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxiangfei2009 commented Oct 8, 2024

dingxiangfei2009 commented Oct 8, 2024

nikomatsakis left a comment

Choose a reason for hiding this comment

bors commented Oct 23, 2024

dingxiangfei2009 commented Oct 30, 2024 • edited Loading

This comment has been minimized.

rustbot commented Nov 6, 2024

celinval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 8, 2024

dingxiangfei2009 commented Oct 6, 2024 •

edited

Loading

dingxiangfei2009 commented Oct 7, 2024 •

edited

Loading

dingxiangfei2009 commented Oct 30, 2024 •

edited

Loading