add covariance checks for parameters

[jmesserly]

we need to add covariance checks for parameters, e.g. List<E>.add ... given

class Foo<T> {
  T _t;
  add(T t) {
    _t = t;
  }

  forEach(void fn(T t)) {
    // No check needed for `fn`
    fn(_t);
  }
}

class Bar extends Foo<int> {
  add(int x) {
    print('Bar.add got $x');
    super.add(x);
  }
}
main() {
  Foo<Object> foo = new Bar();
  foo.add('hi'); // should throw
}

currently I'm adding something simple to JS codegen, but it adds extra checks (Foo.forEach) and it misses checks (Bar.add). Also ideally we'd compute this earlier, in checker/coercion reifier.

[jmesserly]

So, thinking about this again, now that I understand better how types work in Analyzer:

What we want to do is instantiate Foo<T> to its upper bound (edit: meant upper, not lower :) ), look up our base method type Mb and our own method type Md, then for each parameter type Pb_i and Pd_i, see if Pb_i <: Pd_i. If that is not the case, we need a check for parameter i.

(Incidentally: that's the same logic we'd use for covariant overrides, if we allow opt-in to that feature. The extra step here is we need to instantiate our base generics as their lower bounds. I'm assuming that strong mode will already disallow instantiation where it does not match the lower bound, e.g. given Rectangle< T extends num > it will not be allowed to instantiate that with dynamic.)

[leafpetersen]

I'm not sure I follow this. What lower bound do you mean, when you say "instantiate Foo<T> to it's lower bound"?

[jmesserly]

Haha, oops, I meant upper bound :)

[jmesserly]

In other words: Foo<Object>. The covariance problem happens because Foo<int> <: Foo<Object>, which means the static type system will implicitly allow call sites like: Foo<int>.add <: Foo<Object>.add, or in other words, int -> void <: Object -> void, which is not what we want. Replacing the type parameter with its upper bound would be an easy way to find those soundness violations.

(We could certainly look at each parameter and see if the type parameter T occurs in certain positions in the base method's parameters. I thought subtype might be an easy way to get the same effect.)

[jmesserly]

Another example:

class C<T extends num> {
  add(T t) {}
}
class D extends C<int> {
  add(Object obj) {}
}

Here we wouldn't need a check on D.add, because: {num/T}T <: Object.

[leafpetersen]

Ah, I see. Yes, I think that makes sense, and could be a nice way to do it. You may need to think about what exactly the base method you compare to should be. For example:

class C<T> {
  add(T t) {}
}
class D extends C<int> {
  add(int i) {}
}
class E extends D {
  add(num obj) {}

Here both overrides need a check, but if you only compare the type of E.add against D.add I don't think you'll see it. It's possible it's always sufficient to check against the root definitions in the hierarchy (presumably there can be multiple roots). Might be worth thinking through the corner cases though.

[jmesserly]

Chatted with Leaf, another thing we should consider:

It would be nice if Analyzer knows about these implicit downcasts, so we can go through our normal warning logic. For example Iterable<E> or E fn() are downcasts that are likely to fail (warning:DOWN_CAST_COMPOSITE).

[jmesserly]

@rakudrama noticed another bug with the currently implemented logic -- it adds checks inside inner lambda, so for example:

class C<T> {
  Iterable<T> iter;
  m() {
    return iter.any((i) => i != null);
  }
}

there's a check added inside the lambda to assert that i is a T which is not necessary.

[leafpetersen]

https://codereview.chromium.org/1988613006/ addresses the unnecessary checks inside of lambdas, and also eliminates checks when the type parameter is only used covariantly.

[vsmenon]

This issue was moved to dart-lang/sdk#27259