Skip to content

Mark more locals as moved to avoid building drops for them. - #158281

Open
cjgillot wants to merge 5 commits into
rust-lang:mainfrom
cjgillot:drop-moved-locals
Open

Mark more locals as moved to avoid building drops for them.#158281
cjgillot wants to merge 5 commits into
rust-lang:mainfrom
cjgillot:drop-moved-locals

Conversation

@cjgillot

@cjgillot cjgillot commented Jun 23, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

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

rustbot commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

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

rustbot commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

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

theemathas commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

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

@cjgillot

cjgillot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

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

@theemathas 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
@JohnTitor

JohnTitor commented Jun 27, 2026

Copy link
Copy Markdown
Member

@rustbot reroll

@rustbot rustbot assigned jackh726 and unassigned JohnTitor Jun 27, 2026
@cjgillot
cjgillot force-pushed the drop-moved-locals branch from 51861b3 to d128dba Compare July 1, 2026 23:37
@rustbot

This comment has been minimized.

Sign in to view
@cjgillot cjgillot 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 Jul 2, 2026
@cjgillot
cjgillot force-pushed the drop-moved-locals branch from a5dd21c to 4ff8c26 Compare July 5, 2026 03:19
@cjgillot cjgillot 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 Jul 5, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot
cjgillot force-pushed the drop-moved-locals branch from 4ff8c26 to 90e4893 Compare July 5, 2026 11:28
@rust-bors

This comment has been minimized.

Sign in to view
@cjgillot
cjgillot force-pushed the drop-moved-locals branch from 90e4893 to a851b9e Compare July 8, 2026 23:26
@rustbot

This comment has been minimized.

Sign in to view
@cjgillot

cjgillot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 9, 2026
@rust-bors

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@cjgillot
cjgillot force-pushed the drop-moved-locals branch from 734e208 to 4ad2c3f Compare August 3, 2026 02:17
@rustbot

This comment has been minimized.

Sign in to view
@jackh726

jackh726 commented Aug 7, 2026

Copy link
Copy Markdown
Member

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

RalfJung commented Aug 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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

@RalfJung

RalfJung commented Aug 8, 2026

Copy link
Copy Markdown
Member

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

cjgillot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

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)

@cjgillot
cjgillot force-pushed the drop-moved-locals branch from 4ad2c3f to c18975a Compare August 8, 2026 17:10
@rustbot

This comment has been minimized.

Sign in to view
@cjgillot

cjgillot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

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

RalfJung commented Aug 8, 2026

Copy link
Copy Markdown
Member

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.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot

cjgillot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

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

Yes

@cjgillot
cjgillot force-pushed the drop-moved-locals branch from c18975a to 1811466 Compare August 8, 2026 18:35
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung

RalfJung commented Aug 8, 2026

Copy link
Copy Markdown
Member

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

@cjgillot
cjgillot force-pushed the drop-moved-locals branch from 1811466 to 6e4bdc1 Compare August 8, 2026 21:52
@rustbot

rustbot commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

This PR changes MIR

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

RalfJung
RalfJung reviewed Aug 9, 2026
View reviewed changes
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
RalfJung
RalfJung reviewed Aug 9, 2026
View reviewed changes
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
RalfJung
RalfJung reviewed Aug 9, 2026
View reviewed changes
Comment thread compiler/rustc_middle/src/mir/syntax.rs
RalfJung
RalfJung reviewed Aug 9, 2026
View reviewed changes
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
@rust-bors

This comment has been minimized.

Sign in to view
@cjgillot
Add tests.
079d984
@cjgillot
Use a GrowableBitSet for moved_locals.
453091a
@cjgillot
Mark locals used to construct Aggregates as moved.
73118b4 
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.
@cjgillot
Add test.
57f96ff
@cjgillot
Update docs.
a9aef0f
@rustbot

rustbot commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments
@dianne dianne dianne left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@jackh726 jackh726

Labels

needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. perf-regression Performance regression. 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-lang Relevant to the language team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent and nonsensical borrow-checking error from unwind path of destructor of moved-out variable

9 participants

@cjgillot @rustbot @theemathas @JohnTitor @rust-log-analyzer @rust-timer @jackh726 @RalfJung @dianne