Skip to content

Correct and document semantics of yield terminator#156875

Open
cjgillot wants to merge 3 commits into
rust-lang:mainfrom
cjgillot:yield-drop-resume
Open

Correct and document semantics of yield terminator#156875
cjgillot wants to merge 3 commits into
rust-lang:mainfrom
cjgillot:yield-drop-resume

Conversation

@cjgillot
Copy link
Copy Markdown
Contributor

@cjgillot cjgillot commented May 24, 2026 •
edited by rustbot
Loading

View all comments

The current implementation of dataflow for the yield terminator confuses the drop target with unwinding. Which is wrong, in particular when implementing async drops.

r? @RalfJung

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 24, 2026

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

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

@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. labels May 24, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 24, 2026

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot cjgillot force-pushed the yield-drop-resume branch from c2fa5ba to 4141c4c Compare May 24, 2026 12:27
RalfJung
RalfJung reviewed May 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@RalfJung RalfJung left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I don't know nearly enough about async lowering to review this.

@rustbot reroll
(Or maybe you know a good reviewer?)

View changes since this review

Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
@rustbot rustbot assigned adwinwhite and unassigned RalfJung May 24, 2026
@adwinwhite
Copy link
Copy Markdown
Contributor

adwinwhite commented May 24, 2026

@rustbot reroll

@rustbot rustbot assigned oli-obk and unassigned adwinwhite May 24, 2026
RalfJung
RalfJung reviewed May 26, 2026
View reviewed changes
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
oli-obk
oli-obk requested changes May 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@oli-obk oli-obk left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with doc review changes applied.

I was thinking and playing a bit with writing a run test, but it's not really worth it. It's an extreme corner case.

A mir-opt test would still be useful showing how the unwinding targets change now

View changes since this review

@rustbot 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 May 26, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 26, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@cjgillot
Copy link
Copy Markdown
Contributor Author

cjgillot commented May 29, 2026

I was thinking and playing a bit with writing a run test, but it's not really worth it. It's an extreme corner case.

A mir-opt test would still be useful showing how the unwinding targets change now

The only difference I could see in tests is the size of async-drop coroutine state. On the current (wrong) semantics, I got a size increase when trying to change async drop expansion: the resume argument was (wrongly) saved in coroutine state.

@cjgillot cjgillot force-pushed the yield-drop-resume branch from ce3102b to 9ef4f52 Compare May 29, 2026 20:24
@rustbot

This comment has been minimized.

Sign in to view
@cjgillot
Copy link
Copy Markdown
Contributor Author

cjgillot commented May 30, 2026

@bors r=oli-obk

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 30, 2026

📌 Commit 9ef4f52 has been approved by oli-obk

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 30, 2026
@JonathanBrouwer JonathanBrouwer mentioned this pull request May 30, 2026
Merged
rust-bors Bot pushed a commit that referenced this pull request May 30, 2026
@bors
Auto merge of #157149 - JonathanBrouwer:rollup-JIG4HGr, r=JonathanBro…
a857d06 
…uwer

Rollup of 12 pull requests

Successful merges:

 - #154591 (Remove `will_cache_on_disk_for_key_fn`)
 - #156672 (Misc improvements to coroutine transform code)
 - #157027 (HIR ty lowering: Move some things into submodules)
 - #157051 (Allow two object files for a single CGU in CompiledModule)
 - #157100 (Some more per owner things)
 - #153497 (Use `trait_object_dummy_self` more & heavily fix+update related docs)
 - #155638 (Fix tupled closure signature in `AsyncFn` arg mismatch diagnostic)
 - #156826 (style: Clarify nullary call and `()` no-break rule applies past max width)
 - #157004 (Remove unused functions in `value_analysis.rs`)
 - #157032 (Fixed more &x ->&mut x suggestions)
 - #157033 (Note irrefutable while let in loop type errors)
 - #157139 (compiler: `ops::RangeInclusive` → `range::RangeInclusive`)

Failed merges:

 - #156875 (Correct and document semantics of `yield` terminator)
@rust-bors rust-bors Bot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 30, 2026
@rust-bors

This comment has been minimized.

Sign in to view
@rust-bors rust-bors Bot removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 30, 2026
@cjgillot cjgillot force-pushed the yield-drop-resume branch from 9ef4f52 to b542677 Compare May 30, 2026 14:46
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 30, 2026

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
Copy link
Copy Markdown
Contributor Author

cjgillot commented May 30, 2026

@bors r=oli-obk

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 30, 2026

📌 Commit b542677 has been approved by oli-obk

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 30, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request May 30, 2026
@JonathanBrouwer
Rollup merge of rust-lang#156875 - cjgillot:yield-drop-resume, r=oli-obk
95a2369 
Correct and document semantics of `yield` terminator

The current implementation of dataflow for the yield terminator confuses the `drop` target with unwinding. Which is wrong, in particular when implementing async drops.

r? @RalfJung
@JonathanBrouwer JonathanBrouwer mentioned this pull request May 30, 2026
Open
rust-bors Bot pushed a commit that referenced this pull request May 30, 2026
@bors
Auto merge of #157161 - JonathanBrouwer:rollup-3MNUPwu, r=JonathanBro…
f8a08b6 
…uwer

Rollup of 8 pull requests

Successful merges:

 - #156863 (Make hint::cold_path #[cold] so that it works even if the MIR inliner can't inline it)
 - #156875 (Correct and document semantics of `yield` terminator)
 - #157115 ([rustdoc] Fix foreign items macro expansion)
 - #157150 (Revert "drop derive helpers during ast lowering" )
 - #156887 (Rename `-Zdebuginfo-for-profiling` switch)
 - #157039 (rustdoc: correctly propagate cfgs for glob reexports)
 - #157125 (Rewrite the `#[repr]` attribute parser)
 - #157154 (Revert workaround used to select the gcc codegen in the coretests CI)
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

@oli-obk oli-obk oli-obk requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@oli-obk oli-obk

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cjgillot @rustbot @rust-log-analyzer @adwinwhite @RalfJung @oli-obk