Lint on "covariant <Function>"?

[matanlurey]

I've started to see users to try and write the following:

void registerCallback(covariant Function onChange) {}

The problem with this code, is that, unless you are very careful, it is almost always wrong (see examples 1, 2, 3). The reason I've seen users try and write this is because they start with:

void registerCallback(void Function(dynamic) onChange) {}

... and due to dynamic no longer being bottom, it fails statically (covariant Function fails at runtime). The "correct" code is the hardest to remember, but I'm hoping we can also figure out a solution for that (i.e. a lint/hint/style guide change):

void registerCallback<V>(void Function(V) onChange) {
  _actualCallback = (dynamic value) => onChange(value as V);
}

DartPad example showing the following 3 cases:
https://dartpad.dartlang.org/9c0b30f65e8cf84d3107bdc8d23c8dcf

(Is it ever valid/intended to write covariant Function?)

[eernstg]

covariant Function isn't wrong per se, but it is of course a request for a dynamic check rather than a statically safe setup, and you're probably right that it's being used in cases where there are better solutions.

However, here's an example where it is used, and no other solution is available:

abstract class A { void foo(covariant Function f); }
class B1 implements A { void foo(Function({int i}) f) {}}
class B2 implements A { void foo(Function(int) f) {}}

The function types used to annotate f have least upper bound Function, and in that case we must use covariant Function (or covariant dynamic, covariant Object, etc, but covariant Function is the most informative choice available) in order to allow the type annotations in B1 and B2. So if you really need to use those type annotations then that's how you can do it.

On the other hand, if the problem is simply that void Function(dynamic) was used, but that's not working in Dart 2 because dynamic is no longer a bottom type, the type annotation should be changed to specify the bottom type that we actually have today: void Function(Null). That type admits any function that will accept one positional argument.

As an aside: Currently, invocations of a function of type void Function(Null) will be subject to a dynamic check to enforce that the actual argument is null, but that's an over-eager approach and we should instead check that the actual argument satisfies the actual requirement—just compare to myList.add(e) where a similar approach (i.e., requiring the actual argument to satisfy a constraint which is safe and statically expressible) would ensure that we couldn't ever add anything else than null to a list. That would be silly, and we should just stop being silly with function types in this respect. ;-) Anyway, until we get that fixed, we can force the actual argument to be checked against the actual requirement by using a dynamic invocation ((f as dynamic)(e)).

The approach where we use local static information to choose an argument type is interesting, but only safe "one level down":

// Could be an instance method, but the issue comes up
// even for a top-level function.
void registerCallback<V>(void Function(V) onChange) {
  _actualCallback = (dynamic value) => onChange(value);
  // `as V` needed if `--no-implicit-casts`.
}
void Function(dynamic) _actualCallback;

class A<X> { void Function(X) callback; }

main() {
  A<num> a = A((int i) => print("Hello, $i!"));
  registerCallback(a.callback);
}

We will call registerCallback with type argument num, and a.callback will fail to satisfy the constraint (void Function(int) is not a subtype of void Function(num)). So that just moves the discrepancy one step back.

We could introduce support for invariance (A<exactly num> a), and we could then stick to an approach where we use lots of type arguments and keep the whole thing sound.

Alternatively, we could introduce covariance for parameters of arbitrary functions:

registerCallback((covariant SomeType x) => baz(x));

The function literal would yield a function object of type void Function(dynamic) which will check dynamically that its actual argument is SomeType, and the body of the function literal would be allowed to use x as having type SomeType. Just like instance methods, only for all functions.

But the main point still stands: The proper type for a void function which must be unary, but the argument type can be anything (which is then checked dynamically) is void Function(Null), and that doesn't justify using covariant anywhere.

[matanlurey]

OK. It sounds like it worth requesting a lint for this then?

[eernstg]

I think you're right: There are cases where covariant Function can be justified, but they are completely marginal. So it makes sense to assume in general that it's a mistake, and then a developer who actually understands and needs it can always // ignore: the lint.

[matanlurey]

Thanks!

Do you think it makes sense to put this in the --strict-X category of optional analysis checks, have this be a standalone lint (in the linter package). We are starting to lean towards the former (so eventually we can say dartanalyzer --strict, and know what that means), but I could be talked out of it.

/cc @srawlins @natebosch @leafpetersen too.

[eernstg]

It's a little bit like an instance field Null x; which seems to be pointless, but could make sense in a class which implements Something<Null>. That is, it is not really "unstrict", it is just likely to be the result of confused thinking. Do we have anything like --flag-weird-stuff? ;-)

[eernstg]

It is my impression that there's nothing wrong with covariant Function, apart from all the reasons one might have to lint any occurrence of covariant parameters. So, presumably, the question would actually be:

Should we have a lint that flags every covariant parameter?

[matanlurey]

I no longer feel passionate about this issue :). I think it was a bigger issue when migrating to Dart 2.