Initial implementation of UnsafePinned and UnsafeUnpin (RFC 3467)
#136964
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @Noratrieb rustbot has assigned @Noratrieb. Use |
rustbot
added
S-waiting-on-review
|
@rustbot label T-libs-api T-lang T-opsem F-unsafe_pinned S-waiting-on-author -S-waiting-on-review |
rustbot
added
F-unsafe_pinned
|
Failed to set assignee to
|
Sky9x
commented
Feb 13, 2025
Comment on lines
-1067
to
+1071
| if self.cx().is_lang_item(goal.predicate.def_id(), TraitSolverLangItem::Unpin) => | ||
| if self.cx().is_lang_item(goal.predicate.def_id(), TraitSolverLangItem::Unpin) | ||
| || self.cx().is_lang_item( | ||
| goal.predicate.def_id(), | ||
| TraitSolverLangItem::UnsafeUnpin, | ||
| ) => |
There was a problem hiding this comment.
still very unsure if this is ok, I think so? I'm not sure how to "wrap" coroutine fields in UnsafePinned (and if that would make any difference from just not implementing UnsafePinned directly)
Comment on lines
-730
to
+731
| if self.tcx().is_lang_item(def_id, LangItem::Unpin) => | ||
| if self.tcx().is_lang_item(def_id, LangItem::Unpin) | ||
| || self.tcx().is_lang_item(def_id, LangItem::UnsafeUnpin) => |
There was a problem hiding this comment.
also unsure about this, just matching the logic in the new solver for now?
Comment on lines
-26
to
-28
| // We rely on the fact that async/await futures are immovable in order to create | ||
| // self-referential borrows in the underlying coroutine. | ||
| impl<T: Coroutine<ResumeTy, Yield = ()>> !Unpin for GenFuture<T> {} |
|
Unresolved question from the RFC:
Not exactly possible as
still need to figure out generator lowering
hopefully since all (stable) ecosystem code uses |
This comment has been minimized.
This comment has been minimized.
|
In response to #125735 (comment): Does this mean |
This comment was marked as resolved.
This comment was marked as resolved.
RalfJung
reviewed
Feb 13, 2025
| @@ -70,15 +70,15 @@ impl NewPermission { | |||
| access: None, | |||
| protector: None, | |||
| } | |||
| } else if pointee.is_unpin(*cx.tcx, cx.typing_env()) { | |||
| } else if pointee.is_unsafe_unpin(*cx.tcx, cx.typing_env()) { | |||
There was a problem hiding this comment.
This is not the right check. This needs to check whether the type contains an UnsafeUnpin, not whether the type literally is UnsafeUnpin. IOW, this needs to work similar to is_freeze.
There was a problem hiding this comment.
IIUC this check is just a hack and allows too much code; I'm working on updating miri but the changes are non trivial. Am I going in the right direction by adding an UnpinSensitive variant to NewPermission and adding a visit_unpin_sensitive equivalent to visit_freeze_sensitive?
There was a problem hiding this comment.
Yes this is a hack but please preserve the hack for this PR. There's no reason to fix everything at once, that just makes for a monster PR that will be impossible to review.
So, no new NewPermission variants in this PR please.
RalfJung
reviewed
Feb 13, 2025
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The initial PR that adds the new type should probably not remove the |
RalfJung
reviewed
Feb 14, 2025
RalfJung
reviewed
Feb 14, 2025
Comment on lines
+952
to
+955
| /// Note that for backwards compatibility with the new [`UnsafePinned`] wrapper | ||
| /// type, placing this marker in your struct acts as if you wrapped the entire | ||
| /// struct in an `UnsafePinned`. This type will likely eventually be deprecated, | ||
| /// and all new code should be using `UnsafePinned` instead. |
There was a problem hiding this comment.
This is not a stable guarantee we want to make, at least not yet.
RalfJung
reviewed
Feb 14, 2025
|
Thanks for all the feedback, Ralf. I'm splitting this PR into multiple smaller ones, starting with just the libs changes in #137043. |
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Apr 13, 2025
…ross35,RalfJung,WaffleLapkin Initial `UnsafePinned` implementation [Part 1: Libs] Initial libs changes necessary to unblock further work on [RFC 3467](https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html). Tracking issue: rust-lang#125735 This PR is split off from rust-lang#136964, and includes just the libs changes: - `UnsafePinned` struct - private `UnsafeUnpin` structural auto trait - Lang items for both - Compiler changes necessary to block niches on `UnsafePinned` This PR does not change codegen, miri, the existing `!Unpin` hack, or anything else. That work is to be split into later PRs. --- cc `@RalfJung` `@Noratrieb` `@rustbot` label F-unsafe_pinned T-libs-api
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Apr 13, 2025
…ross35,RalfJung,WaffleLapkin Initial `UnsafePinned` implementation [Part 1: Libs] Initial libs changes necessary to unblock further work on [RFC 3467](https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html). Tracking issue: rust-lang#125735 This PR is split off from rust-lang#136964, and includes just the libs changes: - `UnsafePinned` struct - private `UnsafeUnpin` structural auto trait - Lang items for both - Compiler changes necessary to block niches on `UnsafePinned` This PR does not change codegen, miri, the existing `!Unpin` hack, or anything else. That work is to be split into later PRs. --- cc ``@RalfJung`` ``@Noratrieb`` ``@rustbot`` label F-unsafe_pinned T-libs-api
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 14, 2025
Rollup merge of rust-lang#137043 - Sky9x:unsafe-pinned-pt1-libs, r=tgross35,RalfJung,WaffleLapkin Initial `UnsafePinned` implementation [Part 1: Libs] Initial libs changes necessary to unblock further work on [RFC 3467](https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html). Tracking issue: rust-lang#125735 This PR is split off from rust-lang#136964, and includes just the libs changes: - `UnsafePinned` struct - private `UnsafeUnpin` structural auto trait - Lang items for both - Compiler changes necessary to block niches on `UnsafePinned` This PR does not change codegen, miri, the existing `!Unpin` hack, or anything else. That work is to be split into later PRs. --- cc ``@RalfJung`` ``@Noratrieb`` ``@rustbot`` label F-unsafe_pinned T-libs-api
github-actions bot
pushed a commit
to model-checking/verify-rust-std
that referenced
this pull request
Apr 19, 2025
…ross35,RalfJung,WaffleLapkin Initial `UnsafePinned` implementation [Part 1: Libs] Initial libs changes necessary to unblock further work on [RFC 3467](https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html). Tracking issue: rust-lang#125735 This PR is split off from rust-lang#136964, and includes just the libs changes: - `UnsafePinned` struct - private `UnsafeUnpin` structural auto trait - Lang items for both - Compiler changes necessary to block niches on `UnsafePinned` This PR does not change codegen, miri, the existing `!Unpin` hack, or anything else. That work is to be split into later PRs. --- cc ``@RalfJung`` ``@Noratrieb`` ``@rustbot`` label F-unsafe_pinned T-libs-api
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracking issue: #125735
An attempt at an initial implementation for RFC 3467
cc @RalfJung