Implement missing warnings for generic methods

[leafpetersen]

Tracking bug for remaining missing warnings to be implemented in the analyzer for generic methods.

• Arity matching on type instantiations
• Check that type arguments satisfy the bounds
• Check that implicit instantiations result in valid instantiations (strong mode only)
• Check that overrides of a non-generic method with a generic one is valid (implemented but perhaps not correct, see comment below)
• Check for unbound generic type variables
• possible issue in _isFunctionSubtype, see comment (added by @jmesserly)

(edit by @jmesserly: Check that bounds (on both classes and generic methods) are not themselves generic function types -- moved to #25179)

[jmesserly]

I believe artiy warnings during instantiate has been fixed (ElementResolver._resolveGenericMethod). I don't think we check bounds, however.

[jmesserly]

Check that implicit instantiations result in valid instantiations (strong mode only)

Is this covered by StrongTypeSystemImpl.instantiateToBounds + our existing checks? I think we'll implicitly instantiate with the bounds, then later we'll do appropriate type checking against the parameters/return type as we normally would.

One case that is not covered is the "override a generic method with a non-generic method" case.

[jmesserly]

Check for unbound generic type variables

I believe this check already exists in Analyzer:

abstract class Iter {
  List/*<S>*/ map(/*=S*/ f(x)) => <dynamic/*=T*/>[];
}

[warning] The name 'S' is not a type and cannot be used as a parameterized type 
[warning] Undefined class 'S'
[warning] The name 'T' is not a type and cannot be used as a parameterized type
3 warnings found.

[jmesserly]

Check that bounds (on both classes and generic methods) are not themselves generic function types

Just checking, I believe this can't be expressed right now, until we fix #25179? In other words you can't write:

typedef generic S F<S>(S x);
void foo<T extends F>(T t) {}

Something to think about when we implement 25179.

[leafpetersen]

I think that's correct.

[jmesserly]

also can confirm overrides are being checked. _checkForAllInvalidOverrideErrorCodes handles checking for matching number of type params, as well as instantiation if generic overrides non-generic. (around the "// Handle generic function type parameters." comment)

EDIT: this code looks busted so I've unchecked it. In particular this looks wrong:

      if (overriddenFT.typeFormals.isEmpty) {
        overriddenFT = _typeSystem.instantiateToBounds(overriddenFT);
      }

If overriddenFT has no typeFormals, instantiateToBounds is a no-op... that doesn't look right. I think it meant to use overridingFT. Either way this is a bit worrying and needs further investigation.

[jmesserly]

Oh, just noticed: TYPE_ARGUMENT_NOT_MATCHING_BOUNDS is only a warning even in strong mode. I think we should bump it to an error? (easy to do, there's already an error for when it happens in constant evaluation)

[leafpetersen]

Yes, I think that should be an error.

[jmesserly]

edit: this is fixed in f976407. My original message is below for context.

---

BTW @leafpetersen -- I was reading through the code in _isFunctionSubtype ... I think we may currently allow () -> void <: <T>() - > void. This may allow you to override a generic function with a non-generic one. Another subtype we may allow is (*) -> * <: <T>(T) -> T.

I suspect it's caused by this code:

    if (!f1.typeFormals.isEmpty) {
      if (f2.typeFormals.isEmpty) {
        f1 = instantiateToBounds(f1);
        return _isFunctionSubtypeOf(f1, f2);
      } else {
        return _isGenericFunctionSubtypeOf(f1, f2, fuzzyArrows: fuzzyArrows);
      }
    }

In particular, I don't see anything in the method that checks to ensure f2.typeFormals.isEmpty. I expected to find a case like:

} else if (f2.typeFormals.isNotEmpty) {
  // non-generic functions can't subtype generic ones.
  return false;
}

_isGenericFunctionSubtypeOf will always return return false if the parameter count is mismatched, but I don't think we'll get there.

[jmesserly]

Another issue w/ universal types: the spec type system didn't know how to handle them. This led to breaking some invariants, like this assert in our DownCast.create:

    // toT <:_R fromT => to <: fromT
    // NB: classes with call methods are subtypes of function
    // types, but the function type is not assignable to the class
    assert(toT.isSubtypeOf(fromT) || fromT.isAssignableTo(toT));

Fix for both issues on the way.

[bwilkerson]

@leafpetersen Re:

Check that type arguments satisfy the bounds

Can you give me an example of a failing case? I tried to come up with one and failed (that is, analyzer correctly produces an error for this case), so I'm not sure what needs to be done for it:

class C<E extends num> {}
main() {
  C<String> c; // <-- ERROR: 'String' does not extend 'num'
}

[jmesserly]

I think it would happen for generic methods:

import 'dart:math';

Point/*<T>*/ f/*<T extends num>*/(num/*=T*/ x, num/*=T*/ y) {
  return new Point(x, y);
}

main() {
  print(f/*<String>*/('hello', 'world'));
}

$ dart test.dart
Point(hello, world)
$ dartanalyzer --strong test.dart
Analyzing [test.dart]...
No issues found
$ dart -c test.dart
Unhandled exception:
type 'String' is not a subtype of type 'num' of 'x' where
[...]

If I get rid of the <String> I get some warnings (I think DDC treats them as errors)

$ dartanalyzer --strong test.dart
Analyzing [test.dart]...
[warning] The argument type 'String' cannot be assigned to the parameter type 'num'. (/Users/jmesserly/tmp/test.dart, line 8, col 11)
[warning] The argument type 'String' cannot be assigned to the parameter type 'num'. (/Users/jmesserly/tmp/test.dart, line 8, col 20)
2 warnings found.

[bwilkerson]

Of course! I'll look into producing the errors.

[bwilkerson]

Some errors caught by https://codereview.chromium.org/1927103002/.

[jmesserly]

I think Brian fixed this. If we see any more issues we can file a new bug.