Improve highlighting for strong mode error

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

Index: pkg/analyzer/lib/src/task/strong/checker.dart
diff --git a/pkg/analyzer/lib/src/task/strong/checker.dart b/pkg/analyzer/lib/src/task/strong/checker.dart
index 79da6a86d9c37d27a0e93d685adf97e9649d2b36..acffa08ca7cf4a39ee9e390b93ba03def36cd049 100644
--- a/pkg/analyzer/lib/src/task/strong/checker.dart
+++ b/pkg/analyzer/lib/src/task/strong/checker.dart
@@ -345,7 +345,8 @@ class CodeChecker extends RecursiveAstVisitor {
       // 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(
@@ -636,6 +637,55 @@ class CodeChecker extends RecursiveAstVisitor {
     }
   }
 
+  /// 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)) {
@@ -646,7 +696,7 @@ class CodeChecker extends RecursiveAstVisitor {
 
   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
@@ -687,55 +737,6 @@ class CodeChecker extends RecursiveAstVisitor {
     }
   }
 
-  /// 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
@@ -749,7 +750,8 @@ class CodeChecker extends RecursiveAstVisitor {
       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;
@@ -804,55 +806,6 @@ class CodeChecker extends RecursiveAstVisitor {
     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
@@ -955,6 +908,55 @@ class CodeChecker extends RecursiveAstVisitor {
     }
   }
 
+  /// 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
@@ -1058,7 +1060,6 @@ class _OverrideChecker {
   _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) {
@@ -1068,21 +1069,21 @@ class _OverrideChecker {
           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.