Document contravariant function fields page

[matanlurey]

I think this is a big enough hurdle in Dart2 (especially from those used to Dart1) to prioritize. I've had to explain this separately to about 20 people this week, and not having anything good to point to has been a burden.

Related issues in the SDK:

• https://github.com/dart-lang/sdk/issues/33855
• Its is scary to use <Iterable>.firstWhere(orElse: ...) sdk#33841
• Give users more help in solving contravariance issues sdk#33697
• Dart should warn on guaranteed cast errors sdk#33372
• I wish for a way to specify generic variance sdk#33215
• Would like a way to avoid raw generic types sdk#33119
• I wish to remove implicit downcasts by default sdk#31410
• Customer request: Wildcard generics sdk#29777
• Allow variadic control of generics (i.e. List) sdk#33812
• Implement type alias parameter variance for super-bounded types sdk#33444
• Consider making strong mode read check less eager sdk#31391
• Analyzer doesn't catch that method argument has wrong signature sdk#30052
• Exact types sdk#33307
• StreamTransformer is difficult to use in Dart 2.0 sdk#29878

I might have missed some. @munificent I'd be happy to pair with you on writing this rule.

Proposal

AVOID contravariant function fields

In Dart 2, class type arguments (i.e. List<T> are covariant[link to description]), while
function type arguments (i.e. bool Function(T)) are contravariant[link to description]).
This can lead to tricky behavior (and runtime cast exceptions).

class Animal {}
class Cat extends Animal {}

void main() {
  var cats = [new Cat()]; // List<Cat>
  List<Animals> animals = cats; // OK, covariance allows upcast safely (for reads).
}

You can get into a problem when you try to combine a covariant generic type with a
contravariant one. For example, imagine we want a Ecosystem<T>, where T defines
the type of living organism that lives there:

class Ecosystem<T> {}

And imagine you want this class to be useful to consumers without having to sub-class it to
provide functionality, so you decide to take a first-class function to define some behavior,
such as a check if there is enough room for a new animal to join that ecosystem:

class Ecosystem<T> {
  final bool Function(T) canAdd;
  Ecosystem({this.canAdd});
}

This can run into very tricky situations, because an Ecosystem<Cat> can be upcasted to
an Ecosystem<Animal>, but a Function(Cat) cannot accept a Function(Animal)
[insert more explanation why]:

void main() {
  var catWorld = new Ecosystem<Cat>(canAdd: (Cat c) => /* TODO */ true);
  Ecosystem<Animal> animalWorld = catWorld; // OK
  animalWorld.canAdd; // TypeError: (Animal) => bool is not a type of (Cat) => bool
}

This is because of [insert more explanation why].

BAD

class Ecosystem<T> {
  final bool Function(T) canAdd;
  Ecosystem({this.canAdd});
}

GOOD

Ensure the user-visible function is a method on the class, not a first class function:

class Ecosystem<T> {
  final bool Function(T) _canAdd;
  Ecosystem({bool Function(T) canAdd}) : _canAdd = canAdd;
  bool canAdd(T animal) => _canAdd(animal);
}

This works because the T animal above uses the same covariant ... [insert more description].

GOOD

If you must make the user-visible type a function (i.e. you want the field to be mutable), use
bounded sound generics [insert better name of whatever this is actually called]. For example:

class Ecosystem<T> {
  bool Function<S extends T>(S) canAdd;
  Ecosystem({this.canAdd});
}

The S extends T ensures that the type S is ... [insert more description].

/cc @srawlins @leafpetersen @eernstg

[lrhn]

Dart has unsafe covariant class generics. That has consequences.

We use the "covariant by generics" treatment of methods, where List<E>.add effectively has type void Function(Object) at run-time, and it performs a type check that the argument is an E, to delay some of that unsafeness until the covariant type is used (the method is called). We can only do that for method parameters, nothing else.

In particular, we can't do that for function-typed values anywhere. It's not fields that are the problem, you get the same issue with return types of method or function-typed arguments.
Example:

