Don't report redundant type errors in strong mode.

Currently, "sideways casts" -- type errors where one type is assigned
to an unrelated type -- are reported by both ErrorVerifier and strong
mode's Checker. This leads to duplicate errors that the user can see.

ErrorVerifier's errors are generally better: they give the user more
contextual information and are easier to read. So this CL eliminates
Checker's reporting of these errors and only uses ErrorVerifier's.

However, in strong mode, type errors like this are fatal: DDC can't
generate correct code. So this also automatically upgrades all static
type warnings to errors when strong mode is enabled.

[email protected], [email protected]

Review URL: https://codereview.chromium.org/1780783002 .

Author: munificent

pkg/analyzer/lib/source/error_processor.dart

@@ -93,9 +93,37 @@ class ErrorProcessor {
     if (context == null) {
       return null;
     }
+
+    // By default, the error is not processed.
+    ErrorProcessor processor;
+
+    // Give strong mode a chance to upgrade it.
+    if (context.analysisOptions.strongMode) {
+      processor = _StrongModeTypeErrorProcessor.instance;
+    }
+
+    // Let the user configure how specific errors are processed.
     List<ErrorProcessor> processors =
         context.getConfigurationData(CONFIGURED_ERROR_PROCESSORS);
+
     return processors.firstWhere((ErrorProcessor p) => p.appliesTo(error),
-        orElse: () => null);
+        orElse: () => processor);
   }
 }
+
+/// In strong mode, this upgrades static type warnings to errors.
+class _StrongModeTypeErrorProcessor implements ErrorProcessor {
+  static final instance = new _StrongModeTypeErrorProcessor();
+
+  // TODO(rnystrom): As far as I know, this is only used to implement
+  // appliesTo(). Consider making it private in ErrorProcessor if possible.
+  String get code => throw new UnsupportedError(
+      "_StrongModeTypeErrorProcessor is not specific to an error code.");
+
+  /// In strong mode, type warnings are upgraded to errors.
+  ErrorSeverity get severity => ErrorSeverity.ERROR;
+
+  /// Check if this processor applies to the given [error].
+  bool appliesTo(AnalysisError error) =>
+      error.errorCode.type == ErrorType.STATIC_TYPE_WARNING;
+}

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

@@ -846,8 +846,9 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
         }
       }
       _checkForExpectedOneListTypeArgument(node, typeArguments);
-      _checkForListElementTypeNotAssignable(node, typeArguments);
     }
+
+    _checkForListElementTypeNotAssignable(node);
     return super.visitListLiteral(node);
   }
 
@@ -863,8 +864,9 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
         }
       }
       _checkExpectedTwoMapTypeArguments(typeArguments);
-      _checkForMapTypeNotAssignable(node, typeArguments);
     }
+
+    _checkForMapTypeNotAssignable(node);
     _checkForNonConstMapAsExpressionStatement(node);
     return super.visitMapLiteral(node);
   }
@@ -4038,91 +4040,81 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
   }
 
   /**
-   * Verify that the elements given list [literal] are subtypes of the specified
-   * element type. The [typeArguments] are the type arguments.
+   * Verify that the elements given list [literal] are subtypes of the list's
+   * static type.
    *
    * See [CompileTimeErrorCode.LIST_ELEMENT_TYPE_NOT_ASSIGNABLE], and
    * [StaticWarningCode.LIST_ELEMENT_TYPE_NOT_ASSIGNABLE].
    */
