Mark more locals as moved to avoid building drops for them. by cjgillot · Pull Request #158281 · rust-lang/rust

cjgillot · 2026-06-23T02:51:13Z

MIR building skips generating drops for moved-from locals in the topmost scope. This was only used for call terminators, but can be generalized to many other moves. This PR generalizes this to aggregate construction and many other assignments.

This avoids generating drops that would then be removed by drop elaboration.

This PR changes borrowck behaviour: some programs that were rejected are now accepted, see the last commit. That particular case was wrongly rejected.

Fixes #156713

~~Based on #158279 to remove a lint false-positive~~

rustbot · 2026-06-23T02:51:17Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustbot · 2026-06-23T02:51:19Z

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 73 candidates
Random selection from 21 candidates

theemathas · 2026-06-23T03:52:09Z

Does this have user-visible behavior changes? e.g., see #156713

cjgillot · 2026-06-23T22:56:19Z

Yes. With this change, both cases in #156713 pass. We also have a few user-visible consequences with the drop order lint.

JohnTitor · 2026-06-27T01:23:05Z

@rustbot reroll

cjgillot · 2026-07-09T00:03:04Z

@bors try @rust-timer queue

jackh726 · 2026-08-07T23:58:51Z

I'm not personally very familiar with when we are allowed to avoid building drops (specifically, which are these truly count as moves from an opsem perspective).

cc both @rust-lang/opsem and @rust-lang/types

I would be far more comfortable if there was a small and specific test demonstrating the difference from marking each individual site that we record as moved.

RalfJung · 2026-08-08T09:08:40Z

I'm afraid MIR building is mostly outside my wheelhouse.
But Cc @Amanieu since you said "move".

RalfJung · 2026-08-08T09:11:34Z

Is there somewhere a high-level description of the algorithm implemented here and how it defines "move"?
It seems like this first builds MIR and then looks at the just-built MIR to determine what the "moves" are?

cjgillot · 2026-08-08T16:47:19Z

Is there somewhere a high-level description of the algorithm implemented here and how it defines "move"?

At high level, the doc-comment on record_operand_moved is quite good.
There is also the definition by @nikomatsakis here: rust-lang/compiler-team#558

It seems like this first builds MIR and then looks at the just-built MIR to determine what the "moves" are?

This is on the "built" MIR phase. In short, a "move" is an occurrence of Operand::Move, which deinitializes the moved-from place. In that MIR dialect, drop actually means drop-if-initialized, so drops of moved-from locals is a no-op, and can be elided. (See the section on "drops" here: https://doc.rust-lang.org/beta/nightly-rustc/rustc_middle/mir/enum.MirPhase.html)

rustbot · 2026-08-08T17:10:25Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

cjgillot · 2026-08-08T17:13:11Z

I would be far more comfortable if there was a small and specific test demonstrating the difference from marking each individual site that we record as moved.

Latest push adds a mir-opt test file with the different cases, and shows what changes. Not all calls to record_operand_moved have an effect, as it is very conservative, in particular when locals are declared in different scopes.

RalfJung · 2026-08-08T17:14:30Z

a "move" is an occurrence of Operand::Move, which deinitializes the moved-from place

"de-initialize" here refers to the initialization state tracked for drop flags, right?
We don't currently have a MIR semantics where this actually changes the contents of memory to be uninitialized.

cjgillot · 2026-08-08T18:08:28Z

"de-initialize" here refers to the initialization state tracked for drop flags, right?

Yes

RalfJung · 2026-08-08T20:19:39Z

"de-initialize" here refers to the initialization state tracked for drop flags, right?

Yes

This should probably be mentioned in the Operand::Move doc comment.

And some more cases along the way. We have a specific optimization to avoid generating useless drops, use it. In particular, aggregate construction are very similar to function calls for which this is designed.

rustbot · 2026-08-08T21:52:42Z

This PR changes MIR

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

RalfJung · 2026-08-09T05:48:29Z

+    /// During MIR analyzes, it overwrites the place with `uninit` bytes and unschedules drops on
+    /// the given place.


Suggested change

/// During MIR analyzes, it overwrites the place with `uninit` bytes and unschedules drops on

/// the given place.

/// Before drop elaboration, this unschedules drops on the given place.

There's no overwriting happening, or at least it's unclear -- that's the point of the next paragraph.

View changes since the review

RalfJung · 2026-08-09T05:50:28Z

-    /// > consider indirect assignments.
+    /// **Async drop processing**:
+    /// MIR building detects possible async drops, and constructs a complete CFG. To correctly
+    /// handle the coroutine being dropped while itself drops, we need a 'drop' target


"while itself drops"? I can't quite parse this.

View changes since the review

RalfJung · 2026-08-09T05:51:45Z

+        /// operational meaning.
        replace: bool,
-        /// Cleanup to be done if the coroutine is dropped at this suspend point (for async drop).
+        /// Cleanup to be done if the coroutine is dropped at this suspend point, for async drop.


Suggested change

/// Cleanup to be done if the coroutine is dropped at this suspend point, for async drop.

/// Cleanup to be done if the coroutine is dropped at this suspend point, for async drop.

/// Is always `None` after state machine lowering.

Is this right? The interpreter asserts it.

View changes since the review

RalfJung · 2026-08-09T05:52:08Z

+    /// **Async drop processing**:
+    /// MIR building detects possible async drops, and constructs a complete CFG. To correctly
+    /// handle the coroutine being dropped while itself drops, we need a 'drop' target
+    /// similar to `Yield` terminator.


Suggested change

/// similar to `Yield` terminator.

/// similar to `Yield` terminator. That's the `drop` field.

View changes since the review

rust-bors · 2026-08-09T19:54:12Z

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

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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 23, 2026

rustbot assigned JohnTitor Jun 23, 2026

theemathas added T-lang Relevant to the language team needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 23, 2026

rustbot assigned jackh726 and unassigned JohnTitor Jun 27, 2026

cjgillot force-pushed the drop-moved-locals branch from 51861b3 to d128dba Compare July 1, 2026 23:37