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

RFC: Ghost Busting #2357

Closed
wants to merge 16 commits into
base: master
from

Conversation

@Centril
Copy link
Contributor

Centril commented Mar 4, 2018

πŸ–ΌοΈ Rendered

πŸ“ Summary

  1. Defines unused type parameters to act as if they are logically owned and covariant.

  2. Introduces phantom T pseudo-fields which improves the ergonomics of changing
    variance and drop checking behavior when it is needed.
    A phantom T field also has
    the same behavior with respect to auto traits as PhantomData<T> does.

  3. PhantomData<T> is redefined as struct PhantomData<T: ?Sized>; and deprecated.

  4. The lang item phantom_data is removed.

  5. Derive macros for derivable standard library traits will take advantage of statically known
    phantom types and fields to generate more permissive impls.

That's it folks; Happy πŸ‘»πŸ‘€!

πŸ’– Thanks

Thanks to @nikomatsakis and @eddyb for interesting discussions.

@Centril Centril added the T-lang label Mar 4, 2018

While `phantom` fields are not nameable, they permit visibility modifiers on
them. The default visibility of a `phantom` field is private as with other
fields. Therefore, having a type definition with no fields but private `phantom`
fields will cause other modules to not being able to construct the type.

This comment has been minimized.

@Havvy

Havvy Mar 4, 2018

Contributor

not be able*

constructed unless the "field" is visible in the module in question.
This property is crucial because if `phantom` fields always were public, then
the a value of type `Id<A, B>` where `A != B` could be constructed with `Id {}`

This comment has been minimized.

@Havvy

Havvy Mar 4, 2018

Contributor

This specific instance could (and should) be fixed using the non_exhaustive attribute. Do you have a better use case for privacy in phantom fields?

This comment has been minimized.

@Centril

Centril Mar 4, 2018

Author Contributor

The documentation of #[non_exhaustive] says:

Structs marked as non_exhaustive will not be able to be created normally outside of the defining crate.

Essentially, this is the same as pub(crate).

Allowing for more fine grained visibility as pub(super) is more general and lets you control visibility on level not permitted by #[non_exhaustive]. I also believe that pub(crate) is a lot more clear than #[non_exhaustive] wrt. intent and what it means. I think that #[non_exhaustive] should be used for when you actually intend to add more fields or are unsure about that while visibility (pub, ..) should be used for controlling who can see what.

This comment has been minimized.

@notriddle

notriddle Mar 28, 2018

Contributor

It's not like you couldn't work around it by writing

struct Id<A, B> { phantom A, phantom B, _priv: () }

It really makes no sense for phantom fields to be subject to visibility checks. You can always add a unit field, and if that's not acceptable, then the problem can be fixed in a more general way.

This comment has been minimized.

@Centril

Centril Mar 28, 2018

Author Contributor

The idea of using a _priv field is discussed in the alternatives section. You say that there is no reason for phantom fields to be subject to visibility, but a _priv field will need to be passed in struct literals, making things less ergonomic, so there are reasons to make phantom fields subject to visibility. It also increases uniformity of fields to do so.

This comment has been minimized.

@alercah

alercah May 4, 2018

Contributor

It seems to me that phantom () is a more natural way to force a private field, than requiring it be named anyway.

