Improve highlighting for strong mode error

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

 // Copyright (c) 2015, 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.
 
 // TODO(jmesserly): this was ported from package:dev_compiler, and needs to be
 // refactored to fit into analyzer.
 library analyzer.src.task.strong.checker;
 
 import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/dart/ast/ast.dart';
@@ skipping 327 matching lines @@
           ? typeProvider.streamType
           : typeProvider.iterableType;
       var iterableType = _getStaticType(node.iterable);
       var elementType =
           rules.mostSpecificTypeArgument(iterableType, sequenceInterface);
 
       // If the sequence is not an Iterable (or Stream for await for) but is a
       // supertype of it, do an implicit downcast to Iterable<dynamic>. Then
       // we'll do a separate cast of the dynamic element to the variable's type.
       if (elementType == null) {
-        var sequenceType = sequenceInterface.instantiate([DynamicTypeImpl.instance]);
+        var sequenceType =
+            sequenceInterface.instantiate([DynamicTypeImpl.instance]);
 
         if (rules.isSubtypeOf(sequenceType, iterableType)) {
           _recordMessage(DownCast.create(
               rules, node.iterable, iterableType, sequenceType));
           elementType = DynamicTypeImpl.instance;
         }
       }
 
       // If the sequence doesn't implement the interface at all, [ErrorVerifier]
       // will report the error, so ignore it here.
@@ skipping 270 matching lines @@
       }
 
       // Check the rhs type
       if (staticInfo is! CoercionInfo) {
         var paramType = paramTypes.first;
         _checkDowncast(expr.rightHandSide, paramType);
       }
     }
   }
 
+  /// Records a [DownCast] of [expr] from [from] to [to], if there is one.

[Bob Nystrom, 2016/03/28 19:59:40]

Are there changes to this, or did it just move? Is it possible to separate
patches that reorder members from ones that change contents? It's hard to find
the actual diffs in this.

[Brian Wilkerson, 2016/03/28 20:34:33]

No, there are no changes. I was only expecting changes to
_checkIndividualOverridesFromClass. The two strong-mode changes were
unintentional.
I sympathize. The only way I know of to separate such patches is to figure out
which files changed, create a new branch, sort and format the files on the new
branch, then rebase the original changes onto the new branch. Maybe we should
have a test that formats and sorts every file and fails if the before and after
are not the same; that would prevent the skew that causes this kind of noise.
(It somewhat ironic, given that one of the reasons often cited for sorting and
formatting is to prevent this kind of noise.)

+  ///
+  /// If [from] is omitted, uses the static type of [expr].
+  ///
+  /// If [expr] does not require a downcast because it is not related to [to]
+  /// or is already a subtype of it, does nothing.
+  void _checkDowncast(Expression expr, DartType to, {DartType from}) {
+    if (from == null) {
+      from = _getStaticType(expr);
+    }
+
+    // We can use anything as void.
+    if (to.isVoid) return;
+
+    // fromT <: toT, no coercion needed.
+    if (rules.isSubtypeOf(from, to)) return;
+
+    // TODO(vsm): We can get rid of the second clause if we disallow
+    // all sideways casts - see TODO below.
+    // -------
+    // Note: a function type is never assignable to a class per the Dart
+    // spec - even if it has a compatible call method.  We disallow as
+    // well for consistency.
+    if ((from is FunctionType && rules.getCallMethodType(to) != null) ||
+        (to is FunctionType && rules.getCallMethodType(from) != null)) {
+      return;
+    }
+
+    // Downcast if toT <: fromT
+    if (rules.isSubtypeOf(to, from)) {
+      _recordMessage(DownCast.create(rules, expr, from, to));
+      return;
+    }
+
+    // TODO(vsm): Once we have generic methods, we should delete this
+    // workaround.  These sideways casts are always ones we warn about
+    // - i.e., we think they are likely to fail at runtime.
+    // -------
+    // Downcast if toT <===> fromT
+    // The intention here is to allow casts that are sideways in the restricted
+    // type system, but allowed in the regular dart type system, since these
+    // are likely to succeed.  The canonical example is List<dynamic> and
+    // Iterable<T> for some concrete T (e.g. Object).  These are unrelated
+    // in the restricted system, but List<dynamic> <: Iterable<T> in dart.
+    if (from.isAssignableTo(to)) {
+      _recordMessage(DownCast.create(rules, expr, from, to));
+    }
+  }
+
   void _checkFieldAccess(AstNode node, AstNode target, SimpleIdentifier field) {
     if ((_isDynamicTarget(target) || field.staticElement == null) &&
         !_isObjectProperty(target, field)) {
       _recordDynamicInvoke(node, target);
     }
     node.visitChildren(this);
   }
 
   void _checkReturnOrYield(Expression expression, AstNode node,
       {bool yieldStar: false}) {
-    var body = node.getAncestor((n) => n is FunctionBody);
+    FunctionBody body = node.getAncestor((n) => n is FunctionBody);
     var type = _getExpectedReturnType(body, yieldStar: yieldStar);
     if (type == null) {
       // We have a type mismatch: the async/async*/sync* modifier does
       // not match the return or yield type.  We should have already gotten an
       // analyzer error in this case.
       return;
     }
     InterfaceType futureType = typeProvider.futureType;
     DartType actualType = expression?.staticType;
     if (body.isAsynchronous &&
@@ skipping 20 matching lines @@
         op.type == TokenType.MINUS_MINUS) {
       if (_isDynamicTarget(node.operand)) {
         _recordDynamicInvoke(node, node.operand);
       }
       // For ++ and --, even if it is not dynamic, we still need to check
       // that the user defined method accepts an `int` as the RHS.
       // We assume Analyzer has done this already.
     }
   }
 
