dtolnay / request-for-implementation

Crates that don't exist, but should
613 stars 6 forks source link

Library for writing const-compatible phantom types with explicit variance and autotrait behavior #21

Closed dtolnay closed 5 years ago

dtolnay commented 5 years ago

When writing data structures using unsafe code or otherwise unused type parameters, library authors often reach for PhantomData\<T> without adequate consideration.

pub struct IterEmpty<T>(PhantomData<T>);

impl<T> Iterator for IterEmpty<T> {
    type Item = T;
    fn next(&mut self) -> Option<Self::Item> {
        None
    }
}

You might have missed it (as the standard library did too) that PhantomData\<T> is not the right phantom type here. For the empty iterator the phantom type should be Send and Sync regardless of whether T is Send and Sync, as no T ever exists.

The canonical covariant-always-Send-and-Sync type in the standard library is fn() -> T so a library author who realizes PhantomData\<T> is not right would write:

pub struct IterEmpty<T>(PhantomData<fn() -> T>);

This is still a bad choice because function pointers can't exist under the conservative const fn feature gate.

impl<T> IterEmpty<T> {
    pub const fn new() -> Self {
        IterEmpty(PhantomData)
    }
}
error: function pointers in const fn are unstable
   --> src/libcore/iter/sources.rs:277:15
    |
277 |     IterEmpty(PhantomData)
    |               ^^^^^^^^^^^

There are hacks; the next step would be

pub struct IterEmpty<T>(PhantomData<PhantomFnWorkaround<T>>);
struct PhantomFnWorkaround<T>(fn() -> T);

and this is ideal in all ways except that it is:

I want a library for emitting correct const-compatible phantom types (like the last correct solution above) that puts variance and autotrait behavior front and center for readers and reviewers.

pub struct IterEmpty<T>(phantom_data![T, Covariant + AlwaysSendSync]);

It should support any combination of {Covariant, Contravariant, Invariant} × {AlwaysSendSync, NeverSendSync, InheritSendSync}.

Syntax alternative:

#[phantom(Covariant, AlwaysSendSync)]
pub struct IterEmpty<T>(PhantomData);
// expands to
pub struct IterEmpty<T>(PhantomData<$the_crate::CovariantAlwaysSendSync<T>>);
djmcgill commented 5 years ago

Does ghost suit these needs? It has support for all 3 variances and is zero sized. The only thing missing is support for Send/Sync control - it always inherits.

I did also mess around with a solution involving type aliases instead of macros: https://github.com/djmcgill/rich_phantoms/blob/master/src/lib.rs I implemented the tests as doc tests so I get access to compile_fail, if there's a runtime way that a test can fail on a failed cast to a trait object please let me know!

A third option would be a hybrid approach - using ghost to avoid type aliases but having a bunch of different types to control send/sync inheritance - that way you don't need to add to ghost itself at all. That way you have no macros (other than the ones internal to ghost) and don't expose the implementation details like type alises do.

edit: I just saw that you are the author of ghost! That's embarrassing, I guess you already have done most of the work for this one?

dtolnay commented 5 years ago

Thanks for checking in. I think the ghost crate is distinct from this design. The point of ghost is making types that behave like PhantomData (possibly with modified variance) but that are not PhantomData. If a use case requires a phantom type that is not PhantomData (for example because they want to add trait impls or inherent methods, like in inventory::iter::<T>) then they need to use ghost. But the vast majority of use cases including the one in https://github.com/rust-lang/rust/pull/57682 are going to want to use exactly PhantomData itself.

It looks like you are on the right track. For tests I would recommend investing in getting compiletest_rs set up. Check out ref-cast/compiletest for a basic setup.

Also please have a test where you enable feature(staged_api) like in the playground link in https://github.com/rust-lang/rust/pull/57682#issuecomment-455017809 and confirm that this implementation is still going to work with those restrictions.

One minor downside to implementing this as type aliases is that people will be tempted to write them in expression position like they sometimes do with PhantomData:

let x = PhantomCovariantAlwaysSendSync::<&'static str>;
error[E0423]: expected value, found type alias `PhantomCovariantAlwaysSendSync`
 --> src/lib.rs:10:13
  |
7 |    let _x = PhantomCovariantAlwaysSendSync::<&'static str>;
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: can't use a type alias as a constructor

One thing I liked about the two proposals above is that there is no affordance for doing it wrong. I think the first one can even be made to work naturally in expression position:

pub struct IterEmpty<T>(phantom_data![T, Covariant + AlwaysSendSync]);

// Expands to PhantomData::<...>. Expands to the same thing in type position on
// the previous line but the double colon will be ignored up there.
let x = phantom_data![T, Covariant + AlwaysSendSync];
#[phantom(Covariant, AlwaysSendSync)]
pub struct IterEmpty<T>(PhantomData);

// It's clear that we are only allowed to write:
let x = IterEmpty(PhantomData);

// or:
let x = IterEmpty::<T>(PhantomData);
dtolnay commented 5 years ago

On second thought there is a lot to be said for keeping it simple -- I like that your implementation ends up being practically no code at all. That could be an advantage if you're interested in getting these into the standard library (as a private implementation detail for using in private fields of types in libcore and libstd).

djmcgill commented 5 years ago

Okay, how does it look now? https://github.com/djmcgill/rich_phantoms

All of the variants (heh) implemented, compile-tests for pass and fail (the only cases I didn't cover fully are verifying that, for example, the covariant ones aren't contravariant), and a test that uses

#![feature(staged_api)]
#![feature(const_fn)]

but I had to add -Z force-unstable-if-unmarked (or explicitly mark the test fn as unstable), I'm not sure if that still suits the needs or not. Note that this test only runs with cargo test --features unstable-test so that it doesn't break stable.

I had a few thoughts about needing let x: PhantomCovariantInheritSendSync = PhantomData and I see two quick options:

  1. A series of macros phantomCovariantInheritSendSync! which just expand to PhantomData, or
  2. A trait per variant with a typed new method, and then impl that trait for phantom data like:
    trait Foo { fn bar() {} }
    impl<T> Foo for PhantomData<T> {}
    PhantomCovariantAlwaysSendSync::<()>::bar();

    I'm not 100% sure it's necessary though: this library is so small and so specialised that I don't think it's unresonable to expect people to remember that they instantiate with just PhantomData itself. It will be documented on all the types and in the readme.

dtolnay commented 5 years ago

Very nice. I agree it's not necessary to support writing the type parameters in expression position. The readme and crate-level rustdoc will need to show the intended usage.

Some of the structs and the unsafe impls are missing support for T: ?Sized, like PhantomContravariantInheritSendSync<str>.

The FnWorkaround types should be #[doc(hidden)].

Once you get this documented and published to crates.io, I will close out this issue and mark it off the list. Thanks!

djmcgill commented 5 years ago

Here we go! https://crates.io/crates/rich-phantoms

dtolnay commented 5 years ago

Looks good. I filed some follow-up issues but I am calling this done. Thanks!