Document contravariant function fields page

matanlurey commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 3 years ago

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 commented 1 year ago

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

munificent commented 1 year ago

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 commented 1 year ago

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: https://github.com/dart-lang/sdk/issues/59050.

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.
}

eernstg commented 6 days ago

The upcoming experimental lint unsafe_variance would be relevant here. It flags declarations where an instance member has a result type (that's the type of a variable or the return type of another kind of member) that contains a type parameter in a non-covariant position.

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

The solution that relies on privacy needs an // ignore comment because it will only work if an extra discipline is applied to the code "manually":

class Ecosystem<T> {
  // ignore: unsafe_variance
  final bool Function(T) _canAdd; // Only to be accessed on `this`!
  Ecosystem({required bool Function(T) canAdd}) : _canAdd = canAdd;
  bool canAdd(T animal) => _canAdd(animal);
}

The extra discipline is that _canAdd can only be accessed when the receiver is this. The lint doesn't attempt to check that rule, so we can't just silence the lint on all private declarations.

PS: The following approach is not safe:

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

bool test<S extends int>(_) => true;

void main() {
  Ecosystem<num> ecosystem = Ecosystem<int>(canAdd: test);
  ecosystem.canAdd; // Throws.
}

In general, using a type parameter of a class/mixin/enum/... in a member signature in a non-covariant position is unsafe, and the bound of a type parameter is sort of worst case because it is invariant. So that's not a solution.

dart-lang / site-www

Document contravariant function fields page #1017

Proposal

AVOID contravariant function fields

BAD

GOOD

GOOD