-  /// Records a [DownCast] of [expr] from [from] to [to], if there is one.
-  ///
-  /// If [from] is omitted, uses the static type of [expr].
-  ///
-  /// If [expr] does not require a downcast because it is not related to [to]
-  /// or is already a subtype of it, does nothing.
-  void _checkDowncast(Expression expr, DartType to, {DartType from}) {
-    if (from == null) {
-      from = _getStaticType(expr);
-    }
-
-    // We can use anything as void.
-    if (to.isVoid) return;
-
-    // fromT <: toT, no coercion needed.
-    if (rules.isSubtypeOf(from, to)) return;
-
-    // TODO(vsm): We can get rid of the second clause if we disallow
-    // all sideways casts - see TODO below.
-    // -------
-    // Note: a function type is never assignable to a class per the Dart
-    // spec - even if it has a compatible call method.  We disallow as
-    // well for consistency.
-    if ((from is FunctionType && rules.getCallMethodType(to) != null) ||
-        (to is FunctionType && rules.getCallMethodType(from) != null)) {
-      return;
-    }
-
-    // Downcast if toT <: fromT
-    if (rules.isSubtypeOf(to, from)) {
-      _recordMessage(DownCast.create(rules, expr, from, to));
-      return;
-    }
-
-    // TODO(vsm): Once we have generic methods, we should delete this
-    // workaround.  These sideways casts are always ones we warn about
-    // - i.e., we think they are likely to fail at runtime.
-    // -------
-    // Downcast if toT <===> fromT
-    // The intention here is to allow casts that are sideways in the restricted
-    // type system, but allowed in the regular dart type system, since these
-    // are likely to succeed.  The canonical example is List<dynamic> and
-    // Iterable<T> for some concrete T (e.g. Object).  These are unrelated
-    // in the restricted system, but List<dynamic> <: Iterable<T> in dart.
-    if (from.isAssignableTo(to)) {
-      _recordMessage(DownCast.create(rules, expr, from, to));
-    }
-  }
-
   // Produce a coercion which coerces something of type fromT
   // to something of type toT.
   // Returns the error coercion if the types cannot be coerced
   // according to our current criteria.
   /// Gets the expected return type of the given function [body], either from
   /// a normal return/yield, or from a yield*.
   DartType _getExpectedReturnType(FunctionBody body, {bool yieldStar: false}) {
     FunctionType functionType;
     var parent = body.parent;
     if (parent is Declaration) {
       functionType = _elementType(parent.element);
     } else {
       assert(parent is FunctionExpression);
-      functionType = parent.staticType ?? DynamicTypeImpl.instance;
+      functionType =
+          (parent as FunctionExpression).staticType ?? DynamicTypeImpl.instance;
     }
 
     var type = functionType.returnType;
 
     InterfaceType expectedType = null;
     if (body.isAsynchronous) {
       if (body.isGenerator) {
         // Stream<T> -> T
         expectedType = typeProvider.streamType;
       } else {
@@ skipping 34 matching lines @@
     DartType t = expr.staticType ?? DynamicTypeImpl.instance;
 
     // Remove fuzzy arrow if possible.
     if (t is FunctionType && StaticInfo.isKnownFunction(expr)) {
       t = _removeFuzz(t);
     }
 
     return t;
   }
 
-  /// Remove "fuzzy arrow" in this function type.
-  ///
-  /// Normally we treat dynamically typed parameters as bottom for function
-  /// types. This allows type tests such as `if (f is SingleArgFunction)`.
-  /// It also requires a dynamic check on the parameter type to call these
-  /// functions.
-  ///
-  /// When we convert to a strict arrow, dynamically typed parameters become
-  /// top. This is safe to do for known functions, like top-level or local
-  /// functions and static methods. Those functions must already be essentially
-  /// treating dynamic as top.
-  ///
-  /// Only the outer-most arrow can be strict. Any others must be fuzzy, because
-  /// we don't know what function value will be passed there.
-  // TODO(jmesserly): should we use a real "fuzzyArrow" bit on the function
-  // type? That would allow us to implement this in the subtype relation.
-  // TODO(jmesserly): we'll need to factor this differently if we want to
-  // move CodeChecker's functionality into existing analyzer. Likely we can
-  // let the Expression have a strict arrow, then in places were we do
-  // inference, convert back to a fuzzy arrow.
-  FunctionType _removeFuzz(FunctionType t) {
-    bool foundFuzz = false;
-    List<ParameterElement> parameters = <ParameterElement>[];
-    for (ParameterElement p in t.parameters) {
-      ParameterElement newP = _removeParameterFuzz(p);
-      parameters.add(newP);
-      if (p != newP) foundFuzz = true;
-    }
-    if (!foundFuzz) {
-      return t;
-    }
-
-    FunctionElementImpl function = new FunctionElementImpl("", -1);
-    function.synthetic = true;
-    function.returnType = t.returnType;
-    function.shareTypeParameters(t.typeFormals);
-    function.shareParameters(parameters);
-    return function.type = new FunctionTypeImpl(function);
-  }
-
-  /// Removes fuzzy arrow, see [_removeFuzz].
-  ParameterElement _removeParameterFuzz(ParameterElement p) {
-    if (p.type.isDynamic) {
-      return new ParameterElementImpl.synthetic(
-          p.name, typeProvider.objectType, p.parameterKind);
-    }
-    return p;
-  }
-
   /// Given an expression, return its type assuming it is
   /// in the caller position of a call (that is, accounting
   /// for the possibility of a call method).  Returns null
   /// if expression is not statically callable.
   FunctionType _getTypeAsCaller(Expression node) {
     DartType t = node.staticType;
     if (node is SimpleIdentifier) {
       Expression parent = node.parent;
       if (parent is MethodInvocation) {
         t = parent.staticInvokeType;
@@ skipping 82 matching lines @@
     if (info is CoercionInfo) {
       // TODO(jmesserly): if we're run again on the same AST, we'll produce the
       // same annotations. This should be harmless. This might go away once
       // CodeChecker is integrated better with analyzer, as it will know that
       // checking has already been performed.
       // assert(CoercionInfo.get(info.node) == null);
       CoercionInfo.set(info.node, info);
     }
   }
 
+  /// Remove "fuzzy arrow" in this function type.
+  ///
+  /// Normally we treat dynamically typed parameters as bottom for function
+  /// types. This allows type tests such as `if (f is SingleArgFunction)`.
+  /// It also requires a dynamic check on the parameter type to call these
+  /// functions.
+  ///
+  /// When we convert to a strict arrow, dynamically typed parameters become
+  /// top. This is safe to do for known functions, like top-level or local
+  /// functions and static methods. Those functions must already be essentially
+  /// treating dynamic as top.
+  ///
+  /// Only the outer-most arrow can be strict. Any others must be fuzzy, because
+  /// we don't know what function value will be passed there.
+  // TODO(jmesserly): should we use a real "fuzzyArrow" bit on the function
+  // type? That would allow us to implement this in the subtype relation.
+  // TODO(jmesserly): we'll need to factor this differently if we want to
+  // move CodeChecker's functionality into existing analyzer. Likely we can
+  // let the Expression have a strict arrow, then in places were we do
+  // inference, convert back to a fuzzy arrow.
+  FunctionType _removeFuzz(FunctionType t) {
+    bool foundFuzz = false;
+    List<ParameterElement> parameters = <ParameterElement>[];
+    for (ParameterElement p in t.parameters) {
+      ParameterElement newP = _removeParameterFuzz(p);
+      parameters.add(newP);
+      if (p != newP) foundFuzz = true;
+    }
+    if (!foundFuzz) {
+      return t;
+    }
+
+    FunctionElementImpl function = new FunctionElementImpl("", -1);
+    function.synthetic = true;
+    function.returnType = t.returnType;
+    function.shareTypeParameters(t.typeFormals);
+    function.shareParameters(parameters);
+    return function.type = new FunctionTypeImpl(function);
+  }
+
+  /// Removes fuzzy arrow, see [_removeFuzz].
+  ParameterElement _removeParameterFuzz(ParameterElement p) {
+    if (p.type.isDynamic) {
+      return new ParameterElementImpl.synthetic(
+          p.name, typeProvider.objectType, p.parameterKind);
+    }
+    return p;
+  }
+
   DartType _specializedBinaryReturnType(
       TokenType op, DartType t1, DartType t2, DartType normalReturnType) {
     // This special cases binary return types as per 16.26 and 16.27 of the
     // Dart language spec.
     switch (op) {
       case TokenType.PLUS:
       case TokenType.MINUS:
       case TokenType.STAR:
       case TokenType.TILDE_SLASH:
       case TokenType.PERCENT:
@@ skipping 83 matching lines @@
   }
 
   /// Check that individual methods and fields in [subType] correctly override
   /// the declarations in [baseType].
   ///
   /// The [errorLocation] node indicates where errors are reported, see
   /// [_checkSingleOverride] for more details.
   _checkIndividualOverridesFromClass(ClassDeclaration node,
       InterfaceType baseType, Set<String> seen, bool isSubclass) {
     for (var member in node.members) {
-      if (member is ConstructorDeclaration) continue;
       if (member is FieldDeclaration) {
         if (member.isStatic) continue;
         for (var variable in member.fields.variables) {
           var element = variable.element as PropertyInducingElement;
           var name = element.name;
           if (seen.contains(name)) continue;
           var getter = element.getter;
           var setter = element.setter;
           bool found = _checkSingleOverride(
-              getter, baseType, variable, member, isSubclass);
+              getter, baseType, variable.name, member, isSubclass);
           if (!variable.isFinal &&
               !variable.isConst &&
               _checkSingleOverride(
-                  setter, baseType, variable, member, isSubclass)) {
+                  setter, baseType, variable.name, member, isSubclass)) {
             found = true;
           }
           if (found) seen.add(name);
         }
-      } else {
-        if ((member as MethodDeclaration).isStatic) continue;
-        var method = (member as MethodDeclaration).element;
+      } else if (member is MethodDeclaration) {
+        if (member.isStatic) continue;
+        var method = member.element;
         if (seen.contains(method.name)) continue;
         if (_checkSingleOverride(
-            method, baseType, member, member, isSubclass)) {
+            method, baseType, member.name, member, isSubclass)) {
           seen.add(method.name);
         }
       }

[Bob Nystrom, 2016/03/28 19:59:40]

Maybe leave a comment or assert that the only remaining type is
ConstructorDeclaration and we don't need to do anything with it?

[Brian Wilkerson, 2016/03/28 20:34:33]

I'll do that.

     }
   }
 
   /// Check that individual methods and fields in [subType] correctly override
   /// the declarations in [baseType].
   ///
   /// The [errorLocation] node indicates where errors are reported, see
   /// [_checkSingleOverride] for more details.
   ///
   /// The set [seen] is used to avoid reporting overrides more than once. It
@@ skipping 200 matching lines @@
     } while (!current.isObject && !visited.contains(current));
   }
 
   void _recordMessage(StaticInfo info) {
     if (info == null) return;
     var error = info.toAnalysisError();
     if (error.errorCode.errorSeverity == ErrorSeverity.ERROR) _failure = true;
     _reporter.onError(error);
   }
 }