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

Standard library API for immovable types #2349

Merged
merged 14 commits into from
Mar 18, 2018

Conversation

withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Feb 24, 2018

@withoutboats withoutboats added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Feb 24, 2018
@withoutboats
Copy link
Contributor Author

Original text said Move means the opposite of what it means, fixed.

@Havvy
Copy link
Contributor

Havvy commented Feb 24, 2018

Overall, this looks like a great design to me.

The only worry I have is that by having it be only a logic error to impl both Own and Share, but reserve the right to make it UB later is that it's a confusing way of saying that it's UB although not yet dangerous. I'd rather the text just flat out say it is UB to impl both of them.

@withoutboats
Copy link
Contributor Author

withoutboats commented Feb 24, 2018

Here's the marker trait hierarchy that I made with graphviz for helping visualize it:

foo

The only worry I have is that by having it be only a logic error to impl both Own and Share, but reserve the right to make it UB later is that it's a confusing way of saying that it's UB although not yet dangerous. I'd rather the text just flat out say it is UB to impl both of them.

I'm fine with this. To be clearer about my intent, what I'm interested in doing is someday saying unsafe trait Own: !Share and vice versa someday, so that it'd be incoherent to implement both of them. I don't know of any optimizations we could do, its more for mutually exclusive traits. (And I wanted to be emphatic in the next that Share is not a subset of Own or something. Own implies exclusive ownership.)

@bill-myers
Copy link

The RFC defines the "Share" trait, but doesn't use it anywhere.

Also, why is Anchor only applicable to "Own"? Can't you anchor an Rc too, for instance? In fact, it's not clear why Own is needed at all.

Also not really clear why "StableDerefMut" is needed instead of just "StableDeref" (or, for that matter, just Deref and letting the users of the API apply Stable trait bounds if desired), why Pin::new_unchecked must be unsafe, and so on.

Seems like in general the proposal would benefit from explicitly listing the guarantees, proving that they hold and showing why the proposed infrastructure is exactly what is needed to make them hold, and in particular why trait bounds are needed and why functions must be unsafe.

@withoutboats
Copy link
Contributor Author

Glad to answer your questions. :-)

The RFC defines the "Share" trait, but doesn't use it anywhere.

That's because its useful for owning-ref, not Pin. The authors of owning-ref and rental could probably elaborate more how they use it, but I believe its connected to cloning something that contains an owning-ref.

Also, why is Anchor only applicable to "Own"? Can't you anchor an Rc too, for instance? In fact, it's not clear why Own is needed at all.

Anchoring an Rc would not provide the necessary guarantee. Anchor must guarantee that you can never move out of the anchor again. With Rc you could do this:

let rc1 = Rc::new(x);
let rc2 = rc1.clone();

// `x` must never move for this to be safe
let anchor = Anchor::new(rc1);

// do stuff with the anchor

drop(anchor);

// move x out of hte second rc
let x = Rc::try_unwrap(rc2).unwrap();
let new_rc = Anchor::new(Rc::new(x));

// do stuff with the anchor, but now it has moved

Also not really clear why "StableDerefMut" is needed instead of just "StableDeref"

The static string optimization is discussed in the RFC.

or, for that matter, just Deref and letting the users of the API apply Stable trait bounds if desired

I don't know what this means.

Pin::new_unchecked must be unsafe

Pin provides the guarantee that it is only interchangeable with &mut if the type implements Move. This way, you can guarantee when you have a Pin that the address of the type being pointed to will not change.

@jpernst
Copy link

jpernst commented Feb 24, 2018

@bill-myers: As @withoutboats mentions, Share is useful for cloning a self-referential data structure, since it guarantees that, in the new clone, the fields being Share will still point to the same memory after cloning, and thus remain consistent. If the address could change after the clone, then the original struct and the clone would become entangled and you're on the way to segfault city. Rental doesn't currently support cloning, but it would be fairly easy to add with this trait in place.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Just a typo and some phrasing suggestions =)


This would require no extensions to the standard library, but would place the
burden on every user who wants to call resume to guarantee (at the risk of
memory insafety) that their types were not moved, or that they were moveable.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/insafety/unsafety


Note also that this RFC does not propose implementing `StableDerefMut` for
`String`. This is to be forward compatible with the static string optimization,
an optimization which allows values of `&'static str` to be converted to
Copy link
Contributor

Choose a reason for hiding this comment

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

Strike an optimization?