class C<T> {
  T value;
  C(this_value);
  void Function(T value) makeSetter() => (T value) { this.value = value; };
  void computeAndSet(T Function() f) { this.value = f(); }
}
main() {
  var i = C<int>(42);
  C<num> n = i;
  n.value = 42.0;  // fails at run-time (unsurprisingly)
  void Function(num) setter = n.makeSetter();  // fails at run-time.
  var setter = n.makeSetter();  // fails at run-time, because we infer the static type.
  n.computeAndSet(() => 42.0);  // fails at run-time.
}

So, the issue is with any function type in the API which mentions the type variable in a contravariant position, not just fields.

We also can't really avoid these cases (Map.putIfAbsent, Iterable.firstWhere(...orElse) etc, all take functions which return something of the collection's element/value type. Those are inherently unsafe, you need to use them at the correct type. They are also very convenient, so we can't avoid them completely (they exist on Iterable and Map!)

So, restricting return values (not just fields, fields just have getters with their type as return value) is probably a good recommendation, but not enough to completely avoid the issue. Restricting function typed arguments to no use the class type parameter in a contravariant manner is not viable.

As for the "good" example:

class Ecosystem<T> {
  bool Function<S extends T>(S) canAdd;
  Ecosystem({this.canAdd});
}

you are making canAdd a generic function. The argument function must be generic and have T as the exact bound, otherwise the generic function type is not assignable at all. If anything, this is more restrictive than the original non-generic function type.

[munificent]

Yes, I totally agree we need to do something to explain this issue once and for all in a canonical place we can point users to. (Perhaps the runtime error can even link to it.) I think the explanation is too big to cram into "Effective Dart", but maybe we can just create a standalone article for it.

Once I'm back from giving some training and more on top of other tasks, I can try to set aside some time to work on this.

Eventually, I hope we can fix this more directly by supporting static control over variance in generics. Covariant-everything was dubious but maybe reasonable with Dart 1's type system. With a strong type system, the whole value proposition shifts and it looks increasingly like a bad deal to me.

[eernstg]

animalWorld.canAdd; // TypeError: (Animal) => bool is not a type of (Cat) => bool

We shouldn't have any caller-side checks at all. We also shouldn't ignore the soundness issue, of course, but invariance (e.g., declaration-site invariance: dart-lang/language#214) could be used to eliminate the unsoundness.

If we give developers the ability to address this unsoundness issue at the root then we might be able to treat the occurrences that aren't fixed a more harsh treatment: Give these expressions a sound type (that is, a more general type than it has today, but one that the dynamic semantics will actually satisfy). Then there will be some additional downcasts or some newly failing member accesses (because that supertype doesn't have a particular member that the subtype had), but we could then eliminate the whole notion of caller-side checks.

[munificent]

I removed the "EffectiveDart" tag because I think this is too deep of an issue to fit into the guidelines. I think it worth having a doc somewhere that explains how covariant generics work including the consequence of contravariance like the issue here. Over the long term, I would love to move away from unsound covariance in general once we have @eernstg's variance annotations.

[atsansone]

@munificent : Is this still a going concern? Has it been handled elsewhere?

[munificent]

I don't think it's been handled elsewhere as far as I know. Now that the Dart 2.0 migration is done and long gone, it seems to crop up less as an issue. It may be that we can just not worry about documenting this.

[eernstg]

I agree that we might not need to add anything to site-www about "contravariant members".

There is no particular connection between contravariant members and Dart 2. It was unsafe before Dart 2, and it is unsafe today. However, you might say that it is unnecessary to document the wildly unsafe nature of a getter/method return type which is non-covariant in the class type parameters if we get this lint: dart-lang/linter#4111.

The lint would have its own documentation, and the core of this documentation would be printed as a diagnostic message whenever any such member is encountered and the lint is enabled. So developers would get to know about this issue when they need to know about it.

PS: I'm using the phrase 'wildly unsafe' because the caller-side check will throw just because the member is accessed, even if it is a function that would accept the given invocation:

class B<X> {
  foo(X x) {} // A regular method.
}

class C<X> {
  final void Function(X) foo; // A field with the same function type as `B.foo`.
  C(this.foo);
}

void main() {
  B<num> b = B<int>();
  b.foo; // OK, tearing off a statically known method will never fail.
  b.foo(1); // OK, there is a dynamic check, but it succeeds.

  C<num> c = C<int>((i) {});
  c.foo; // Just mention it, and we die.
  c.foo(1); // Would work, but we never reach the point where the function is called.
}