ceylon / ceylon-spec

DEPRECATED
Apache License 2.0
108 stars 34 forks source link

Variance checking is too strict for method type parameters #1280

Open jvasileff opened 9 years ago

jvasileff commented 9 years ago

Discussed here, variance annotations can be used on method type parameters as a hint to type argument inference.

But this doesn't work with shared methods, resulting in errors like the following when in is used for a return value (is this the only hint that actually changes anything?):

Contravariant (in) type parameter Absent appears in covariant or invariant location in type: String|Absent

A couple examples:

shared
String|Absent dontBeAnOverachiever<in Absent>()
        given Absent satisfies Null
    =>  if (is Absent null)
        then null
        else ""; // presumably expensive to calculate

and perhaps controversially:

shared
dynamic Element {
    shared formal String? id;
}

shared
dynamic Document {
    // Create a type hole; default to package.Element,
    // not Nothing which would be *way* over-promising
    shared formal
    Element? getElementById<in Element>(String id)
            given Element satisfies package.Element;
}
gavinking commented 9 years ago

But this doesn't work with shared methods

Nor with un-shared methods now, due to a recent bugfix.

gavinking commented 9 years ago

So want to be able to mark type parameters which are actually outputs as if they were inputs, just to trick the type inference algorithm? You'll have to talk me into that....

jvasileff commented 9 years ago

What? No, I'm definitely not going to try to talk you into that.

My initial thought was that variance annotations didn't make sense in that location, but you said/pointed out that they were a feature. So I thought I'd try to use them, if only in a couple unusual situations.

Definitely go ahead and close if this issue isn't of value; this was more of a bug report than a request.

gavinking commented 9 years ago

Well, it's not a bug per se, but if you were able to convince me that it's truly a misfeature to check the variance annotations against the return type of the method, we can easily relax that restriction.

jvasileff commented 9 years ago

