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

ManuallyDrop should be an object-safe receiver type #64351

Open
withoutboats opened this issue Sep 10, 2019 · 17 comments
Open

ManuallyDrop should be an object-safe receiver type #64351

withoutboats opened this issue Sep 10, 2019 · 17 comments
Labels
F-arbitrary_self_types `#![feature(arbitrary_self_types)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@withoutboats
Copy link
Contributor

withoutboats commented Sep 10, 2019

We should extend the set of library-defined self types on stable to include types involving ManuallyDrop. ManuallyDrop is a transparent wrapper around another type which is completely unconstrained; there should be no technical limitations on making it an object-safe receiver type.

To be explicit, I am proposing to make the following types valid self types on stable, and all but the first would be object safe:

  • ManuallyDrop<Self>
  • &ManuallyDrop<Self>
  • &mut ManuallyDrop<Self>
  • Box<ManuallyDrop<Self>>
  • Rc<ManuallyDrop<Self>>
  • Arc<ManuallyDrop<Self>>
  • Pin<&ManuallyDrop<Self>>
  • Pin<&mut ManuallyDrop<Self>>
  • Pin<Box<ManuallyDrop<Self>>>
  • Pin<Rc<ManuallyDrop<Self>>>
  • Pin<Arc<ManuallyDrop<Self>>>

This is similar to but distinct from #56193, which is about making coercions valid involving a manually drop wrapped around a pointer type.

In particular, I am personally interested in having Pin<&mut ManuallyDrop<Self>> as an object-safe receiver type for an unsafe trait method in my code, as it assists users in understanding the guarantees the caller is giving them - that it will never access or move this value again, even to drop it, do with that what you will.

cc @mikeyhew @rust-lang/lang @rust-lang/wg-unsafe-code-guidelines

@withoutboats withoutboats added F-arbitrary_self_types `#![feature(arbitrary_self_types)]` T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 10, 2019
@withoutboats
Copy link
Contributor Author

(Note: I believe for now, the way to express the method I'm interested in so that it is object safe is self: Pin<&mut Self> and document that it is safe to ptr::read from Self if Self: Unpin, but I'm not 100% clear on whether its actually safe to ptr::read from Self with the way unsafe code guidelines are shaking out.)

@comex
Copy link
Contributor

comex commented Sep 10, 2019

👎 . We should focus on stabilizing arbitrary_self_types rather than giving the standard library more special privileges.

@mikeyhew
Copy link
Contributor

I'm not sure I agree with @comex, I don't think adding another special case would get in the way of stabilizing arbitrary_self_types. But I do get his/her/their frustration. All of the above listed receiver types are usable with the arbitrary_self_types feature, and they are object-safe. I don't necessarily think that the entire feature should be stabilized in one go, but we should work toward getting an RFC merged and stabilizing parts of it.

If we do add ManuallyDrop as a special case, I'm pretty sure all we have to do is implement the Receiver trait for it.

@withoutboats
Copy link
Contributor Author

These things don't block one another - stabilizing the ability for users to define custom receivers doesn't have any kind of ordering with which std types can be custom receivers. We've already given std the special privilege to use this feature beyond what other crates can do (as we have with many other features), these types were just overlooked from the set we stabilized for no particular reason. Blocking progress in one way to create pressure to make progress in another is filibuster logic.

Thanks for pointing out that its as easy as adding impls @mikeyhew!

@comex
Copy link
Contributor

comex commented Sep 11, 2019

The reason I care about ordering is that every new std-exclusive feature makes it harder to migrate from std functionality to alternatives.

In fact, the ordering is the only aspect of these features I care about. I'm not personally interested in arbitrary_self_types for its own sake. Rather, my sole interest is in minimizing the amount of std-exclusive functionality. Stabilizing arbitrary_self_types would reduce it; adding this would increase it.

That said, this is admittedly only a small extension to an existing set of functionality. If it were up to me, the existing set wouldn't have been stabilized until arbitrary_self_types was ready, but it's obviously not up to me. :)

@mikeyhew
Copy link
Contributor

If we do add ManuallyDrop as a special case, I'm pretty sure all we have to do is implement the Receiver trait for it.

I just realized that's not true. Right now, if the feature flag is not enabled, we require that the receiver type implements Receiver + Deref<Target=Self>. &ManuallyDrop<Self> implements Deref<Target=ManuallyDrop<Self>> and does not work. I had a PR (#55880) that allowed composed receiver types like &&Self in stable, but we ended up going with these more strict requirements in #56805. Allowing composed receivers in general would probably be the easiest way to allow &ManuallyDrop<Self>. To allow &ManuallyDrop<Self> but not allow &&Self would be more challenging and probably not worth it.

