Infer parameter types on overrides

lib/src/checker/resolver.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.
 
 /// Encapsulates how to invoke the analyzer resolver and overrides how it
 /// computes types on expressions to use our restricted set of types.
 library dev_compiler.src.checker.resolver;
 
 import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/src/generated/ast.dart';
@@ skipping 150 matching lines @@
       }
 
       if (_options.inferFromOverrides) {
         // Infer field types from overrides first, otherwise from initializers.
         var pending = new Set<VariableDeclaration>();
         cls.members
             .where(_isInstanceField)
             .forEach((f) => _inferFieldTypeFromOverride(f, pending));
         if (pending.isNotEmpty) _inferVariableFromInitializer(pending);
 
-        // Infer return-types from overrides
+        // Infer return-types and param-types from overrides
         cls.members
             .where((m) => m is MethodDeclaration && !m.isStatic)
-            .forEach(_inferMethodReturnTypeFromOverride);
+            .forEach(_inferMethodTypesFromOverride);
       } else {
         _inferVariableFromInitializer(cls.members
             .where(_isInstanceField)
             .expand((f) => f.fields.variables));
       }
     }
     classes.forEach(visit);
   }
 
   void _reanalyzeVar(Map<Source, RestrictedResolverVisitor> visitors,
@@ skipping 39 matching lines @@
       }
 
       // Use type from the override.
       var newType = type.returnType;
       varElement.type = newType;
       varElement.getter.returnType = newType;
       if (!varElement.isFinal) varElement.setter.parameters[0].type = newType;
     }
   }
 
-  void _inferMethodReturnTypeFromOverride(MethodDeclaration method) {
+  void _inferMethodTypesFromOverride(MethodDeclaration method) {
     var methodElement = method.element;
-    if ((methodElement is MethodElement ||
-            methodElement is PropertyAccessorElement) &&
-        methodElement.returnType.isDynamic &&
-        method.returnType == null) {
-      var enclosingElement = methodElement.enclosingElement as ClassElement;
-      var type = searchTypeFor(enclosingElement.type, methodElement);
+    if (methodElement is! MethodElement &&
+        methodElement is! PropertyAccessorElement) return;
+
+    var enclosingElement = methodElement.enclosingElement as ClassElement;
+    FunctionType type =
+        null; // searchTypeFor(enclosingElement.type, methodElement);
+
+    // Infer the return type if omitted
+    if (methodElement.returnType.isDynamic && method.returnType == null) {
+      type = searchTypeFor(enclosingElement.type, methodElement);
       if (type != null && !type.returnType.isDynamic) {
         methodElement.returnType = type.returnType;
       }
     }
+
+    // Infer parameter types if omitted

[Brian Wilkerson, 2015/08/25 14:06:29]

In the ported version of the code we get all of the overridden methods, not just
one, and only infer a return type if they are equal. I'm guessing that for
parameter types we would want to pick the least upper bound of the corresponding
(non-dynamic) parameter types, possibly discarding Object.

[vsm, 2015/08/25 15:43:47]

Perhaps we should do equality for params as well then?

[Brian Wilkerson, 2015/08/25 15:54:51]

For the return type we'd need a greatest lower bound computation, which we don't
have. I decided that it wasn't worth writing it (mostly because I think most of
the time it wouldn't come up with anything useful).

For the parameter types we'd need a least upper bound, and I don't like the
implementation we have, so yes, we might want to leave it as equals until we can
get a good implementation of LUB.

[Leaf, 2015/08/25 17:03:50]

I think equality is fine for now.  For return types, we could eventually at
least handle the case that one is a subtype of the others.

+    if (method.parameters == null) return;
+    var parameters = method.parameters.parameters;
+    var length = parameters.length;
+    for (int i = 0; i < length; ++i) {
+      var parameter = parameters[i];
+      if (parameter is SimpleFormalParameter && parameter.type == null) {
+        type = type ?? searchTypeFor(enclosingElement.type, methodElement);

[Brian Wilkerson, 2015/08/25 14:06:29]

I can't say I'm fond of the lazy initialization technique of using '??', but
that aside, it might be better to hoist this line outside the for loop,
especially if searchTypeFor can ever return null, even though we might
needlessly perform the search one time.

[vsm, 2015/08/25 15:43:47]

Yes, it looks like a fairly expensive operation though & I was trying to
preserve that property of the original code - i.e., don't search unless there is
an untyped variable to infer.

Good catch on the null return.  Added a short-circuiting check for that.

+        if (type != null &&
+            type.parameters.length > i &&
+            !type.parameters[i].type.isDynamic) {
+          parameter.element.type = type.parameters[i].type;
+        }
+      }
+    }
   }
 
   void _inferVariableFromInitializer(Iterable<VariableDeclaration> variables) {
     for (var variable in variables) {
       var declaration = variable.parent as VariableDeclarationList;
       // Only infer on variables that don't have any declared type.
       if (declaration.type != null) continue;
       if (_options.onlyInferConstsAndFinalFields &&
           !declaration.isFinal &&
           !declaration.isConst) {
@@ skipping 460 matching lines @@
     }
   }
 
   // Review note: no longer need to override visitFunctionExpression, this is
   // handled by the analyzer internally.
   // TODO(vsm): in visitbinaryExpression: check computeStaticReturnType result?
   // TODO(vsm): in visitFunctionDeclaration: Should we ever use the expression
   // type in a (...) => expr or just the written type?
 
 }