Give users more help in solving contravariance issues

[matanlurey]

See #33119 (comment) for background.

DDC throws with:

TypeError: Closure 'Manager_getResponse_closure': type '(SubContext) => Response<SubContext>' is not a subtype of type '(Context) => Response<Context>'

Unfortunately this doesn't really explain anything, and because most of Dart is based on covariance, I (incorrectly) thought "WTF, its a Response<SubContext>, that's even BETTER than Response<Context>". Of course, that is not the case...

• Does our language tour or Dart 2 FAQ have anything to help here?
• Can the error message call out "contravariance" or something to help the user?

In #33119 (comment) specifically, we are statically asking for a Callback<Response<Context>>, but at runtime getting Callback<Response<SubContext>>, which of course is contravariantly invalid.

[jmesserly]

yeah this problem is really bad and seems to be getting worse. For whatever reason, callbacks like this are becoming more common. We originally did not see this style used much across the code base, and thus expected it to be rarely hit in practice.

@leafpetersen has some ideas for fixing this in the type system, so this code is simply accepted (and succeeds with a runtime cast). IMO that's a good solution in the short term (longer term I want to see invariant generics and/or sound co/contravariant generics.)

[vsmenon]

Hmm, in general, cases where a type parameter T is used contravariantly:

class Foo<T> {
  void Function(T) callback;
}

is a nasty foot-gun. If we want to avoid the strange runtime error, options appear to be:
(1) Disallow the contra-variant T (in getters / fields / return types). This would break a lot of code.
(2) Treat the contra-variant T as Object in the type system. I.e., anything assigned to callback has to actually be (Object) => void.
(3) Use this as a signal that T must be invariant in Foo. I.e., Foo<Object> f = new Foo<String>() becomes a static error.

In Matan's example, (3) would mean a static error here:

Response<Context> response = manager.getResponse();

[matanlurey]

Thanks both @jmesserly and @vsmenon for following up!

I'd love to see how much work it would be to do (3), and what the impact on google3 looks like.

[jmesserly]

(1) agree this is too breaking.

(2) seems essentially the same as 1? Like we're still disallowing contravariant T, but instead of an error, we're silently making it Object for you ... that seems like it would be really confusing. I have a field Callback<T> callback but I can't set a function that takes a parameter T?

(3) seems very breaking as well. We'd need to enforce invariance on those types for runtime casts. It will break because Angular constructs components with <dynamic>, but the models usually have some other <T> on there (if I'm understanding Angular's behavior from threads). I totally want invariance but I don't think we're ready for it (if we were, we shouldn't be hitting these errors, and they wouldn't be so hard to fix).

I haven't had a chance to chat with Leaf yet, but I think we were thinking something more along the lines of resurrecting a fuzzy arrow type approach:

class C<T> {
  final Function(T) callback;
  C(this.callback);
}
main() {
  C<Object> c = new C<int>((int x) => x.abs());
  var f = c.callback; // previously fails, now works
  // f has type `(?) -> dynamic`
  // `?` means we don't know the param type, need to check it before assuming.
  f(42); // works
  f('hello'); // fails runtime param type check
  Function(Object) g = f; // fails runtime cast
  Function(int) h = f; // works
  c.callback = f; // works
}

This would fix most of the examples I think, with no code changes. We could provide a way for users to write these types as well, depending on whether this is a short term hack or not.

Long term yes sound variance please. Need to make Angular understand generics :)

[matanlurey]

We're working on the generics issue. FWIW, the work-around has been to loosten the expectations to dynamic anyway, so I'm not sure the invariant type will be strictly speaking breaking (or not any more breaking than the current behavior).

For example, we advise users to rewrite:

class ParentComponent {
  String renderName(Animal a) => a.name
}

// <T> is always _dynamic_ in AngularDart today.
class ChildComponent<T> {
  @Input()
  String Function(T) itemRenderer;
}

... to ..

class ParentComponent {
  String renderName(Object a) => (a as Animal).name;
}

[eernstg]

Note also an option (4), which is the one that I've recommended, which says that members whose type contains a type variable from the enclosing class in a contravariant or invariant position should be given a static type which is sound (that is, a common supertype of all the possible actual types):

Foo<T>.member will have static type [Null/T]S when S is the declared type of member and T occurs contravariantly in S; and static type Function when T occurs invariantly in S and S is a function type; and static type Object(*) when T occurs invariantly in S and S is a generic instantiation of a class; and the static type S when T occurs covariantly or does not occur at all in S.

The (*) is there because we might instead want to use a superinterface where T is not invariant and not contravariant, e.g., because it doesn't depend on T at all.

For option (3), I'd recommend that we just support invariance as a separate feature. With that, developers would be able to get a safe and informative type for a "contravariant member" when they are ready to give up on covariance (which is perfectly fine for all the members whose type is actually covariant).

[jmesserly]

with (4) you wouldn't be able to call the function, though? Unless you pass null.

[eernstg]

Yes, because we should check that actual arguments satisfy the actual requirements. Relying on a check which guarantees that relationship statically would prevent us from ever putting anything else than null into a list (because there is no other statically expressible type than Null which is guaranteed to be less-equal than the actual type argument of any given list).

I do realize that we get that for free as long as we choose to check the type of an actual argument whose type is covariant (from class or via the covariant modifier) in the body of the callee, and it's more difficult to perform the same check from the caller side, but we can at least start "doing the right thing" in this respect in the case where the statically known parameter type contains a Null because it's in the signature one of these "contravariant members". (They aren't that common after all.)

Another case where it would be very useful to check against the actual parameter type rather than a statically known Null is with callbacks where we basically just want to specify the arity and somehow have a reason to trust that the call will be ok: int foo(int Function(Null) f) { f(something); }. That's currently done using (f as dynamic)(something), which means that we will get a completely dynamic invocation where we forget that the function is statically guaranteed to take one positional argument, etc.

[vsmenon]

Bump. @davidmorgan hit this again in built_value.

void main() {
  var i = new FunctionHolder<int>()..function = (int x) {};
  var j = i as FunctionHolder<dynamic>;
  print(i.function == j.function);
}

class FunctionHolder<T> {
  Function(T) function;
}

A runtime error occurs when we access j.function as the actual function isn't consistent with the static type of j.

[vsmenon]

@jmesserly - as an initial step, perhaps we can improve the runtime error on j.function?

[matanlurey]

In this particular case I think @davidmorgan hit it in the VM, so DDC would not be enough.

[vsmenon]

Good point. Are injecting the cast in kernel or the backends? If the former, we should probably mark it specially.

[jmesserly]

Good point. Are injecting the cast in kernel or the backends? If the former, we should probably mark it specially.

The front ends inject the cast (CFE/Analyzer). You can probably tell it's one of these casts already, because it will be an implicit cast to/from the same function type. Marking them would be cleaner though. I don't recall if Kernel does that or not.

[eernstg]

Considering the options mentioned by @vsmenon here, we do have an applicable device today:

(1) Disallow the contra-variant T (in getters / fields / return types). This would break a lot of code.

The lint unsafe_variance will flag these.

(2) Treat the contra-variant T as Object in the type system. I.e., anything assigned to callback has to actually be (Object) => void.

This is essentially dart-lang/language#296. We don't have that, though.

(3) Use this as a signal that T must be invariant in Foo. I.e., Foo f = new Foo() becomes a static error.

Again, the lint unsafe_variance will send this signal where needed.