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

Support variance for type parameters #738

Merged
merged 2 commits into from
Feb 12, 2015

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 26, 2015

A revised proposal for properly supporting variance in Rust's type system.

Rendered view.

@nikomatsakis nikomatsakis self-assigned this Jan 26, 2015
@dgrunwald
Copy link
Contributor

👍 I like PhantomData, this will make writing FFI code significantly easier.

Variance for type parameters: This is one of the things that users don't notice if it works properly, and is utterly confusing when types are unnecessarily invariant. This alone is a good reason to infer variance instead of requiring explicit variance annotations.

which can't do any harm. Here is an example:

```rust
fn approx_context<'long,'short>(t: &Context<'long>, data: &'short Data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to take a reference for the first parameter? As it is, the example is a compile error because do_something expects a Context, but gets passed a &Context in approx_context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@bill-myers
Copy link

Another alternative could be to have inference as this RFC suggests, but then emit an error if a variance annotation does not match the inferred variance, telling the user exactly what change needs to be made:

type parameter `T` is covariant, so you should declare `Foo` as `struct Foo<+T>`.
If you didn't intend that, add a `PhantomData<fn() -> T>` marker or equivalent to make `T` invariant.

Only inference makes it easier to write code, but it does violate the Rust philosophy that the types of public APIs should be explicitly written out.

Regarding annotations, the obvious "lightweight" approach is to add a single punctuation character for each variance, for instance "=" or nothing for invariance, "+" for covariance, "-" for contravariance.

Not sure whether this is better or not than just inference, but being the only violation of having explicit types for APIs is a bit disturbing.

desired behavior as a fn that I realized I had it backward, and
quickly found the counterexample I give above. This gives me
confidence that expressing variance in terms of data and fns is more
reliable than trying to divine the correct results directly.*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

divine → derive?

@kennytm
Copy link
Member

kennytm commented Jan 27, 2015

Instead of PhantomData<*mut T>, could PhantomData<&'static mut T> work?

@nikomatsakis
Copy link
Contributor Author

@kennytm using PhantomData<&'static mut T> would require that T:'static, so it's probably not what you want.

@dgrunwald
Copy link
Contributor

Regarding annotations, the obvious "lightweight" approach is to add a single punctuation character for each variance, for instance "=" or nothing for invariance, "+" for covariance, "-" for contravariance.

"+" or "-" will be hard to understand for users not familiar with variance.
If we have to use explicit annotations, I'd prefer the C# approach using the keywords "in" and "out":
nothing = invariant, "out" = covariant, "in" = contravariant

@nikomatsakis
Copy link
Contributor Author

@bill-myers yes, I agree but also disagree.

The reasons I would prefer not to have explicit variance annotations are listed in the RFC, but I'll repeat them:

  1. What do we call these annotations? Symbols like -, +, etc are notoriously confusing. Keywords like "covariant" are as well. Keywords like "in" and "out", as used in C#, don't fit so well in Rust, where the "interface" and "data" declarations are separated (for example, Vec<out T> just doesn't make sense to me).
  2. Having both annotations and phantom data seems very non-DRY, and I've found that the phantom data approach is both more broadly applicable and more reliable in terms of getting the right results.

That said, it is nice to be able to declare the expected variance (and I've found myself using internal Rust compiler debugging magic to force it to print out the results of inference on occasion). I imagine it might be nice to have a standard way of declaring the results you expect -- one option remains the writing of unit tests asserting the expected behavior.

Note that we've been using inference for quite some time now actually, it's just that it only applies to lifetime parameters (but it is quite crucial there; Rust would not work at all without it). Experience has been fairly positive thus far I would say -- the only real issues I see are when the expected results don't materialize (particularly around Option, where lifetime checking is much stricter, as the RFC shows).

@nikomatsakis
Copy link
Contributor Author

Ah, and note that having some variance declaration also requires us to settle on a terminology around lifetime parameters. Overall it just feels like a lot more complexity to present the user with.

@kennytm
Copy link
Member

kennytm commented Jan 27, 2015

@nikomatsakis Ah I see, thanks.

@glaebhoerl
Copy link
Contributor

I'm also in the camp of preferring interfaces to be made explicit, but respect the objection that there's no (obvious) ergonomic and easily comprehensible way to do so. PhantomData is a cool idea should we choose to go with inference.

My one small contribution to the question of potential explicit syntax is that I believe variances should be thought of and presented as additional capabilities granted, relative to the baseline of "invariant", to the user of the API - just like everything else that you publicly expose in an API is an additional capability relative to not having exposed it. So one capability is "can be up-casted", another is "can be down-casted" ("cast" is maybe not the right word), if you have only one or the other it's what we call "covariant" or "contravariant", while if you have both it's "bivariant". The crucial point is that this is the reverse of how they're usually framed, which is in terms of how the type parameter is used by/within the type being defined, which induces restrictions: where in means it's used as a (function) input, meaning you can't cast it in one direction, out means it's present as a data member or other "positive" position, meaning you can't cast it in the other, and the combination of the two results in invariance. What we want is for the two keywords (whatever we're able to come up with) to correspond to how the type parameter is not used by the type being defined, which allows for greater freedom for the user of the type, with the combination of both keywords (meaning the parameter is not used at all - neither in positive nor negative positions) expressing bivariance.

(Incidentally, this also happens to be how the Functor and Contrafunctor type classes are used by Glasgow Haskell's lens library.)

@nrc
Copy link
Member

nrc commented Jan 27, 2015

Nice, I'm in favour of the inference approach and using 'annotations' for assistance (explicit variance annotations are nearly always awful). I was thinking that this is kind of hacking around writing exactly what the author means, but I think what I was thinking of is the phantom keyword stuff in the future directions section - I'd be keen to see that implemented asap.

As another reason to avoid explicit annotations, I imagine that the fact that Rust subtyping and thus variance only affects lifetimes will further confuse users - variance in other languages is almost always discussed in terms of containers and the type of elements. That we use implicit coercions for many subtyping-ish things, but that these are not covered by the variance annotations might be odd.

Question: is there any effect in using the traits here as bounds on type parameters? e.g., fn foo<X: PhantomFn() -> i32>(x: X) { ... }


2. Rewrite safe abstractions to use `*const` (or even `usize`) instead
of `*mut`, casting to `*mut` only they have a `&mut self`
method. This is probably the most conservative option.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for 2 - this is what I do in my personal projects, I didn't realise the std libs used *mut this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have currently implemented option 2: that is, *mut is invariant. I had been leaning towards what I think was 3 (ignore *mut references entirely and reauire explicit markers), but I fear that that is more error-prone. Basically if people aren't thinking carefully it's easy to make a covariant type that ought to have been invariant. Option 2 is correct by default, and requires you to opt-in (by using distinct types) to covariance. On the variance branch I've just been rewriting standard library stuff not to use raw *mut pointers but rather safe wrappers like Unique, which I think improves the code overall, since it's more declarative. It also handles the OIBIT requirements and so forth. Those are currently unstable though, we'll probably want some iteraiton on their design -- right now they are just kind of shallow wrappers around a raw pointer, we might be able to do better. (Note also that Unique on the branch is quite different from Unique on master).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hashmap (and my working branch of btree) don't use Unique because they contain what is essentially a void pointer to "bytes".

@llogiq
Copy link
Contributor

llogiq commented Jan 29, 2015

One possibly less confusing syntax for denoting the variance type would be

  • T (or =T) for invariant type (_ equals T)
  • T/ for covariant type T (read like T over _)
  • /T for contravariant type T (read like _ over T)

The idea is that the / denotes the position of our specified type related to T in an assumed hierarchy.

I don't know how much this would complicate the parser, though.

In any event, inference has my vote.

@Gankra
Copy link
Contributor

Gankra commented Jan 30, 2015

CC @gereeter

PhantomData<T> // covariance
PhantomData<*mut T> // invariance, but see "unresolved question"
PhantomData<Cell<T>> // invariance
PhantomData<fn() -> T> // contravariant

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be covariant? fn() -> T is roughly isomorphic to T, so the result should be the same as if PhantomData<T> were used. Alternatively, this could be changed to fn(T).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sorry, I meant fn(T)

@nikomatsakis
Copy link
Contributor Author

@nick29581

I was thinking that this is kind of hacking around writing exactly what the author means, but I think what I was thinking of is the phantom keyword stuff in the future directions section - I'd be keen to see that implemented asap.

Me too, I'd be happy to revise the RFC to include the phantom syntax as a first-class thing, even if I'd want to land a branch that uses the markers for now. The usability improvements of having concrete syntax would be quite high, I think, and given how central all of this is to the type system, it seems justified to me.

Question: is there any effect in using the traits here as bounds on type parameters? e.g., fn foo<X: PhantomFn() -> i32>(x: X) { ... }

You can do it but it's meaningless -- since PhantomFn defines no methods, and is implemented for all types, adding such a bound buys you nothing. The only place that PhantomFn is useful is in the supertraits listing.

## Why variance should be inferred

Actually, lifetime parameters already have a notion of variance, and
this varinace is fully inferred. In fact, the proper variance for type
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should read "variance".

@theemathas
Copy link

-1 for variance inference. I believe that while having implicit variance would help user if it works properly, it would cause utter confusion when it doesn't. The fact that rustdoc hides private fields doesn't help at all when someone hits a variance error in an external library. Cases that require anything other than contravariant lifetimes require (relatively) advanced concepts any way, and the confusion most likely stems from the fact that the names are scary.

I think it is simpler to use contravariance by default, use struct Foo<no_shrink_lifetime 'a> {...} or #[no_shrink_lifetime('a)] for invariant lifetimes, and use struct Foo<allow_extend_lifetime + no_shrink_lifetime 'a> {...} or #[allow_extend_lifetime('a)] #[no_shrink_lifetime('a)] for covariance

@tomjakubowski
Copy link
Contributor

The fact that rustdoc hides private fields doesn't help at all when someone hits a variance error in an external library.

It shouldn't be too hard (knock on wood) for rustdoc to show variance on type/lifetime parameters. There's an open issue for that here rust-lang/rust#22108

@llogiq
Copy link
Contributor

llogiq commented Feb 11, 2015

@theemathas, the cardinality of confusion on inference failure is directly proportional to the quality of error messages and documentation. As long as: a) the rules aren't too arcane to be explained to Joe coder and b) it is possible for the compiler to show the location in the code where to add markers/annotations/whatever, confusion should be quite limited.

@aturon
Copy link
Member

aturon commented Feb 12, 2015

While there has not been a great deal of feedback, there is broad agreement that the new Phantom markers are clearer than today's markers (though could perhaps be more clear with special syntax), and that some variance for type parameters is needed. While some have expressed reservations about inference here, it's worth remembering that this inference has long been performed (though not largely used) in the past. Given that the variance is strictly related to lifetimes (the only form of subtyping), being implicit here seems worth at least attempting, and the implementation has been sitting on the side being rebased for quite a long time now.

We've decided to merge this RFC and land the implementation to gain some experience with this system. Please try it out and give feedback as to whether inference ever leads to unclear results in practice.

Tracking issue

@tomaka
Copy link

tomaka commented Feb 16, 2015

In my library I currently have this code:

pub struct TextureType1 { handle: u32 }
pub struct TextureType2 { handle: u32 }
// ... around 50 different texture types

pub struct FramebufferObject<'a> {
    attachments: Vec<u32>,
    marker: ContravariantLifetime<'a>,
} 

impl<'a> FramebufferObject<'a> {
    pub fn attach1(&mut self, tex: &'a mut TextureType1) {
        self.attachments.push(tex.handle);
    }

    pub fn attach2(&mut self, tex: &'a mut TextureType2) {
        self.attachments.push(tex.handle);
    }
}

This ensures that the FramebufferObject doesn't live longer that the attached Textures.

How would I do that with PhantomData? Do I need to create some kind of enum containing all the possible PhantomData types or something?

@pnkfelix
Copy link
Member

@tomaka if I properly understand what you are doing, I think you would write something like: marker: PhantomData<&'a ()>,

In other words, the phantom data represents the hidden references to data of lifetime 'a.

@alexchandel
Copy link

Congrats guys, my code got a little uglier.

use std::marker::MarkerTrait;
pub trait LoadCommandBody: MarkerTrait {}

Thanks for that.

@steveklabnik
Copy link
Member

@alexchandel please keep it constructive.

@petrochenkov
Copy link
Contributor

Constructive: at least put MarkerTrait in the prelude.

@tomaka
Copy link

tomaka commented Feb 23, 2015

MarkerTrait definitely looks hacky.

The syntax trait A : B {} is supposed to mean that all types that implement A must also implement B.
When you write trait A : MarkerTrait {}, it looks like MarkerTrait applies of the trait A itself, which is contrary to this definition. It makes no sense for a regular type like i32 to implement MarkerTrait.

@petrochenkov
Copy link
Contributor

@tomaka
MarkerTrait is fun, every sized type implicitly implement it.

fn main() {
    fn f<T: std::marker::MarkerTrait>(_: T) {}
    struct S;
    f(S);
}

@tomaka
Copy link

tomaka commented Feb 23, 2015

I'm complaining about the name MarkerTrait. Why would i32 implement a trait whose name is MarkerTrait? There has to be a better way to name this, like NoSelfVariance or something.

@NXTangl
Copy link

NXTangl commented Dec 10, 2015

@tomaka Perhaps Rust should differentiate inheritance from type application. As it is, traits implementing traits is more like subtyping than trait implementation--if a type implements a trait, it implements all 'supertraits' of the trait, just as if a value is an instance of a type, it is an instance of all supertypes of the type. (I think this is why Haskell chose to use the class/instance terminology for its typeclasses.) In that case, traits describe sets of types (or, more generally, relations on types [in the database sense] which Rust represents using associated types) and types describe sets of values, and values don't need to be sets.

@burdges
Copy link

burdges commented Dec 11, 2015

Yes, MarkerTrait needs a new name. Is it clearer if you explain it in terms of marking something? I don't think so.

If you dislike NoSelfVariance, which at least warns folks about the strangeness, then maybe SetOfTypes, PhantomMethod, StaticNeeded, etc. And one should consider prefixing the term with a term like Common.. or Typical.. like TypicalSelfCovariance or CommonPhantomSelf. Just whatever makes the most sense when you think about writing the documentation that explains it.

As an aside, I initially wrote the above with PhantomConstructor, but appears that's actually wrong. It's a phantom getter method really.

@Gankra
Copy link
Contributor

Gankra commented Dec 11, 2015

Neither PhantomFn nor MarkerTrait exist in today's Rust. This RFC's text is fairly outdated wrt to the current state of the type system.

PhantomData is the only mechanism one uses to specify extra semantics.

@nikomatsakis
Copy link
Contributor Author

Seems like it'd probably be worth revising the RFC. I'm surprised I didn't do that. I guess I just wrote an internals thread on the topic? This one, specifically: https://internals.rust-lang.org/t/refining-variance-and-traits/1787

@aidanhs aidanhs mentioned this pull request Aug 15, 2016
@Centril Centril added A-variance Variance related proposals & ideas A-typesystem Type system related proposals & ideas A-drop Proposals relating to the Drop trait or drop semantics labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-drop Proposals relating to the Drop trait or drop semantics A-typesystem Type system related proposals & ideas A-variance Variance related proposals & ideas
Projects
None yet
Development

Successfully merging this pull request may close these issues.