Respecting privacy also allows us to encode the [following idiom][screeps_api]:
```rust
pub struct Thing {

This comment has been minimized.

@Havvy

Havvy Mar 4, 2018

Contributor

Also should be using the non_exhaustive attribute.

This comment has been minimized.

@Centril

Centril Mar 4, 2018

Author Contributor

Hmm yes, in this case I'm inclined to agree... However on a second look, the example was just wrong and didn't add anything, so I just removed it =)

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Mar 4, 2018

My first thought here is that the RFC is missing a section convincing me that unused type parameters and phantom data are such a common problem that adding special syntax is justified. For example, doesn't this force macros that consume struct definitions to understand the special phantom pseudo-keyword?

In particular, I'm not convinced that phantom: ::std::marker::PhantomData<T> is so substantially worse than phantom T would be. Certainly if (1) is a big issue, then one could argue for prelude inclusion instead -- PhantomData is a pretty unique name -- for far less impact than new syntax.

(3) acknowledge the field by adding an expression PhantomData when a value of the type is to be constructed.

This one's more interesting to me, but I think it's largely applicable to many ZSTs, not just PhantomData. Like if I make a pub struct Foo(()); to keep it from being externally constructed, I need to pass the (). There are a bunch of possible alternatives that would work for this, like a trait on types that are only ever set to their default, an attribute on the field to set to default, field initializers, allowing them to be unnamed (like #2102 since the type has no fields, and thus "disappears" other than its type), etc.

More permissive automatic deriving

Wouldn't your #2353 handle this just fine? And I think it needs a more realistic example than struct Label<T>;, since it seems that one would be handled just fine by type Label<T> = PhantomData<T>; And the Vec example certainly can't derive most of its traits.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Mar 4, 2018

For example, doesn't this force macros that consume struct definitions to understand the special phantom pseudo-keyword?

For all practical intents and purposes, yes.

In particular, I'm not convinced that phantom: ::std::marker::PhantomData<T> is so substantially worse than phantom T would be.

I believe that (3) makes it substantially worse. If we can provide a good and alternative way to solve (3) together with (1) (with prelude inclusion..), then I believe they are on par.

Certainly if (1) is a big issue, then one could argue for prelude inclusion instead -- PhantomData is a pretty unique name -- for far less impact than new syntax.

That's a good and cheap alternative, but it does not solve 2. and 3. (the latter is the most annoying part to me..). I've added this suggestion to the list of alternatives.

a bunch of possible alternatives that would work for this, like a trait on types that are only ever set to their default, an attribute on the field to set to default, field initializers, allowing them to be unnamed (like #2102 since the type has no fields, and thus "disappears" other than its type), etc.

Can you expand on these?

Wouldn't your #2353 handle this just fine?

It would indeed, but it would be less automatic since you'd have to acknowledge the phantomness doubly on the type parameter(s) (or the type) and with a PhantomData<P> field. In cases where you don't need phantom T to change variance, the advantage of struct Label<T>; is 0 steps vs. 2 steps with PhantomData<T> + #[derive_no_bound].

What I like about unused parameters => phantom P is that they together form a neat consistent package.

and covariant.

2. Introduces `phantom T` pseudo-fields which improves the ergonomics of
changing [variance] and [drop checking] behavior when it is needed.

This comment has been minimized.

@matthewjasper

matthewjasper Mar 4, 2018

And auto trait implementations?

This comment has been minimized.

@Centril

Centril Mar 4, 2018

Author Contributor

Elaborate?

This comment has been minimized.

@matthewjasper

matthewjasper Mar 4, 2018

PhantomData<T> only implements auto traits that T does. This is used in fmt to opt out of all auto traits.

This comment has been minimized.

@Centril

Centril Mar 4, 2018

Author Contributor

Today I learned... Interesting. I'll write something about it when I'm more lucid =P

This comment has been minimized.

@Centril

Centril Mar 5, 2018

Author Contributor

I've updated the RFC accordingly now.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Mar 4, 2018

but it does not solve 2

I think that "add a pseudo-field using phantom" and "add a read field using PhantomData" are essentially the same for (2). But of course I like the "you have to mention all your parameters somehow to say what you wanted" behaviour, so I'm sure we have different perspectives here.

Can you expand on these?

Sure, though be warned this is an unfiltered splat of things -- I haven't thought about the details for most, and some of them might just be just terrible.

  • trait ImplicitDefault : Default {} and impl ImplicitDefault for (), then struct initializers add x: ImplicitDefault::default() for any field x that wasn't specified. (Could also change elseless-if desugar to else { ImplicitDefault::default() }, but that's a different conversation.)
  • struct Foo(#[always_default] ());, allowed only on a suffix of the fields for tuple-structs, prohibits providing a value in initializers and always uses Default::default()
  • struct Foo(() = ());, along the lines of #1806
  • struct Foo { _: () }, the struct still contains the type, but initializing and reading "inlines" the fields -- all zero of them -- so there's nothing to need to specify in the initializer.

```rust
struct MyVec<T> {
phantom T,

This comment has been minimized.

@leodasvacas

leodasvacas Mar 4, 2018

Is writing phantom T here optional?

This comment has been minimized.

@Centril

Centril Mar 4, 2018

Author Contributor

No, phantom T is required to let dropck know that T is logically owned, which is not the case if it only sees data: *const T. I will clarify.


```rust
#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct Label<T>;

This comment has been minimized.

@leodasvacas

leodasvacas Mar 4, 2018

Here it seems phantom T is indeed optional.

This comment has been minimized.

@Centril

Centril Mar 4, 2018

Author Contributor

Yep, that's right.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Mar 4, 2018

@scottmcm Ah yes, (2) only goes away morally when you don't have to acknowledge unused type parameters.

But of course I like the "you have to mention all your parameters somehow to say what you wanted" behaviour, so I'm sure we have different perspectives here.

I guess the central question wrt. eliding unused parameters is if we believe that we help or hurt users substantially by not/throwing a "you have to use the parameter and think about variance always" error in their face.

When not dealing with unsafe code, the importance of invariance/not shouldn't even arise and owned covariance is the right thing; I think this is very common due to "unconstrained type parameters" when trying to impl a trait, so you add a type param and now you have to add PhantomData also. This seems quite the unergonomic experience when zero unsafe code is involved. Personally I think we should avoid penalizing safe code for the worries of unsafe code.

I'd also like to note that even if you get "unused type parameter" thrown in your face, some people might be inclined just to casually add PhantomData<P> (assuming P is the type parameter) without giving it much thought. Furthermore, variance is non-trivial and incredibly easy to get wrong even if you think about it, a case in point is:

trait ImplicitDefault : Default {}

I think that's essentially https://github.com/Centril/rfcs/blob/rfc/ghost-busting/text/0000-ghost-busting.md#alternative-allow-filling-unspecified-fields-with-default
I also got the idea from #1806.

struct Foo(#[always_default] ());

Seems like a variation on ImplicitDefault. Interesting, but seems quite niche tho.

struct Foo { _: () }

I think this was @eddyb's idea? This would be: struct Foo<T> { _: PhantomData<T> } and work for any ZST?

@leodasvacas

This comment has been minimized.

Copy link

leodasvacas commented Mar 4, 2018

I see two possible intents of a user that gets the unused type parameter error:

  1. They forgot to use it (forgot to add a field) or removed the use (removed the field) and forgot to remove the parameter.
  2. They meant to use PhantomData.

The status-quo optimizes the experience of writing definitions for the first case, this RFC optimizes it for the second case. Which is better depends on what we believe the common case is. I believe that 1 is the common case and the status-quo is better, this RFC could retain the benefits of the status-quo by making the phantom T annotation mandatory.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Mar 4, 2018

@leodasvacas I think that's a good analysis. I'd like to add that I think that 1. is more common when "in-transition" (refactoring) as opposed to just starting out with a new type. That a field is missing will be very noticeable when you try to use that field but notice that it isn't there.

I'm certainly open to only keeping phantom T if we believe that is the right balance.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Mar 4, 2018

+1 to defaults for unused type parameters (while keeping unused_type_parameters as a usual unused lint that can be turned off during prototyping) (this also should be enough to get rid of phantom_data lang item).

-1 to the new special syntax and phantom fields sometimes behaving as normal fields and sometimes not.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Mar 4, 2018

@petrochenkov Might perhaps be better to make #[allow(unused_type_parameters)] the default and then #[deny(unused_type_parameters)] after?

-1 to the new special syntax and phantom fields sometimes behaving as normal fields and sometimes not.

Heh... and I was hoping to not get caught between two camps wanting to change things in opposite directions =P

What in particular are you referring to wrt. "sometimes [..] sometimes not"?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Mar 4, 2018

@Centril

What in particular are you referring to wrt. "sometimes [..] sometimes not"?

Privacy treatment, for example (phantom is a field) and construction/pattern matching (phantom is not a field).

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Mar 4, 2018

@petrochenkov

Privacy treatment, for example (phantom is a field) and construction/pattern matching (phantom is not a field).

Ah; Why tho is this a problem (and what category of problem, teachability, consistency, compiler complexity, ..)?

Personally, I think this discrepancy falls naturally out of the way PhantomData<T> is used today - you never care about / want to pattern match on a PhantomData, and adding it in construction is a pain, but privacy is a matter of correctness so you have to care?

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Mar 4, 2018

I agree with @scottmcm that I don't feel like specific syntax is required to replace PhantomData, I don't believe removing PhantomData is a thing worth pursuing.

If the community decides it is worth pursuing though, I don't think the way described here is a bad way to do it.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Mar 4, 2018

I don't like the general idea of this PR to add a custom keyword to the language for a niche feature... Rust actually came from that place where it had tons of language features. They then gradually moved to the standard library. Now we see built in stuff re-appearing like the ? operator or now this PR....

Adding PhantomData to the prelude should be enough. I side with @scottmcm and @nox here.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Mar 4, 2018

I don't like the general idea of this PR to add a custom keyword to the language for a niche feature...

I don't agree that phantoms are niche; I find that they show up plenty when using a little more advanced type features.

Now we see built in stuff re-appearing like the ? operator or now this PR....

And ? was added for good reason, it improved ergonomics over try!(..) a lot. And PhantomData has both ergonomical and technical problems (deriving). Improving phantoms by introducing phantom T was also suggested in 2014, before 1.0. For me, the way phantoms dealt with are really about the behavior of the language, and relegating it to libraries worsens ergonomics.

Adding PhantomData to the prelude should be enough.

Certainly open to that if we decide that is the better route. I'd like for us to also find a solution that solves the greatest ergonomics problems wrt. phantoms: the need to provide marker: PhantomData, in literals...

@Centril Centril closed this Mar 4, 2018

@Centril Centril reopened this Mar 4, 2018

@nical

This comment has been minimized.

Copy link

nical commented Mar 4, 2018

I am very very sympathetic with removing the need for declaring a PhantomData<..> member. I use phantom type parameters a lot for various things like math types or Ids and the mandatory member makes it impossible to have a simple and clean public interface.

Take euclid for instance: I would love for people to be able to write v = Vector2D { x: 1.0, y: 2.0 }; but has to be either v = Vector2D { x: 1.0, y: 2.0, _marker: PhantomData }; everywhere or force a constructor like Vector2D::new(1.0, 2.0); on everyone.

My experience so far has been that for every use of phantom type parameters I have had to trade a safer API with degraded ergonomics, and I would use them a lot more if it wasn't this way.

I understand that the cost of adding a keyword is big and maybe too big to fix this. however, some of the things this RFC wants to solve could maybe be achieved in other ways. Maybe with something like attributes?

#[phantom(T)]
Vetcor2D<T> {
  x: f32,
  y: f32,
}
type ScreenVector = Vector2D<ScreenSpace>;
type WorldVector = Vector2D<WorldSpace>;

let a = WorldVector { x: f32, y: f32 };
let WorldVector { x, y } = some_function();
// etc. Notice how nice it is to not deal with the extra member here.

This kind of thing is painless to do in C++.

I believe that removing the need to add members for unused type parameters is very much worth pursuing and could be done in a backward-compatible way without the cost of adding syntax, I am mentioning attributes but there probably many other ways.

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Mar 5, 2018

@nical AFAIK you can't build structs from a type alias, so even if PhantomData was unneeded you still wouldn't be able to do WorldVector { x, y }. That is a separate issue and doesn't change you point though.

Did you ever consider doing things like that?

struct Unit<U>(PhantomData<U>);

enum World {}
const WORLD: Unit<World> = Unit(PhantomData);

Vector2D { x, y, u: WORLD }

I think this requires as many imports as what you described with #[phantom], for almost no additional syntax boilerplate.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Mar 5, 2018

@nical

I understand that the cost of adding a keyword is big and maybe too big to fix this.

Actually, I think the changes to the grammar are larger if you want to go the attribute route, since then, you have to allow arbitrary types inside attributes as in #[phantom(<type>)] (AFAIK this is not permitted yet).

If you take a look at the grammar changes here: https://github.com/Centril/rfcs/blob/rfc/ghost-busting/text/0000-ghost-busting.md#grammar, you see that no new keywords are introduced (phantom T is contextual) and that the delta is quite small.

I'd argue that the mental cost is also quite small or even reduced compared to PhantomData<T> since there now are (substantially, I think) fewer places where explicit phantoms turn up.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Mar 5, 2018

@quininer, @fstirlitz, @tanriol, can you elaborate on your concerns?

@kennytm, what is confusing you? can I elaborate on something?

@kornelski

This comment has been minimized.

Copy link
Contributor

kornelski commented Jan 12, 2019

This RFC is mostly about syntax sugar.

I think Rust should have more room to experiment with various syntaxes by supporting "transpilers" like JavaScript's Babel. This way users could try out, in real-world code, alongside other syntax changes, whether it makes sense as a phantom field, where clause, magic parameter or something else.

https://internals.rust-lang.org/t/pre-rfc-first-class-support-for-compile-to-rust-languages/7610

@vorner

This comment has been minimized.

Copy link

vorner commented Jan 12, 2019

I don't have a strong opinion about the introduction of new syntax for phantom T. However, I'm concerned about disabling the warning for unused type parameters. In the history of receiving that warning on my own code, for me it has almost always meant "you forgot to make the code match the type parameters" or "you deleted some code and need to adjust the type parameters" or "you wrote the type parameters you thought you needed and forgot to update them when it turned out you didn't", and almost never meant "you need a PhantomData".

Recently, I've written some code that uses phantom data more than the usual amount, and it actually was the β€žYou need PhantomDataβ€œ case. And considering the experience, I'd say I'm against defaulting to PhantomData<T> for the unused T. While the former problem (I forgot to remove T) would manifest early on as β€žType annotations neededβ€œ, defaulting in my case would probably lead to very confusing problems much further away.

In other words, at least half of the time I didn't end up using PhantomData<T>, but something like PhantomData<fn() -> T> or PhantomData<fn(T)>, because the structure didn't own any T (even conceptually). It either produced it or consumed it. If there was the default, I wouldn't be reminded by the compiler to think about how it should act and if someone tried to use the structure with T: !Send, the whole structure would end up !Send β€’ but that's not the case if the structure can generate a new T in whatever thread it currently lives in. I believe this goes to similar category of not allowing any fields to be missing from initialization of a structure (because the default might be wrong), but on the type level.

@alercah

This comment has been minimized.

Copy link
Contributor

alercah commented Jan 12, 2019

Based on my experience I think I'm inclined to agree.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 18, 2019

This has been open for a long time,and I feel like it doesn't have sufficient justification. There's a solution for this problem already, it's documented and understood, and I don't think the proposed syntax is a sufficient improvement to justify expanding the surface area of the language.

Thus:

@rfcbot close

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 18, 2019

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Jan 18, 2019

@pnkfelix I've seen talk of similar things before (I've called something similar ImplicitDefault, and I think it came up in the Ok(()) discussions), but I don't think it's ever gotten to a full RFC proposal.

@kornelski

This comment has been minimized.

Copy link
Contributor

kornelski commented Jan 19, 2019

A new trait would work only for marker types, but a default value for fields could work for marker types and other more general cases. There's a postponed RFC for it: #1806 Given that constexpr has become usable, maybe it'd be a good time to reopen it?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 20, 2019

I'd be in favor of a much smaller RFC that just made the variance implied by _marker: PhantomData<T> the default variance if the type parameter isn't used in the struct. I believe a few things:

  • The vast majority of interactions with the PhantomData type come from people doing compiler-driven-development who have mocked out their struct, and just want to get the code to compile but need a parameter in the interface. These people gain nothing at all from requirements around having PhantomData, since eventually the parameter will appear in the definition properly.
  • Requiring PhantomData is not seriously causing people to think about variance. There is an easy solution that doesn't require deep thought and is nearly always correct: _marker: PhantomData<T>. I think this is how most users solve the bug; the remainder already understand variance and hopefully don't need a reminder. I don't think this "explicitness" is buying anything; its overly conservative and just naggy.

However the phantom T type parameter change is, IMO, not a worthwhile addition to the language when those use cases can just use the PhantomData type.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 20, 2019

Actually on second thought rather than getting rid of the error entirely, I'm in favor of downgrading it to a warning, using the default variance rules if the type is omitted. That way you still get nagged if, once you've finished filling out your code, you still haven't used that type parameter (encouraging you to use PhantomData then if its really necessary), but mid-mock you just get another dead code warning that you can ignore, as you are probably already doing.

Anyway that's a different RFC from this one.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Jan 21, 2019

(@withoutboats I like the idea of a warning, which seems like a good resolution; just w.r.t. previous comment, note @vorner's comment above, according to which PhantomData<T> is not nearly always correct.)

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 21, 2019

@glaebhoerl I read @vorner's comment, I think @vorner's experience is squarely in the very small space excluded by "nearly always." But I think reducing unused type parameters to a warning would suffice for everyone.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 21, 2019

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Jan 21, 2019

What about a deny by default lint?

@alercah

This comment has been minimized.

Copy link
Contributor

alercah commented Jan 21, 2019

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 31, 2019

πŸ”” This is now entering its final comment period, as per the review above. πŸ””

@Centril Centril removed the I-nominated label Jan 31, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 31, 2019

I feel like it'd be useful to enumerate some of the scenarios where phantom data is annoying.

One use case for me is kind of "typed indices" (I think the pattern is more general, but I can't think of a good summary term, so I'll leave it at that). I have been annoying a number of times because I have structs that are like

struct Index<T> { .. }

where the T is not stored within the index, but more something that we want to "remember", because some other bit of code will do something like:

fn get<T>(x: impl Index<T>) -> T

In these cases, I basically always want invariant, and I... probably don't really want to consider T owned by the index. The auto-trait behavior is kind of neither here nor there. Typically, the index is sort of 'inert' without the container. But it'd be interesting to collect more such examples and try to see if that is true for all of them.

(Honestly, I feel like what I want here is sort of an "associated type" for a struct, e.g., struct Index { type T; ... }, where I can write Index<T = ...>. Except I don't want necessarily want to write that, and it sort of opens up a can of worms.)

I also agree with @withoutboats that refactoring is annoying.

@nical

This comment has been minimized.

Copy link

nical commented Jan 31, 2019

I feel like it'd be useful to enumerate some of the scenarios where phantom data is annoying.

Another scenario where phantom data is annoying (somewhat similar to @nikomatsakis's Index<T> example) is euclid's strongly typed 2D/3D linear algebra crate where types can be tagged with a dummy phantom type prevent mistakes. for example one will typically create aliases such as:

type WorldPoint2D = TypedPointe3D<f32, WorldSpace>;
type ScreenPoint2D = TypedPoint3D<f32, ScreenSpace>;
type Projection = TypedTransform3D<f32, WorldSpace, ScreenSpace>;

TypedPoint3D source code: https://github.com/servo/euclid/blob/1244c867a8a7bcd4333c6555ea1f7305abf997d9/src/point.rs#L417

It'd be great if euclid could let you write:

// No can do, gotta use WorldVector3D::new(1.0, 2.0, 3.0)
let v: WorldVector3D = Vector3D { x: 1.0, y: 2.0, z: 3.0 };

However at the moment the language enforces the _unit phantom data member to be explicitly specified.
Also as the author of this code, I've had to learn about fancy words like "variance" to rub the compiler the right way and do something that is simple in C++ (at least in comparison).

My other main exposure to phantom types is the sid crate which corresponds to @nikomatsakis's Index<T> example I think.

In both cases the phantom type is only there as a some sort of tag and properties of the actual type T don't really matter.

@burdges

This comment has been minimized.

Copy link

burdges commented Jan 31, 2019

A priori, there is no subtyping relationship among unit types, so you would not expect the variance to matter. In fact, any normal data would be covariant because someone might define traits Metric and Kilo or whatever.

As an aside, I just learned that no variance annotation in a C# interface denotes invariance, which does not exist ion Rust, but still might confuse someone.

Anyway there is nothing wrong with defining some #[covariant_default] attribute.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 10, 2019

The final comment period, with a disposition to close, as per the review above, is now complete.

By the power vested in me by Rust, I hereby close this RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment