Don't call dinvoke on Object methods

lib/src/checker/rules.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.
 
 library dev_compiler.src.checker.rules;
 
 import 'package:analyzer/src/generated/ast.dart';
 import 'package:analyzer/src/generated/element.dart';
 import 'package:analyzer/src/generated/resolver.dart';
 
@@ skipping 25 matching lines @@
   bool maybeNonNullableType(DartType t) => false;
 
   StaticInfo checkAssignment(Expression expr, DartType t, bool constContext);
 
   DartType getStaticType(Expression expr) => expr.staticType;
 
   DartType elementType(Element e);
 
   bool isDynamic(DartType t);
   bool isDynamicTarget(Expression expr);
-  bool isDynamicGet(Expression expr);
+  bool isDynamicGet(Expression expr, String name);
   bool isDynamicCall(Expression call);
 }
 
 class DartRules extends TypeRules {
   DartRules(TypeProvider provider) : super(provider);
 
   bool isSubTypeOf(DartType t1, DartType t2) {
     return t1.isSubtypeOf(t2);
   }
 
@@ skipping 10 matching lines @@
     return null;
   }
 
   DartType elementType(Element e) {
     return (e as dynamic).type;
   }
 
   /// By default, all invocations are dynamic in Dart.
   bool isDynamic(DartType t) => true;
   bool isDynamicTarget(Expression expr) => true;
-  bool isDynamicGet(Expression expr) => true;
+  bool isDynamicGet(Expression expr, String name) => true;
   bool isDynamicCall(Expression call) => true;
 }
 
 class RestrictedRules extends TypeRules {
   final CheckerReporter _reporter;
   final RulesOptions options;
   final List<DartType> _nonnullableTypes;
   DownwardsInference inferrer;
 
   DartType _typeFromName(String name) {
@@ skipping 422 matching lines @@
   }
 
   bool isDynamic(DartType t) => options.ignoreTypes || t.isDynamic;
 
   /// Returns `true` if the target expression is dynamic.
   bool isDynamicTarget(Expression expr) =>
       options.ignoreTypes || getStaticType(expr).isDynamic;
 
   /// Returns `true` if the expression is a dynamic property access or prefixed
   /// identifier.
-  bool isDynamicGet(Expression expr) {
+  bool isDynamicGet(Expression expr, String name) {
+    // Check if this is a library-level (top-level access)
     if (options.ignoreTypes) return true;
     var t = getStaticType(expr);
     // TODO(jmesserly): we should not allow all property gets on `Function`
-    return t.isDynamic || t.isDartCoreFunction;
+    return (t.isDynamic || t.isDartCoreFunction) &&
+        name != 'hashCode' &&

[Jacob, 2015/04/02 18:29:49]

Add TODO: get all object properties from the AST.

+        name != 'runtimeType';
   }
 
   /// Returns `true` if the expression is a dynamic function call or method
   /// invocation.
   bool isDynamicCall(Expression call) {
     if (options.ignoreTypes) return true;
     var t = getStaticType(call);
+
     // TODO(jmesserly): fix handling of types with `call` methods. These are not
     // FunctionType, but they also aren't dynamic calls.
+
     if (t.isDynamic || t.isDartCoreFunction || t is! FunctionType) {
+      // Special case Object.toString().  This is always safe to invoke.

[Jacob, 2015/04/02 18:29:49]

add a TODO to lookup all methods on Object instead of hardcoding.

[Jennifer Messerly, 2015/04/02 18:56:53]

Agreed. It's not too hard to look up the methodelement:
   provider.objectType.element.getMethod('toString')

I guess the analyzer has not resolved `call.element` to point at it? A quick
test with:
```dart
    Object foo;
    foo.toString();

    var bar;
    bar.toString();
```

"go to definition" for the former will jump to Object.toString(), for the latter
it says "no definition to go to"

I wonder if we should implement this at resolution time. It feels a bit off to
add a special cases like this to isDynamicCall. (also, instead of isDynamicCall
we should probably just check if resolution failed to find an element. That
means dynamic. And it's simpler.)

[vsm, 2015/04/02 19:21:56]

Good point.  I'll add the lookup.
 

+      if (call is SimpleIdentifier && call.name == 'toString') {

[Jennifer Messerly, 2015/04/02 18:56:53]

also, what about noSuchMethod? :)

[vsm, 2015/04/02 19:21:56]

We need the dinvoke to verify the type of the argument.  Actually, something
lightweight would do - but we  need something.

+        var parent = call.parent;
+        // Verify this is a method invocation with no arguments.
+        if (parent is MethodInvocation &&

[Jennifer Messerly, 2015/04/02 18:56:53]

getting the parent here feels a bit odd. we have to do it in the codegen
sometimes (no way to pass that down) but here we could just pass in the
MethodInvocation, or move this logic into visitMethodInvocation.

also: would this work for tear offs?

var f = obj.toString;
f();

[vsm, 2015/04/02 19:21:56]

Interesting thought.

If we infer that f is ()->*, we won't need the dinvoke.  If we only infer that f
is dynamic or Function, we need it. 

Perhaps we should be handling this earlier.

+            parent.methodName == call &&
+            parent.argumentList.arguments.isEmpty) {
+          // Verify that the target is an object (and not a library prefix).

[Jennifer Messerly, 2015/04/02 18:56:53]

this can't happen right? calls to library prefixes are never dynamic... they're
statically resolved top level methods.

[vsm, 2015/04/02 19:21:56]

It can ... see toString in my test.

+          var target = parent.realTarget;
+          if (target != null &&
+              (target is! SimpleIdentifier ||
+                  target.staticElement is! PrefixElement)) {
+            return false;
+          }
+        }
+      }
       return true;
     }
     // Dynamic as the parameter type is treated as bottom.  A function with
     // a dynamic parameter type requires a dynamic call in general.
     // However, as an optimization, if we have an original definition, we know
     // dynamic is reified as Object - in this case a regular call is fine.
     if (call is SimpleIdentifier) {
       var element = call.staticElement;
       if (element is FunctionElement || element is MethodElement) {
         // An original declaration.
@@ skipping 214 matching lines @@
     var entries = e.entries;
     bool inferEntry(MapLiteralEntry entry) {
       return inferExpression(entry.key, kType) &&
           inferExpression(entry.value, vType);
     }
     var b = entries.every(inferEntry);
     if (b) annotateMapLiteral(e, targs);
     return b;
   }
 }