Avoid contravariant function fields

[matanlurey]

Background: dart-lang/site-www#1017

I got positive support for this rule from other internal TLs on dart-lints@:

For example:

class Model<T> {
  final bool Function(T) isGood;
  Model(this.isGood);  
}

void main() {
  var typed = new Model<String>((e) => e == 'Matan');
  Model<dynamic> untyped = typed;
  untyped.isGood; // Error: (dynamic) => bool not (String) => bool.
}

Here is my proposed lint in short:

class Model<T> {
  // LINT: avoid_contravariant_function_fields
  final bool Function(T) isGood;
  Model(this.isGood);
}

I imagine we could allow private function fields accessed through a safe method:

class Model<T> {
  // OK since private and accessed through a member element.
  final bool Function(T) _isGood;
  Model(this._isGood);

  // OK since covariance check
  bool isGood(T element) => _isGood(element);
}

If we ever get variance control, invariant "T" could be allowed safely.

The one real issue with be landing this lint, there already many (mis)-uses of this pattern. I could see (a) adding yet-another annotation @suppressUnsafeGeneric, (b) using // ignore: avoid_contravariant_function_fields, or (c) making this a non-error in google3 (i.e. a hint on new code only).

[eernstg]

We're considering invariance support in the language. In this situation, I believe declaration-site invariance (dart-lang/language#214) would fit quite well.

[matanlurey]

I wonder, @srawlins - do you think this is still valuable?

[srawlins]

I think we can keep this open, if only as an approximate, partial, lightweight, supplemental linter rule to dart-lang/language#214

[eernstg]

Yes, please! With Dart of today, this lint can inform developers that they're creating a dangerous situation if they ever declare an instance variable whose type has a non-covariant occurrence of a class type variable (or a method with such an occurrence in its return type, etc).

But if and when we get support for statically checked variance (declaration-site, a more recent proposal: dart-lang/language#524, or use-site: dart-lang/language#753, or preferably both), developers would then have the tool they need in order to eliminate the potential for run-time errors.

class C<X> { // Make it `in X` or `inout X`, and the danger goes away.
  void Function(X) myDangerousFunction;
  C(this.myDangerousFunction);
}

I think the dynamic error that we have today is considerably more dangerous than having a method parameter with the same property (like List.add): The caller-side check that may cause an exception here kicks in if the run-time type of the receiver is a subtype of the static type that involves variance (e.g., the static type is C<num> and the dynamic type is C<int>). For example:

class C<X> {
  void Function(X) f;
  C(this.f);
}

void main() {
  C<num> c = C<int>((int i) => print(i));
  (c as dynamic).f(1); // No problem: pass `1` to a `void Function(int)`.
  c.f(1); // Throw, even though `1` _is_ an OK argument to `c.f`!
}

So when we're calling c.f(1) it isn't sufficient that 1 is a type correct argument to the function yielded by c.f, the evaluation throws already when we're evaluating c.f to obtain the function itself, and that function doesn't have the static type.

So I believe that we need this lint without statically checked variance, and we need it with statically checked variance, but in the latter case we would need to include advice about using statically checked variance to eliminate the problem.

[eernstg]

The upcoming experimental lint unsafe_variance could be seen as a response to this request. See also dart-lang/site-www#1017 (comment).