Skip to content
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

Rename Pin to PinMut, and some more breaking changes #50497

Merged
merged 5 commits into from
May 8, 2018

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented May 7, 2018

As discussed at [1] §3 and [2] and [3], a formal look at pinning requires considering a distinguished "shared pinned" mode/typestate. Given that, it seems desirable to at least eventually actually expose that typestate as a reference type. This renames Pin to PinMut, freeing the name Pin in case we want to use it for a shared pinned reference later on.

[1] https://www.ralfj.de/blog/2018/04/10/safe-intrusive-collections-with-pinning.html
[2] rust-lang/rfcs#2349 (comment)
[3] #49150 (comment)

Cc @withoutboats

RalfJung added 2 commits May 7, 2018 12:44
As discussed at [1] §3 and [2] and [3], a formal look at pinning requires considering a
distinguished "shared pinned" mode/typestate.  Given that, it seems desirable to
at least eventually actually expose that typestate as a reference type.  This
renames Pin to PinMut, freeing the name Pin in case we want to use it for a
shared pinned reference later on.

[1] https://www.ralfj.de/blog/2018/04/10/safe-intrusive-collections-with-pinning.html
[2] rust-lang/rfcs#2349 (comment)
[3] rust-lang#49150 (comment)
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2018
@withoutboats
Copy link
Contributor

r? @withoutboats

There are a few other breaking changes I'd like to see made to these APIs, and probably it makes sense to do them in the same merge. I could build off this, or you could if you're interested in @RalfJung. The main thing is changing the methods that currently take &mut Pin to take just PinMut, like map and I think a few others.

@RalfJung
Copy link
Member Author

RalfJung commented May 7, 2018

The main thing is changing the methods that currently take &mut Pin to take just PinMut, like map and I think a few others.

I actually already have this prepared for map, just pushed that to this branch.

The only other candidate I can think of is get_mut (EDIT: added to this PR). Did you have others in mind?

@RalfJung
Copy link
Member Author

RalfJung commented May 7, 2018

There are a few other breaking changes I'd like to see made to these APIs, and probably it makes sense to do them in the same merge.

One thing coming to my mind right now: Renaming borrow to reborrow? I recall @cramertj wondering how to do a reborrow for PinMut, and it is quite important to understand that this is indeed possible. Though maybe that just calls for a documentation change? (EDIT: Added some possible improvement to the PR.)

@RalfJung RalfJung changed the title Rename Pin to PinMut Rename Pin to PinMut, and some more breaking changes May 7, 2018
@withoutboats
Copy link
Contributor

I think get_mut was the other, thanks!

@withoutboats
Copy link
Contributor

withoutboats commented May 7, 2018

calling it reborrow seems good. make it use method syntax too then (seems unlikely to conflict)?

@RalfJung
Copy link
Member Author

RalfJung commented May 7, 2018

Done (the rename).

As far as method syntax is concerned, I agree it is unlikely to conflict but is that good enough? OTOH, method syntax would be really useful here. I amended the commit to use self.

@withoutboats
Copy link
Contributor

@bors r+

cc @cramertj

@bors
Copy link
Contributor

bors commented May 7, 2018

📌 Commit 939c25a has been approved by withoutboats

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2018
@cramertj
Copy link
Member

cramertj commented May 7, 2018

This LGTM, although I'd really like to solidify what Pin gives you v.s. &PinMut. If I really can't rely on &PinMut and Pin providing similar guarantees, then I think we should add Pin ASAP, as I need it for a small handful of things.

@RalfJung
Copy link
Member Author

RalfJung commented May 7, 2018

This LGTM, although I'd really like to solidify what Pin gives you v.s. &PinMut. If I really can't rely on &PinMut and Pin providing similar guarantees

I'd think (but did not check in my model yet) that you can deref an &'a PinMut<'b, T> into a Pin<'a, T>.

@cramertj
Copy link
Member

cramertj commented May 7, 2018

@RalfJung I'm confused-- to me it seems like that conflicts with your previous statements about & and &Pin(Mut) being equivalent.

@RalfJung
Copy link
Member Author

RalfJung commented May 7, 2018

Those statements (IIRC) were made under the assumption that the model does not have a "shared pinned" typestate. I have two models of all of this in my head:

  1. Without "shared pinned". In this model, we can safely transmute between &PinMut<T> and &&T; shared references "forget" about pinning. I don't see a good reason for having shared pinned references in this model, they are not useful for e.g. intrusive collections. This is the model I sketched in my first post on the topic.
  2. With "shared pinned". Nobody likes model 1 because nobody thinks these transmutes should be legal and there is no other benefit. In this model, the transmute is not legal, and it makes sense to have a reference type that exposes this "shared pinned" typestate. Using this reference type makes sense e.g. for intrusive collections. This is the model I am assuming we will actually use, which is the reason I wrote this RFC.

The statement above about dereferencing &PinMut<T> was made in model 2.

@cramertj
Copy link
Member

cramertj commented May 7, 2018

@RalfJung Ah, I was always assuming that we had "shared pinned" as a valid typestate-- that explains why I was so confused by your post on the subject. I never questioned that the typestate should exist, but rather whether it was necessary to include a built-in type for it (rather than just using &Pin).

@pythonesque
Copy link
Contributor

pythonesque commented May 7, 2018

@cramertj &Pin is really annoying to work with since you can't do field projections with it (because you need to get an &&Pin<T> to the field somehow). So the only reason not to have it is if &Pin is never useful for anything at all (which was the case previously).

@RalfJung
Copy link
Member Author

RalfJung commented May 7, 2018

@cramertj Also see #49150 (comment) where I essentially wrote what @pythonesque said above (I think @comex was the first to make this observation).

@cramertj
Copy link
Member

cramertj commented May 7, 2018

You're both right, of course, and I've changed my mind after using it in practice 😄

@bors
Copy link
Contributor

bors commented May 8, 2018

⌛ Testing commit 939c25a with merge c166b03...

bors added a commit that referenced this pull request May 8, 2018
Rename Pin to PinMut, and some more breaking changes

As discussed at [1] §3 and [2] and [3], a formal look at pinning requires considering a distinguished "shared pinned" mode/typestate.  Given that, it seems desirable to at least eventually actually expose that typestate as a reference type.  This renames Pin to PinMut, freeing the name Pin in case we want to use it for a shared pinned reference later on.

[1] https://www.ralfj.de/blog/2018/04/10/safe-intrusive-collections-with-pinning.html
[2] rust-lang/rfcs#2349 (comment)
[3] #49150 (comment)

Cc @withoutboats
@bors
Copy link
Contributor

bors commented May 8, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: withoutboats
Pushing c166b03 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants