Reduce false positives of tail-expr-drop-order from consumed values by dingxiangfei2009 · Pull Request #129864 · rust-lang/rust

dingxiangfei2009 · 2024-09-01T18:00:07Z

To reduce false positives, the lint does not fire if the locals are consumed/moved, or the values with significant drop are consumed/moved at the tail expression location.

For this, we rely on solving a small dataflow to determine whether a Local is either live, or moved/dropped.

I am also printing the type involved for easier diagnosis.

dingxiangfei2009 · 2024-09-01T18:31:25Z

I also found that std::io::Error is considered to have significant drop. How weird.

compiler-errors · 2024-09-02T04:17:25Z

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

+    type Error = !;
+
+    fn typeck_results(&self) -> Self::TypeckResults<'_> {
+        self.0.typeck(self.1)


This is going to be extremely expensive for the query, for the record. I would probably make some helper that caches the typeck results after the first call, or better yet just load the typeck results yourself and pass them here.

I should drop this code, since we are not using this anymore.

compiler-errors · 2024-09-02T04:18:36Z

compiler/rustc_hir_typeck/src/lint.rs

+        match place_with_id.place.base {
+            PlaceBase::Rvalue => {
+                self.nodes.insert(place_with_id.hir_id);
+            }
+            PlaceBase::Local(id) => {
+                self.nodes.insert(id);
+            }
+            PlaceBase::Upvar(upvar) => {
+                self.nodes.insert(upvar.var_path.hir_id);
+            }


Doesn't this count partial moves as full moves? Isn't that still not correct for drop ordering?

Also, doesn't this not respect different paths correctly? What about consuming operations that happen on only one branch of a conditional path? I don't know if this sort of analysis is supported with ExprUseVisitor, which has no "flow" state.

For now I don't have a good strategy towards that. How do we track partial moves and drops currently? Can we invoke machinery from borrow checker or somewhere else for such information?

I have switched to treat partial moves not as full moves. It could still get noisy but still safe.

compiler-errors · 2024-09-02T04:20:58Z

compiler/rustc_hir_typeck/src/lint.rs

+    }
+}
+
+fn extract_tail_expr_consuming_nodes<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> &'tcx HirIdSet {


Make this into a hook, not a query; I don't believe it needs any sort of caching, since that has overhead. This should also explain very clearly what it does.

jieyouxu · 2024-09-02T09:56:56Z

I opened a zulip thread at https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/More.20precise.20lint.20impl.20for.20Edition.202024.20tail-expr-drop-order to discuss this lint, because as @compiler-errors mentioned I feel like this might almost need to be some kind of flow analysis.

compiler-errors · 2024-09-10T18:27:45Z

@rustbot author

bors · 2024-09-14T19:31:51Z

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

rustbot · 2024-09-22T20:12:34Z

This PR changes MIR

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

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

RalfJung · 2024-09-22T20:20:11Z

compiler/rustc_middle/src/mir/syntax.rs

+        place: Place<'tcx>,
+        target: BasicBlock,
+        unwind: UnwindAction,
+        scope: Option<(DefId, ItemLocalId)>,


Please also add a comment explaining the meaning of this new field.

Could you make this a ClearCrossCrate? Then it can just be a HirId.

dingxiangfei2009 · 2024-09-22T20:46:24Z

@rustbot ready

Reimplement the lint with flow information.
- MaybeLiveLocal is eventually not used. It treats a move out of a whole local as a Use site and an "obligatory" drop on the same local later in the control flow as a Use site as well, even though the local is technically not live at all after the move and the later drop is treated as no-op.
We are still not exactly very precise here. I don't have a clue for treatment of partial moves. Help needed. 🙇

dingxiangfei2009 · 2024-09-22T21:37:01Z

I am looking at MaybeInitializedPlaces and MaybeUninitializedPlaces. They might be useful for identifying the partial moves and move paths.

bors · 2024-09-23T05:16:42Z

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

dingxiangfei2009 · 2024-09-23T21:40:37Z

