Signal errors and warnings for invalid returns

Implement the feature spec for checking returns against inferred
function return types:

https://github.com/dart-lang/sdk/blob/master/docs/language/informal/invalid_returns.md

with the change that an invalid return without a subexpression is
a warning, not an error.  This is because it is a warning in the
analyzer and it would be a breaking change to turn it into an
error.

The check for valid returns is moved to exactly where we handle
returns, rather than in ensureAssignable which is used as a
helper in a lot of places. A bug in type inference was fixed: we
would use `void` for the type of return without a subexpression
instead of `null`.  To accommodate that we would use a
non-standard subtyping relation for returns.  This could lead to
us inferring a return type of `void` in cases where we should
not.

Change-Id: Iee9ece9c722f47efa305f49490d3022d0bbb9f44
Reviewed-on: https://dart-review.googlesource.com/72403
Commit-Queue: Kevin Millikin <[email protected]>
Reviewed-by: Aske Simon Christensen <[email protected]>

Author: Kevin Millikin

pkg/front_end/lib/src/fasta/fasta_codes_generated.dart

@@ -6284,7 +6284,7 @@ const Code<Null> codeReturnFromVoidFunction = messageReturnFromVoidFunction;
 const MessageCode messageReturnFromVoidFunction = const MessageCode(
     "ReturnFromVoidFunction",
     analyzerCode: "RETURN_OF_INVALID_TYPE",
-    severity: Severity.warning,
+    severity: Severity.error,
     message: r"""Can't return a value from a void function.""");
 
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
@@ -6297,6 +6297,16 @@ const MessageCode messageReturnTypeFunctionExpression = const MessageCode(
     severity: Severity.errorLegacyWarning,
     message: r"""A function expression can't have a return type.""");
 
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const Code<Null> codeReturnWithoutExpression = messageReturnWithoutExpression;
+
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const MessageCode messageReturnWithoutExpression = const MessageCode(
+    "ReturnWithoutExpression",
+    analyzerCode: "RETURN_WITHOUT_VALUE",
+    severity: Severity.warning,
+    message: r"""Must explicitly return a value from a non-void function.""");
+
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const Template<Message Function(Uri uri_)> templateSdkRootNotFound =
     const Template<Message Function(Uri uri_)>(
@@ -7421,7 +7431,7 @@ const Code<Null> codeVoidExpression = messageVoidExpression;
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const MessageCode messageVoidExpression = const MessageCode("VoidExpression",
     analyzerCode: "USE_OF_VOID_RESULT",
-    severity: Severity.warning,
+    severity: Severity.error,
     message: r"""This expression has type 'void' and can't be used.""");
 
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.

pkg/front_end/lib/src/fasta/kernel/kernel_shadow_ast.dart

@@ -2377,15 +2377,10 @@ class ReturnJudgment extends ReturnStatement implements StatementJudgment {
           isVoidAllowed: true);
       inferredType = judgment.inferredType;
     } else {
-      inferredType = const VoidType();
-    }
-    // Analyzer treats bare `return` statements as having no effect on the
-    // inferred type of the closure.  TODO(paulberry): is this what we want
-    // for Fasta?
-    if (judgment != null) {
-      closureContext.handleReturn(inferrer, inferredType, expression,
-          fileOffset, !identical(returnKeywordLexeme, "return"));
+      inferredType = inferrer.coreTypes.nullClass.rawType;
     }
+    closureContext.handleReturn(inferrer, this, inferredType,
+        !identical(returnKeywordLexeme, "return"));
     inferrer.listener.returnStatement(this, fileOffset, tokens, null);
   }
 }