The `Pin` type is a wrapper around a mutable reference. If the type it
references is `!Move`, the `Pin` type guarantees that the referenced data will
never be moved again. It has a relatively small API. It is added to both
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to?

Pin has a relatively small API that is added to both std::mem and core::mem.

### Pinning to the heap: The `Anchor` type

The `Anchor` wrapper takes a type that implements `StableDeref` and `Own`, and
prevents users from moving data out of that unless it implements `Move`. It is
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph is a bit hard to read. Suggestions:

  • s/moving data out of that unless it/moving data out of the anchor unless data/
  • s/It is/Anchor is/

@smmalis37
Copy link

smmalis37 commented Feb 24, 2018

Random thoughts I have while reading:

Personally I would strongly vote for making it a compile error to implement both Own and Share on a type. This could be loosened in the future of course, but it seems wrong to me to leave the opportunity open for something to go wrong when we can make it impossible.

Could you include examples of pointer types that would not implement StableDeref? Are there any currently?

I imagine the error messages that will need to be printed in cases like trying to move something that's pinned elsewhere could get messy. Any thoughts on those?

@clarfonthey
Copy link
Contributor

I'm having trouble wrapping my head around this, partially because I'm tired but also because a few details are left out. I'll leave a few comments regarding some things that might be best to clarify.

@clarfonthey
Copy link
Contributor

So I'm tired and I stayed up longer than I should have deciphering this RFC and my comments probably show that. But my basic thoughts:

  1. Own/Share should be in their own RFC, ditto for StableDeref. IMHO if those features can't stand on their own, it's too much to implement for just this RFC.
  2. This RFC could really do with separating out the ref-level and guide-level explanations. I'm sure you just copied the old template but there are a lot of useful questions to answer in the new template that were left out here.
  3. Honestly, I'd like a lot more explanation on the layman's terms of how this would work for the average user. You didn't touch at all on how this modifies the usage of generators in general, or how it'd apply to other useful uses of generators like iterators and futures. Because Iterator in particular does not reference Pin at all, I don't see a clear way to take a generator and turn it into an iterator.
  4. Really I'm just struggling with how this all works, and while a lot of people like to jab at how difficult it is to learn Rust's borrow system and other parts, this takes the cake for difficulty. I honestly think this would seriously push people away from using generators because of the API complexity, and for that reason, I don't see why this RFC is better than the old one.
  5. Okay but, how does this actually stop us from moving immovable generators?

@clarfonthey
Copy link
Contributor

Also in general I think that any RFC that tries to generalise the concept of immovable types beyond immovable generators, should explain how this could also apply to self-referential structs. Because honestly, you could special-case immovable generators in the most hacky way possible and just show an error of "you can't move that" whenever you tried to move one of the anonymous types referencing those.

But, because you're generalising this, you should definitely include how this would help people make self-referential structs, with examples of how it would work, even if you don't have a proposal for how one would create such things. Because if you're not going to generalise it beyond generators, why have all this?

@gerryxiao
Copy link

Doesn't the arbitray self type need another RFC before this?

@SimonSapin
Copy link
Contributor

Just to make sure I understand: Cow<[T]> for example sometimes owns T items like the Own marker describes, and sometimes shares them like the Share marker describes, but it does neither always and so it should not implement these traits. Does that sound correct?

This trait is a lang item, but only to generate negative impls for certain
generators. Unlike previous `?Move` proposals, and unlike some traits like
`Sized` and `Copy`, this trait does not impose any particular semantics on
types that do or don't implement it.

Choose a reason for hiding this comment

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

That can't literally be true, if it had no implications it would be useless. Indeed under unresolved questions the definition "it's safe to convert between &mut T and Pin<T>" is given. Is this paragraph trying to say the trait isn't "language magic" that impacts fundamental rules the compiler checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I meant it doesn't interact with the compiler in any special way. Its just used by Pin to enforce invariants.

@bill-myers
Copy link

bill-myers commented Feb 24, 2018

You are right regarding Rc, but it seems an Anchor<Rc<T>> could be an useful thing.

I think it's safe if you only allow creation of Anchor<Rc<T>> when Rc<T> has no other references. One option could be to have "is_unique" be a method in the Share trait, and having an Anchor constructor for Share types that gives an Option and returns None if is_unique returns false.

BTW, a related thing: shouldn't Anchor<T> provide a safe way to get an &T in addition to the unsafe method get_mut to get an &mut T? This would be safe for Box, but wouldn't be safe for Rc, so it could be provided depending on a trait bound.

