Skip to content

Phantom variance ZSTs #488

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

Closed
jhpratt opened this issue Nov 16, 2024 · 13 comments
Closed

Phantom variance ZSTs #488

jhpratt opened this issue Nov 16, 2024 · 13 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@jhpratt
Copy link
Member

jhpratt commented Nov 16, 2024

Proposal

Problem statement

Expressing the intended variance of a generic type or lifetime is possible but difficult to understand without additional documentation.

PhantomData<&'a ()> // covariant in 'a
PhantomData<fn(&'a ())> // contravariant in 'a
PhantomData<fn(&'a ()) -> &'a ()> // invariant in 'a

PhantomData<T> // covariant in T
PhantomData<fn(T)> // contravariant in T
PhantomData<fn(T) -> T> // invariant in T

Motivating examples or use cases

The previous types are used in a number of places, including in the standard library. Many such uses are not documented as to the meaning or intent.

Solution sketch

Add the following to core::marker, which is derived from the type_variance crate.

pub struct PhantomCovariant<T: ?Sized>(PhantomData<fn(T)>);
pub struct PhantomInvariant<T: ?Sized>(PhantomData<fn(T) -> T>);
pub struct PhantomContravariant<T: ?Sized>(PhantomData<fn(T)>);
pub struct PhantomCovariantLifetime<'a>(PhantomCovariant<&'a ()>);
pub struct PhantomInvariantLifetime<'a>(PhantomInvariant<&'a ()>);
pub struct PhantomContravariantLifetime<'a>(PhantomContravariant<&'a ()>);

// later trait impl(self) Variance {}, utilizing restrictions
pub trait Variance: Sealed {}
impl<T: ?Sized> Variance for PhantomCovariant<T> {}
impl<T: ?Sized> Variance for PhantomInvariant<T> {}
impl<T: ?Sized> Variance for PhantomContravaiant<T> {}
impl<T: ?Sized> Variance for PhantomCovariantLifetime<T> {}
impl<T: ?Sized> Variance for PhantomInvariantLifetime<T> {}
impl<T: ?Sized> Variance for PhantomContravaiantLifetime<T> {}

pub fn variance<T: Variance>() -> T {}

All deriveable traits will be implemented. The Debug implementation should output the variance of T using core::any::type_name.

Alternatives

  • Leave this as a crate and don't include it in core.
  • Create new lang items (or use a hack similar to ghost) to permit constructing the values directly. This does not need to be done now, as starting with the above is forward compatible with later making it a lang item.

Links and related work

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@jhpratt jhpratt added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Nov 16, 2024
@scottmcm
Copy link
Member

TBH, I never remember which one's Covariant and which one's Contravariant, so hopefully we could find better names for those.

I find C#'s in/out much easier to get right.

Invariant and Lifetime make good sense to me, though.

@programmerjake
Copy link
Member

what if they were named value, fnarg, and invariant (or some similar scheme)? that could help...
I think the problem with in/out is that for things you're not sticking in a function type, you generally want out, but that name has no obvious connection to just passing a value around.

@jhpratt
Copy link
Member Author

jhpratt commented Nov 16, 2024

Value and Argument/FnArgument work for me if people think it's clearer. I'm not sure whether they should be prefixed with something (e.g. PhantomValue) for clarity, however.

@programmerjake
Copy link
Member

Value and Argument/FnArgument work for me if people think it's clearer. I'm not sure whether they should be prefixed with something (e.g. PhantomValue) for clarity, however.

we could just use PhantomData directly instead of PhantomValue. so:

pub struct PhantomData<T: ?Sized>;
pub struct PhantomArg<T: ?Sized>(PhantomData<fn(T)>);
pub struct PhantomInvariant<T: ?Sized>(PhantomData<(fn(T), T)>);
pub struct PhantomLifetime<'a>(PhantomData<&'a ()>);
pub struct PhantomArgLifetime<'a>(PhantomArg<&'a ()>);
pub struct PhantomInvariantLifetime<'a>(PhantomInvariant<&'a ()>);

@ChayimFriedman2
Copy link

I'd prefer that they will get some language support, so to instantiate one I can write e.g. Convariant (like PhantomData) instead of Covariant::new().

@jhpratt
Copy link
Member Author

jhpratt commented Nov 16, 2024

Of course PhantomData could be used instead of PhantomValue 🤦 I think it would be best to keep the Lifetime type separate for simplicity, especially if they ultimately become language items.

As to whether they should be lang items from the start, it is forward compatible to not do this initially. That's not to mention that it's a libs ACP, though T-lang could certainly weigh in unofficially if they wanted.

@Amanieu
Copy link
Member

Amanieu commented Nov 19, 2024

We discussed this in the libs-api meeting. We felt that this is useful to have and would make code clearer to read. However we would prefer names closer to the type_variance crate. Specifically, we are considering the following API:

pub struct PhantomCovariant<T: ?Sized>(PhantomData<fn(T)>);
pub struct PhantomInvariant<T: ?Sized>(PhantomData<fn(T) -> T>);
pub struct PhantomContravariant<T: ?Sized>(PhantomData<fn(T) -> T>);
pub struct PhantomCovariantLifetime<'a'>(PhantomCovariant<&'a ()>);
pub struct PhantomInvariantLifetime<'a'>(PhantomInvariant<&'a ()>);
pub struct PhantomContravariantLifetime<'a'>(PhantomContravariant<&'a ()>);

We felt that having explicit names with the concept variance was the clearest terminology. Someone writing unsafe code should read the documentation for these types which clearly explains the implications of each of these kinds of variance.

The variant function can be avoided if we allow these types to be constructed as unit struct, just like PhantomData.

@jhpratt
Copy link
Member Author

jhpratt commented Nov 19, 2024

Specifically, we are considering the following API:

I'll update the issue to this effect.

Someone writing unsafe code should read the documentation for these types which clearly explains the implications of each of these kinds of variance.

That's certainly reasonable.

The variant function can be avoided if we allow these types to be constructed as unit struct, just like PhantomData.

(assuming you mean variance)

Is this also desired? I can look into how ghost works and potentially emulate it so as to avoid adding lang items.

@cuviper
Copy link
Member

cuviper commented Nov 19, 2024

Couldn't they just be type aliases to allow unit construction? I think it would still be strongly-typed enough with the type parameter.

@programmerjake
Copy link
Member

Couldn't they just be type aliases to allow unit construction?

you can't construct using a type alias unless you use struct literal syntax (Struct {}): https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8e41c93116bd91ed44af97d7a119fd88
you need to also have a const with the same name.

@jhpratt
Copy link
Member Author

jhpratt commented Nov 21, 2024

@Amanieu Given the specific design was brought up in the team meeting, is this considered accepted? I ask because it wasn't explicitly stated (and closed) as is typically the case.

@Amanieu
Copy link
Member

Amanieu commented Nov 22, 2024

Given that we proposed quite a significant change from what was proposed, we wanted to give you a change to respond if you had any feedback. But otherwise yes, you can consider this accepted.

@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Nov 22, 2024
@jhpratt
Copy link
Member Author

jhpratt commented Nov 23, 2024

What I originally proposed was far closer; I had adjusted it based on feedback. The only difference was that I originally wanted to uplift the crate directly (such that Lifetime was its own struct). I don't have strong opinions and more wanted something. Closing as accepted.

@jhpratt jhpratt closed this as completed Nov 23, 2024
jhpratt added a commit to jhpratt/rust that referenced this issue Jan 27, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 27, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 27, 2025
Rollup merge of rust-lang#135807 - jhpratt:phantom-variance, r=Amanieu

Implement phantom variance markers

ACP accepted rust-lang/libs-team#488

Tracking issue rust-lang#135806
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this issue Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants