fixes #27586, prefer context type in generic inference

fixes #27586, prefer downwards context type in generic inference
fixes #27625, Object constraints were not tracked in inference
fixes #27933, pin return type from downwards inference

We now prefer to pick the bound (lower or upper) that had some information on, and it also improves inference error messages somewhat (still a ways to go).

The way this works is we now have a type representing an unknown type: ?. We use ? when performing downward inference steps, instead of `dynamic`. This allows more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T, we previously pushed down Iterable<Future<dynamic>>. The dynamic caused loss of information. Now we push down Iterable<Future<?>>, allowing us to infer the right type there.

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

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

[Jennifer Messerly, 2016-10-28 23:56:43]

Description was changed from

==========
fixes #27586, downward constraints should take priority
also fixes #27625, Object constraints were not tracked in inference

The way this works is we now have a type representing an unkown type: ?. We use
? when performing downward inference steps, instead of `dynamic`. This allows
more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T,
we pushed down Iterable<Future<dynamic>>. The dynamic caused loss of
information. Now we push down Iterable<Future<?>>, allowing us to infer the
right type there.
==========

to

==========
fixes #27586, downward constraints should take priority (over upwards ones)
fixes #27625, Object constraints were not tracked in inference

We now prefer to pick the bound (lower or upper) that had some information on,
and it also improves inference error messages somewhat (still a ways to go).

The way this works is we now have a type representing an unknown type: ?. We use
? when performing downward inference steps, instead of `dynamic`. This allows
more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T,
we previously pushed down Iterable<Future<dynamic>>. The dynamic caused loss of
information. Now we push down Iterable<Future<?>>, allowing us to infer the
right type there.
==========

[Jennifer Messerly, 2016-10-29 01:12:29]

jmesserly@google.com changed reviewers:
+ leafp@google.com

[Jennifer Messerly, 2016-10-29 01:12:30]

It might be worth going over this one in person, but sending it to you as well.
:)

[Jennifer Messerly, 2016-10-29 01:12:55]

Description was changed from

==========
fixes #27586, downward constraints should take priority (over upwards ones)
fixes #27625, Object constraints were not tracked in inference

We now prefer to pick the bound (lower or upper) that had some information on,
and it also improves inference error messages somewhat (still a ways to go).

The way this works is we now have a type representing an unknown type: ?. We use
? when performing downward inference steps, instead of `dynamic`. This allows
more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T,
we previously pushed down Iterable<Future<dynamic>>. The dynamic caused loss of
information. Now we push down Iterable<Future<?>>, allowing us to infer the
right type there.
==========

to

==========
fixes #27586, prefer downwards context type in generic inference
fixes #27625, Object constraints were not tracked in inference

We now prefer to pick the bound (lower or upper) that had some information on,
and it also improves inference error messages somewhat (still a ways to go).

The way this works is we now have a type representing an unknown type: ?. We use
? when performing downward inference steps, instead of `dynamic`. This allows
more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T,
we previously pushed down Iterable<Future<dynamic>>. The dynamic caused loss of
information. Now we push down Iterable<Future<?>>, allowing us to infer the
right type there.
==========

[Jennifer Messerly, 2016-11-30 03:29:42]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[Jennifer Messerly, 2016-11-30 03:29:42]

Hi Leaf, Vijay, here's the updated CL.

I had to split out a few issues:
https://github.com/dart-lang/sdk/issues/27917
https://github.com/dart-lang/sdk/issues/27933

they are both fixable but they cause us to reject more code. With 27917 I hit
many issues even in Analyzer's own tests. With 27933, I made it pretty far but
we reject some packages. There might be a way to tackle it, but I think it'll be
easier to address in its own patch.

[vsm, 2016-11-30 04:05:33]

lgtm

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:4346: //return
UnknownInferredType.upperBoundForType(t);
Did you mean to keep this commented line?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:644: DartType inferred
= ts.inferGenericFunctionOrType/*<InterfaceType>*/(
Do you really want the lhs type annotation?  Looks like an unnecessary upcast
from InterfaceType to DartType.  Or you could tighten it.  Would

InterfaceType inferred = ts.inferGenericFunctionOrType(...

work?

https://codereview.chromium.org/2456803004/diff/100001/pkg/dev_compiler/lib/s...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/dev_compiler/lib/s...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5366: List/*<T>*/
_visitList/*<T>*/(Iterable nodes) {
Why was this change needed?

[Jennifer Messerly, 2016-11-30 04:35:04]

Thanks Vijay! BTW. I found a bug, it might be in DDC, I'm not sure yet, it only
shows up in one of DDC's tests. Digging in. When we tear off math.max, we don't
instantiate it with a type parameter, leading to a failure at runtime.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:4346: //return
UnknownInferredType.upperBoundForType(t);
On 2016/11/30 04:05:32, vsm wrote:
> Did you mean to keep this commented line?

Yeah, I can remove it. I kinda put it there to go with the TODO. It's the more
correct thing in some sense but we can't do it yet.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:644: DartType inferred
= ts.inferGenericFunctionOrType/*<InterfaceType>*/(
On 2016/11/30 04:05:33, vsm wrote:
> Do you really want the lhs type annotation?  Looks like an unnecessary upcast
> from InterfaceType to DartType.  Or you could tighten it.  Would
> 
> InterfaceType inferred = ts.inferGenericFunctionOrType(...
> 
> work?

yeah good idea. That'd work.

https://codereview.chromium.org/2456803004/diff/100001/pkg/dev_compiler/lib/s...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/dev_compiler/lib/s...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5366: List/*<T>*/
_visitList/*<T>*/(Iterable nodes) {
On 2016/11/30 04:05:33, vsm wrote:
> Why was this change needed?

the type was completely wrong before. It's mixing up Dart AST nodes and JS AST
nodes. The return type definitely is not the same as the input type. Possibly we
could have two generic types T and R.

To be honest, DDC has some typing sloppiness in what its visit methods do, it
would be great to clean up at some point.

[Jennifer Messerly, 2016-11-30 04:43:28]

On 2016/11/30 04:35:04, Jennifer Messerly wrote:
> Thanks Vijay! BTW. I found a bug, it might be in DDC, I'm not sure yet, it
only
> shows up in one of DDC's tests. Digging in. When we tear off math.max, we
don't
> instantiate it with a type parameter, leading to a failure at runtime.
> 
>
https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
> File pkg/analyzer/lib/src/generated/resolver.dart (right):
> 
>
https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
> pkg/analyzer/lib/src/generated/resolver.dart:4346: //return
> UnknownInferredType.upperBoundForType(t);
> On 2016/11/30 04:05:32, vsm wrote:
> > Did you mean to keep this commented line?
> 
> Yeah, I can remove it. I kinda put it there to go with the TODO. It's the more
> correct thing in some sense but we can't do it yet.
> 
>
https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
> File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):
> 
>
https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
> pkg/analyzer/lib/src/generated/static_type_analyzer.dart:644: DartType
inferred
> = ts.inferGenericFunctionOrType/*<InterfaceType>*/(
> On 2016/11/30 04:05:33, vsm wrote:
> > Do you really want the lhs type annotation?  Looks like an unnecessary
upcast
> > from InterfaceType to DartType.  Or you could tighten it.  Would
> > 
> > InterfaceType inferred = ts.inferGenericFunctionOrType(...
> > 
> > work?
> 
> yeah good idea. That'd work.
> 
>
https://codereview.chromium.org/2456803004/diff/100001/pkg/dev_compiler/lib/s...
> File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):
> 
>
https://codereview.chromium.org/2456803004/diff/100001/pkg/dev_compiler/lib/s...
> pkg/dev_compiler/lib/src/compiler/code_generator.dart:5366: List/*<T>*/
> _visitList/*<T>*/(Iterable nodes) {
> On 2016/11/30 04:05:33, vsm wrote:
> > Why was this change needed?
> 
> the type was completely wrong before. It's mixing up Dart AST nodes and JS AST
> nodes. The return type definitely is not the same as the input type. Possibly
we
> could have two generic types T and R.
> 
> To be honest, DDC has some typing sloppiness in what its visit methods do, it
> would be great to clean up at some point.

yeah we lost a dart.gbind call in stack_trace package. Going to track that down
tomorrow. I didn't expect an effect on the generic function instantiation code
path, but it must've done so.

[Leaf, 2016-11-30 05:24:30]

lgtm

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6177: DartType rawType =
classElement?.type;
Comment here, just for the reader, maybe?  Here's what I think is going on:

// At this point rawType is the interface type whose class is the class being
created,
// and whose type arguments are the type parameters of the class declaration.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6194:
inferenceContext.matchTypes(classTypeName.type, contextType);
Aargh... I'm lost again.  What's the difference between classTypeName.type and
rawType?  Can you explain in a 

// comment for dummies

so that I don't have to ask again next time I look at this code?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6197: rawType is InterfaceType) {
Can matchTypes return ? for one of the targs?  If so, will this do the right
thing here? e.g. if matchTypes returned <int, ?>, is filling in ? what we want?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6207: if (contextTypes.isEmpty &&
//comment

I think this is instance failure?  In which case we are now filling in with ?
which I guess is basically just dynamic?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6237: if (contextType is
FutureUnionType) {
Consider making this pattern a method on InferenceContext?  

InferenceContext.getValueContext?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1548: /// Apply an argument
constraint, which asserts that the [declaredType]
Should this say "Apply a return constraint", or something like that?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1577: bool downwardsInferPhase:
false}) {
Can you add a comment explaining downwardsInferPhase?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1788: return '\n\nA type could
not be found that satisfies these:\n$joined'
Do we need to worry about this list getting too long?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1927: /// The type parameter
that is constrainted by [lowerBound] or [upperBound].
constrainted -> constrained

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:2084: /// This type will only
appear in a downward inference context for type
Is this actually true?  In resolver.dart on 6212  it looked to me as if we were
substituting this into a constructor invocation.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/strong_mode_test.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:1460: void
test_genericFunction_upwardsAndDownwards_Object() {
Can you add a version of this using generic methods and one using constructors,
since those go through somewhat different paths in places?

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:1467: List<Object> eee = [new
Object()];
Do we have any test cases for inference which uses the new partial contexts?

[Jennifer Messerly, 2016-11-30 06:33:02]

jmesserly@google.com changed reviewers:
+ keertip@google.com

[Jennifer Messerly, 2016-11-30 06:33:03]

Thanks Leaf, I will go through these tomorrow!

BTW for patch testing purposes, I've uploaded to
https://github.com/dart-lang/sdk/tree/27586_infer_down

given that DDC tests pass I *think* it's in good shape now but always a chance
it regresses something in the internal code

[Jennifer Messerly, 2016-11-30 21:19:29]

Thanks all. I've uploaded a new version, re-testing.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6177: DartType rawType =
classElement?.type;
On 2016/11/30 05:24:29, Leaf wrote:
> Comment here, just for the reader, maybe?  Here's what I think is going on:
> 
> // At this point rawType is the interface type whose class is the class being
> created,
> // and whose type arguments are the type parameters of the class declaration.

sure :) to be honest I don't understand this code very well either. "rawType"
was moved from a few lines below.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6194:
inferenceContext.matchTypes(classTypeName.type, contextType);
On 2016/11/30 05:24:29, Leaf wrote:
> Aargh... I'm lost again.  What's the difference between classTypeName.type and
> rawType?  Can you explain in a 
> 
> // comment for dummies
> 
> so that I don't have to ask again next time I look at this code?

I would but.... I don't understand this code either :) :) :). I think someone
named Leaf might've been the original author ;) :D

I can try to see if there's any difference between rawType and
classTypeName.type. Off hand I couldn't tell you.

EDIT: so I did some digging, match types requires the default dynamic
substitution. That seems problematic because if we've instantiatedToBounds, I'm
not sure matchTypes will work? Anyway we have a TODO above to replace matchTypes
but I've added another TODO regarding the issue.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6197: rawType is InterfaceType) {
On 2016/11/30 05:24:29, Leaf wrote:
> Can matchTypes return ? for one of the targs?  If so, will this do the right
> thing here? e.g. if matchTypes returned <int, ?>, is filling in ? what we
want?

it shouldn't be able to, because` InferenceContext.getTypes` will eliminate `?`

whether it should or not is a good question. This code will work differently
if/when we unify with the rest of generic inference. The downwards phase
preserves `?`, the final upwards phase eliminates it.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6207: if (contextTypes.isEmpty &&
On 2016/11/30 05:24:29, Leaf wrote:
> //comment
> 
> I think this is instance failure?  In which case we are now filling in with ?
> which I guess is basically just dynamic?

I think the idea was: for downwards inference, e.g. `new C(...)`, we will use
`?` in the initial downwards phase, and it will end up replaced with `dynamic`
when we finish. As to why that matters, I'm not sure. I think I was trying to
bring this code more in line with how generic function/method downward inference
works. A simple test case did not discover why it matters, so for now I've
removed it.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6237: if (contextType is
FutureUnionType) {
On 2016/11/30 05:24:29, Leaf wrote:
> Consider making this pattern a method on InferenceContext?  
> 
> InferenceContext.getValueContext?

great idea, done!

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1548: /// Apply an argument
constraint, which asserts that the [declaredType]
On 2016/11/30 05:24:29, Leaf wrote:
> Should this say "Apply a return constraint", or something like that?

Done. This used to have a beautiful doc comment but I had to reword it once I
removed the "isSubtypeOf(contextType, declaredType);" below, must've messed it
up at that point.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1577: bool downwardsInferPhase:
false}) {
On 2016/11/30 05:24:29, Leaf wrote:
> Can you add a comment explaining downwardsInferPhase?

Great idea. Done.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1788: return '\n\nA type could
not be found that satisfies these:\n$joined'
On 2016/11/30 05:24:29, Leaf wrote:
> Do we need to worry about this list getting too long?

I'm not sure. The list will explain the types of each argument that affects that
type parameter, the return type (if affects the type param), and extends clause
for that type param, if any.

In practice it's 2-4 constraints typically.

The goal with the error message is to be maximally informative about what we
knew when we failed to infer. Happy for any ideas on how to make it shorter
while still communicating to the user.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1927: /// The type parameter
that is constrainted by [lowerBound] or [upperBound].
On 2016/11/30 05:24:29, Leaf wrote:
> constrainted -> constrained

Done.

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:2084: /// This type will only
appear in a downward inference context for type
On 2016/11/30 05:24:29, Leaf wrote:
> Is this actually true?  In resolver.dart on 6212  it looked to me as if we
were
> substituting this into a constructor invocation.

yes. It will definitely be substituted, but only temporarily when we are doing
downwards inference. It happens not only for constructors but other nodes like
MethodInvocation.staticInvokeType as well.

it sucks that to communicate between parts of Resolver, we frequently store
things on the AST/Elements, only to overwrite it shortly after. I don't like it,
but the flaw predates our involvement in Analyzer.

I'm pretty sure we have a bug to replace the InstanceCreationExpression
matchTypes ... i'll make sure we have one to track that downwards inference
phase for generics should not store anything on the nodes/elements.

[Jennifer Messerly, 2016-11-30 21:24:02]

would one of y'all mind PTAL on the test baselines?

There are some changes due to last night's fix for generic method instantiation.
I addressed an old TODO in inferFunctionTypeInstantiation, but it resulted in a
few test expectation changes.

Similarly removing the code that pushes down `?` for constructors, (per
request), resulted in a few "inferred type" hints disappearing.

[vsm, 2016-11-30 22:24:45]

lgtm

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

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:662: var b = new
C<Object>(/*info:INFERRED_TYPE_LITERAL*/[123]);
Is there any way to show/test that one works and the other doesn't?

[Leaf, 2016-12-01 04:36:27]

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6207: if (contextTypes.isEmpty &&
On 2016/11/30 21:19:28, Jennifer Messerly wrote:
> On 2016/11/30 05:24:29, Leaf wrote:
> > //comment
> > 
> > I think this is instance failure?  In which case we are now filling in with
?
> > which I guess is basically just dynamic?
> 
> I think the idea was: for downwards inference, e.g. `new C(...)`, we will use
> `?` in the initial downwards phase, and it will end up replaced with `dynamic`
> when we finish. As to why that matters, I'm not sure. I think I was trying to
> bring this code more in line with how generic function/method downward
inference
> works. A simple test case did not discover why it matters, so for now I've
> removed it.

Ok, I think this makes sense then.  I didn't get that this was just for the
purposes of downwards inference, then eliminated.  I think this code is right
(or in the right ballpark).  I think where it would make a difference is
something like this:

class A<T> {
  A(List<T> x) {}
}

void main() {
  new A([3]);
}

If we do downwards inference of [] with List<dynamic> as the context, we will
get a different answer than if we do downwards inference with List<?>.  Does
that seem right?

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/type_system_test.dart (right):

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/type_system_test.dart:978:
[clonable.type.instantiate([dynamicType])]);
Is this what we expect the inference engine to do now - that is, return
instantiation with dynamic even if that's incorrect for the bound?  Fine if so,
I just want to be sure that this is intentional, and that the correct error
messages are still emitted.

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/src/...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (left):

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2131: void
test_genericMethods_correctlyRecognizeGenericUpperBound_comment() {
Is this just being removed as a duplicate of one that doesn't use the comment
syntax?

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

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2399:
takeOOI(/*error:INVALID_CAST_FUNCTION*/math.max);
What's happening here?  This doesn't seem right to me.  I think this means that
we are inferring a type here, presumably Object?  But that doesn't satisfy the
bound for math.max.  Or are we just suppressing that error? I would hypothesize
that if you changed this to:

var f = math.max;
takeOOI(f);

then you would no longer get any error (or at least not the cast error), which
doesn't seem right.

[Jennifer Messerly, 2017-01-06 22:31:45]

Hi Leaf,

Here's the answer to the previous comments & another version that's passing. See
my question regarding "matchTypes".

I'm going to take a shot at rolling in that fix too. Will let you know how it
goes!

- Jenny

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6207: if (contextTypes.isEmpty &&
On 2016/12/01 04:36:27, Leaf wrote:
> On 2016/11/30 21:19:28, Jennifer Messerly wrote:
> > On 2016/11/30 05:24:29, Leaf wrote:
> > > //comment
> > > 
> > > I think this is instance failure?  In which case we are now filling in
with
> ?
> > > which I guess is basically just dynamic?
> > 
> > I think the idea was: for downwards inference, e.g. `new C(...)`, we will
use
> > `?` in the initial downwards phase, and it will end up replaced with
`dynamic`
> > when we finish. As to why that matters, I'm not sure. I think I was trying
to
> > bring this code more in line with how generic function/method downward
> inference
> > works. A simple test case did not discover why it matters, so for now I've
> > removed it.
> 
> Ok, I think this makes sense then.  I didn't get that this was just for the
> purposes of downwards inference, then eliminated.  I think this code is right
> (or in the right ballpark).  I think where it would make a difference is
> something like this:
> 
> class A<T> {
>   A(List<T> x) {}
> }
> 
> void main() {
>   new A([3]);
> }
> 
> If we do downwards inference of [] with List<dynamic> as the context, we will
> get a different answer than if we do downwards inference with List<?>.  Does
> that seem right?

yeah, I think so. Maybe I should take a shot at unifying this w/ the rest of
inference, instead of using matchTypes here? Should I try that here or in a
follow up?

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/type_system_test.dart (right):

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/type_system_test.dart:978:
[clonable.type.instantiate([dynamicType])]);
On 2016/12/01 04:36:27, Leaf wrote:
> Is this what we expect the inference engine to do now - that is, return
> instantiation with dynamic even if that's incorrect for the bound?  Fine if
so,
> I just want to be sure that this is intentional, and that the correct error
> messages are still emitted.

yeah, but good point, we should also validate the error code. Added.

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/src/...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (left):

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2131: void
test_genericMethods_correctlyRecognizeGenericUpperBound_comment() {
On 2016/12/01 04:36:27, Leaf wrote:
> Is this just being removed as a duplicate of one that doesn't use the comment
> syntax?

yup

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

https://codereview.chromium.org/2456803004/diff/160001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2399:
takeOOI(/*error:INVALID_CAST_FUNCTION*/math.max);
On 2016/12/01 04:36:27, Leaf wrote:
> What's happening here?  This doesn't seem right to me.  I think this means
that
> we are inferring a type here, presumably Object?  But that doesn't satisfy the
> bound for math.max.  Or are we just suppressing that error? I would
hypothesize
> that if you changed this to:
> 
> var f = math.max;
> takeOOI(f);
> 
> then you would no longer get any error (or at least not the cast error), which
> doesn't seem right.

good catch! fixed.

https://codereview.chromium.org/2456803004/diff/180001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2456803004/diff/180001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1926: var context =
InferenceContext.getContext(node);
this was a fix compared to previous patch; I had incorrectly used getType here
and that was the cause of the stacktrace fold+math.max not being inferred
correctly.

previously we'd push down (*, int) -> *. Now we push down (?, int) -> ? which
infers correctly.

[Jennifer Messerly, 2017-01-11 02:24:38]

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6207: if (contextTypes.isEmpty &&
On 2017/01/06 22:31:45, Jennifer Messerly wrote:
> On 2016/12/01 04:36:27, Leaf wrote:
> > On 2016/11/30 21:19:28, Jennifer Messerly wrote:
> > > On 2016/11/30 05:24:29, Leaf wrote:
> > > > //comment
> > > > 
> > > > I think this is instance failure?  In which case we are now filling in
> with
> > ?
> > > > which I guess is basically just dynamic?
> > > 
> > > I think the idea was: for downwards inference, e.g. `new C(...)`, we will
> use
> > > `?` in the initial downwards phase, and it will end up replaced with
> `dynamic`
> > > when we finish. As to why that matters, I'm not sure. I think I was trying
> to
> > > bring this code more in line with how generic function/method downward
> > inference
> > > works. A simple test case did not discover why it matters, so for now I've
> > > removed it.
> > 
> > Ok, I think this makes sense then.  I didn't get that this was just for the
> > purposes of downwards inference, then eliminated.  I think this code is
right
> > (or in the right ballpark).  I think where it would make a difference is
> > something like this:
> > 
> > class A<T> {
> >   A(List<T> x) {}
> > }
> > 
> > void main() {
> >   new A([3]);
> > }
> > 
> > If we do downwards inference of [] with List<dynamic> as the context, we
will
> > get a different answer than if we do downwards inference with List<?>.  Does
> > that seem right?
> 
> yeah, I think so. Maybe I should take a shot at unifying this w/ the rest of
> inference, instead of using matchTypes here? Should I try that here or in a
> follow up?

Hello again! I've now removed matchTypes and this uses normal inference. All
Analyzer and DDC tests pass.

[Jennifer Messerly, 2017-01-11 03:26:33]

On 2017/01/11 02:24:38, Jennifer Messerly wrote:
>
https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
> File pkg/analyzer/lib/src/generated/resolver.dart (right):
> 
>
https://codereview.chromium.org/2456803004/diff/100001/pkg/analyzer/lib/src/g...
> pkg/analyzer/lib/src/generated/resolver.dart:6207: if (contextTypes.isEmpty &&
> On 2017/01/06 22:31:45, Jennifer Messerly wrote:
> > On 2016/12/01 04:36:27, Leaf wrote:
> > > On 2016/11/30 21:19:28, Jennifer Messerly wrote:
> > > > On 2016/11/30 05:24:29, Leaf wrote:
> > > > > //comment
> > > > > 
> > > > > I think this is instance failure?  In which case we are now filling in
> > with
> > > ?
> > > > > which I guess is basically just dynamic?
> > > > 
> > > > I think the idea was: for downwards inference, e.g. `new C(...)`, we
will
> > use
> > > > `?` in the initial downwards phase, and it will end up replaced with
> > `dynamic`
> > > > when we finish. As to why that matters, I'm not sure. I think I was
trying
> > to
> > > > bring this code more in line with how generic function/method downward
> > > inference
> > > > works. A simple test case did not discover why it matters, so for now
I've
> > > > removed it.
> > > 
> > > Ok, I think this makes sense then.  I didn't get that this was just for
the
> > > purposes of downwards inference, then eliminated.  I think this code is
> right
> > > (or in the right ballpark).  I think where it would make a difference is
> > > something like this:
> > > 
> > > class A<T> {
> > >   A(List<T> x) {}
> > > }
> > > 
> > > void main() {
> > >   new A([3]);
> > > }
> > > 
> > > If we do downwards inference of [] with List<dynamic> as the context, we
> will
> > > get a different answer than if we do downwards inference with List<?>. 
Does
> > > that seem right?
> > 
> > yeah, I think so. Maybe I should take a shot at unifying this w/ the rest of
> > inference, instead of using matchTypes here? Should I try that here or in a
> > follow up?
> 
> Hello again! I've now removed matchTypes and this uses normal inference. All
> Analyzer and DDC tests pass.

... and I'm now moving ahead on https://github.com/dart-lang/sdk/issues/27933.
As before, I have it set up to roll that into its own branch, or merge here,
depending on review timing.

[Jennifer Messerly, 2017-01-12 22:25:40]

Description was changed from

==========
fixes #27586, prefer downwards context type in generic inference
fixes #27625, Object constraints were not tracked in inference

We now prefer to pick the bound (lower or upper) that had some information on,
and it also improves inference error messages somewhat (still a ways to go).

The way this works is we now have a type representing an unknown type: ?. We use
? when performing downward inference steps, instead of `dynamic`. This allows
more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T,
we previously pushed down Iterable<Future<dynamic>>. The dynamic caused loss of
information. Now we push down Iterable<Future<?>>, allowing us to infer the
right type there.
==========

to

==========
fixes #27586, prefer downwards context type in generic inference
fixes #27625, Object constraints were not tracked in inference
fixes #27933, pin return type from downwards inference

We now prefer to pick the bound (lower or upper) that had some information on,
and it also improves inference error messages somewhat (still a ways to go).

The way this works is we now have a type representing an unknown type: ?. We use
? when performing downward inference steps, instead of `dynamic`. This allows
more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T,
we previously pushed down Iterable<Future<dynamic>>. The dynamic caused loss of
information. Now we push down Iterable<Future<?>>, allowing us to infer the
right type there.
==========

[Jennifer Messerly, 2017-01-12 22:26:08]

On 2017/01/11 03:26:33, Jennifer Messerly wrote:
> 
> ... and I'm now moving ahead on https://github.com/dart-lang/sdk/issues/27933.
> As before, I have it set up to roll that into its own branch, or merge here,
> depending on review timing.

this fix is now included in the latest patch set.

[Leaf, 2017-01-13 04:47:33]

Still looking at this, but calling it quits for tonight.  couple of quick
questions on the tests.

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

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:659: var a = new
C<dynamic>([123]);
This is surprising to me.  I would expect this to still pin the type of the
argument.

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1409: List<dynamic> l1
= [3];
Are we just not recording this inference anymore?

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1926: // TODO(leafp):
The use of matchTypes in visitInstanceCreationExpression
TODO is done?

[Jennifer Messerly, 2017-01-14 02:28:38]

Thanks Leaf, I've addressed these. I also backed out of the "take bounds into
account to refine the context type", however note that that introduces strong
mode errors, unlike my previous version. A common pattern is:

    Iterable<Node> nodes = element.querySelectorAll(someQuery);

Because Element's querySelectorAll has <T extends Element> this usage pattern
will always be an inference error.

Another one was:

    identical(wrapFunction(...), otherFn);

wrapFunction is declared as `T extends Function`. But the context is Object
because of `identical`. So it's another guaranteed inference error. An easy way
to fix it is to pull out the subexpression into a "var x = subExpression;"
because that lets upwards inference work unimpeded.

My inclination is actually to go back, so we have an easier time integrating
this change into google3, and then stage that in as a separate change perhaps.
But I'll defer to your opinion of course.

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

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:659: var a = new
C<dynamic>([123]);
On 2017/01/13 04:47:33, Leaf wrote:
> This is surprising to me.  I would expect this to still pin the type of the
> argument.

The absence of a message indicates it's List<dynamic>... for better or worse
we've always reported it that way. See
pkg/analyzer/lib/src/generated/static_type_analyzer.dart, visitListLiteral.

It kind of makes sense ... otherwise every non-empty list literal would be
"inferred" as something.

I think we need to improve the test framework though. I would love to be able to
drop a /*type:List<dynamic>*/ comment in there to assert it.

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1409: List<dynamic> l1
= [3];
On 2017/01/13 04:47:33, Leaf wrote:
> Are we just not recording this inference anymore?

previously this was inferred as List<int>, it's now inferred to List<dynamic>,
which is not (and has never been) reported as inference. (see my answer to your
previous comment for more discussion, I believe this example is the same)

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1926: // TODO(leafp):
The use of matchTypes in visitInstanceCreationExpression
On 2017/01/13 04:47:33, Leaf wrote:
> TODO is done?

yes, good catch!

[Jennifer Messerly, 2017-01-17 20:22:10]

On 2017/01/14 02:28:38, Jennifer Messerly wrote:
> Thanks Leaf, I've addressed these. I also backed out of the "take bounds into
> account to refine the context type", however note that that introduces strong
> mode errors, unlike my previous version. A common pattern is:
> 
>     Iterable<Node> nodes = element.querySelectorAll(someQuery);
> 

Here's my intuition of why we should take into account the bounds: we don't want
downwards inference to be worse than our simple "instantiate to bounds" default
rule.

[Leaf, 2017-01-24 18:26:56]

First round of comments.  See also https://codereview.chromium.org/2653833003/ .

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1520: /// Where this type
constraint came from, used only for error messages.
I don't think this comment is true now.  The origin is used to decide whether or
not a parameter is fixed via downwards inference or not.

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6622: // Update the static element
as well. This is used in some cases, such as
length

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1581: var hasKnownReturnType =
returnContextType != null &&
I don't quite understand this.  This is checking whether anything is unknown in
the return type.  It seems to be only used in the heuristic for choosing upper
vs lower bounds, but I don't understand concretely what it's doing, and why this
is the right thing.  You might have a return ContextType which looks like:

(int, ?) -> ?

This will be treated as not having a knownReturnType, right?  But if we're using
that to constrain

(S, T) -> U

then it seems like this type is unknown for T and U, but known for S.

But again, I don't really understand this.  Do you have some examples of where
this makes a difference?

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1603: //     <T extends
Clonable<T>>
Do we really get anything out of this in this case?  It seems a bit odd.  We
constrain it to Clonable<dynamic>, but that's not a valid instantiation for T. 
So it seems like either it was already something which satisfied Clonable<T>, or
else adding in this constraint doesn't help us at all.   Or am I missing
something?  In any case, it doesn't hurt to have it, just curious about the
comment.

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1609:
constrainTypeParameterIsSubtypeOf(typeParam,
This doesn't quite make sense if the type parameter appears contravariantly in
the bound.  It seems like maybe using the unknown type as the thing to
substitute in for the uninferred variables might be better?  WDYT?

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1882: /// This class has no
effect on type inference computation itself.
I don't think this is true anymore, I think you use this to decide which ones to
fix when, right?

[Jennifer Messerly, 2017-02-01 09:31:39]

Thanks Leaf. I've updated it. Interested to hear thoughts and chat about
tomorrow?

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/260001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1520: /// Where this type
constraint came from, used only for error messages.
On 2017/01/24 18:26:55, Leaf wrote:
> I don't think this comment is true now.  The origin is used to decide whether
or
> not a parameter is fixed via downwards inference or not.

yeah, I can update that. My original plan was to track the fixed type parameter
in some way (e.g. on the AstNode) but I realized it could be recomputed easily
enough on the way back up.

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6622: // Update the static element
as well. This is used in some cases, such as
On 2017/01/24 18:26:55, Leaf wrote:
> length

Done.

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1581: var hasKnownReturnType =
returnContextType != null &&
On 2017/01/24 18:26:56, Leaf wrote:
> I don't quite understand this.  This is checking whether anything is unknown
in
> the return type.  It seems to be only used in the heuristic for choosing upper
> vs lower bounds, but I don't understand concretely what it's doing, and why
this
> is the right thing.  You might have a return ContextType which looks like:
> 
> (int, ?) -> ?
> 
> This will be treated as not having a knownReturnType, right?  But if we're
using
> that to constrain
> 
> (S, T) -> U
> 
> then it seems like this type is unknown for T and U, but known for S.
> 
> But again, I don't really understand this.  Do you have some examples of where
> this makes a difference?

yeah. it's obsolete code! removed :)

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1603: //     <T extends
Clonable<T>>
On 2017/01/24 18:26:55, Leaf wrote:
> Do we really get anything out of this in this case?  It seems a bit odd.  We
> constrain it to Clonable<dynamic>, but that's not a valid instantiation for T.

> So it seems like either it was already something which satisfied Clonable<T>,
or
> else adding in this constraint doesn't help us at all.   Or am I missing
> something?  In any case, it doesn't hurt to have it, just curious about the
> comment.

Let's step back a bit from the F-bounded case. Should we use the type parameter
bound at all? Empirically the answer is yes, we find it helps prevent invalid
instantiations such as:

    // this would be bad:
    Object obj = math.min/*infer <Object>, ERROR*/(1, 2);

A similar case may arise with upwards inference.

Once we decide to use the type parameter bound, now we're faced with what to do
in the f-bounded examples. Traditionally, we borrowed the "instantiate to
bounds" logic, because that's what a generic would do absent any inference that
takes into account parameters or return type:

    f<T extends C<T>>() { /*...*/ }
    var x = f/*infer: default instantiate to bounds*/();

Thus, in some sense, all inference can be seen as extending the capabilities of
instantiate-to-bounds.

That said, I can see where you're coming from. Given a case like:

    T f<T extends num>(List<T> list) => ...
    var x = f(/*infer <num> or <int> here?*/[1, 2, 3]); 

... we probably want to push down "?"

I can invest more time into bounds. The theory was that f-bounded wasn't all
that important, so this tries mainly to prevent totally invalid choices (e.g.
inferring `?` in the final inference phase, inferring an unsubstituted type
parameter which could then leak out of the binder.

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1609:
constrainTypeParameterIsSubtypeOf(typeParam,
On 2017/01/24 18:26:56, Leaf wrote:
> This doesn't quite make sense if the type parameter appears contravariantly in
> the bound.  It seems like maybe using the unknown type as the thing to
> substitute in for the uninferred variables might be better?  WDYT?

does my comment above address this?

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1882: /// This class has no
effect on type inference computation itself.
On 2017/01/24 18:26:56, Leaf wrote:
> I don't think this is true anymore, I think you use this to decide which ones
to
> fix when, right?

fixed comment

[Jennifer Messerly, 2017-02-01 09:41:37]

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/280001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1603: //     <T extends
Clonable<T>>
On 2017/02/01 09:31:39, Jennifer Messerly wrote:
> On 2017/01/24 18:26:55, Leaf wrote:
> > Do we really get anything out of this in this case?  It seems a bit odd.  We
> > constrain it to Clonable<dynamic>, but that's not a valid instantiation for
T.
> 
> > So it seems like either it was already something which satisfied
Clonable<T>,
> or
> > else adding in this constraint doesn't help us at all.   Or am I missing
> > something?  In any case, it doesn't hurt to have it, just curious about the
> > comment.
> 
> Let's step back a bit from the F-bounded case. Should we use the type
parameter
> bound at all? Empirically the answer is yes, we find it helps prevent invalid
> instantiations such as:
> 
>     // this would be bad:
>     Object obj = math.min/*infer <Object>, ERROR*/(1, 2);
> 
> A similar case may arise with upwards inference.
> 
> Once we decide to use the type parameter bound, now we're faced with what to
do
> in the f-bounded examples. Traditionally, we borrowed the "instantiate to
> bounds" logic, because that's what a generic would do absent any inference
that
> takes into account parameters or return type:
> 
>     f<T extends C<T>>() { /*...*/ }
>     var x = f/*infer: default instantiate to bounds*/();
> 
> Thus, in some sense, all inference can be seen as extending the capabilities
of
> instantiate-to-bounds.
> 
> That said, I can see where you're coming from. Given a case like:
> 
>     T f<T extends num>(List<T> list) => ...
>     var x = f(/*infer <num> or <int> here?*/[1, 2, 3]); 
> 
> ... we probably want to push down "?"
> 
> I can invest more time into bounds. The theory was that f-bounded wasn't all
> that important, so this tries mainly to prevent totally invalid choices (e.g.
> inferring `?` in the final inference phase, inferring an unsubstituted type
> parameter which could then leak out of the binder.

Here's an example of how inference and instantiate-to-bounds behavior work
together:

    T f<S, T extends S>(S x) => null;
    void test() { var x = f(3); }

... there are more examples like that in the tests. By inferring S via upwards
info we are also able to infer T.

I'm trying to see if I can make it more order-invariant.

[Leaf, 2017-02-17 06:41:22]

Round one of comments.

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:4264: // TODO(jmesserly): our
downwards inference code is not designed to handle
What's the status of this?  We have user facing Null available now.  Should we
follow up in another CL, or can we DTRT here now?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:217: if
(contextType.typeFormals.isNotEmpty || fnType.typeFormals.isEmpty) {
Not new to this CL, but in passing I think this is kind of wrong.

S Function<T>(T x) f<S>();

int Function<T>(T x) f = f();  

Seems like it should work, and I think this will cause it to bail out.  Am I
wrong?   

If not, maybe TODO it?

Also, do you know what the check that the context type is not generic is
supposed to catch here?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:237: // TODO(jmesserly): should
we issue an inference error here, or rely on it
I don't know should we?  :)  Is this a question we should resolve in this CL?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:269: DartType
declaredReturnType,
I think this is always either equal to genericType (when T is InterfaceType) or
genericType.returnType (when T is Functiontype).  Since you have to do an is
check anyway, what do you think of just passing in genericType and projecting
out returnType if it exists?

Alternatively, you could split this into two non-generic entry points which call
this internal generic with the appropriate args.

FunctionType inferGenericFunction(FunctionType genericType. ...) {
  return _inferGenericFunctionOrType(genericType, ..., genericType.returnType,
...)
}

WDYT?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1554:
_shouldConsiderTypeParameterBound(returnContextType)) {
Why do we check if the context type is fully known?  This seems neither
necessary nor sufficient.  e.g.

returnContextType = Map<num, ?>

trying to instantiate 

  f((List<int> x) => x[0]);

where

  Map<S, T> f<S extends int, T extends String, U extends Iterable<int>>(int f(U
x));

then S will be constrained by the context type, and we do want to apply the
information from the bounds now, even though the returnContextType is partially
unknown.  But we don't want to constrain T and U yet, since if we constrain U
now we will over-constrain it.

But if 

  returnContextType = Map<int, String>

we still don't want to (yet) constrain U, we want to wait until we hit the
upwards pass.

Do we not have a way of tracking which variables have been constrained yet?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1607: // NOTE: types that are
used by inference, such as `?` and future unions,
remove "future unions" from comment now?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1667: if (variance.passedIn) {
Can you explain this bit too me (probably worth a comment).  isFixed basically
tells you whether there were any constraints for this variable.  Why are we
preferring an upper bound for contra-variant parameters that are un-constrained
(in this case we shouldn't have a bound , right?), but lower bound otherwise,
etc?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1689: if (lower.isDartCoreNull)
{
I'm not sure about this heuristic now.  We're going to be inferring Null for

  var x = Null;

and as the return type for

  (int x) { return null; }

so it feels like

  [null]

should be List<Null> as well.



WDYT?  Are there examples where this goes really bad?

[Leaf, 2017-02-17 22:26:04]

Some more comments.  At some point can you rebase and upload a branch to github?
 Then I can play with this a bit, and Keerti can run a nightly with it so that
we can see the impact on google3 code.

thanks!

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1867: "must be a
'$contextType'."
 Can we say anything that's more helpful than "must be"?  Maybe something like:

"used where a $contextType is required"

or something like that?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1959:
UnknownInferredType.typeIsUnknown(upperBound) ||
Just trying to think through this code.  This is used to decide whether to treat
a type parameter as fixed or not.  It kind of feel like we should only ignore
the return constraints if both of these are unfixed.  Example:

returnContextType is int -> ?

matched against declared return type of T -> T

we end up with 

int <: T

T <: ?

seems like maybe we should use the int constraint here?

Does that make sense?  Very corner case.

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/strong_mode_test.dart (right):

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:91:
fail_constrainedByBounds3() async {
I think I added these to test ordering behavior of inference in one of you CLs. 
Has this ordering behavior been removed/changed?  If so this should be updated
to test the new behavior.

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:114:
fail_constrainedByBounds5() async {
ditto

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:145: T mk<T extends int>(T x)
=> null;
This test looks bogus.  I think maybe the intention was to check that we used
the bound to narrow the inferred type from FutureOr<int> to int?  So maybe the
argument to mk should be 42?  And the assertions should be checking for _isInt?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:152:
fail_futureOr_downwards8() async {
Also looks bogus to me now - I think should be T extends Future<int>?  And then
should pass?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:165:
fail_pinning_multipleConstraints1() async {
Why is this failing?  Are we inferring Null?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:245:
pass_returnType_variance2() async {
Should this be test_?  Or is this a valid prefix?

[Jennifer Messerly, 2017-03-14 02:07:07]

Thanks Leaf! I have a new & improved version if you could take a look please.

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:4264: // TODO(jmesserly): our
downwards inference code is not designed to handle
On 2017/02/17 06:41:21, Leaf wrote:
> What's the status of this?  We have user facing Null available now.  Should we
> follow up in another CL, or can we DTRT here now?

fixed! new code handles Null

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:217: if
(contextType.typeFormals.isNotEmpty || fnType.typeFormals.isEmpty) {
On 2017/02/17 06:41:21, Leaf wrote:
> Not new to this CL, but in passing I think this is kind of wrong.
> 
> S Function<T>(T x) f<S>();
> 
> int Function<T>(T x) f = f();  
> 
> Seems like it should work, and I think this will cause it to bail out.  Am I
> wrong?   
> 
> If not, maybe TODO it?
> 
> Also, do you know what the check that the context type is not generic is
> supposed to catch here?

I don't understand your example.
Since this is not related to the CL can we take it offline?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:237: // TODO(jmesserly): should
we issue an inference error here, or rely on it
On 2017/02/17 06:41:21, Leaf wrote:
> I don't know should we?  :)  Is this a question we should resolve in this CL? 


let's leave changes to generic tear-offs out of scope for the CL. :)

universal functions being passed to places where instantiations are expected is
fairly rare. It arose from us not having syntax to explicitly instantiate them.
IIRC, the language team was thinking about getting rid of this support anyway?

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:269: DartType
declaredReturnType,
On 2017/02/17 06:41:22, Leaf wrote:
> I think this is always either equal to genericType (when T is InterfaceType)
or
> genericType.returnType (when T is Functiontype).  Since you have to do an is
> check anyway, what do you think of just passing in genericType and projecting
> out returnType if it exists?
> 
> Alternatively, you could split this into two non-generic entry points which
call
> this internal generic with the appropriate args.
> 
> FunctionType inferGenericFunction(FunctionType genericType. ...) {
>   return _inferGenericFunctionOrType(genericType, ..., genericType.returnType,
> ...)
> }
> 
> WDYT?

done

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1607: // NOTE: types that are
used by inference, such as `?` and future unions,
On 2017/02/17 06:41:21, Leaf wrote:
> remove "future unions" from comment now?

done

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1667: if (variance.passedIn) {
On 2017/02/17 06:41:22, Leaf wrote:
> Can you explain this bit too me (probably worth a comment).  isFixed basically
> tells you whether there were any constraints for this variable.  Why are we
> preferring an upper bound for contra-variant parameters that are
un-constrained
> (in this case we shouldn't have a bound , right?), but lower bound otherwise,
> etc?

done! this is gone in new code :)

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1689: if (lower.isDartCoreNull)
{
On 2017/02/17 06:41:21, Leaf wrote:
> I'm not sure about this heuristic now.  We're going to be inferring Null for
> 
>   var x = Null;
> 
> and as the return type for
> 
>   (int x) { return null; }
> 
> so it feels like
> 
>   [null]
> 
> should be List<Null> as well.
> 
> 
> 
> WDYT?  Are there examples where this goes really bad?

done! this is gone now in new code :)

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1867: "must be a
'$contextType'."
On 2017/02/17 22:26:03, Leaf wrote:
>  Can we say anything that's more helpful than "must be"?  Maybe something
like:
> 
> "used where a $contextType is required"
> 
> or something like that?

fixed

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1959:
UnknownInferredType.typeIsUnknown(upperBound) ||
On 2017/02/17 22:26:03, Leaf wrote:
> Just trying to think through this code.  This is used to decide whether to
treat
> a type parameter as fixed or not.  It kind of feel like we should only ignore
> the return constraints if both of these are unfixed.  Example:
> 
> returnContextType is int -> ?
> 
> matched against declared return type of T -> T
> 
> we end up with 
> 
> int <: T
> 
> T <: ?
> 
> seems like maybe we should use the int constraint here?
> 
> Does that make sense?  Very corner case.

fixed in the new code :)

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/strong_mode_test.dart (right):

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:91:
fail_constrainedByBounds3() async {
On 2017/02/17 22:26:03, Leaf wrote:
> I think I added these to test ordering behavior of inference in one of you
CLs. 
> Has this ordering behavior been removed/changed?  If so this should be updated
> to test the new behavior.

it's passing now!

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:114:
fail_constrainedByBounds5() async {
On 2017/02/17 22:26:03, Leaf wrote:
> ditto

same, this is now passing!

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:145: T mk<T extends int>(T x)
=> null;
On 2017/02/17 22:26:03, Leaf wrote:
> This test looks bogus.  I think maybe the intention was to check that we used
> the bound to narrow the inferred type from FutureOr<int> to int?  So maybe the
> argument to mk should be 42?  And the assertions should be checking for
_isInt?

yup, I fixed it & it now passes

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:152:
fail_futureOr_downwards8() async {
On 2017/02/17 22:26:03, Leaf wrote:
> Also looks bogus to me now - I think should be T extends Future<int>?  And
then
> should pass?

Hmmm, maybe you're right. I actually went ahead and fixed it before I saw your
comment, by allowing inference's way of combining constraints figure out that
GLB(FutureOr<int>, Future<Object>) should be Future<int>. But we could remove
that code easily.

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:165:
fail_pinning_multipleConstraints1() async {
On 2017/02/17 22:26:03, Leaf wrote:
> Why is this failing?  Are we inferring Null?

it's passing now

https://codereview.chromium.org/2456803004/diff/320001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:245:
pass_returnType_variance2() async {
On 2017/02/17 22:26:03, Leaf wrote:
> Should this be test_?  Or is this a valid prefix?

good catch. fixed & test passes :)

[Leaf, 2017-03-14 22:37:40]

just a checkpoint of comments since you're preparing another patch set in
parallel.

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:4237: * contains `?` this will be
returned. You can use [getValueContext] if you
Is this residual?  I can't find getValueContext anywhere.

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:4259: // type E must be a subtype
of the context type T, i.e. T is an upper bound.
length.

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1580: /// recurse into our
constraint solving code.
I think the second part of this comment is obsolete?

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1595: equals: (x, y) =>
x.location == y.location,
Why is using the location safe here?  There seems to be code (e.g. in
StaticTypeAnalyzer.constructorToGenericFunctionType) which creates generic types
with synthetic type variables, and I think synthetic type variables will all
have the same location?  Is that right?  Or am I confused?

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/strong_mode_test.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:103:
test_constrainedByBounds3() async {
Sort methods in file?

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:514: // variable if they are
ordered right to left.
Looks like your code now handles right to left?  Update the comment to reflect
that this is a positive test now?

[Jennifer Messerly, 2017-03-14 23:49:10]

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:4237: * contains `?` this will be
returned. You can use [getValueContext] if you
On 2017/03/14 22:37:40, Leaf wrote:
> Is this residual?  I can't find getValueContext anywhere.

fixed!

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:4259: // type E must be a subtype
of the context type T, i.e. T is an upper bound.
On 2017/03/14 22:37:40, Leaf wrote:
> length.

fixed, this method is gone now.

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1580: /// recurse into our
constraint solving code.
On 2017/03/14 22:37:40, Leaf wrote:
> I think the second part of this comment is obsolete?

done

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1595: equals: (x, y) =>
x.location == y.location,
On 2017/03/14 22:37:39, Leaf wrote:
> Why is using the location safe here?  There seems to be code (e.g. in
> StaticTypeAnalyzer.constructorToGenericFunctionType) which creates generic
types
> with synthetic type variables, and I think synthetic type variables will all
> have the same location?  Is that right?  Or am I confused?  

Type variables are an absolute mess, is the short answer. =(

Through the course of this CL, I've slowly attempted to fix them. But the
problems run deep.

We need *something* to denote the identity of the type variable. Either
something off of TypeParameterElement, or the Element itself. It's unclear to me
what is best. I'm open to suggestions.

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/strong_mode_test.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:103:
test_constrainedByBounds3() async {
On 2017/03/14 22:37:40, Leaf wrote:
> Sort methods in file?

I can do that. It will destroy the diff, but that tradeoff is up to you, so I'll
go ahead and do it.

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/strong_mode_test.dart:514: // variable if they are
ordered right to left.
On 2017/03/14 22:37:40, Leaf wrote:
> Looks like your code now handles right to left?  Update the comment to reflect
> that this is a positive test now?

Done.

[Jennifer Messerly, 2017-03-14 23:55:16]

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1595: equals: (x, y) =>
x.location == y.location,
On 2017/03/14 23:49:09, Jennifer Messerly wrote:
> On 2017/03/14 22:37:39, Leaf wrote:
> > Why is using the location safe here?  There seems to be code (e.g. in
> > StaticTypeAnalyzer.constructorToGenericFunctionType) which creates generic
> types
> > with synthetic type variables, and I think synthetic type variables will all
> > have the same location?  Is that right?  Or am I confused?  
> 
> Type variables are an absolute mess, is the short answer. =(
> 
> Through the course of this CL, I've slowly attempted to fix them. But the
> problems run deep.
> 
> We need *something* to denote the identity of the type variable. Either
> something off of TypeParameterElement, or the Element itself. It's unclear to
me
> what is best. I'm open to suggestions.

I dug into this more. All I'm doing here is what ElementImpl.== does. So yes, it
looks like location is safe. In any case nothing changed for the worse by doing
this, it's simply handling TypeParameterMember better.

have a look at ElementImpl.location and the ElementLocation class ... they're
doing some interesting stuff, but basically walking up the enclosingElements and
collecting all "name" components. I think it will work for our purposes.

[Leaf, 2017-03-15 00:27:13]

Okay, i've got to call it a night.  A couple of comments, but basically
everything LGTM except what looks like a typo in type_system_test and a concern
I called out in resolver.dart.  If you agree with my comment in resolver.dart,
go ahead and fix and land.  Otherwise, I'd say still go ahead and land it and we
can iterate on the remaining issue.

Thanks!  Almost there!!

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1080: bool
_typeParameterBoundsSubtype(
Can you write a comment for this?  I won't remember why it's here in a week or
two... :)

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1595: equals: (x, y) =>
x.location == y.location,
On 2017/03/14 23:55:16, Jennifer Messerly wrote:
> On 2017/03/14 23:49:09, Jennifer Messerly wrote:
> > On 2017/03/14 22:37:39, Leaf wrote:
> > > Why is using the location safe here?  There seems to be code (e.g. in
> > > StaticTypeAnalyzer.constructorToGenericFunctionType) which creates generic
> > types
> > > with synthetic type variables, and I think synthetic type variables will
all
> > > have the same location?  Is that right?  Or am I confused?  
> > 
> > Type variables are an absolute mess, is the short answer. =(
> > 
> > Through the course of this CL, I've slowly attempted to fix them. But the
> > problems run deep.
> > 
> > We need *something* to denote the identity of the type variable. Either
> > something off of TypeParameterElement, or the Element itself. It's unclear
to
> me
> > what is best. I'm open to suggestions.
> 
> I dug into this more. All I'm doing here is what ElementImpl.== does. So yes,
it
> looks like location is safe. In any case nothing changed for the worse by
doing
> this, it's simply handling TypeParameterMember better.
> 
> have a look at ElementImpl.location and the ElementLocation class ... they're
> doing some interesting stuff, but basically walking up the enclosingElements
and
> collecting all "name" components. I think it will work for our purposes.

Ok, thanks for looking into it.  I'm still a little frightened that there's a
latent issue here with synthetic type parameters, but sounds like this isn't
making anything worse.

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:1624: /// [covariant] must be
true if [t1] is a declared type of the generic
I'm not sure I understand why covariant is needed, and it makes me a little
nervous.  That is, I think it's fine, but if it's really needed I think it's
because of scary type variable madness in the analyzer element model.  I think
covariant here is just tracking which side of the matching problem contains the
type variables that we are matching against.  I would claim that should be
redundant: _constraints contains the list of variables we are solving for, and
those variables can only appear on why side of the query.  If they can appear on
both sides, if my understanding is correct, then that means that two different
instances of the same binding site are being conflated (i.e. essentially
capture).

I'm fine with leaving this as is for now, but if this is based on concrete test
cases or issues that you encountered without it, can you share them with me
offline so I can either understand where my understanding went off the rails, or
else figure out what bugs to file... :)

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/type_system_test.dart (right):

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/type_system_test.dart:1060: _inferCall(f, [],
Is this right?  This looks like a call to f with no arguments, right?  Is that
what you mean to be testing here?  Also a bit weird that the return type if
matching T -> void against num -> int.  Is this supposed to maybe be <T>() -> (T
->T) against num-> int?

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

https://codereview.chromium.org/2456803004/diff/370001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2257: /*!!!
What's happening here?

https://codereview.chromium.org/2456803004/diff/390001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2456803004/diff/390001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:5952:
ts.upperBoundForType(functionType.returnType));
Do we really want to eliminate this here?  I think we don't.  If I'm checking 
(x) => [3] at List<?> -> List<?>, then I want to check [3] at List<?>, not
List<dynamic> (since the latter will constrain to dynamic, whereas the former
will allow upwards inference to flow out the type from the list element).  Is
that right?

[Jennifer Messerly, 2017-03-18 00:00:08]

Description was changed from

==========
fixes #27586, prefer downwards context type in generic inference
fixes #27625, Object constraints were not tracked in inference
fixes #27933, pin return type from downwards inference

We now prefer to pick the bound (lower or upper) that had some information on,
and it also improves inference error messages somewhat (still a ways to go).

The way this works is we now have a type representing an unknown type: ?. We use
? when performing downward inference steps, instead of `dynamic`. This allows
more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T,
we previously pushed down Iterable<Future<dynamic>>. The dynamic caused loss of
information. Now we push down Iterable<Future<?>>, allowing us to infer the
right type there.
==========

to

==========
fixes #27586, prefer downwards context type in generic inference
fixes #27625, Object constraints were not tracked in inference
fixes #27933, pin return type from downwards inference

We now prefer to pick the bound (lower or upper) that had some information on,
and it also improves inference error messages somewhat (still a ways to go).

The way this works is we now have a type representing an unknown type: ?. We use
? when performing downward inference steps, instead of `dynamic`. This allows
more accurate tracking of type constraints.

For example: given:

    var x = await Future.wait([a, b]);

Future.wait<T>'s argument type is Iterable<Future<T>>. Since we didn't know T,
we previously pushed down Iterable<Future<dynamic>>. The dynamic caused loss of
information. Now we push down Iterable<Future<?>>, allowing us to infer the
right type there.

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

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

[Jennifer Messerly, 2017-03-18 00:00:10]

Committed patchset #25 (id:410001) manually as
f021a7019b0ba8515457d0bceb56af62273fb735 (presubmit successful).