cc @jieyouxu

I also gave a hard thought about upvars and whether we should lint against upvars. I have arrived at the conclusion that we should not lint against the case where upvars are dropped later than the tail expression temporaries.

What I mean is the following example, where f does not move out self.

fn should_we_lint_or_not() -> impl FnOnce() -> (?, ?) {
    let x = LoudDropper;
    move || (x.f(), LoudDropper.f())
}

Upvar x is "embedded" into the closure move || .. and, sure enough, is obliged to live as long as the closure. Upon completing evaluation of (x.f(), ..) the LoudDropper is indeed dropped first. This is a contract that we have been honoring. I think now that it was a mistake to lint against this case because the supposed transpose in drop order did not happen. This is also confirmed by the MIR output of the closure body if we check this out in the playground.

I am going to flip the test on this to not linting against.

…ves-from-consumed-droppers, r=<try> Reduce false positives of tail-expr-drop-order from consumed values r? `@jieyouxu` To reduce false positives, the lint does not fire if the locals are consumed/moved, or the values with significant drop are consumed/moved at the tail expression location. For this, we rely on solving a small dataflow to determine whether a `Local` is either live, or moved/dropped. I am also printing the type involved for easier diagnosis.

bors · 2024-09-25T15:30:52Z

⌛ Trying commit 2457860 with merge 7014e13...

GrigorenkoPV · 2024-09-25T16:32:21Z

compiler/rustc_mir_transform/Cargo.toml

 rustc_hir = { path = "../rustc_hir" }
 rustc_index = { path = "../rustc_index" }
 rustc_infer = { path = "../rustc_infer" }
+rustc_lint = { path = "../rustc_lint" }


Wouldn't rustc_lint_defs suffice?

bors · 2024-09-25T17:15:13Z

☀️ Try build successful - checks-actions
Build commit: 7014e13 (7014e13d5becc920d4bea3cd87942c8a13d359bf)

compiler-errors · 2024-09-25T17:19:32Z

@craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-129604/retry-regressed-list.txt start=master#38352b01ae4af9300be03b805d6db68c45e51068 end=try#7014e13d5becc920d4bea3cd87942c8a13d359bf+rustflags=-Dtail_expr_drop_order

craterbot · 2024-09-25T17:19:37Z

🚨 Error: missing desired crates: {"dym", "maxplus", "vkopt-message-parser", "lucia", "rustoku_gui", "flp-tsl", "awsl-pest", "roast-2d", "cl-traits", "cleu-orm-derive", "future-iter", "ndsparse", "genai-custom", "restruct"}

🆘 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 · 2024-09-25T17:22:26Z

@craterbot run mode=check-only p=1 crates=https://gist.githubusercontent.com/compiler-errors/cc84423a4a9fbc5eb598383fe2555467/raw/a11fe3433fefddaab0038ee75cd2e0ab8852748a/crates start=master#38352b01ae4af9300be03b805d6db68c45e51068 end=try#7014e13d5becc920d4bea3cd87942c8a13d359bf+rustflags=-Dtail_expr_drop_order

craterbot · 2024-09-25T17:22:31Z

👌 Experiment pr-129864 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 · 2024-09-26T08:10:01Z

🚧 Experiment pr-129864 is now running

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

craterbot · 2024-09-26T11:17:32Z

