fixes #27586, prefer context type in generic inference

pkg/analyzer/lib/src/generated/resolver.dart

Index: pkg/analyzer/lib/src/generated/resolver.dart
diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
index df3ad03b5db7488cf0265ee48f554e1d04641400..adf6ab360712a2cf7833ba613bf75a070eae4b06 100644
--- a/pkg/analyzer/lib/src/generated/resolver.dart
+++ b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -4318,9 +4318,9 @@ class InferenceContext {
   static DartType getType(AstNode node) {
     DartType t = getContext(node);
     if (t is FutureUnionType) {
-      return t.type;
+      return _substituteForUnknown(t.type);
     }
-    return t;
+    return _substituteForUnknown(t);
   }
 
   /**
@@ -4331,10 +4331,20 @@ class InferenceContext {
     if (t == null) {
       return DartType.EMPTY_LIST;
     }
-    if (t is FutureUnionType) {
-      return t.types;
-    }
-    return <DartType>[t];
+    Iterable<DartType> result = t is FutureUnionType ? t.types : [t];
+    return result.map(_substituteForUnknown).where((t) => t != null);
+  }
+
+  static DartType _substituteForUnknown(DartType t) {
+    if (t == null) return null;
+    // Since the type is being used for downwards inference, the expression
+    // type E must be a subtype of the context type T, i.e. T is an upper bound.
+    //
+    // TODO(jmesserly): our downwards inference code is not designed to handle
+    // the bottom type, so we need to prevent it from resulting here.
+    // Instead use `dynamic`.
+    //return UnknownInferredType.upperBoundForType(t);

[vsm, 2016/11/30 04:05:32]

Did you mean to keep this commented line?

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

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.

+    return UnknownInferredType.substituteDynamic(t);
   }
 
   /**
@@ -5398,7 +5408,7 @@ class ResolverVisitor extends ScopedVisitor {
 
   @override
   Object visitArgumentList(ArgumentList node) {
-    DartType callerType = InferenceContext.getType(node);
+    DartType callerType = InferenceContext.getContext(node);
     if (callerType is FunctionType) {
       Map<String, DartType> namedParameterTypes =
           callerType.namedParameterTypes;
@@ -6155,18 +6165,23 @@ class ResolverVisitor extends ScopedVisitor {
     // check this don't work, since we may have been instantiated
     // to bounds in an earlier phase, and we *do* want to do inference
     // in that case.
-    if (classTypeName.typeArguments == null) {
+
+    if (strongMode && classTypeName.typeArguments == null) {
       // Given a union of context types ` T0 | T1 | ... | Tn`, find the first
       // valid instantiation `new C<Ti>`, if it exists.
       // TODO(jmesserly): if we support union types for real, `new C<Ti | Tj>`
       // will become a valid possibility. Right now the only allowed union is
       // `T | Future<T>` so we can take a simple approach.
-      for (var contextType in InferenceContext.getTypes(node)) {
+
+      TypeDefiningElement classElement = classTypeName.type?.element;
+      DartType rawType = classElement?.type;

[Leaf, 2016/11/30 05:24:29]

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.

[Jennifer Messerly, 2016/11/30 21:19:28]

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

+      Iterable<DartType> contextTypes = InferenceContext.getTypes(node);
+      for (var contextType in contextTypes) {
         if (contextType is InterfaceType &&
             contextType.typeArguments != null &&
             contextType.typeArguments.isNotEmpty) {
           // TODO(jmesserly): for generic methods we use the
-          // StrongTypeSystemImpl.inferGenericFunctionCall, which appears to
+          // StrongTypeSystemImpl.inferGenericFunctionOrType, which appears to
           // be a tad more powerful than matchTypes.
           //
           // For example it can infer this case:
@@ -6177,9 +6192,9 @@ class ResolverVisitor extends ScopedVisitor {
           // See _inferArgumentTypesFromContext in this file for use of it.
           List<DartType> targs =
               inferenceContext.matchTypes(classTypeName.type, contextType);

[Leaf, 2016/11/30 05:24:29]

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?

[Jennifer Messerly, 2016/11/30 21:19:28]

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.

-          if (targs != null && targs.any((t) => !t.isDynamic)) {
-            ClassElement classElement = classTypeName.type.element;
-            InterfaceType rawType = classElement.type;
+          if (targs != null &&
+              targs.any((t) => !t.isDynamic) &&
+              rawType is InterfaceType) {

[Leaf, 2016/11/30 05:24:29]

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?

[Jennifer Messerly, 2016/11/30 21:19:28]

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.

             InterfaceType fullType =
                 rawType.substitute2(targs, rawType.typeArguments);
             // The element resolver uses the type on the constructor name, so
@@ -6189,10 +6204,19 @@ class ResolverVisitor extends ScopedVisitor {
           }
         }
       }
+      if (contextTypes.isEmpty &&

[Leaf, 2016/11/30 05:24:29]

//comment

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

[Jennifer Messerly, 2016/11/30 21:19:28]

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.

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

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?

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

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?

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

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

+          rawType is InterfaceType &&
+          rawType.typeArguments.isNotEmpty) {
+        node.constructorName.type.type = rawType.substitute2(
+            new List.filled(
+                rawType.typeArguments.length, UnknownInferredType.instance),
+            rawType.typeArguments);
+      }
     }
     node.constructorName?.accept(this);
-    FunctionType constructorType = node.constructorName.staticElement?.type;
-    if (constructorType != null) {
+    ConstructorElement constructor = node.constructorName.staticElement;
+    FunctionType constructorType = constructor?.type;
+    if (strongMode && constructorType != null) {
       InferenceContext.setType(node.argumentList, constructorType);
     }
     node.argumentList?.accept(this);
@@ -6209,7 +6233,10 @@ class ResolverVisitor extends ScopedVisitor {
 
   @override
   Object visitListLiteral(ListLiteral node) {
-    DartType contextType = InferenceContext.getType(node);
+    DartType contextType = InferenceContext.getContext(node);
+    if (contextType is FutureUnionType) {

[Leaf, 2016/11/30 05:24:29]

Consider making this pattern a method on InferenceContext?  

InferenceContext.getValueContext?

[Jennifer Messerly, 2016/11/30 21:19:28]

great idea, done!

+      contextType = (contextType as FutureUnionType).type;
+    }
     List<DartType> targs = null;
     if (node.typeArguments != null) {
       targs = node.typeArguments.arguments.map((t) => t.type).toList();
@@ -6234,7 +6261,10 @@ class ResolverVisitor extends ScopedVisitor {
 
   @override
   Object visitMapLiteral(MapLiteral node) {
-    DartType contextType = InferenceContext.getType(node);
+    DartType contextType = InferenceContext.getContext(node);
+    if (contextType is FutureUnionType) {
+      contextType = (contextType as FutureUnionType).type;
+    }
     List<DartType> targs = null;
     if (node.typeArguments != null) {
       targs = node.typeArguments.arguments.map((t) => t.type).toList();
@@ -6686,18 +6716,18 @@ class ResolverVisitor extends ScopedVisitor {
       DartType originalType = node.function.staticType;
       DartType returnContextType = InferenceContext.getContext(node);
       TypeSystem ts = typeSystem;
-      if (returnContextType != null &&
-          node.typeArguments == null &&
+      if (node.typeArguments == null &&
           originalType is FunctionType &&
           originalType.typeFormals.isNotEmpty &&
           ts is StrongTypeSystemImpl) {
-        contextType = ts.inferGenericFunctionCall(
+        contextType = ts.inferGenericFunctionOrType/*<FunctionType>*/(
             typeProvider,
             originalType,
-            DartType.EMPTY_LIST,
+            ParameterElement.EMPTY_LIST,
             DartType.EMPTY_LIST,
             originalType.returnType,
-            returnContextType);
+            returnContextType,
+            downwards: true);
       }
 
       InferenceContext.setType(node.argumentList, contextType);