fixes #27586, prefer context type in generic inference

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

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library analyzer.src.generated.resolver;
 
 import 'dart:collection';
 
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart';
@@ skipping 4300 matching lines @@
    * Look for a single contextual type attached to [node], and returns the type
    * if found, otherwise null.
    *
    * If [node] has a contextual union type like `T | Future<T>` this will
    * simplify it to only return `T`. If the caller can handle a union type,
    * [getContext] should be used instead.
    */
   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);
   }
 
   /**
    * Like [getContext] but expands a union type into a list of types.
    */
   static Iterable<DartType> getTypes(AstNode node) {
     DartType t = getContext(node);
     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);
   }
 
   /**
    * Attach contextual type information [type] to [node] for use during
    * inference.
    */
   static void setType(AstNode node, DartType type) {
     if (type == null || type.isDynamic) {
       clearType(node);
     } else {
@@ skipping 1043 matching lines @@
       assert(parent is PartOfDirective);
     } else {
       elementAnnotationImpl.annotationAst =
           new ConstantAstCloner().cloneNode(node);
     }
     return null;
   }
 
   @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;
       List<DartType> normalParameterTypes = callerType.normalParameterTypes;
       List<DartType> optionalParameterTypes = callerType.optionalParameterTypes;
       int normalCount = normalParameterTypes.length;
       int optionalCount = optionalParameterTypes.length;
 
       NodeList<Expression> arguments = node.arguments;
       Iterable<Expression> positional =
@@ skipping 736 matching lines @@
 
   @override
   Object visitInstanceCreationExpression(InstanceCreationExpression node) {
     TypeName classTypeName = node.constructorName.type;
     // TODO(leafp): Currently, we may re-infer types here, since we
     // sometimes resolve multiple times.  We should really check that we
     // have not already inferred something.  However, the obvious ways to
     // 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:
           //
           //     class E<S, T> extends A<C<S>, T> { ... }
           //     A<C<int>, String> a0 = /*infer<int, String>*/new E("hello");
           //
           // 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
             // infer it first
             typeAnalyzer.inferConstructorName(node.constructorName, fullType);
             break;
           }
         }
       }
+      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);
     node.accept(elementResolver);
     node.accept(typeAnalyzer);
     return null;
   }
 
   @override
   Object visitLabel(Label node) => null;
 
   @override
   Object visitLibraryIdentifier(LibraryIdentifier node) => null;
 
   @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();
     } else if (contextType is InterfaceType) {
       InterfaceType listD =
           typeProvider.listType.instantiate([typeProvider.dynamicType]);
       targs = inferenceContext.matchTypes(listD, contextType);
     }
     if (targs != null && targs.length == 1 && !targs[0].isDynamic) {
       DartType eType = targs[0];
       InterfaceType listT = typeProvider.listType.instantiate([eType]);
       for (Expression child in node.elements) {
         InferenceContext.setType(child, eType);
       }
       InferenceContext.setType(node, listT);
     } else {
       InferenceContext.clearType(node);
     }
     super.visitListLiteral(node);
     return null;
   }
 
   @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();
     } else if (contextType is InterfaceType) {
       InterfaceType mapD = typeProvider.mapType
           .instantiate([typeProvider.dynamicType, typeProvider.dynamicType]);
       targs = inferenceContext.matchTypes(mapD, contextType);
     }
     if (targs != null && targs.length == 2 && targs.any((t) => !t.isDynamic)) {
       DartType kType = targs[0];
@@ skipping 431 matching lines @@
       // Use propagated type inference for lambdas if not in strong mode.
       _inferFunctionExpressionsParametersTypes(node.argumentList);
       return;
     }
 
     DartType contextType = node.staticInvokeType;
     if (contextType is FunctionType) {
       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);
     }
   }
 
   void _inferFormalParameterList(FormalParameterList node, DartType type) {
     if (typeAnalyzer.inferFormalParameterList(node, type)) {
       // TODO(leafp): This gets dropped on the floor if we're in the field
       // inference task.  We should probably keep these infos.
@@ skipping 4051 matching lines @@
       return null;
     }
     if (identical(node.staticElement, variable)) {
       if (node.inSetterContext()) {
         result = true;
       }
     }
     return null;
   }
 }