🎉 Experiment pr-129864 is completed!
📊 13314 regressed and 0 fixed (70615 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

…er, r=<try> [crater] validate impact of marking more types as `#[rustc_insignificant_dtor]` on tail expr drop order lint Validate impact of rust-lang#130914 on rust-lang#129864 and the tail expr drop order lint. r? `@ghost`

…, r=Amanieu Mark some more types as having insignificant dtor These were caught by rust-lang#129864 (comment), which is implementing a lint for some changes in drop order for temporaries in tail expressions. Specifically, the destructors of `CString` and the bitpacked repr for `std::io::Error` are insignificant insofar as they don't have side-effects on things like locking or synchronization; they just free memory. See some discussion on rust-lang#89144 for what makes a drop impl "significant"

Rollup merge of rust-lang#130914 - compiler-errors:insignificant-dtor, r=Amanieu Mark some more types as having insignificant dtor These were caught by rust-lang#129864 (comment), which is implementing a lint for some changes in drop order for temporaries in tail expressions. Specifically, the destructors of `CString` and the bitpacked repr for `std::io::Error` are insignificant insofar as they don't have side-effects on things like locking or synchronization; they just free memory. See some discussion on rust-lang#89144 for what makes a drop impl "significant"

…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.

…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.

…t-2, r=nikomatsakis 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.

dingxiangfei2009 · 2024-11-29T11:27:38Z

I am closing this since the second attempt achieve greater success.

A PoC for detecting future borrow checking error is working. What remains is to work out a good error message and suggestion. I am working on it. 🚧

rustbot assigned jieyouxu Sep 1, 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 Sep 1, 2024

dingxiangfei2009 force-pushed the reduce-false-positives-from-consumed-droppers branch 2 times, most recently from 491fb13 to de2d256 Compare September 1, 2024 18:29

compiler-errors suggested changes Sep 2, 2024

View reviewed changes

compiler-errors self-assigned this Sep 2, 2024

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2024

dingxiangfei2009 force-pushed the reduce-false-positives-from-consumed-droppers branch from de2d256 to 08ec439 Compare September 22, 2024 20:12

RalfJung reviewed Sep 22, 2024

View reviewed changes

dingxiangfei2009 force-pushed the reduce-false-positives-from-consumed-droppers branch from 08ec439 to e71ad9f Compare September 22, 2024 20:31

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 22, 2024

This comment has been minimized.

Sign in to view

dingxiangfei2009 added 6 commits September 24, 2024 07:07

reduce false positives of tail-expr-drop-order from consumed values

081c77f

add explanation for the new scope info on drop terminators

ed12b18

handle synthetic defs

eab51ce

switch to MaybeInitializedPlaces

9da076b

revert gate test to work only when migration is triggered

acad84c

fmt

f131d7d

GrigorenkoPV reviewed Sep 25, 2024

View reviewed changes

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 Sep 25, 2024

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 26, 2024

This was referenced Sep 27, 2024

Mark some more types as having insignificant dtor #130914

Merged

[crater] validate impact of marking more types as #[rustc_insignificant_dtor] on tail expr drop order lint #130930

Closed

dingxiangfei2009 mentioned this pull request Oct 6, 2024

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

Merged

dingxiangfei2009 mentioned this pull request Oct 9, 2024

Stabilize shorter_tail_lifetime #131445

Closed

compiler-errors assigned nikomatsakis and unassigned compiler-errors Nov 4, 2024

dingxiangfei2009 closed this Nov 29, 2024

Uh oh!

Conversation

dingxiangfei2009 commented Sep 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dingxiangfei2009 commented Sep 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Sep 2, 2024

Uh oh!

compiler-errors commented Sep 10, 2024

Uh oh!

bors commented Sep 14, 2024

Uh oh!

rustbot commented Sep 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingxiangfei2009 commented Sep 22, 2024

Uh oh!

dingxiangfei2009 commented Sep 22, 2024

Uh oh!

This comment has been minimized.

bors commented Sep 23, 2024

Uh oh!

dingxiangfei2009 commented Sep 23, 2024

Uh oh!

bors commented Sep 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Sep 25, 2024

Uh oh!

compiler-errors commented Sep 25, 2024

Uh oh!

craterbot commented Sep 25, 2024

Uh oh!

compiler-errors commented Sep 25, 2024

Uh oh!

craterbot commented Sep 25, 2024

Uh oh!

craterbot commented Sep 26, 2024

Uh oh!

craterbot commented Sep 26, 2024

Uh oh!

dingxiangfei2009 commented Nov 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

dingxiangfei2009 commented Sep 1, 2024 •

edited

Loading