Also, shouldn't Anchor provide a safe variant of get_mut() if T::Target: Move, along with into_inner?

Also not really clear why "StableDerefMut" is needed instead of just "StableDeref"
The static string optimization is discussed in the RFC.

But wouldn't it be enough to only require StableDerefMut to provide DerefMut and pin() on Anchor, rather than having it be a bound for all operations? (in particular, Deref on Anchor). If Anchor doesn't provide DerefMut or pin, then the string is not going to move due to the small string optimization, no?

Pin::new_unchecked must be unsafe

Right, this is of course needed because due to &mut reborrowing there would be a naked &mut after Pin is dropped, while the StackPinned construction prevents this because the StackPinned has a lifetime reference that prevents it existing afterwards.

@bill-myers
Copy link

A thought experiment regarding StableDeref: let's say we have a struct that contains a Box<(T, T)> and whose implementation of Deref generates a random number and then either returns an &T for the first or for the second T depending on it.

Even though the result of Deref is not constant, It seems like it would be safe to implement StableDeref on that, and the RFC as written seems to allow it, but maybe it should be clarified that StableDeref only requires that data of type T is not moved, and not that the returned address is constant.

@nikomatsakis
Copy link
Contributor

Very nice RFC. Covers a lot of ground, cleanly written. I definitely prefer this to both ?Move and the unsafe variant. As you say, it accomplishes similar goals to ?Move without its downsides.

I have some questions/comments, some "observations", and a few nits.

Observation. One thing I think might be nice in the RFC is a sort of appendix that shows how you would use this to drive a generator containing internal references. I imagine it is something like this:

let my_generator = || ...;

// might be nice to have a helper for this, like `Anchor::in_box(...)`
// or something.
let mut p = Anchor::new(Box::new(my_generator)); 
p.pin().resume(); // ?

this trait [Move] does not impose any particular semantics on types that do or don't implement it.

Question: Well, you state later "Move really means that its safe to convert between Pin<T> and &mut T, for example", so that seems to be some kind of semantics. Maybe I don't understand what you are trying to say with this sentence. In any case, I think it's important to state precisely what this trait requires of its implementors if we can. I think I would say it like this:

"If T: Move, this means that it is safe to convert from Pin<T> to &mut T. Typically, this means that the type T does not contain any form of "internal references", and hence if a user were to invoke mem::swap on two &mut T references -- thus moving their referents -- it would not invalidate any Pin values."

Ah, I think I see some of the complication here. In a way, Pin itself is a kind of "carrier" that can be used by types to achieve guarantees, but doesn't itself impose constraints (and Move is indeed the way types can opt out of those constraints). So, for example, if we extend the Generator trait to require Pin, then we are not saying generators cannot move. But we are giving generators the opportunity to be immobile (by implememting !Move).

It makes sense, but I agree it might be good to change the name Move; it is very suggestive, perhaps too suggestive. Among other things, even in the original ?Move proposal, not implementing Move never meant that a type could not be moved -- only that it could not be moved once pinned (which in the original proposal was implicit with borrowing).

I have to wonder if a name like PinMut might be more appropriate -- or DerefMutWhenPinned if you want to get very English-y. This is not a trait we expect users to interact with directly, right? (Except if they are implementing types that want to opt-out from it.)

Observation: That actually makes me wonder: maybe conversion from Pin<T> to &mut T should offer some sort of "callback" that gives the type the chance to make changes? I could imagine a type that has internal references that are caches, for example, which could be cleared when using DerefMut.

I suppose we could add this later. I imagine we'd do something like this:

trait PrepDerefMutHook {
    fn prep_pin_deref_mut(&mut self);
}

impl<T: ?Sized> PrepDerefMutHook for T {
    fn post_move(&mut self) { }
}

where PrepDerefMutHook::prep_pin_deref_mut() is invoked as part of converting from Pin<T> to &mut T. Types that need to can then specialize PostMoveHook, which enables them to implement Move (where they cannot now).

We retain the liberty to assume that no type ever does implement both - we could upgrade this from a logic error to undefined behavior, we could make changes that would break any code that implements both traits for the same type.

Question: By this, do you mean that we might add some coherence feature in the future to causes said code to stop compiling?

StableDerefMut

Question: Should this be a trait alias? It doesn't seem to provide any additional guarantees of its own, right?