-  bool _checkForListElementTypeNotAssignable(
-      ListLiteral literal, TypeArgumentList typeArguments) {
-    NodeList<TypeName> typeNames = typeArguments.arguments;
-    if (typeNames.length < 1) {
-      return false;
-    }
-    DartType listElementType = typeNames[0].type;
+  void _checkForListElementTypeNotAssignable(ListLiteral literal) {
+    // Determine the list's element type. We base this on the static type and
+    // not the literal's type arguments because in strong mode, the type
+    // arguments may be inferred.
+    DartType listType = literal.staticType;
+    assert(listType is InterfaceTypeImpl);
+
+    List<DartType> typeArguments =
+        (listType as InterfaceTypeImpl).typeArguments;
+    assert(typeArguments.length == 1);
+
+    DartType listElementType = typeArguments[0];
+
     // Check every list element.
-    bool hasProblems = false;
     for (Expression element in literal.elements) {
       if (literal.constKeyword != null) {
         // TODO(paulberry): this error should be based on the actual type of the
         // list element, not the static type.  See dartbug.com/21119.
-        if (_checkForArgumentTypeNotAssignableWithExpectedTypes(
+        _checkForArgumentTypeNotAssignableWithExpectedTypes(
             element,
             listElementType,
-            CheckedModeCompileTimeErrorCode.LIST_ELEMENT_TYPE_NOT_ASSIGNABLE)) {
-          hasProblems = true;
-        }
-      }
-      if (_checkForArgumentTypeNotAssignableWithExpectedTypes(
-          element,
-          listElementType,
-          StaticWarningCode.LIST_ELEMENT_TYPE_NOT_ASSIGNABLE)) {
-        hasProblems = true;
+            CheckedModeCompileTimeErrorCode.LIST_ELEMENT_TYPE_NOT_ASSIGNABLE);
       }
+      _checkForArgumentTypeNotAssignableWithExpectedTypes(element,
+          listElementType, StaticWarningCode.LIST_ELEMENT_TYPE_NOT_ASSIGNABLE);
     }
-    return hasProblems;
   }
 
   /**
    * Verify that the key/value of entries of the given map [literal] are
-   * subtypes of the key/value types specified in the type arguments. The
-   * [typeArguments] are the type arguments.
+   * subtypes of the map's static type.
    *
    * See [CompileTimeErrorCode.MAP_KEY_TYPE_NOT_ASSIGNABLE],
    * [CompileTimeErrorCode.MAP_VALUE_TYPE_NOT_ASSIGNABLE],
    * [StaticWarningCode.MAP_KEY_TYPE_NOT_ASSIGNABLE], and
    * [StaticWarningCode.MAP_VALUE_TYPE_NOT_ASSIGNABLE].
    */
-  bool _checkForMapTypeNotAssignable(
-      MapLiteral literal, TypeArgumentList typeArguments) {
-    // Prepare maps key/value types.
-    NodeList<TypeName> typeNames = typeArguments.arguments;
-    if (typeNames.length < 2) {
-      return false;
-    }
-    DartType keyType = typeNames[0].type;
-    DartType valueType = typeNames[1].type;
-    // Check every map entry.
-    bool hasProblems = false;
+  void _checkForMapTypeNotAssignable(MapLiteral literal) {
+    // Determine the map's key and value types. We base this on the static type
+    // and not the literal's type arguments because in strong mode, the type
+    // arguments may be inferred.
+    DartType mapType = literal.staticType;
+    assert(mapType is InterfaceTypeImpl);
+
+    List<DartType> typeArguments =
+        (mapType as InterfaceTypeImpl).typeArguments;
+    assert(typeArguments.length == 2);
+    DartType keyType = typeArguments[0];
+    DartType valueType = typeArguments[1];
+
     NodeList<MapLiteralEntry> entries = literal.entries;
     for (MapLiteralEntry entry in entries) {
       Expression key = entry.key;
       Expression value = entry.value;
       if (literal.constKeyword != null) {
         // TODO(paulberry): this error should be based on the actual type of the
         // list element, not the static type.  See dartbug.com/21119.
-        if (_checkForArgumentTypeNotAssignableWithExpectedTypes(key, keyType,
-            CheckedModeCompileTimeErrorCode.MAP_KEY_TYPE_NOT_ASSIGNABLE)) {
-          hasProblems = true;
-        }
-        if (_checkForArgumentTypeNotAssignableWithExpectedTypes(
+        _checkForArgumentTypeNotAssignableWithExpectedTypes(key, keyType,
+            CheckedModeCompileTimeErrorCode.MAP_KEY_TYPE_NOT_ASSIGNABLE);
+        _checkForArgumentTypeNotAssignableWithExpectedTypes(
             value,
             valueType,
-            CheckedModeCompileTimeErrorCode.MAP_VALUE_TYPE_NOT_ASSIGNABLE)) {
-          hasProblems = true;
-        }
-      }
-      if (_checkForArgumentTypeNotAssignableWithExpectedTypes(
-          key, keyType, StaticWarningCode.MAP_KEY_TYPE_NOT_ASSIGNABLE)) {
-        hasProblems = true;
-      }
-      if (_checkForArgumentTypeNotAssignableWithExpectedTypes(
-          value, valueType, StaticWarningCode.MAP_VALUE_TYPE_NOT_ASSIGNABLE)) {
-        hasProblems = true;
+            CheckedModeCompileTimeErrorCode.MAP_VALUE_TYPE_NOT_ASSIGNABLE);
       }
+      _checkForArgumentTypeNotAssignableWithExpectedTypes(
+          key, keyType, StaticWarningCode.MAP_KEY_TYPE_NOT_ASSIGNABLE);
+      _checkForArgumentTypeNotAssignableWithExpectedTypes(
+          value, valueType, StaticWarningCode.MAP_VALUE_TYPE_NOT_ASSIGNABLE);
     }
-    return hasProblems;
   }
 
   /**
@@ -5694,25 +5686,23 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
         : _typeProvider.iterableType;
 
     // Use an explicit string instead of [loopType] to remove the "<E>".
-    String loopTypeName = node.awaitKeyword != null
-        ? "Stream"
-        : "Iterable";
+    String loopTypeName = node.awaitKeyword != null ? "Stream" : "Iterable";
 
     // The object being iterated has to implement Iterable<T> for some T that
     // is assignable to the variable's type.
     // TODO(rnystrom): Move this into mostSpecificTypeArgument()?
     iterableType = iterableType.resolveToBound(_typeProvider.objectType);
-    DartType bestIterableType = _typeSystem.mostSpecificTypeArgument(
-        iterableType, loopType);
+    DartType bestIterableType =
+        _typeSystem.mostSpecificTypeArgument(iterableType, loopType);
     if (bestIterableType == null) {
       _errorReporter.reportTypeErrorForNode(
           StaticTypeWarningCode.FOR_IN_OF_INVALID_TYPE,
-          node,
+          node.iterable,
           [iterableType, loopTypeName]);
     } else if (!_typeSystem.isAssignableTo(bestIterableType, variableType)) {
       _errorReporter.reportTypeErrorForNode(
           StaticTypeWarningCode.FOR_IN_OF_INVALID_ELEMENT_TYPE,
-          node,
+          node.iterable,
           [iterableType, loopTypeName, variableType]);
     }
   }

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

@@ -206,6 +206,18 @@ class StrongTypeSystemImpl extends TypeSystem {
       return false;
     }
 
+    // Don't allow a non-generic function where a generic one is expected. The
+    // former wouldn't know how to handle type arguments being passed to it.
+    // TODO(rnystrom): This same check also exists in FunctionTypeImpl.relate()
+    // but we don't always reliably go through that code path. This should be
+    // cleaned up to avoid the redundancy.
+    if (fromType is FunctionType &&
+        toType is FunctionType &&
+        fromType.typeFormals.isEmpty &&
+        toType.typeFormals.isNotEmpty) {
+      return false;
+    }
+
     // If the subtype relation goes the other way, allow the implicit downcast.
     // TODO(leafp): Emit warnings and hints for these in some way.
     // TODO(leafp): Consider adding a flag to disable these?  Or just rely on

pkg/analyzer/lib/src/task/strong/checker.dart

@@ -145,7 +145,7 @@ class CodeChecker extends RecursiveAstVisitor {
     if (expr is ParenthesizedExpression) {
       checkAssignment(expr.expression, type);
     } else {
-      _recordMessage(_checkAssignment(expr, type));
+      _checkDowncast(expr, type);
     }
   }
 
@@ -565,33 +565,23 @@ class CodeChecker extends RecursiveAstVisitor {
     node.visitChildren(this);
   }
 
-  StaticInfo _checkAssignment(Expression expr, DartType toT) {
-    final fromT = _getStaticType(expr);
-    final Coercion c = _coerceTo(fromT, toT);
-    if (c is Identity) return null;
-    if (c is CoercionError) return new StaticTypeError(rules, expr, toT);
-    if (c is Cast) return DownCast.create(rules, expr, c);
-    assert(false);
-    return null;
-  }
-
   void _checkCompoundAssignment(AssignmentExpression expr) {
     var op = expr.operator.type;
     assert(op.isAssignmentOperator && op != TokenType.EQ);
     var methodElement = expr.staticElement;
     if (methodElement == null) {
-      // Dynamic invocation
+      // Dynamic invocation.
       _recordDynamicInvoke(expr, expr.leftHandSide);
     } else {
-      // Sanity check the operator
+      // Sanity check the operator.
       assert(methodElement.isOperator);
       var functionType = methodElement.type;
       var paramTypes = functionType.normalParameterTypes;
       assert(paramTypes.length == 1);
       assert(functionType.namedParameterTypes.isEmpty);
       assert(functionType.optionalParameterTypes.isEmpty);
 
-      // Check the lhs type
+      // Check the LHS type.
       var staticInfo;
       var rhsType = _getStaticType(expr.rightHandSide);
       var lhsType = _getStaticType(expr.leftHandSide);
@@ -606,10 +596,9 @@ class CodeChecker extends RecursiveAstVisitor {
           // This is also slightly different from spec, but allows us to keep
           // compound operators in the int += num and num += dynamic cases.
           staticInfo = DownCast.create(
-              rules, expr.rightHandSide, Coercion.cast(rhsType, lhsType));
+              rules, expr.rightHandSide, new Cast(rhsType, lhsType));
           rhsType = lhsType;
         } else {
-          // Static type error
           staticInfo = new StaticTypeError(rules, expr, lhsType);
         }
         _recordMessage(staticInfo);
@@ -618,8 +607,7 @@ class CodeChecker extends RecursiveAstVisitor {
       // Check the rhs type
       if (staticInfo is! CoercionInfo) {
         var paramType = paramTypes.first;
-        staticInfo = _checkAssignment(expr.rightHandSide, paramType);
-        _recordMessage(staticInfo);
+        _checkDowncast(expr.rightHandSide, paramType);
       }
     }
   }
@@ -675,12 +663,18 @@ class CodeChecker extends RecursiveAstVisitor {
     }
   }
 
-  Coercion _coerceTo(DartType fromT, DartType toT) {
-    // We can use anything as void
-    if (toT.isVoid) return Coercion.identity(toT);
+  /// Records a [DownCast] of [expr] to [toT], if there is one.
+  ///
+  /// If [expr] does not require a downcast because it is not related to [toT]
+  /// or is already a subtype of it, does nothing.
+  void _checkDowncast(Expression expr, DartType toT) {
+    DartType fromT = _getStaticType(expr);
+
+    // We can use anything as void.
+    if (toT.isVoid) return;
 
-    // fromT <: toT, no coercion needed
-    if (rules.isSubtypeOf(fromT, toT)) return Coercion.identity(toT);
+    // fromT <: toT, no coercion needed.
+    if (rules.isSubtypeOf(fromT, toT)) return;
 
     // TODO(vsm): We can get rid of the second clause if we disallow
     // all sideways casts - see TODO below.
@@ -690,11 +684,14 @@ class CodeChecker extends RecursiveAstVisitor {
     // well for consistency.
     if ((fromT is FunctionType && rules.getCallMethodType(toT) != null) ||
         (toT is FunctionType && rules.getCallMethodType(fromT) != null)) {
-      return Coercion.error();
+      return;
     }
 
     // Downcast if toT <: fromT
-    if (rules.isSubtypeOf(toT, fromT)) return Coercion.cast(fromT, toT);
+    if (rules.isSubtypeOf(toT, fromT)) {
+      _recordMessage(DownCast.create(rules, expr, new Cast(fromT, toT)));
+      return;
+    }
 
     // TODO(vsm): Once we have generic methods, we should delete this
     // workaround.  These sideways casts are always ones we warn about
@@ -707,10 +704,8 @@ class CodeChecker extends RecursiveAstVisitor {
     // Iterable<T> for some concrete T (e.g. Object).  These are unrelated
     // in the restricted system, but List<dynamic> <: Iterable<T> in dart.
     if (fromT.isAssignableTo(toT)) {
-      return Coercion.cast(fromT, toT);
+      _recordMessage(DownCast.create(rules, expr, new Cast(fromT, toT)));
     }
-
-    return Coercion.error();
   }
 
   // Produce a coercion which coerces something of type fromT

pkg/analyzer/lib/src/task/strong/info.dart

@@ -34,6 +34,9 @@ class Cast extends Coercion {
   Cast(DartType fromType, DartType toType) : super(fromType, toType);
 }
 
+
+// TODO(rnystrom): Analyzer no longer produces or uses anything except Cast,
+// so this should be eliminated once DDC no longer uses it.
 // The abstract type of coercions mapping one type to another.
 // This class also exposes static builder functions which
 // check for errors and reduce redundant coercions to the identity.