@@ -3134,7 +3129,8 @@ class ShadowTypeInferrer extends TypeInferrerImpl {
   DartType inferFieldTopLevel<Expression, Statement, Initializer, Type>(
       ShadowField field, bool typeNeeded) {
     if (field.initializer == null) return const DynamicType();
-    return inferExpression(field.initializer, const UnknownType(), typeNeeded);
+    return inferExpression(field.initializer, const UnknownType(), typeNeeded,
+        isVoidAllowed: true);
   }
 
   @override

pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart

@@ -36,6 +36,7 @@ import 'package:kernel/ast.dart'
         ProcedureKind,
         PropertyGet,
         PropertySet,
+        ReturnStatement,
         StaticGet,
         SuperMethodInvocation,
         SuperPropertyGet,
@@ -74,6 +75,7 @@ import '../fasta_codes.dart'
         Message,
         Template,
         messageReturnFromVoidFunction,
+        messageReturnWithoutExpression,
         messageVoidExpression,
         noLength,
         templateArgumentTypeNotAssignable,
@@ -203,10 +205,24 @@ class ClosureContext {
   /// wrapping this type in `Stream` or `Iterator`, as appropriate.
   DartType _inferredUnwrappedReturnOrYieldType;
 
+  /// Whether the function is an arrow function.
+  bool isArrow;
+
+  /// A list of return statements in functions whose return type is being
+  /// inferred.
+  ///
+  /// The returns are checked for validity after the return type is inferred.
+  List<ReturnStatement> returnStatements;
+
+  /// A list of return expression types in functions whose return type is
+  /// being inferred.
+  List<DartType> returnExpressionTypes;
+
   factory ClosureContext(TypeInferrerImpl inferrer, AsyncMarker asyncMarker,
       DartType returnContext, bool needToInferReturnType) {
     assert(returnContext != null);
-    DartType declaredReturnType = returnContext;
+    DartType declaredReturnType =
+        greatestClosure(inferrer.coreTypes, returnContext);
     bool isAsync = asyncMarker == AsyncMarker.Async ||
         asyncMarker == AsyncMarker.AsyncStar;
     bool isGenerator = asyncMarker == AsyncMarker.SyncStar ||
@@ -228,23 +244,86 @@ class ClosureContext {
   }
 
   ClosureContext._(this.isAsync, this.isGenerator, this.returnOrYieldContext,
-      this.declaredReturnType, this._needToInferReturnType) {}
+      this.declaredReturnType, this._needToInferReturnType) {
+    if (_needToInferReturnType) {
+      returnStatements = [];
+      returnExpressionTypes = [];
+    }
+  }
+
+  bool checkValidReturn(TypeInferrerImpl inferrer, DartType returnType,
+      ReturnStatement statement, DartType expressionType) {
+    if (statement.expression == null) {
+      if (isAsync) {
+        returnType = inferrer.typeSchemaEnvironment.unfutureType(returnType);
+      }
+      if (returnType is! VoidType &&
+          returnType is! DynamicType &&
+          returnType != inferrer.coreTypes.nullClass.rawType) {
+        statement.expression = inferrer.helper.wrapInProblem(
+            new NullLiteral()..fileOffset = statement.fileOffset,
+            messageReturnWithoutExpression,
+            noLength)
+          ..parent = statement;
+        return false;
+      }
+    } else {
+      if (isAsync) {
+        returnType = inferrer.typeSchemaEnvironment.unfutureType(returnType);
+        expressionType =
+            inferrer.typeSchemaEnvironment.unfutureType(expressionType);
+      }
+      if (!isArrow && returnType is VoidType) {
+        if (expressionType is! VoidType &&
+            expressionType is! DynamicType &&
+            expressionType != inferrer.coreTypes.nullClass.rawType) {
+          statement.expression = inferrer.helper.wrapInProblem(
+              statement.expression, messageReturnFromVoidFunction, noLength)
+            ..parent = statement;
+          return false;
+        }
+      } else if (expressionType is VoidType) {
+        if (returnType is! VoidType &&
+            returnType is! DynamicType &&
+            returnType != inferrer.coreTypes.nullClass.rawType) {
+          statement.expression = inferrer.helper.wrapInProblem(
+              statement.expression, messageVoidExpression, noLength)
+            ..parent = statement;
+          return false;
+        }
+      }
+    }
+    return true;
+  }
 
   /// Updates the inferred return type based on the presence of a return
   /// statement returning the given [type].
-  void handleReturn(TypeInferrerImpl inferrer, DartType type,
-      Expression expression, int fileOffset, bool isArrow) {
+  void handleReturn(TypeInferrerImpl inferrer, ReturnStatement statement,
+      DartType type, bool isArrow) {
     if (isGenerator) return;
-    if (inferrer.ensureAssignable(
-            returnOrYieldContext, type, expression, fileOffset,
-            isReturnFromAsync: isAsync,
-            isReturn: true,
-            declaredReturnType: declaredReturnType,
-            isArrow: isArrow) !=
-        null) {
-      type = greatestClosure(inferrer.coreTypes, returnOrYieldContext);
+    // The first return we see tells us if we have an arrow function.
+    if (this.isArrow == null) {
+      this.isArrow = isArrow;
+    } else {
+      assert(this.isArrow == isArrow);
     }
+
     if (_needToInferReturnType) {
+      // Add the return to a list to be checked for validity after we've
+      // inferred the return type.
+      returnStatements.add(statement);
+      returnExpressionTypes.add(type);
+
+      // The return expression has to be assignable to the return type
+      // expectation from the downwards inference context.
+      if (statement.expression != null &&
+          inferrer.ensureAssignable(returnOrYieldContext, type,
+                  statement.expression, statement.fileOffset,
+                  isReturnFromAsync: isAsync, isVoidAllowed: true) !=
+              null) {
+        // Not assignable, use the expectation.
+        type = greatestClosure(inferrer.coreTypes, returnOrYieldContext);
+      }
       var unwrappedType = type;
       if (isAsync) {
         unwrappedType = inferrer.typeSchemaEnvironment.unfutureType(type);
@@ -256,6 +335,16 @@ class ClosureContext {
             .getStandardUpperBound(
                 _inferredUnwrappedReturnOrYieldType, unwrappedType);
       }
+      return;
+    }
+
+    // If we are not inferring a type we can immediately check that the return
+    // is valid.
+    if (checkValidReturn(inferrer, declaredReturnType, statement, type) &&
+        statement.expression != null) {
+      inferrer.ensureAssignable(returnOrYieldContext, type,
+          statement.expression, statement.fileOffset,
+          isReturnFromAsync: isAsync, isVoidAllowed: true);
     }
   }
 
@@ -294,13 +383,20 @@ class ClosureContext {
     assert(_needToInferReturnType);
     DartType inferredType =
         inferrer.inferReturnType(_inferredUnwrappedReturnOrYieldType);
-    if (!_analyzerSubtypeOf(inferrer, inferredType, returnOrYieldContext)) {
+    if (!inferrer.typeSchemaEnvironment
+        .isSubtypeOf(inferredType, returnOrYieldContext)) {
       // If the inferred return type isn't a subtype of the context, we use the
       // context.
       inferredType = greatestClosure(inferrer.coreTypes, returnOrYieldContext);
     }
 
-    return _wrapAsyncOrGenerator(inferrer, inferredType);
+    inferredType = _wrapAsyncOrGenerator(inferrer, inferredType);
+    for (int i = 0; i < returnStatements.length; ++i) {
+      checkValidReturn(inferrer, inferredType, returnStatements[i],
+          returnExpressionTypes[i]);
+    }
+
+    return inferredType;
   }
 
   DartType _wrapAsyncOrGenerator(TypeInferrerImpl inferrer, DartType type) {
@@ -316,19 +412,6 @@ class ClosureContext {
       return type;
     }
   }
-
-  static bool _analyzerSubtypeOf(
-      TypeInferrerImpl inferrer, DartType subtype, DartType supertype) {
-    if (supertype is VoidType) {
-      if (subtype is VoidType) return true;
-      if (subtype is InterfaceType &&
-          identical(subtype.classNode, inferrer.coreTypes.nullClass)) {
-        return true;
-      }
-      return false;
-    }
-    return inferrer.typeSchemaEnvironment.isSubtypeOf(subtype, supertype);
-  }
 }
 
 /// Enum denoting the kinds of contravariance check that might need to be
@@ -585,22 +668,9 @@ abstract class TypeInferrerImpl extends TypeInferrer {
   Expression ensureAssignable(DartType expectedType, DartType actualType,
       Expression expression, int fileOffset,
       {bool isReturnFromAsync: false,
-      bool isReturn: false,
-      bool isVoidAllowed,
-      bool isArrow: false,
-      DartType declaredReturnType,
+      bool isVoidAllowed: false,
       Template<Message Function(DartType, DartType)> template}) {
-    isVoidAllowed ??= isArrow;
     assert(expectedType != null);
-    if (isReturn &&
-        !isArrow &&
-        !isValidReturn(declaredReturnType, actualType, isReturnFromAsync)) {
-      TreeNode parent = expression.parent;
-      Expression errorNode = helper.wrapInProblem(
-          expression, messageReturnFromVoidFunction, noLength);
-      parent?.replaceChild(expression, errorNode);
-      return errorNode;
-    }
     expectedType = greatestClosure(coreTypes, expectedType);
 
     DartType initialExpectedType = expectedType;
@@ -617,28 +687,6 @@ abstract class TypeInferrerImpl extends TypeInferrer {
         expectedType = futuredExpectedType;
       }
     }
-    if (isReturn && !isArrow) {
-      if (expectedType is VoidType) {
-        isVoidAllowed = true;
-        if (actualType is! VoidType &&
-            actualType is! DynamicType &&
-            !isNull(actualType)) {
-          // Error: not assignable.  Perform error recovery.
-          TreeNode parent = expression.parent;
-          Expression errorNode = helper.wrapInProblem(
-              expression, messageReturnFromVoidFunction, noLength);
-          parent?.replaceChild(expression, errorNode);
-          return errorNode;
-        }
-      } else {
-        DartType flattened = typeSchemaEnvironment.unfutureType(expectedType);
-        if (flattened is VoidType) {
-          isVoidAllowed = true;
-        } else {
-          isVoidAllowed = expectedType is DynamicType;
-        }
-      }
-    }
 
     // We don't need to insert assignability checks when doing top level type
     // inference since top level type inference only cares about the type that
@@ -721,63 +769,6 @@ abstract class TypeInferrerImpl extends TypeInferrer {
     }
   }
 
-  bool isValidReturn(
-      DartType returnType, DartType expressionType, bool isAsync) {
-    final DartType t = returnType;
-    final DartType s = expressionType;
-    if (!isAsync) {
-      if (t is DynamicType) {
-        // * `return exp;` where `exp` has static type `S` is a valid return if:
-        //   * `T` is `dynamic`
-        return true;
-      }
-
-      if (t is VoidType) {
-        // * `return exp;` where `exp` has static type `S` is a valid return if:
-        //   * `T` is `void`
-        //   * and `S` is `void` or `dynamic` or `Null`
-        return s is VoidType || s is DynamicType || isNull(s);
-      } else {
-        // * `return exp;` where `exp` has static type `S` is a valid return if:
-        //   * `T` is not `void`
-        //   * and `S` is not `void`
-        //   * and `S` is assignable to `T`
-        return s is! VoidType;
-      }
-    }
-    final DartType flattenT = typeSchemaEnvironment.unfutureType(t);
-
-    // * `return exp;` where `exp` has static type `S` is a valid return if:
-    //   * `flatten(T)` is `dynamic` or `Null`
-    if (flattenT is DynamicType || isNull(flattenT)) return true;
-
-    // * `return exp;` where `exp` has static type `S` is a valid return if:
-    //   * `T` is `void`
-    //   * and `S` is `void`, `dynamic` or `Null`
-    if (t is VoidType) {
-      if (s is VoidType || s is DynamicType || isNull(s)) return true;
-    } else {
-      final DartType flattenS = typeSchemaEnvironment.unfutureType(s);
-      // * `return exp;` where `exp` has static type `S` is a valid return if:
-      //   * `T` is not `void`
-      //   * `flatten(T)` is `void`
-      //   * and `flatten(S)` is `void`, `dynamic` or `Null`
-      if (flattenT is VoidType) {
-        if (flattenS is VoidType ||
-            flattenS is DynamicType ||
-            isNull(flattenS)) {
-          return true;
-        }
-      }
-
-      // * `return exp;` where `exp` has static type `S` is a valid return if:
-      //   * `T` is not `void`
-      //   * and `flatten(S)` is not `void`
-      if (flattenS is! VoidType) return true;
-    }
-    return false;
-  }
-
   bool isNull(DartType type) {
     return type is InterfaceType && type.classNode == coreTypes.nullClass;
   }
@@ -1215,10 +1206,12 @@ abstract class TypeInferrerImpl extends TypeInferrer {
     assert(closureContext == null);
     this.helper = helper;
     var actualType = inferExpression(
-        initializer, declaredType ?? const UnknownType(), declaredType != null);
+        initializer, declaredType ?? const UnknownType(), declaredType != null,
+        isVoidAllowed: true);
     if (declaredType != null) {
       ensureAssignable(
-          declaredType, actualType, initializer, initializer.fileOffset);
+          declaredType, actualType, initializer, initializer.fileOffset,
+          isVoidAllowed: declaredType is VoidType);
     }
     this.helper = null;
   }