fix part of #25200, reject non-generic function subtype of generic function

fix part of #25200, we now reject non-generic fn subtype of generic fn

A few other fixes:
* inference of generic function expressions now works. It was broken because they ignored type parameters.

* fixes subtype of generic functions in the "normal" type system. These types are only possible if generic methods support is enabled, which can be accessed via a flag. The generic methods DEP is designed to work with the normal Dart type system, too, so we try to keep these code paths working. ("normal" type equality for function types already handled this correctly.) This is also needed because strong mode delegates to normal type system for some cases, e.g. to pick warning vs error.

* also refactors structural comparison of two generic function types, along the lines of recent changes, so there is less code duplication. This was my original motivation, but reviewing the code led to the discovery of the other issues.

* refactors strong_test_helper. The existing way of matching up errors did not work with generic function expressions like: /*<T>*/(x) => x. Simplified it to just look for the right offset. This also exposed a bug in a test -- previously, it was possible to have an error that wasn't validated.

R=leafp@google.com, rnystrom@google.com

Committed: https://github.com/dart-lang/sdk/commit/f97640745e533b47fe9208f4724b0ab5b9230dc9

[Jennifer Messerly, 2016-02-08 21:36:42]

Description was changed from

==========
fix part of #25200, we now reject non-generic fn subtype of generic fn

A few other fixes:
* inference of generic function expressions now works. It was broken because
they ignored type parameters.

* fixes subtype of generic functions in the "normal" type system. These types
are only possible if generic methods support is enabled, which can be accessed
via a flag. The generic methods DEP is designed to work with the normal Dart
type system, too, so we try to keep these code paths working. ("normal" type
equality for function types already handled this correctly.)

* also refactors structural comparison of two generic function types, along the
lines of recent changes, so there is less code duplication. This was my original
motivation, but reviewing the code led to the discovery of the other issues.
==========

to

==========
fix part of #25200, we now reject non-generic fn subtype of generic fn

A few other fixes:
* inference of generic function expressions now works. It was broken because
they ignored type parameters.

* fixes subtype of generic functions in the "normal" type system. These types
are only possible if generic methods support is enabled, which can be accessed
via a flag. The generic methods DEP is designed to work with the normal Dart
type system, too, so we try to keep these code paths working. ("normal" type
equality for function types already handled this correctly.) This is also needed
because strong mode delegates to normal type system for some cases, e.g. to pick
warning vs error.

* also refactors structural comparison of two generic function types, along the
lines of recent changes, so there is less code duplication. This was my original
motivation, but reviewing the code led to the discovery of the other issues.

* refactors strong_test_helper. The existing way of matching up errors did not
work with generic function expressions like: /*<T>*/(x) => x. Simplified it to
just look for the right offset. This also exposed a bug in a test -- previously,
it was possible to have an error that wasn't validated.
==========

[Jennifer Messerly, 2016-02-08 21:36:42]

jmesserly@google.com changed reviewers:
+ leafp@google.com, paulberry@google.com, rnystrom@google.com

[Jennifer Messerly, 2016-02-08 21:38:58]

Hey everyone, here's a working version of this CL now.

Feel free to comment on anything, but especially:

@Leaf -- mind taking a look at type system/inference changes?
@Paul, Bob -- mind taking a look at structuralCompareGeneric, and change to
tests?

[Jennifer Messerly, 2016-02-08 21:54:27]

A few comments to help review.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:7305: FunctionType
matchFunctionTypeParameters(
I feel like there's got to be a better way to factor this. It's sort of
duplicating the subtype logic. But we needed to do something to handle universal
types correctly.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:7333: return fnType.instantiate(
This is a case like:

    // declare generic function
    T f<T>(T t) => null;
    // declare variable with universal type
    var v = f;
    // downward infer
    v = <S>(x) => x;

It would be wrong to infer <S>(T) -> T. So we can now infer <S>(S) -> S. This is
similar to what member inference does now for generic methods:

abstract class MyIterable<T> extends Iterable<T> {
  // infers as <S>(T -> S) -> Iterable<S>
  map<S>(fn(x));
}

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (left):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:169: // TODO(jmesserly): for
performance, we could refactor this so the _infer
I went ahead and addressed this TODO. That's what `allowPartialSolution` is for.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:291: return
_isFunctionSubtypeOfRaw(f1, f2, fuzzyArrows: fuzzyArrows);
I feel like we don't want to drop the fuzzyArrows flag here, but I might be
missing something.

(Also we can skip checking typeFormals again -- both will be empty at this point
-- that's why it calls straight into the "raw type" version)

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:295: } else if
(f2.typeFormals.isNotEmpty) {
this was the else case that I believe was missing

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1500: v =
/*info:INFERRED_TYPE_CLOSURE*//*<T>*/(int x) => null;
this was rather broken before -- not just the implementation, but it was
impossible to test because we matched up the error locations incorrectly. I
spent a lot of time trying to figure out what was going on. Eventually ended up
rewriting much of the test infrastructure so it diffs errors in an easier to
understand/debug way.

[Bob Nystrom, 2016-02-09 01:25:18]

LGTM.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:740: *
Writing nit: It's easy to get into the habit of future tense, but present is
usually better.

http://www.writer-on-line.com/content/view/319/66/~Articles/Technical-Writing...

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:764: List<DartType> variablesFresh =
new List<DartType>();
<DartType>[]

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:769: new
TypeParameterElementImpl(p2.name, -1);
What do you think about either making the offset optional or adding a
.synthetic() named constructor?

Should its isSynthetic property return true?

[Leaf, 2016-02-09 21:29:25]

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:703: 
Why does this ignore the generic case?  If just not handled yet, then maybe
TODO?

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:707: (DartType t, DartType s) =>
Non-actionable comment, but the TypeImpl/Type distinction doesn't seem to be
very coherent in the code right now.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:717: // This type cast is safe,
because _trivialFunctionRelation checks for it.
It feels like this code could become part of structuralCompareGeneric.  It would
presumably need to be parameterized over the instantiation to bounds though,
which might start to get heavy.  It almost feels like structuralCompareXXX
should be shared methods on the TypeSystem class.  I think we don't have access
to an appropriate type system instance here though, unfortunately.  So maybe
this is also a non-actionable comment, but food for thought?

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:721: List<DartType> dynamicTypeArgs
= new List<DartType>.filled(
Would be nice to use instantiateToBounds here, but I guess that doesn't work out
with the way that the current separation between TypeSystem methods and *Type
methods works.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:732: return structuralCompare(
See comment below on the structuralCompare/structuralCompareGeneric split.  I
think this code gets clearer if there's just a little logic to decide whether to
instantiate to bounds or return false in the mismatch case, and otherwise just
calls directly to structuralCompare.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:748: static bool
structuralCompareGeneric(
I'm not very keen on the current factoring of this into structuralCompare and
structuralCompareGeneric (and some of the resultant consequences).  It seems to
be almost always the case that there is some twisty conditional logic to see
whether or not to call structuralCompare, or to call structuralCompareGeneric
with a callback that will essentially end up calling structuralCompare.  This
gives the small optimization benefit of in some cases not checking for the
presence of formals twice, at quite a bit of readability expense.  I think this
code would be much clearer if the top level function was just structuralCompare,
with named parameters for the various sub-component comparison callbacks as
needed.  This could then be factored out to structuralCompareTypeFormals and
structuralCompareParametersAndReturn or somesuch if desired and/or if there
really is some code that truly needs to be able to call directly into only one
of these as well.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:809: } else if (this == other) {
Obviously not a change introduced into this CL (in fact, I think maybe not even
introduced by the refactoring), but I'm just getting caught up on some of the
changes to the code since I last saw it, and this jumped out.  This last check
for full equality seems like a bad idea to me.  I don't think that checking
equality is much faster than doing the full check, and doing a full equality
check at every iteration makes this code quadratic in some unlikely cases, and
twice as slow in lots of likely cases.

I believe that the code can be quadratic because equality is linear, and
_trivialFunctionRelation is called as a short-circuit check on every call to
various relations (e.g. more specific than).  For two nested function types that
are each nested n levels deep that differ only at the very last node, we will
check for equality n times.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:56: /// parameters from the
actual argument types, and return it. If we can't.
"return it" -> "return the instantiated function type"?

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:64: /// running the subtype
algorithm, and when we reach a type parameter Pj,
Pj should be Tj?

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:97: // make any type information
resulting from it more precise.
This is purely to make error messages better, right?  Presumably if we couldn't
find a solution, then the solution we get here is invalid, and we will get an
error somewhere else?  But by allowing the partial instantiation, we reduce the
errors to a fairly minimal set?  Will it be confusing to see errors in terms of
the partially instantiated types?  I'm feeling a little wobbly about what's
going on with this - maybe clarifying in comments, possibly with some examples
would be worthwhile.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:126: // formals as we check the
parameters and return type.
I would suggest phrasing this comment differently.  I think what's going in is
that notionally what we are doing is instantiating fnType with fresh type
variables (or re-using it's existing type variables) to produce an instantiated
function type, and then unifying with the contextType to solve for the type
variables.  As an optimization, we can avoid allocating a new function type
simply by calling a version of the subtype function which ignores the type
formals and solving for the free type parameters of the body of the universal.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:291: return
_isFunctionSubtypeOfRaw(f1, f2, fuzzyArrows: fuzzyArrows);
I really think this code would be better of being clearer and slightly
efficient.  e.g. something like:

if (f1.typeFormals.length != f2.typeFormals.length) {
  if (f1.typeFormals.isNotEmpty) {
    f1 = instantiateToBounds(f1);
 } else {
   return false;
}
return _isGenericFunctionSubtypeOf(f1, f2, fuzzyArrows: fuzzyArrows);

Yes, _isGenericFunctionSubtypeOf will have to recheck for type formals, but... I
think it's worth it for clarity.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:295: } else if
(f2.typeFormals.isNotEmpty) {
On 2016/02/08 21:54:27, John Messerly wrote:
> this was the else case that I believe was missing

Can you add a test for this in type_system_test.dart, probably in
test_genericFunction_generic_monomorphic ?

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:307: bool
_isFunctionSubtypeOfRaw(FunctionType f1, FunctionType f2,
I don't really like the name of this - Raw doesn't really mean anything to me. 
Consider being more verbose, since this is not intended to be used much?
_isFunctionSubtypeOfIgnoringTypeFormals?  _isUniversalTypeBodySubtypeOf?

[Jennifer Messerly, 2016-02-10 22:42:08]

Hey Leaf & Bob! thanks for comments, think I got them all. Let me know what you
think of this version.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:703: 
On 2016/02/09 21:29:25, Leaf wrote:
> Why does this ignore the generic case?  If just not handled yet, then maybe
> TODO?

Good catch. Fixed!

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:707: (DartType t, DartType s) =>
On 2016/02/09 21:29:25, Leaf wrote:
> Non-actionable comment, but the TypeImpl/Type distinction doesn't seem to be
> very coherent in the code right now.  

Yeah, definitely. I'm trying to get things strong mode clean (in another branch)
and there are definitely issues around unsafe *Impl downcasts.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:717: // This type cast is safe,
because _trivialFunctionRelation checks for it.
On 2016/02/09 21:29:25, Leaf wrote:
> It feels like this code could become part of structuralCompareGeneric.  It
would
> presumably need to be parameterized over the instantiation to bounds though,
> which might start to get heavy.  It almost feels like structuralCompareXXX
> should be shared methods on the TypeSystem class.  I think we don't have
access
> to an appropriate type system instance here though, unfortunately.  So maybe
> this is also a non-actionable comment, but food for thought?

Great idea. Done.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:721: List<DartType> dynamicTypeArgs
= new List<DartType>.filled(
On 2016/02/09 21:29:25, Leaf wrote:
> Would be nice to use instantiateToBounds here, but I guess that doesn't work
out
> with the way that the current separation between TypeSystem methods and *Type
> methods works.

Added instantiateToBounds parameter.

I do wonder if this code (now called "FunctionTypeImpl.relate" as in "relate two
function types") should be in TypeSystem, though.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:732: return structuralCompare(
On 2016/02/09 21:29:25, Leaf wrote:
> See comment below on the structuralCompare/structuralCompareGeneric split.  I
> think this code gets clearer if there's just a little logic to decide whether
to
> instantiate to bounds or return false in the mismatch case, and otherwise just
> calls directly to structuralCompare.

Fixed :)

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:740: *
On 2016/02/09 01:25:18, Bob Nystrom wrote:
> Writing nit: It's easy to get into the habit of future tense, but present is
> usually better.
> 
>
http://www.writer-on-line.com/content/view/319/66/~Articles/Technical-Writing...

Acknowledged.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:748: static bool
structuralCompareGeneric(
On 2016/02/09 21:29:25, Leaf wrote:
> I'm not very keen on the current factoring of this into structuralCompare and
> structuralCompareGeneric (and some of the resultant consequences).  It seems
to
> be almost always the case that there is some twisty conditional logic to see
> whether or not to call structuralCompare, or to call structuralCompareGeneric
> with a callback that will essentially end up calling structuralCompare.  This
> gives the small optimization benefit of in some cases not checking for the
> presence of formals twice, at quite a bit of readability expense.  I think
this
> code would be much clearer if the top level function was just
structuralCompare,
> with named parameters for the various sub-component comparison callbacks as
> needed.  This could then be factored out to structuralCompareTypeFormals and
> structuralCompareParametersAndReturn or somesuch if desired and/or if there
> really is some code that truly needs to be able to call directly into only one
> of these as well. 

All factoring is fixed :D. At least as much as possible without moving code over
to TypeSystem.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:764: List<DartType> variablesFresh =
new List<DartType>();
On 2016/02/09 01:25:18, Bob Nystrom wrote:
> <DartType>[]

Done.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:769: new
TypeParameterElementImpl(p2.name, -1);
On 2016/02/09 01:25:18, Bob Nystrom wrote:
> What do you think about either making the offset optional or adding a
> .synthetic() named constructor?
> 
> Should its isSynthetic property return true?

Done.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:809: } else if (this == other) {
On 2016/02/09 21:29:25, Leaf wrote:
> Obviously not a change introduced into this CL (in fact, I think maybe not
even
> introduced by the refactoring), but I'm just getting caught up on some of the
> changes to the code since I last saw it, and this jumped out.  This last check
> for full equality seems like a bad idea to me.  I don't think that checking
> equality is much faster than doing the full check, and doing a full equality
> check at every iteration makes this code quadratic in some unlikely cases, and
> twice as slow in lots of likely cases.
> 
> I believe that the code can be quadratic because equality is linear, and
> _trivialFunctionRelation is called as a short-circuit check on every call to
> various relations (e.g. more specific than).  For two nested function types
that
> are each nested n levels deep that differ only at the very last node, we will
> check for equality n times.

Good catch. Removed the unnecessary ==.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:56: /// parameters from the
actual argument types, and return it. If we can't.
On 2016/02/09 21:29:25, Leaf wrote:
> "return it" -> "return the instantiated function type"?

Done.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:64: /// running the subtype
algorithm, and when we reach a type parameter Pj,
On 2016/02/09 21:29:25, Leaf wrote:
> Pj should be Tj?

Done. Good catch.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:97: // make any type information
resulting from it more precise.
On 2016/02/09 21:29:25, Leaf wrote:
> This is purely to make error messages better, right?  Presumably if we
couldn't
> find a solution, then the solution we get here is invalid, and we will get an
> error somewhere else?  But by allowing the partial instantiation, we reduce
the
> errors to a fairly minimal set? 

Yes to all. :)

> Will it be confusing to see errors in terms of
> the partially instantiated types?

Let's say our function returns `Map<T, U>` and we inferred T := int but not U.

For example, you might see an error about `Map<int, dynamic>` instead of
`Map<dynamic, dynamic>`.

the `int` might be helpful, for example, if you did `map.keys` you'd get
`Iterable<int>`, which might avoid further errors.

I'm trying to avoid an error in one parameter causing more downstream errors
than it needs to.

This is definitely different from how we handle:

    foo(<T, U>(...) => ...);

if we fail to infer only T but not U, we leave it as a generic type.

>  I'm feeling a little wobbly about what's
> going on with this - maybe clarifying in comments, possibly with some examples
> would be worthwhile.

I added a little bit, but it's definitely a heuristic we should get more user
experience/feedback with.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:126: // formals as we check the
parameters and return type.
On 2016/02/09 21:29:25, Leaf wrote:
> I would suggest phrasing this comment differently.  I think what's going in is
> that notionally what we are doing is instantiating fnType with fresh type
> variables (or re-using it's existing type variables) to produce an
instantiated
> function type, and then unifying with the contextType to solve for the type
> variables. 

Hah, yes, indeed. :) It now works like that. More clear?

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:291: return
_isFunctionSubtypeOfRaw(f1, f2, fuzzyArrows: fuzzyArrows);
On 2016/02/09 21:29:25, Leaf wrote:
> I really think this code would be better of being clearer and slightly
> efficient.  e.g. something like:
> 
> if (f1.typeFormals.length != f2.typeFormals.length) {
>   if (f1.typeFormals.isNotEmpty) {
>     f1 = instantiateToBounds(f1);
>  } else {
>    return false;
> }
> return _isGenericFunctionSubtypeOf(f1, f2, fuzzyArrows: fuzzyArrows);
> 
> Yes, _isGenericFunctionSubtypeOf will have to recheck for type formals, but...
I
> think it's worth it for clarity.

Done as part of the Grand Refactoring :)

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:295: } else if
(f2.typeFormals.isNotEmpty) {
On 2016/02/09 21:29:25, Leaf wrote:
> On 2016/02/08 21:54:27, John Messerly wrote:
> > this was the else case that I believe was missing
> 
> Can you add a test for this in type_system_test.dart, probably in
> test_genericFunction_generic_monomorphic ?

I believe this is covered by the tests I added to checker tests.

https://codereview.chromium.org/1678313002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:307: bool
_isFunctionSubtypeOfRaw(FunctionType f1, FunctionType f2,
On 2016/02/09 21:29:25, Leaf wrote:
> I don't really like the name of this - Raw doesn't really mean anything to me.

> Consider being more verbose, since this is not intended to be used much?
> _isFunctionSubtypeOfIgnoringTypeFormals?  _isUniversalTypeBodySubtypeOf?

It's gone :D

[Jennifer Messerly, 2016-02-10 22:43:25]

p.s.

it'd probably be good to reach a fixed point on this refactoring ... I know we
could keep pulling the thread, but really I'm just shooting for an overall
improvement & fixing various bugs along the way. :)

[Leaf, 2016-02-11 01:00:53]

lgtm, thanks!

[Jennifer Messerly, 2016-02-11 21:42:53]

Description was changed from

==========
fix part of #25200, we now reject non-generic fn subtype of generic fn

A few other fixes:
* inference of generic function expressions now works. It was broken because
they ignored type parameters.

* fixes subtype of generic functions in the "normal" type system. These types
are only possible if generic methods support is enabled, which can be accessed
via a flag. The generic methods DEP is designed to work with the normal Dart
type system, too, so we try to keep these code paths working. ("normal" type
equality for function types already handled this correctly.) This is also needed
because strong mode delegates to normal type system for some cases, e.g. to pick
warning vs error.

* also refactors structural comparison of two generic function types, along the
lines of recent changes, so there is less code duplication. This was my original
motivation, but reviewing the code led to the discovery of the other issues.

* refactors strong_test_helper. The existing way of matching up errors did not
work with generic function expressions like: /*<T>*/(x) => x. Simplified it to
just look for the right offset. This also exposed a bug in a test -- previously,
it was possible to have an error that wasn't validated.
==========

to

==========
fix part of #25200, we now reject non-generic fn subtype of generic fn

A few other fixes:
* inference of generic function expressions now works. It was broken because
they ignored type parameters.

* fixes subtype of generic functions in the "normal" type system. These types
are only possible if generic methods support is enabled, which can be accessed
via a flag. The generic methods DEP is designed to work with the normal Dart
type system, too, so we try to keep these code paths working. ("normal" type
equality for function types already handled this correctly.) This is also needed
because strong mode delegates to normal type system for some cases, e.g. to pick
warning vs error.

* also refactors structural comparison of two generic function types, along the
lines of recent changes, so there is less code duplication. This was my original
motivation, but reviewing the code led to the discovery of the other issues.

* refactors strong_test_helper. The existing way of matching up errors did not
work with generic function expressions like: /*<T>*/(x) => x. Simplified it to
just look for the right offset. This also exposed a bug in a test -- previously,
it was possible to have an error that wasn't validated.

R=leafp@google.com, rnystrom@google.com

Committed:
https://github.com/dart-lang/sdk/commit/f97640745e533b47fe9208f4724b0ab5b9230dc9
==========

[Jennifer Messerly, 2016-02-11 21:42:54]

Committed patchset #3 (id:40001) manually as
f97640745e533b47fe9208f4724b0ab5b9230dc9 (presubmit successful).