I would argue that if you are allowed to provide a "hint" via annotations, it should be valid to provide a hint that triggers a result that is different than what the type checker would have produced without the hint. This does have some value since you cannot provide a default value for type parameters in this situation (the first parameter only? I don't remember).

If you can't provide a hint that does anything useful, then I don't see any reason to allow variance annotations on function type parameters.

FWIW, I thoroughly confused myself when I stumbled upon this by reading an interface full of these annotations that I had previously written, for no reason at all. This at least won't happen to me again.

All in all though, I really didn't intend to elevate this to a higher priority than it may deserve.

RossTate commented 9 years ago

I agree: annotations should override the type checker's default inference strategy.

gavinking commented 9 years ago

annotations should override the type checker's default inference strategy.

Well they do.

The purpose of this feature is to let you write a function like this:

Consumer<T> createConsumer<in T>(Something<T> something) 
        => Consumer(something);

And get the same type inference algorithm as if you instantiated the contravariant type Consumer directly. If you were to remove the in annotation here, you would get the type inference strategy we use for invariant (and covariant) types.

Another example of where it makes sense:

Invariant<in T> createConsumer<in T>(Something<T> something) 
        => Invariant(something);

The only thing I don't let you write is stuff like this:

Covariant<T> createProducer<in T>(T something) 
        => Covariant(something);

since T here is clearly occurring covariantly in the signature of the function. The question is: should we allow that anyway.

jvasileff commented 9 years ago

I see. That is a different example than the one in the thread.

Can it be written as:

Consumer<Upper> createConsumer<T>(Something<T> something)
        given T satisfies Upper
        => Consumer(something);

If so, we could be back to disallowing annotations to avoid programmer error. If not,

should we allow that anyway

I guess would be a test of @sadmac7000's assertion that

Restrictions are always strictly less intuitive to have than not

gavinking commented 9 years ago

@jvasileff What is Upper? Anyway, no, I don't see how it could be written that way.

That is a different example than the one in the thread.

Ah yeas, one of the examples @lucaswerkmeister gives only passed the typechecker due to a (fixed) bug in 1.1.

jvasileff commented 9 years ago

What is Upper

Whatever you want it to be, I think. In your case that is missing a satisfies constraint, Anything:

Consumer<Anything> createConsumer<T>(Something<T> something) 
        => Consumer(something);
gavinking commented 9 years ago

@jvasileff I don't follow.

My example is, making it more concrete:

interface Sink<in T> {
    shared formal void accept(T t);
}
Sink<T> createSplitter<in T>(Sink<T> left, Sink<T> right) 
        => object satisfies Sink<T> {
    shared actual void accept(T t) { left.accept(t); right.accept(t); }
};

It's clear that in this code example:

Sink<Foo> fooSink = ... ;
Sink<Bar> barSink = ... ;
value sink = createSplitter(fooSink, barSink);

That I should get back a Sink<Foo&Bar>, not a Sink<Foo|Bar>.

jvasileff commented 9 years ago

Ah, yes, I see. My example was way too simplistic.

RossTate commented 9 years ago

The only thing I don't let you write is stuff like this:

Covariant<T> createProducer<in T>(T something) 
        => Covariant(something);

since T here is clearly occurring covariantly in the signature of the function. The question is: should we allow that anyway.

So if y'all reject this right now by saying in is disallowed when the return type is not contravariant with respect to the parameter, then I'm happy except that it seems stupid to require that annotation when you could just infer it. If ever you do allow this sort of example, then the inference algorithm should ignore that covariance of the return type and use the strategy it would for contravariant types (i.e. give T the least precise type possible).

gavinking commented 9 years ago

I'm happy except that it seems stupid to require that annotation when you could just infer it.

Well you can infer variance from a class or interface definition too, but you wouldn't get very nice error messages if you get it wrong :)

jvasileff commented 9 years ago

Going for symmetry:

There are a couple unusual uses for:

T fun<in T>(...) => nothing;

which are not currently allowed. However, in the contravariant case, the default is:

Anything(T) fun<out T>(...) => nothing;

which I believe is analogous to the disallowed case - type inference results in the least useful return type. Shouldn't the default be:

Anything(T) fun<in T>(...) => nothing;

And, are there useful or interesting uses for Anything(T) fun<out T>(...) {}? Is this something that is more normal or valuable than T fun<in T>(...) {}?

Note that annotations in the invariant case are disallowed. Apologies if I have this wrong again!

gavinking commented 9 years ago

@jvasileff As usual (i.e. just like with classes), the invariant case is an overconstrained problem and so we can't always pick a T that satisfies all the constraints. Therefore Ceylon handles the invariant case using the algorithm for covariance, and sometimes fails. Yes, there is an asymmetry here, but I don't think there is a clearly better solution, out of the available options:

  1. never do type arg inference for invariant type params, or
  2. apply both algorithms and fail when they don't agree.

Now, I think what you're saying here is that for a function, we could infer the variance of the type parameter from the return type, and that's surely true, but it would be different to what we do for classes, and honestly people might find it just as surprising as what happens today.

RossTate commented 9 years ago

I think what we're saying is that for a function, we could infer the type arguments that produce the most precise type whenever there is such a most precise type. This would be consistent with your goal to have principal types. The algorithmic strategy for this would be to infer the variance of the type parameters from the return type and then use the corresponding strategy (i.e. assign to upper bound or lower bound). In the case of invariance there is no optimal strategy, so you default to covariance, but you could let people override this default. A relevant memory is that one of the first usage bugs reported had to do with ad-hoc-ly inferring type arguments for invariant type parameters.

jvasileff commented 9 years ago

the invariant case

I definitely agree with the default covariant treatment, and I also think it matches real-world non-programming expectations.

My minor point was that for hinting, this would be the case you might want to override, but it is disallowed. Of course, you can't provide a hint for classes, so I don't think many would be surprised at not being able to for functions.

Now, I think what you're saying here

Yes. But I think classes are different. And for functions, if it is "always" right to use the contravariant algorithm when the return is contravariant in the parameter, then it might be nice to unburden the programmer with providing "in".

And, if the variance is inferred, that would re-open the possibility of disallowing variance annotations for functions, which I think are more likely to be used incorrectly, by accident, than for real benefit.

Of course I think this is mostly consistent with @RossTate's note, but I'll press submit anyway...

jvasileff commented 9 years ago

On the original topic, there is another real world case where the preferred inference would be to provide the "least capable" result to the caller—the metamodel.

For reference, Ceylon 1.1.0 has the Attribute interface:

interface Attribute<in Container, out Get = Anything, in Set = Nothing>

Now, with ClassOrInterface.getAttribute():

print(`Object`.getAttribute("string")?.bind(true)?.get());
// error:
//      ceylon run: Incompatible type: actual type of applied declaration is
//      Attribute<Object,String,Nothing> is not compatible with expected type:
//      Attribute<Nothing,Nothing,Nothing>. Try passing the type argument explicitly
//      with: memberApply<Object,String,Nothing>()

(Per the other part of the discussion, a better error/default inference would be Attribute<Anything,Nothing,Anything>, but that is not relevant ATM.)

So the signature we really want is:

Attribute<Container, Get, Set>? getAttribute<out Container, in Get, out Set>(String name);

The signature we currently have is:

Attribute<Container, Get, Set>? getAttribute<Container=Nothing, Get=Anything, Set=Nothing>(String name);

which is an attempt at using defaults to override the undesirable standard type inference in this case. But due to the way defaults must work, we're left with the above error unless we provide at least the first argument at the call site. So these are the current options:

// works (disable inference entirely)
`Object`.getAttribute<Nothing,Anything,Nothing>("string")?.bind(true)?.get();

// works (specify the first argument, let defaults handle the rest)
`Object`.getAttribute<Object>("string")?.bind(true)?.get();
`Object`.getAttribute<Nothing>("string")?.bind(true)?.get();
jvasileff commented 9 years ago

With https://github.com/ceylon/ceylon-spec/issues/1302 and other bug fixes, it is now, for the most part, prohibited to use in or out in cases that would change the normal behavior. So that's good.

But this now breaks:

shared void enumerateAttributes() {
    class Model() {
        shared String name = "";
        shared String nickname = "";
    }
    Attribute<T,Anything,Nothing>[] getAttributes<T>(
            ClassOrInterface<T> clazz) {
        print(`T`); // 1
        return clazz.getAttributes<T>();
    }
    print(getAttributes(`Model`)); // 2
}

because in is inferred for getAttributes.T (based on the return), making T Anything. This results in the rather useless return value [].

If we change getAttributes to return void, out is inferred based on the argument clazz<out Model>, and T becomes Model, which is what we want.

I think this is an important enough example to tip the scales in favor of removing the restriction on using in and out hints in conflict with a type parameter's usage in a method's return type. This might also open the possibility of replacing defaults with hints for methods in the meta model api.

gavinking commented 9 years ago

we're left with the above error unless we provide at least the first argument at the call site.

@jvasileff I'm not disagreeing with you but I want to note that we now have the option of a diamond <>.

jvasileff commented 9 years ago

Yeah, the use site <> definitely helps, but I'd argue:

  1. With hints, an API designer can unburden the user with having to be aware of and understand variance and inference rules, and
  2. Defaults don't always solve the problem, as in cases that look like the last example

My concern before was that hints might be confusing when used by accident. But I've come to believe that that is the lesser concern.