@withoutboats
Copy link
Contributor Author

@mikeyhew is it challenging to implement the work for composed receivers but not stabilize them? I would think that's just a matter of the Receiver impls. Composed receivers are a bit more uncertain to mebecause they make the set of receivers infinite with absurdity like self: &&&mut &Self

@cramertj
Copy link
Member

I'm in the process of working to stabilize composed receivers in #64325

@nikomatsakis
Copy link
Contributor

I personally have no objection to extending the set of stabilized types a bit farther. That said, I would appreciate a few more details on the proposed use cases.

Otherwise:

  • a stabilization write-up, of course, which would mostly be a small extension to what @withoutboats has written but with some added test cases
  • we should of course have such test cases
  • and coverage in the rust reference

would all seem to be in order.

Seems like it might also make sense to combine this effort with #64325?

@RalfJung
Copy link
Member

What are the rules a type has to satisfy to be a "well-behaved" object-safe receiver type? Is there a "checklist" such that we can be sure a type that checks all the boxes is good to go as objcect-safe receiver type? This should be documented somewhere persistently, IMO before we go on stabilizing more of those. I am, naturally, particularly interested in any interaction with unsafe code and guarantees it would have to promise to uphold.

Yes, such docs work is not a lot of fun, but it is important (and with Pin, when we all agreed we'd improve the docs before stabilization, nobody did it... so based on that experience, I think such docs should be a blocker for landing stabilizations).

@withoutboats
Copy link
Contributor Author

withoutboats commented Sep 18, 2019

What are the rules a type has to satisfy to be a "well-behaved" object-safe receiver type? Is there a "checklist" such that we can be sure a type that checks all the boxes is good to go as objcect-safe receiver type?

Being represented as a pointer to the type and (transitively) safely dereferencing to it. There are some additional traits involved. I agree the present situation is not great, but I think this has to do with stabilizing the arbitrary_self_types feature much more than with extending the set of std-privileged types.

@RalfJung
Copy link
Member

Being represented as a pointer to the type and (transitively) safely dereferencing to it.

ManuallyDrop is not a pointer type though.
How does it determine how many indirections to take? Like, ManuallyDrop<&Self> and Box<&Self> look "syntactically similar" but are obviously different in terms of representation.

I agree the present situation is not great, but I think this has to do with stabilizing the arbitrary_self_types feature much more than with extending the set of std-privileged types.

Well, we better make sure the std-privileged types we add are actually safe. That's hard to do until we have a proper handle of what that actually means.

@withoutboats
Copy link
Contributor Author

withoutboats commented Sep 18, 2019

@RalfJung yes, ManuallyDrop<Self> is not object safe as a receiver just like self is not.

There are types we have held off on adding because they would involve making UCG related decisions - nonnull and raw ptrs, for example, because we haven't determined if the vtable pointer needs to be valid and how that relates to calling trait object methods, etc. Which is why the requirements now are narrowly types that we are confident would guarantee a valid vtable and data pointer (which includes ManuallyDrop).

@RalfJung
Copy link
Member

Oh, and receiver types don't "nest", right? So all of the ones you are listing in the OP are exactly one pointer indirection.

However, Rc and Arc do not dereference to &self -- at least not in the sense of the MIR-level * operator. Or do you mean that the Deref impl is called?

@withoutboats
Copy link
Contributor Author

@RalfJung yea, I meant the deref impl, which in those cases has to also do the offset past the reference counts.. In terms of nesting, currently they don't, but cramertj has a PR to allow them, though any that are double indirection (or more) would not be object safe.

My understanding is that if the type guarantees that Self is valid, there shouldn't be an intersection with UCG. That's why we've excluded types that don't necessarily guarantee that so far.

@RalfJung
Copy link
Member

though any that are double indirection (or more) would not be object safe.

Why that?

My understanding is that if the type guarantees that Self is valid, there shouldn't be an intersection with UCG.

On first sight, that sounds like a reasonable assumption.

@withoutboats
Copy link
Contributor Author

Why that?

I think its just an implementation limitation on how we access the vtable right now, @mikeyhew would know more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-arbitrary_self_types `#![feature(arbitrary_self_types)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants