Pin stabilization #56939
Pin stabilization #56939
Conversation
rust-highfive
added
the
S-waiting-on-review
Mark-Simulacrum
added
the
relnotes
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| #[inline(always)] | ||
| pub unsafe fn new_unchecked(pointer: P) -> Pin<P> { | ||
| Pin { pointer } | ||
| } | ||
|
|
||
| /// Get a pinned shared reference from this pinned pointer. | ||
| #[unstable(feature = "pin", issue = "49150")] | ||
| #[stable(feature = "pin", since = "1.33.0")] | ||
| #[inline(always)] | ||
| pub fn as_ref(self: &Pin<P>) -> Pin<&P::Target> { |
There was a problem hiding this comment.
I talked with @withoutboats in person about this a bit as I was slightly concerned about this, so I just wanted to make sure to write down some thoughts. The AsRef trait is the libs team's worst offender in terms of "thorns in our side about backwards compatibility", as adding AsRef anywhere seems to break due to subtle inference changes.
Along those lines, I think it definitely makes sense to be consistent with self vs not in Pin, but I just wanted to raise this and make sure it'd been considered. I think @withoutboats mentioned they'd talk with you @cramertj about the ergonomic importance of self-vs-not. It seems like the primary motivation for self everywhere was ergonomics, and I'd just want to make sure it's well motivated to take such a common name like as_ref!
Concretely my worry is that we may not be able to add inherent self-based methods to Pin in the future if they cause too much breakage in the ecosystem (due to shadowing conflicts), which would cause us to have two idioms.
Again though, just wanted to make sure I brought that up and ensure it was weighed!
There was a problem hiding this comment.
The ergonomic improvement from self in as_mut and set at least are pretty important because it's common to see them chained, which gets pretty ugly otherwise. The most common instance of this is Pin::get_mut_unchecked(Pin::as_mut(self)) inside futures-rs (it's pretty common to see Pin::as_mut used for reborrowing in combination with other methods). set is another one where I personally just get it wrong every time and try to spell it self.set(value) rather than Pin::set(Pin::as_mut(self), value);-- I think you can probably see why ;).
I'm totally sympathetic to the concerns here and am interested in any ideas you might have for making this easier. One thing I will say is that I think it's much more common to call Pin methods on a Pin<P<T>> than it is to call e.g. Rc methods on an Rc<T>, so I'd been using that to justify to myself why Pin was "different" in this respect.
There was a problem hiding this comment.
Ok sure thing, I just wanted to double-confirm. I agree that the usage pattern of Pin seems like it favors inherent methods more often is good motivation for breaking the norm.
There was a problem hiding this comment.
One thing @alexcrichton and I talked about was making only as_ref and as_mut associated functions, so you'd get Pin::as_mut(self).get_mut_uncheckedand so on. What would you think about that @cramertj?
There was a problem hiding this comment.
I'd probably want something not called as_mut that did reborrowing in that case. I can define it as a trait in the pin_utils crate or something, but that seems like it kind of defeats the purpose.
There was a problem hiding this comment.
(to be clear: I'm fine making this change if it's the one @rust-lang/libs wants, it's just much more ergonomic and easier to read the chained-method version)
alexcrichton
added
the
T-libs-api
|
@bors: r+ I'm going to go ahead and approve this because we have plenty of time in the rest of the cycle to tweak and modify things, but I believe it's clear at this point we're not going to fundamentally change directions. While I won't pretend to speak for @aturon, @Centril I'm like 95% sure that @aturon wouldn't want to block this on figuring out I'm also gonna cc @rust-lang/libs on this as well, there's a few tweaks to the original issue (and the original issue is sort of massive). If any of y'all are interested but have reservations please feel free to comment and I can r- and we can discuss! |
|
📌 Commit c3ab2033e1ac20ff3536fbe754116d7a33f00d2f has been approved by |
bors
added
S-waiting-on-bors
Sure; Aaron is; You and me already discussed it plenty in private; but I'd like to hear from Aaron specifically so that I'm sure that our high level future plans are relatively in-sync.
I'm not saying we should change directions or that we should figure out I also don't think there's time pressure to do it now since 1.33 master=>beta promotion will happen on the 16th of January (T-2 from 18th of January) and Aaron should be back by then so I don't think it will risk missing 1.33 if we wait a bit. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
cramertj
force-pushed
the
pin-stabilization
branch
from
c3ab203
to
9229488
Compare
|
@Centril If you have specific concerns about the API and its interaction with |
|
@bors r=alexcrichton |
|
📌 Commit 9229488 has been approved by |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r=alexcrichton |
|
📌 Commit 46a4ff6 has been approved by |
|
@Centril I can speak for @aturon somewhat, having spoken to him about this recently. This is my opinion which I understand him to agree with (I also understand his opinion to be that we should stabilize these APIs immediately). The reality has always been that a type alias is not sufficient to backwards compatibly convert from library pins to language-supported pins, because replacing a struct with a type alias is not actually backwards compatible (because of things like destructuring exposing the reality). It's clear that if we wanted to add However, I also think its not very likely that we will directly add an
I think @aturon agrees with this, and his comment was worded somewhat poorly - he just meant that we can in the future add |
…richton Pin stabilization This implements the changes suggested in rust-lang#55766 (comment) and stabilizes the `pin` feature. @alexcrichton also listed several "blockers" in that issue, but then in [this comment](rust-lang#55766 (comment)) mentioned that they're more "TODO items": > In that vein I think it's fine for a stabilization PR to be posted at any time now with FCP lapsed for a week or so now. The final points about self/pin/pinned can be briefly discussed there (if even necessary, they could be left as the proposal above). Let's settle these last bits here and get this thing stabilized! :) r? @alexcrichton cc @withoutboats
…richton Pin stabilization This implements the changes suggested in rust-lang#55766 (comment) and stabilizes the `pin` feature. @alexcrichton also listed several "blockers" in that issue, but then in [this comment](rust-lang#55766 (comment)) mentioned that they're more "TODO items": > In that vein I think it's fine for a stabilization PR to be posted at any time now with FCP lapsed for a week or so now. The final points about self/pin/pinned can be briefly discussed there (if even necessary, they could be left as the proposal above). Let's settle these last bits here and get this thing stabilized! :) r? @alexcrichton cc @withoutboats
|
☔ The latest upstream changes (presumably #56805) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
cramertj
force-pushed
the
pin-stabilization
branch
from
46a4ff6
to
861df06
Compare
|
Sorry, I noticed this was going to conflict with #56805, but didn't bother commenting because it was way farther behind in the queue. Didn't realize it might be included in a rollup. |
|
@bors r=alexcrichton |
|
📌 Commit 861df06 has been approved by |
bors
added
S-waiting-on-bors
…richton Pin stabilization This implements the changes suggested in rust-lang#55766 (comment) and stabilizes the `pin` feature. @alexcrichton also listed several "blockers" in that issue, but then in [this comment](rust-lang#55766 (comment)) mentioned that they're more "TODO items": > In that vein I think it's fine for a stabilization PR to be posted at any time now with FCP lapsed for a week or so now. The final points about self/pin/pinned can be briefly discussed there (if even necessary, they could be left as the proposal above). Let's settle these last bits here and get this thing stabilized! :) r? @alexcrichton cc @withoutboats
Rollup of 14 pull requests Successful merges: - #56188 (enum type instead of variant suggestion unification ) - #56342 (Improve docs for collecting into `Option`s) - #56916 (Fix mutable references in `static mut`) - #56917 (Simplify MIR generation for logical operations) - #56939 (Pin stabilization) - #56953 (Mark tuple structs as live if their constructors are used) - #56964 (Remove `TokenStream::JointTree`.) - #56966 (Correct strings for raw pointer deref and array access suggestions) - #57020 (Point to cause of `fn` expected return type) - #57032 (fix deprecation warnings in liballoc benches) - #57053 (Fix alignment for array indexing) - #57062 (Fix a comment) - #57067 (Stabilize min_const_unsafe_fn in 1.33) - #57078 (Ignore two tests on s390x) Failed merges: r? @ghost
|
I'm concerned about a couple of the method names here. Would it be possible to un-stabilize some of them or rename them before this hits stable? Specifically:
These methods all return pinned references, while their names suggest they return the bare reference, which is confusing. I also just think that 11 methods and associated functions is a lot to stabilize in one go, and it would be nice to do things more incrementally. |
|
FWIW, I think that matches how e.g. |
|
It was really hard to understand what pinning does, largely because the term I suggest renaming (Do I understand this correctly?) I hope my view as an outsider was helpful, although it is kind of late for the refactoring ^^ |
|
@Julius-Beides I happen to agree with your position, but this question was already debated at considerable length in #55766, which is linked from the original post here. Please try to be thorough in checking whether what you are about to propose has already been suggested! |
|
@glaebhoerl I think that's different. |
|
@glaebhoerl Thank you for your answer. |
Actually, maybe that's why the names |
|
Not sure where to ask this, but has this 'issue' been fixed yet?: #56256 |
The `Unpin` bound was originally added in rust-lang#56939 following the recommendation of @withoutboats in rust-lang#55766 (comment) That comment does not give explicit justification for why the bound should be added. The relevant context was: > [ ] Remove `impl<P> Unpin for Pin<P>` > > This impl is not justified by our standard justification for unpin > impls: there is no pointer direction between `Pin<P>` and `P`. Its > usefulness is covered by the impls for pointers themselves. > > This futures impl (link to the impl changed in this PR) will need to > change to add a `P: Unpin` bound. The decision to remove the unconditional impl of `Unpin for Pin` is sound (these days there is just an auto-impl for when `P: Unpin`). But, I think the decision to also add the `Unpin` bound for `impl Future` may have been unnecessary. Or if that's not the case, I'd be very interested to have the argument for why written down somewhere. The bound _appears_ to not be needed, since the presence of a `Pin<P>` should indicate that it's safe to project to `Pin<&mut P::Target>` just like for `Pin::as_mut`.
… r=m-ou-se Remove P: Unpin bound on impl Future for Pin We can safely produce a `Pin<&mut P::Target>` without moving out of the `Pin` by using `Pin::as_mut` directly. The `Unpin` bound was originally added in rust-lang#56939 following the recommendation of `@withoutboats` in rust-lang#55766 (comment) That comment does not give explicit justification for why the bound should be added. The relevant context was: > [ ] Remove `impl<P> Unpin for Pin<P>` > > This impl is not justified by our standard justification for unpin impls: there is no pointer direction between `Pin<P>` and `P`. Its usefulness is covered by the impls for pointers themselves. > > This futures impl (link to the impl changed in this PR) will need to change to add a `P: Unpin` bound. The decision to remove the unconditional impl of `Unpin for Pin` is sound (these days there is just an auto-impl for when `P: Unpin`). But, I think the decision to also add the `Unpin` bound for `impl Future` may have been unnecessary. Or if that's not the case, I'd be very interested to have the argument for why written down somewhere. The bound _appears_ to not be needed, as demonstrated by the change requiring no unsafe code and by the existence of `Pin::as_mut`.
… r=m-ou-se Remove P: Unpin bound on impl Future for Pin We can safely produce a `Pin<&mut P::Target>` without moving out of the `Pin` by using `Pin::as_mut` directly. The `Unpin` bound was originally added in rust-lang#56939 following the recommendation of ``@withoutboats`` in rust-lang#55766 (comment) That comment does not give explicit justification for why the bound should be added. The relevant context was: > [ ] Remove `impl<P> Unpin for Pin<P>` > > This impl is not justified by our standard justification for unpin impls: there is no pointer direction between `Pin<P>` and `P`. Its usefulness is covered by the impls for pointers themselves. > > This futures impl (link to the impl changed in this PR) will need to change to add a `P: Unpin` bound. The decision to remove the unconditional impl of `Unpin for Pin` is sound (these days there is just an auto-impl for when `P: Unpin`). But, I think the decision to also add the `Unpin` bound for `impl Future` may have been unnecessary. Or if that's not the case, I'd be very interested to have the argument for why written down somewhere. The bound _appears_ to not be needed, as demonstrated by the change requiring no unsafe code and by the existence of `Pin::as_mut`.
This implements the changes suggested in #55766 (comment) and stabilizes the
pinfeature. @alexcrichton also listed several "blockers" in that issue, but then in this comment mentioned that they're more "TODO items":Let's settle these last bits here and get this thing stabilized! :)
r? @alexcrichton
cc @withoutboats