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

Always implement Unpin for PhantomData #62786

Closed
wants to merge 1 commit into from

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jul 18, 2019

This PR makes it so that PhantomData<T> implements Unpin no matter what T is.

Interestingly enough, the situation is the same for Send and Sync. PhantomData only implements Send if T: Send and Sync if T: Sync. I don't know whether that's on purpose, or if it's an overlook.

@rust-highfive
Copy link
Collaborator

r? @aidanhs

(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 Jul 18, 2019
@seanmonstar
Copy link
Contributor

It's on purpose for Send and Sync. A common pattern is PhantomData<Rc<()>> to ensure a type is !Send.

In my opinion, it's probably better to keep the same with Unpin, in case someone were to expect the same rules.

@Mark-Simulacrum
Copy link
Member

r? @cramertj perhaps? Not sure who the "owners" in this space are today.

@rust-highfive rust-highfive assigned cramertj and unassigned aidanhs Jul 18, 2019
@nagisa
Copy link
Member

nagisa commented Jul 19, 2019

It's on purpose for Send and Sync.

Is it? Where was that ratified? Do we have tests to ensure this continues to hold?

A common pattern is PhantomData<Rc<()>> to ensure a type is !Send.

Seems like an invalid pattern to me, at least in vacuum. It is not semver breaking to add additional impls for PhantomData because it is Send, Sync and Unpin ∀T (by the virtue of being a ZST without any attached semantics) but doing so would potentially make unsafe code using this pattern unsound.

That being said, if it does have a widespread usage and this behaviour is not yet ratified in e.g. documentation, we may want to ratify it sooner rather than later.

@nagisa nagisa added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jul 19, 2019
@seanmonstar
Copy link
Contributor

Here's the first one I found in libstd:

_dont_send_or_sync_me: PhantomData<*mut ()>,

@tomaka
Copy link
Contributor Author

tomaka commented Jul 19, 2019

It's worth mentioning the PhantomPinned type whose purpose is to not implement Unpin.
People are expected to use PhantomPinned rather than PhantomData<SomethingUnpin>.

@nagisa
Copy link
Member

nagisa commented Jul 19, 2019

Here's the first one I found in libstd

libstd is not a typical codebase, because it can assume significantly more than any other rust codebase and use all the unstable details it wants.

Either way, I think we either document/ratify PhantomData<T> to behave just like T for the purposes of propagating autotraits (so the current behaviour for Send, Sync and Unpin and we do not land this PR) or we go ahead and implement Send, Sync, Unpin and all similar auto traits for PhandomData<T> by considering behaviour of the type regardless of its generic.

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2019
@Dylan-DPC-zz
Copy link

@tomaka @aidanhs @rust-lang/libs any updates?

@tomaka
Copy link
Contributor Author

tomaka commented Aug 6, 2019

I guess we can close as wont-fix.

@tomaka tomaka closed this Aug 6, 2019
@tomaka tomaka deleted the unpin-phantomdata branch August 6, 2019 06:46
@Dylan-DPC-zz Dylan-DPC-zz removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants