-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
Original text said |
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 |
Here's the marker trait hierarchy that I made with graphviz for helping visualize it:
I'm fine with this. To be clearer about my intent, what I'm interested in doing is someday saying |
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. |
Glad to answer your questions. :-)
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.
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
The static string optimization is discussed in the RFC.
I don't know what this means.
|
@bill-myers: As @withoutboats mentions, |
There was a problem hiding this 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 =)
text/0000-pin_and_move.md
Outdated
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/insafety/unsafety
text/0000-pin_and_move.md
Outdated
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strike an optimization?
text/0000-pin_and_move.md
Outdated
|
||
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 |
There was a problem hiding this comment.
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 bothstd::mem
andcore::mem
.
text/0000-pin_and_move.md
Outdated
### 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 |
There was a problem hiding this comment.
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/
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? |
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. |
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:
|
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? |
Doesn't the arbitray self type need another RFC before this? |
Just to make sure I understand: |
text/0000-pin_and_move.md
Outdated
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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?
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?
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. |
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. |
Very nice RFC. Covers a lot of ground, cleanly written. I definitely prefer this to both 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(); // ?
Question: Well, you state later "Move really means that its safe to convert between "If Ah, I think I see some of the complication here. In a way, It makes sense, but I agree it might be good to change the name I have to wonder if a name like Observation: That actually makes me wonder: maybe conversion from 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
Question: By this, do you mean that we might add some coherence feature in the future to causes said code to stop compiling?
Question: Should this be a trait alias? It doesn't seem to provide any additional guarantees of its own, right?
Question: Is there a need for this stronger form of
Question: I wonder what the relationship between Nit: I would find it clearer to state this in a positive way: "The I'm not saying this isn't equivalent; just that
Observation: This is going to put more pressure on having user-land types that can be "reborrowed", I imagine.
Observation: Similarly, I wonder if this will put pressure on having a some kind of "auto-pin" so that
Nit: This is not shown in the RFC, is it? |
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). |
On a related note, something I have often wanted is a type like Anyway, my point with all of this is that you could also have a method like 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 |
I think we should go further, and forbid that behavior. Basically say that This raises an interesting point concerning 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 This latter question is more a problem for Unsafe Code Guidelines, but it's interesting to think about. |
It seems like I... also proposed that at some point. :P #1283 |
@Diggsey I don't believe you need to dereference to immutable pin; you can just dereference to an immutable reference. |
@withoutboats but that loses the information that the type is pinned. It seems like I should be able to separate out the 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 |
Thinking about it more, there's actually two possible variations of the
In this variation,
In this variation, |
What exactly do you mean by "mapping" here? |
@RalfJung the |
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 |
@Diggsey Either way...
This seems plausible. I was starting with the existing |
I think this is only true for In my first example, you would be able to obtain an I think both
With the generator example, it would be unsafe to map an However, it would be safe to map a |
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 |
I see... So does this mean that 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. |
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
That would be rather inconsistent. Making a field |
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
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 |
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
You mean, something like |
You could use |
oops somehow I didn't think of However, obtaining an
Agreed. And even with how I think the difference between what you proposed and what I expected is only in what I have to say I find your POV very surprising, for two reasons:
For these reasons, I see very little reason to declare "You can go from |
@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 😛 |
@Diggsey @RalfJung considering how this RFC has already been merged, shouldn't this discussion be happening in rust-lang/rust#49150? |
@clarcharr Yeah probably. |
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)
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
Rendered