That crate draws no distinction between StableDeref and StableDerefMut. This does not leave forward compatibility for the static string optimization mentioned previously.

Question: Is there a need for this stronger form of StableDerefMut that we can identify? (e.g., from users of the stable_deref_trait crate?)

If the type it references is !Move, the Pin type guarantees that the referenced data will never be moved again.

Question: I wonder what the relationship between !Move and !Sized values ought to be. For example, we currently permit coercions from &mut dyn (Trait + 'long) to &mut dyn (Trait + 'short), which -- to be sound -- implies that one cannot e.g. swap unless T: Sized. (You might imagine that you could swap two &mut T values as long as you know that the dynamic sizes are equal.) This seems awfully close to saying that dyn Trait types could be Move (since in fact they cannot be moved).

Nit: I would find it clearer to state this in a positive way:

"The Pin type guarantees that the referenced data will never be moved again unless it implements Move."

I'm not saying this isn't equivalent; just that !Move is a bit of subtle notation. It's also worth pointing out that the auto trait nature of Pin means that one really ought to implement !Move as soon as possible.

pub fn borrow<'b>(this: &'b mut Pin<'a, T>) -> Pin<'b, T>;

Observation: This is going to put more pressure on having user-land types that can be "reborrowed", I imagine.

For types which implement Move, Pin is essentially the same as an &mut T.

Observation: Similarly, I wonder if this will put pressure on having a some kind of "auto-pin" so that foo.bar() can become Pin::new(&mut foo).bar(). (It'd be nice to do this generically, so that other user-land types can benefit.)

Because Anchor implements StableDeref and Own

Nit: This is not shown in the RFC, is it?

@nikomatsakis
Copy link
Contributor

Personally I would strongly vote for making it a compile error to implement both Own and Share on a type. This could be loosened in the future of course, but it seems wrong to me to leave the opportunity open for something to go wrong when we can make it impossible.

I think I agree with @smmalis37 -- maybe we should make them lang-items with some custom coherence logic for now, which we either aim to generalize later or just drop if we decide it's not worth it. I was thinking that I felt uncomfortable with the idea of "we reserve the right to make your stable code some compiling" (though I know that we already have a very similar problem around unsafe code and UB).

@nikomatsakis
Copy link
Contributor

@bill-myers

You are right regarding Rc, but it seems an Anchor<Rc> could be an useful thing.

I think it's safe if you only allow creation of Anchor<Rc> when Rc has no other references. One option could be to have "is_unique" be a method in the Share trait, and having an Anchor constructor for Share types that gives an Option and returns None if is_unique returns false.

On a related note, something I have often wanted is a type like RcBox (maybe a bad name). This would be something that is layout-compatible with an Rc, but statically known to have a reference count of one. Usually my use case is that I something like this: I want to build up (say) a HashMap<Key, Rc<Vec<Value>>>, so that I can cheaply clone the values later. But during construction those values are unique, so I can grow the vectors in place. Today, I do this with Rc::get_mut().unwrap(), basically, but it'd be nice to build up a HashMap<Key, RcBox<Vec<Value>>>, and then be able transmute that to the final type. (This would work best if we added some form of "safe transmute" as @glaebhoerl has proposed from time to time.)

Anyway, my point with all of this is that you could also have a method like get_mut, let's call it get_box:

impl<T> Rc<T> {
    fn get_box(self) -> Result<RcBox<T>, Rc<T>> {
        if /* reference count is one */ { Ok(transmute(Self)) } else { Err(self) }
    }
}

and -- finally -- implement Anchor for RcBox.

@nikomatsakis
Copy link
Contributor

Even though the result of Deref is not constant, It seems like it would be safe to implement StableDeref on that, and the RFC as written seems to allow it, but maybe it should be clarified that StableDeref only requires that data of type T is not moved, and not that the returned address is constant.

I think we should go further, and forbid that behavior. Basically say that StableDeref means that, for any given value, the same pointer is returned every time. It's stronger than we need right now, but it is useful for other things, and the present, weaker form is not particularly useful.

This raises an interesting point concerning &T types. I think it is reasonable to ask whether, in this code, p and q must contain the same pointer:

let x = 3;

{
    let p = &x;
    ...
}

{
    let q = &x;
    ...
}

It is certainly observable today that they do -- from safe code, no less -- but it'd be nice for the compiler to be able to promote x to a register, only pinning it on the stack temporarily. In that case, the addresses might well be different. It seems to me that the even the stronger definition of StableDeref would still allow that, since p and q are distinct values.

This latter question is more a problem for Unsafe Code Guidelines, but it's interesting to think about.

@glaebhoerl
Copy link
Contributor

RcBox

It seems like I... also proposed that at some point. :P #1283

@withoutboats
Copy link
Contributor Author

@Diggsey I don't believe you need to dereference to immutable pin; you can just dereference to an immutable reference.

@Diggsey
Copy link
Contributor

Diggsey commented Apr 6, 2018

@withoutboats but that loses the information that the type is pinned.

It seems like I should be able to separate out the RefCell/RwLock part, and have a simple PinRc or PinArc that gives you an ImmutablePin to its interior, and then a PinCell<T>/PinRwLock<T> which never gives out mutable references, but has a method which takes an ImmutablePin<Self> and returns a Pin<T>.

The previous discussion came to the conclusion that it was not possible to usefully obtain pins to the inside of types with interior mutability, and as a result of that there's no use for an ImmutablePin type, but I believe this conclusion is only valid if you only consider the existing Cell/RefCell types, and that building more restrictive versions of those types which only give out Pin references, and never mutable references, can be a useful and safe building block.

@Diggsey
Copy link
Contributor

Diggsey commented Apr 6, 2018

Thinking about it more, there's actually two possible variations of the ImmutablePin, depending on which axiom you accept:

  • Mapping an ImmutablePin is unsafe

In this variation, ImmutablePin is strictly a Pin which doesn't allow mutations while the ImmutablePin is alive, and a PinCell type could give out both Pin and ImmutablePin references to its interior (but not at the same time). Mapping is unsafe in general because after the inner ImmutablePin is dropped, there's nothing stopping the outer pinned type being mutated (moving the inner value). Mapping may still be safe in specific cases, where the types involved can prevent mutations that would move the inner value.

  • Mapping an ImmutablePin is always safe

In this variation, ImmutablePin is more interesting: the immutability last forever. As soon as an ImmutablePin is obtained, a Pin can never be obtained to the same value. A PinCell type could give out both Pin and ImmutablePin references to its interior, but as soon as it gives out an ImmutablePin, it becomes "frozen" in that mode. This variation is the one that would allow me to split PinRc into composable pieces.

@RalfJung
Copy link
Member

RalfJung commented Apr 6, 2018

What exactly do you mean by "mapping" here?

@Diggsey
Copy link
Contributor

Diggsey commented Apr 6, 2018

@RalfJung the Pin::map method - ie. it takes a function mapping a reference under a lifetime and returns a new Pin. There may be edge cases I haven't considered for the fully general "map" method, but I'm confident about the more limited case of mapping from an ImmutablePin<T> to an ImmutablePin<T::FieldType> (ie. mapping to a direct field of a struct).

@RalfJung
Copy link
Member

I wrote a blog post looking at intrusive collections with pinning, and came to the conclusion that indeed, having a shared pinned reference type may be a good idea after all. I think we should either declare that &&T and &Pin<T> are interchangeable as types (which makes the collection proposed by @cramertj above unsound), or we should have a type for shared pinned references. The details are in the post.

@RalfJung
Copy link
Member

@Diggsey
I am still not sure I entirely follow your reasoning here -- whether we can go from ImmutablePin<Struct> to an immutable pin of the field will depend on the type, I guess, but I am fairly sure that for public fields we want to say that generally, this is allowed. And we could express that even if ImmutablePin is &Pin. If you could provide some code, maybe that would help. :)

Either way...

The previous discussion came to the conclusion that it was not possible to usefully obtain pins to the inside of types with interior mutability, and as a result of that there's no use for an ImmutablePin type, but I believe this conclusion is only valid if you only consider the existing Cell/RefCell types, and that building more restrictive versions of those types which only give out Pin references, and never mutable references, can be a useful and safe building block.

This seems plausible. I was starting with the existing RefCell API and assumed we'd just (if at all) add new methods. I agree we likely could have a PinRefCell which exclusively works with pinned references. This would probably also be incompatible with making &&T and &Pin<T> interchangeable types. Good catch! Having both kinds of RefCell in one type would be nicer, but it is good to know that having two separate types is at least possible.

@Diggsey
Copy link
Contributor

Diggsey commented Apr 10, 2018

I am still not sure I entirely follow your reasoning here -- whether we can go from ImmutablePin to an immutable pin of the field will depend on the type, I guess, but I am fairly sure that for public fields we want to say that generally, this is allowed. And we could express that even if ImmutablePin is &Pin. If you could provide some code, maybe that would help. :)

I think this is only true for Unpin types - for !Unpin types the intention is to usually to mutate them (eg. the only current example of !Unpin is Generator which will happily move out of its fields). Admittedly it is difficult to talk about "general cases" when there's only one or two working examples.

In my first example, you would be able to obtain an ImmutablePin to a generator, be unable to run it for that period of time, and then later revert to a Pin to that generator and continue running it.

I think both ImmutablePin types I described can be allowed at the same time: let's call them ImmutablePin and FrozenPin:

  • ImmutablePin: observing this type guarantees that the target will never be moved in memory again, and the target is immutable for the lifetime of the ImmutablePin

  • FrozenPin: observing this type guarantees that the target will never be moved or mutated again

With the generator example, it would be unsafe to map an ImmutablePin to a field of the generator (ignoring for the moment that generator fields are not accessible) because you may later continue running the generator.

However, it would be safe to map a FrozenPin, because this prevents the generator from ever continuing, and so its fields are frozen in place, and therefore have the same pinning guarantees as the generator itself.

@RalfJung
Copy link
Member

With the generator example, it would be unsafe to map an ImmutablePin to a field of the generator (ignoring for the moment that generator fields are not accessible) because you may later continue running the generator.

Please elaborate. I don't see any principled source of unsafety here -- other than the usual problem of exposing fields as public that should have been private. Mapping a mutable reference on a Vec to one of the fields would also be unsafe, but that has nothing to do with mutable references and everything to do with these fields being private. I think the same is happening here.

@Diggsey
Copy link
Contributor

Diggsey commented Apr 10, 2018

Please elaborate. I don't see any principled source of unsafety here -- other than the usual problem of exposing fields as public that should have been private. Mapping a mutable reference on a Vec to one of the fields would also be unsafe, but that has nothing to do with mutable references and everything to do with these fields being private. I think the same is happening here.

I see... So does this mean that !Pin types are not supposed to publicly expose parts of themselves that they mutate while pinned? That was not at all obvious to me, and it seems like an explicit choice: I don't believe there's anything forcing that choice.

One could equally well choose to say that mapping is unsafe except where explicitly allowed by the type, and in that case exposing fields that are also mutated would be fine.

@RalfJung
Copy link
Member

RalfJung commented Apr 10, 2018

does this mean that !Pin types are not supposed to publicly expose parts of themselves that they mutate while pinned? That was not at all obvious to me, and it seems like an explicit choice: I don't believe there's anything forcing that choice.

I have no idea how you jumped from "Generates cannot expose all of their fields" to this conclusion. It would really help me if you could give a concrete piece of code demonstrating what you think goes wrong. For example, I should be able to form a pair of two !Unpin futures just fine, and go from Pin<(F1, F2)> to pins of the individual futures, and then subsequently call poll on either or both of these futures. (F1, F2) is clearly a !Unpin type, and yet making its fields public is fine. Clearly you are thinking of another situation, but I cannot imagine which.

One could equally well choose to say that mapping is unsafe except where explicitly allowed by the type, and in that case exposing fields that are also mutated would be fine.

That would be rather inconsistent. Making a field pub is the explicit statement by the type that going to this field is okay; why should pinned references work any different that mutable references in this regard?
I would rephrase your "frozen" reference as: Giving strict read-only access (no interior mutability allowed) to private fields cannot be used to break a type's invariants. That's true; giving read-only access to a Vec's length or capacity fields would also not break anything. We probably still don't want to do this. As far as I am concerned, if "mapping" to a field (to use your terminology) is unsafe, that's a very clear sign that this field should be private.

@Diggsey
Copy link
Contributor

Diggsey commented Apr 10, 2018

I have no idea how you jumped from "Generates cannot expose all of their fields" to this conclusion. It would really help me if you could give a concrete piece of code demonstrating what you think goes wrong. For example, I should be able to form a pair of two !Unpin futures just fine, and go from Pin<(F1, F2)> to pins of the individual futures, and then subsequently call poll on either or both of these futures.

That's a good example: let's say I am implementing a future combinator which wraps an inner pair of futures:

struct MyFuture<F1, F2>(F1, F2);

Now a Pin<MyFuture> could only support one of:

  1. polling the inner futures, publicly allow mapping to the inner futures
  2. moving out of the inner futures

You are saying that 1) should be the "default" behaviour, making the "pinning" property transitive. I think based on your last paragraph I agree that option is preferable as the default, but I had initially assumed that the "pinning" property is non-transitive, and that types would have to opt-in to say that their fields are pinned if they are pinned.

I think this is akin to the difference of const between C++ and Rust.

@RalfJung
Copy link
Member

RalfJung commented Apr 10, 2018

Indeed I've been assuming that pinning is "transitive" (or, as I'd call it, "structural") for pairs and such. It probably stops being transitive at a pointer indirection.

In fact, the RFC contains Pin::map for exactly this purpose: To be able to access a field. The reason this method is unsafe is that we cannot guarantee that the closure will access a field as opposed to, say, access the contents of a Box. If &pin ever becomes a primitive type, turning &pin (F1, F2) into &pin F1 will be a safe operation. At least, that's always been my reading of the RFC. Looking at it again, this is not as clear as I thought. @withoutboats it may be worth saying explicitly that using Pin::map to project to a public field is always considered a safe operation.

moving out of the inner futures

You mean, something like let x = my_future_pin.1? Yeah that'd be moving out of something pinned, that shouldn't be allowed. How would you even do that? Pin is a reference, you can't move out of a reference.

@Diggsey
Copy link
Contributor

Diggsey commented Apr 10, 2018

How would you even do that? Pin is a reference, you can't move out of a reference.

You could use mem::replace - this would require unsafe code to get a &mut from the Pin but that unsafety would not need to escape MyFuture - ie. MyFuture could present a safe, public API.

@RalfJung
Copy link
Member

oops somehow I didn't think of mem::replace and mem::swap.

However, obtaining an &mut out of a Pin is unsafe as you said.

MyFuture could present a safe, public API.

Agreed. And even with how Pin works now, it can present such an API for both cases. It could have a function fn get_first_future(Pin<Self>) -> &mut F1 and implement that with unsafe code, and that would be fine as long as (a) the field is private, and (b) no other code inside MyFuture ever obtains a Pin<F1> to the first field. Providing both get_first_future and ever calling poll on the first future results in unsoundness.

I think the difference between what you proposed and what I expected is only in what pub at a field means -- that's also public API, so it better be safe. When unsafe code is involved and there are public fields, they need special considerations because they grant extra power so external safe code. The question is, should pub say "You can go from Pin<MyFuture> to Pin of the public field", or should it say "You can go from Pin<MyFuture> to &mut of the public field". If I understand you correctly, you expected the latter? Is it fair to say that this is the core of our discussion here?

I have to say I find your POV very surprising, for two reasons:

  • First of all, usually such properties are inherited for struct fields. Even in C++, if you have a *const Struct, you are not allowed to mutate the fields. constness in C++ ends at a pointer indirection (unlike in Rust), but it does happen transitively across fields. You said above that this "is akin to the difference of const between C++ and Rust", but I don't think that's the case.
  • Secondly, this is about moving data around. It seems fairly intuitive that moving a struct is the same as moving its fields. So I never even considered any other possible interpretation, which is why I was so surprised by your first comments. :)

For these reasons, I see very little reason to declare "You can go from Pin<MyFuture> to &mut of the public field". But I do agree we have to be explicit about how pub on fields interacts with Pin, which is why I asked @withoutboats above if that could be clarified in the RFC. I wonder what the process is for such RFC text changes after the RFC got accepted?

@Diggsey
Copy link
Contributor

Diggsey commented Apr 11, 2018

@RalfJung cool, I think we're entirely on the same page - I believe the process is to just open a "normal" PR to the RFC repo that updates the relevant text.

I think my unusual initial POV is little more than just not having a good intuition for this quite yet, so I wouldn't read too much into it 😛

@clarfonthey
Copy link
Contributor

@Diggsey @RalfJung considering how this RFC has already been merged, shouldn't this discussion be happening in rust-lang/rust#49150?

@RalfJung
Copy link
Member

@clarcharr Yeah probably.

bors pushed a commit to rust-lang/rust that referenced this pull request May 8, 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)
bors added a commit to rust-lang/rust 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
@Centril Centril added A-pinning Proposals relating to pinning. A-types-libstd Proposals & ideas introducing new types to the standard library. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pinning Proposals relating to pinning. A-types-libstd Proposals & ideas introducing new types to the standard library. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.