Skip to content

Initial UnsafePinned impl [Part 2: Lowering] #139896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sky9x
Copy link
Contributor

@Sky9x Sky9x commented Apr 16, 2025

Coroutine lowering part of RFC 3467.
Tracking issue: #125735
Part 1: #137043

  • Coroutines now track which of their stored locals are borrowed across suspension points and thus must be considered pinned.
  • UnsafeUnpin for coroutines:
    • if the coro is movable, it always implements UnsafeUnpin. (movable coros never have pinned fields)
    • if it is immovable (static), it implements UnsafeUnpin iff none of its fields are pinned.

Still TODO (for later PRs):

  • Coroutine layout (restore niche opts)
  • Codegen (noalias metadata etc)
  • Miri

@rustbot label F-unsafe_pinned F-coroutines A-coroutines

cc @RalfJung @compiler-errors @traviscross

@rustbot
Copy link
Collaborator

rustbot commented Apr 16, 2025

r? @lcnr

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

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) A-coroutines Area: Coroutines F-coroutines `#![feature(coroutines)]` F-unsafe_pinned `#![feature(unsafe_pinned)]` labels Apr 16, 2025
@Sky9x Sky9x marked this pull request as ready for review April 16, 2025 07:11
@rustbot
Copy link
Collaborator

rustbot commented Apr 16, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member

Movable coroutines now always implement UnsafeUnpin and immovable coroutines now never implement UnsafeUnpin.

"always" sounds wrong; a movable coroutine could have an UnsafeUnpin type as a field, right?

I understand this is consistent with what we do with Unpin today, but I wonder why we don't just let the auto trait do its thing: if all variables for all yield points are UnsafeUnpin, we just propagate that. So even immotable coroutines could be UnsafeUnpin if they happen to not have any references across yield points.

Coroutine layout (restore niche opts)

This is missing context -- what is the interaction with niche ops?

@Sky9x Sky9x force-pushed the unsafe-pinned-pt2-lowering branch from f44b09f to 03ef470 Compare April 16, 2025 19:30
@Sky9x
Copy link
Contributor Author

Sky9x commented Apr 16, 2025

Movable coroutines now always implement UnsafeUnpin and immovable coroutines now never implement UnsafeUnpin.

"always" sounds wrong; a movable coroutine could have an UnsafeUnpin type as a field, right?

I understand this is consistent with what we do with Unpin today, but I wonder why we don't just let the auto trait do its thing: if all variables for all yield points are UnsafeUnpin, we just propagate that. So even immotable coroutines could be UnsafeUnpin if they happen to not have any references across yield points.

Updated the trait solving logic to only implement UnsafeUnpin if no fields are pinned. This allows immovable (static) coros with no pinned fields to implement UnsafeUnpin.

Coroutine layout (restore niche opts)

This is missing context -- what is the interaction with niche ops?

#129313 Disabled niches in coroutines because of #63818. The planned "Part 3: Codegen" will restore this.

@Sky9x
Copy link
Contributor Author

Sky9x commented Apr 16, 2025

I'm not sure how to write tests checking impls of UnsafeUnpin as the trait is private in libcore

@@ -711,6 +722,7 @@ fn locals_live_across_suspend_points<'tcx>(
// of the local, which happens using the `intersect` operation below.
borrowed_locals_cursor.seek_before_primary_effect(loc);
live_locals.union(borrowed_locals_cursor.get());
locals_borrowed_across_any_suspension_point.union(borrowed_locals_cursor.get());
Copy link
Contributor Author

@Sky9x Sky9x Apr 16, 2025

Choose a reason for hiding this comment

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

This should prob use its own dataflow analysis to avoid false positives (see borrow_not_held_across_yield mir test) (EDIT: WIP)

@RalfJung
Copy link
Member

#129313 Disabled niches in coroutines because of #63818. The planned "Part 3: Codegen" will restore this.

Ah, but only outside pinned fields I assume. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coroutines Area: Coroutines F-coroutines `#![feature(coroutines)]` F-unsafe_pinned `#![feature(unsafe_pinned)]` 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants