fix #28008, fix #28009 implement FutureOr<T>

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

Index: pkg/analyzer/lib/src/generated/static_type_analyzer.dart
diff --git a/pkg/analyzer/lib/src/generated/static_type_analyzer.dart b/pkg/analyzer/lib/src/generated/static_type_analyzer.dart
index b3ec3eb61a30dffadb3b6a2dc1be868cfc9ee549..9ff7ec1b41f6c9ecea3f361e8b9c99038f35a0dc 100644
--- a/pkg/analyzer/lib/src/generated/static_type_analyzer.dart
+++ b/pkg/analyzer/lib/src/generated/static_type_analyzer.dart
@@ -811,7 +811,7 @@ class StaticTypeAnalyzer extends SimpleAstVisitor<Object> {
       Expression target = node.realTarget;
       if (target != null) {
         DartType targetType = target.bestType;
-        if (_isAsyncFutureType(targetType)) {
+        if (targetType.isDartAsyncFuture) {
           // Future.then(closure) return type is:
           // 1) the returned Future type, if the closure returns a Future;
           // 2) Future<valueType>, if the closure returns a value.
@@ -1419,11 +1419,23 @@ class StaticTypeAnalyzer extends SimpleAstVisitor<Object> {
       // TODO(brianwilkerson) Determine whether this can still happen.
       staticType2 = _dynamicType;
     }
+
     DartType staticType =
-        _typeSystem.getLeastUpperBound(staticType1, staticType2);
-    if (staticType == null) {
-      staticType = _dynamicType;
+        _typeSystem.getLeastUpperBound(staticType1, staticType2) ??
+            _dynamicType;
+
+    // TODO(jmesserly): this behavior needs to be specified.
+    // The goal here is to avoid LUB concluding Object when we already had a
+    // valid FutureOr<T> as the downward context type for a conditional.

[Leaf, 2017/01/24 21:56:26]

I'm not keen on putting this in just as a hack for FutureOr.  My take is that
this should be a separate feature, and should apply to all cases where we have a
downwards context.   Can we do this in a follow up CL, and make it apply to all
contexts?  This should also work for ?? and other related operators.

[Jennifer Messerly, 2017/01/25 00:33:35]

Done.

+    if (_strongMode && staticType.isObject) {

[Paul Berry, 2017/01/24 17:45:13]

Why put this logic here and not in _typeSystem.getLeastUpperBound?  Putting it
here means that we'll have to replicate this logic in summary type inference;
putting it in _typeSystem.getLeastUpperBound would ensure that summary type
inference and non-summary type inference stay consistent.

[Jennifer Messerly, 2017/01/24 18:58:20]

yeah ... the problem tho is we have two implementations of resolution? I don't
know the best way to attack that. almost every type inference change hits it. I
would love to learn to fix the summary code, but I don't think it gets the same
coverage for strong mode (e.g. it doesn't seem to test that strong mode errors
were produced in inferred_type_test & checker_test?) ... and i don't really
understand how it works. Would be happy to discuss this larger issue though.
I don't know if we want to implement it in LUB for the language. The problem
with LUB is it's synthesizing types and used in many places.

Conditional expressions are a bit special, we've already checked the types for
both branches in strong mode, and we had a downwards context FutureOr<T>. So
this one is sound and matches the request in
https://github.com/dart-lang/sdk/issues/25368

[Paul Berry, 2017/01/24 19:19:09]

Agreed.  It's really unfortunate that we have two implementations of resolution.
 As part of the front_end work, I am hoping to get us to the point where so much
code is shared that it's not really two implementations anymore, it's just two
slightly different code paths that lead to the same inference engine (one code
path for when we're inferring something that's visible as part of summaries, one
code path for when we're inferring a local variable).  But it's going to take
some time to get there.
I'm glad to help with this--the more people who understand the summary code the
better.
The summary logic doesn't do error checking so there's no error checking to
validate.  What we do instead is we run each test in inferred_type_test through
both kinds of inference and verify that they produce the same set of types. 
IIRC, the reason checker_test isn't tested with summaries is that all of its
assertions are based on validating that the expected errors occur; since summary
logic doesn't do error checking, there's nothing to test.

If there's anything else missing from summary validation I'd love to talk about
it and get the test coverage up.
That would be nice.  Let's schedule a meeting to talk about it.
Ok, fair enough.  In that case, given the current architecture, I don't think we
can get around the need to duplicate this logic in the summary inference engine.

I'll look into doing that as soon as I can.

[Paul Berry, 2017/01/24 20:14:50]

Ack!  I just re-read this and realized that it only applies in the case where
downward inference is in play (which never happens during summary inference). 
So forget everything I said about needing to duplicate this logic for summaries.
 Sorry for the noise.

[Jennifer Messerly, 2017/01/25 00:33:35]

ahhh gotcha :)

yeah that explains why a lot of these downwards expression inference changes
don't need to affect summary generation. good to know!

+      DartType context = InferenceContext.getContext(node);
+      if (context != null && context.isDartAsyncFutureOr &&
+          _typeSystem.isSubtypeOf(staticType1, context) &&
+          _typeSystem.isSubtypeOf(staticType2, context)) {
+        staticType = context;
+      }
     }
+
     _recordStaticType(node, staticType);
     DartType propagatedType1 = expr1.propagatedType;
     DartType propagatedType2 = expr2.propagatedType;
@@ -1520,6 +1532,9 @@ class StaticTypeAnalyzer extends SimpleAstVisitor<Object> {
           : _typeProvider.iterableType;
       return genericType.instantiate(<DartType>[type]);
     } else if (body.isAsynchronous) {
+      if (type.isDartAsyncFutureOr) {

[Paul Berry, 2017/01/24 17:45:13]

I'm worried that this behavior hasn't been implemented in the summary type
inference logic yet.  But I don't recall seeing test failures.  Is this behavior
tested in pkg/analyzer/test/src/task/strong/inferred_type_test.dart?

[Jennifer Messerly, 2017/01/24 18:58:20]

this is the same logic that was below, on line 2084 in the old code. It's just
moved here.
(_computeReturnTypeOfFunction only has one caller:
_inferLocalFunctionReturnType)

as to whether it was implemented previously in summary code, I don't know.
yeah that's where most of its tests are

[Paul Berry, 2017/01/24 19:19:09]

Ok, I didn't realize this was preexisting behavior.  In which case, if the
summary code isn't replicating this, it's a preexisting bug in summary code so
it shouldn't block this CL.  I'll investigate the summary code and follow up
with a CL (if necessary) as soon as I can.

+        type = (type as InterfaceType).typeArguments[0];
+      }
       return _typeProvider.futureType
           .instantiate(<DartType>[type.flattenFutures(_typeSystem)]);
     } else {
@@ -1962,44 +1977,6 @@ class StaticTypeAnalyzer extends SimpleAstVisitor<Object> {
           argTypes.add(argumentList.arguments[i].staticType);
         }
       }
-
-      // Special case Future<T>.then upwards inference. It has signature:
-      //
-      //     <S>(T -> (S | Future<S>)) -> Future<S>
-      //
-      // Based on the first argument type, we'll pick one of these signatures:
-      //
-      //     <S>(T -> S) -> Future<S>
-      //     <S>(T -> Future<S>) -> Future<S>
-      //
-      // ... and finish the inference using that.
-      if (argTypes.isNotEmpty && _resolver.isFutureThen(fnType.element)) {
-        var firstArgType = argTypes[0];
-        var firstParamType = paramTypes[0] as FunctionType;
-        if (firstArgType is FunctionType) {
-          var argReturnType = firstArgType.returnType;
-          // Skip the inference if we have the top type. It can only lead to
-          // worse inference. For example, this happens when the lambda returns
-          // S or Future<S> in a conditional.
-          if (!argReturnType.isObject && !argReturnType.isDynamic) {
-            DartType paramReturnType = fnType.typeFormals[0].type;
-            if (_resolver.isSubtypeOfFuture(argReturnType)) {
-              // Given an argument of (T) -> Future<S>, instantiate with <S>
-              paramReturnType =
-                  _typeProvider.futureType.instantiate([paramReturnType]);
-            }
-
-            // Adjust the expected parameter type to have this return type.
-            var function = new FunctionElementImpl(firstParamType.name, -1)
-              ..isSynthetic = true
-              ..shareParameters(firstParamType.parameters)
-              ..returnType = paramReturnType;
-            function.type = new FunctionTypeImpl(function);
-            // Use this as the expected 1st parameter type.
-            paramTypes[0] = function.type;
-          }
-        }
-      }
       return ts.inferGenericFunctionCall(fnType, paramTypes, argTypes,
           fnType.returnType, InferenceContext.getContext(node),
           errorReporter: _resolver.errorReporter, errorNode: errorNode);
@@ -2081,12 +2058,7 @@ class StaticTypeAnalyzer extends SimpleAstVisitor<Object> {
     // * we also normalize bottom to dynamic here.
     if (_strongMode && (computedType.isBottom || computedType.isDynamic)) {
       DartType contextType = InferenceContext.getContext(body);
-      if (contextType is FutureUnionType) {
-        // TODO(jmesserly): can we do something better here?
-        computedType = body.isAsynchronous ? contextType.type : _dynamicType;
-      } else {
-        computedType = contextType ?? _dynamicType;
-      }
+      computedType = contextType ?? _dynamicType;
       recordInference = !computedType.isDynamic;
     }
 
@@ -2221,23 +2193,6 @@ class StaticTypeAnalyzer extends SimpleAstVisitor<Object> {
   }
 
   /**
-   * Return `true` if the given [Type] is the `Future` form the 'dart:async'
-   * library.
-   */
-  bool _isAsyncFutureType(DartType type) =>
-      type is InterfaceType &&
-      type.name == "Future" &&
-      _isAsyncLibrary(type.element.library);
-
-  /**
-   * Return `true` if the given library is the 'dart:async' library.
-   *
-   * @param library the library being tested
-   * @return `true` if the library is 'dart:async'
-   */
-  bool _isAsyncLibrary(LibraryElement library) => library.name == "dart.async";
-
-  /**
    * Return `true` if the given library is the 'dart:html' library.
    *
    * @param library the library being tested