fix #552, Object members on native types

lib/src/compiler/code_generator.dart

Index: lib/src/compiler/code_generator.dart
diff --git a/lib/src/compiler/code_generator.dart b/lib/src/compiler/code_generator.dart
index f758d14bbcabe38bcf874d80a1988036442dae46..210c5f613dd8c7b31dd2200ecbc448a80aedd93b 100644
--- a/lib/src/compiler/code_generator.dart
+++ b/lib/src/compiler/code_generator.dart
@@ -224,12 +224,18 @@ class CodeGenerator extends GeneralizingAstVisitor
     // Collect all Element -> Node mappings, in case we need to forward declare
     // any nodes.
     var nodes = new HashMap<Element, AstNode>.identity();
-    compilationUnits.map(_collectElements).forEach(nodes.addAll);
+    var sdkBootstrappingFns = new List<FunctionElement>();
+    for (var unit in compilationUnits) {
+      _collectElements(unit, nodes, sdkBootstrappingFns);
+    }
     _loader = new ElementLoader(_emitModuleItem, nodes);
 
     // Add implicit dart:core dependency so it is first.
     emitLibraryName(dartCoreLibrary);
 
+    // Emit SDK bootstrapping functions first, if any.
+    sdkBootstrappingFns.forEach(_loader.emitDeclaration);
+
     // Visit each compilation unit and emit its code.
     //
     // NOTE: declarations are not necessarily emitted in this order.
@@ -319,8 +325,13 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   /// Collect toplevel elements and nodes we need to emit, and returns
   /// an ordered map of these.
-  static Map<Element, AstNode> _collectElements(CompilationUnit unit) {
-    var map = <Element, AstNode>{};
+  static void _collectElements(CompilationUnit unit, Map<Element, AstNode> map,
+      List<FunctionElement> runtimeHelpers) {
+    var library = unit.element.library;
+    if (_isDartRuntime(library)) {
+      runtimeHelpers.addAll(unit.element.functions);
+    }
+
     for (var declaration in unit.declarations) {
       if (declaration is TopLevelVariableDeclaration) {
         for (var field in declaration.variables.variables) {
@@ -330,7 +341,6 @@ class CodeGenerator extends GeneralizingAstVisitor
         map[declaration.element] = declaration;
       }
     }
-    return map;
   }
 
   void _emitModuleItem(AstNode node) {
@@ -961,7 +971,7 @@ class CodeGenerator extends GeneralizingAstVisitor
 
     // Generate a corresponding virtual getter / setter.
     var name = _elementMemberName(methodElement,
-        allowExtensions: _extensionTypes.isNativeClass(type.element));
+        useExtension: _extensionTypes.isNativeClass(type.element));
     if (method.isGetter) {
       // Generate a setter
       if (field.setter != null || !propertyOverrideResult.foundSetter)
@@ -1115,7 +1125,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     if (extensions.isNotEmpty) {
       var methodNames = <JS.Expression>[];
       for (var e in extensions) {
-        methodNames.add(_elementMemberName(e, allowExtensions: false));
+        methodNames.add(_elementMemberName(e, useExtension: false));
       }
       body.add(js.statement('dart.defineExtensionMembers(#, #);', [
         className,
@@ -1153,7 +1163,7 @@ class CodeGenerator extends GeneralizingAstVisitor
           continue;
         }
         var memberName = _elementMemberName(element,
-            allowExtensions: _extensionTypes.isNativeClass(classElem));
+            useExtension: _extensionTypes.isNativeClass(classElem));
         var parts = _emitFunctionTypeParts(element.type);
         var property =
             new JS.Property(memberName, new JS.ArrayInitializer(parts));
@@ -1209,7 +1219,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       var dartxNames = <JS.Expression>[];
       for (var m in methods) {
         if (!m.isAbstract && !m.isStatic && m.element.isPublic) {
-          dartxNames.add(_elementMemberName(m.element, allowExtensions: false));
+          dartxNames.add(_elementMemberName(m.element, useExtension: false));
         }
       }
       for (var f in fields) {
@@ -1217,7 +1227,7 @@ class CodeGenerator extends GeneralizingAstVisitor
           for (var d in f.fields.variables) {
             if (d.element.isPublic) {
               dartxNames.add(
-                  _elementMemberName(d.element.getter, allowExtensions: false));
+                  _elementMemberName(d.element.getter, useExtension: false));
             }
           }
         }
@@ -1242,7 +1252,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     var extensionMembers = new HashSet<String>();
     for (var t in types) {
       for (var m in [t.methods, t.accessors].expand((e) => e)) {
-        if (!m.isStatic) extensionMembers.add(m.name);
+        if (!m.isStatic && m.isPublic) extensionMembers.add(m.name);

[vsm, 2016/05/09 20:55:56]

Do we still need Bob's fix here:

https://codereview.chromium.org/1944293003/diff/1/tool/input_sdk/private/ddc_...

[Jennifer Messerly, 2016/05/10 00:33:57]

Well, Bob's test is still passing so that's a good sign :)

But I don't think they're related. Bob's fix was runtime helpers that need to
operate on private members like `dsend(this, _somePrivateField, ...)`.

This code is building up the list of members we want to implement:

class MyIter implements Iterable {
  get iterator => ...;
}
// DDC generates code here to also implement [dartx.iterator]


I think it follows from Dart's SDK library structure that it is impossible for a
user-defined type to implement a private member on an SDK interface (because
they are not part of that dart:* library)

(In general, private symbols on interfaces are a bit of an anti-pattern for that
very reason. No one can implement them outside the library.)

       }
     }
 
@@ -1676,7 +1686,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     return annotate(
         new JS.Method(
             _elementMemberName(node.element,
-                allowExtensions: _extensionTypes.isNativeClass(type.element)),
+                useExtension: _extensionTypes.isNativeClass(type.element)),
             fn,
             isGetter: node.isGetter,
             isSetter: node.isSetter,
@@ -2479,9 +2489,8 @@ class CodeGenerator extends GeneralizingAstVisitor
       }
     }
     if (_isObjectMemberCall(target, name)) {
-      // Object methods require a helper for null checks & native types.
       assert(typeArgs == null); // Object methods don't take type args.
-      return js.call('dart.#(#, #)', [memberName, jsTarget, args]);
+      return js.call('dart.#(#, #)', [name, jsTarget, args]);
     }
 
     jsTarget = new JS.PropertyAccess(jsTarget, memberName);
@@ -2834,11 +2843,6 @@ class CodeGenerator extends GeneralizingAstVisitor
     return new JS.VariableInitialization(name, _visitInitializer(node));
   }
 
-  bool _isFinalJSDecl(AstNode field) =>
-      field is VariableDeclaration &&
-      field.isFinal &&
-      _isJSInvocation(field.initializer);
-
   /// Try to emit a constant static field.
   ///
   /// If the field's initializer does not cause side effects, and if all of
@@ -2904,10 +2908,9 @@ class CodeGenerator extends GeneralizingAstVisitor
       eagerInit = false;
     }
 
-    // Treat `final x = JS('', '...')` as a const (non-lazy) to help compile
-    // runtime helpers.
-    // TODO(jmesserly): we don't track dependencies correctly for these.
-    var isJSTopLevel = field.isFinal && _isFinalJSDecl(field);
+    // Treat dart:runtime stuff as safe to eagerly evaluate.
+    // TODO(jmesserly): it'd be nice to avoid this special case.
+    var isJSTopLevel = field.isFinal && _isDartRuntime(element.library);
     if (eagerInit || isJSTopLevel) {
       // Remember that we emitted it this way, so re-export can take advantage
       // of this fact.
@@ -3600,8 +3603,7 @@ class CodeGenerator extends GeneralizingAstVisitor
   /// For example `null.toString()` is legal in Dart, so we need to generate
   /// that as `dart.toString(obj)`.
   bool _isObjectMemberCall(Expression target, String memberName) {
-    // Is this a member on `Object`?
-    if (!isObjectProperty(memberName)) {
+    if (!isObjectMember(memberName)) {
       return false;
     }
 
@@ -3679,12 +3681,13 @@ class CodeGenerator extends GeneralizingAstVisitor
       if (isSuper) {
         result = js.call('dart.bind(this, #, #.#)', [name, jsTarget, name]);
       } else if (_isObjectMemberCall(target, memberName)) {
-        result = js.call('dart.bind(#, #, dart.#)', [jsTarget, name, name]);
+        result = js.call('dart.bind(#, #, dart.#)',
+            [jsTarget, _propertyName(memberName), memberName]);
       } else {
         result = js.call('dart.bind(#, #)', [jsTarget, name]);
       }
     } else if (_isObjectMemberCall(target, memberName)) {
-      result = js.call('dart.#(#)', [name, jsTarget]);
+      result = js.call('dart.#(#)', [memberName, jsTarget]);
     } else {
       result = js.call('#.#', [jsTarget, name]);
     }
@@ -4152,8 +4155,7 @@ class CodeGenerator extends GeneralizingAstVisitor
   ///
   /// Unlike call sites, we always have an element available, so we can use it
   /// directly rather than computing the relevant options for [_emitMemberName].
-  JS.Expression _elementMemberName(ExecutableElement e,
-      {bool allowExtensions: true}) {
+  JS.Expression _elementMemberName(ExecutableElement e, {bool useExtension}) {
     String name;
     if (e is PropertyAccessorElement) {
       name = e.variable.name;
@@ -4164,7 +4166,7 @@ class CodeGenerator extends GeneralizingAstVisitor
         type: (e.enclosingElement as ClassElement).type,
         unary: e.parameters.isEmpty,
         isStatic: e.isStatic,
-        allowExtensions: allowExtensions);
+        useExtension: useExtension);
   }
 
   /// This handles member renaming for private names and operators.
@@ -4212,7 +4214,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       {DartType type,
       bool unary: false,
       bool isStatic: false,
-      bool allowExtensions: true}) {
+      bool useExtension}) {
     // Static members skip the rename steps.
     if (isStatic) return _propertyName(name);
 
@@ -4232,19 +4234,20 @@ class CodeGenerator extends GeneralizingAstVisitor
       name = '+$name';
     }
 
-    // Dart "extension" methods. Used for JS Array, Boolean, Number, String.
-    var baseType = type;
-    while (baseType is TypeParameterType) {
-      baseType = baseType.element.bound;
-    }
-    if (allowExtensions &&
-        baseType != null &&
-        _extensionTypes.hasNativeSubtype(baseType) &&
-        !isObjectProperty(name)) {
-      return js.call('dartx.#', _propertyName(name));
+    var result = _propertyName(name);
+
+    if (useExtension == null) {
+      // Dart "extension" methods. Used for JS Array, Boolean, Number, String.
+      var baseType = type;
+      while (baseType is TypeParameterType) {
+        baseType = baseType.element.bound;
+      }
+      useExtension = baseType != null &&
+          _extensionTypes.hasNativeSubtype(baseType) &&
+          !isObjectMember(name);
     }
 
-    return _propertyName(name);
+    return useExtension ? js.call('dartx.#', result) : result;
   }
 
   JS.TemporaryId _emitPrivateNameSymbol(LibraryElement library, String name) {
@@ -4290,18 +4293,22 @@ class CodeGenerator extends GeneralizingAstVisitor
   /// a subtype of int and double (and num). It's in our "dart:_interceptors".
   bool _isNumberInJS(DartType t) => rules.isSubtypeOf(t, types.numType);
 
-  bool _isObjectGetter(String name) {
-    PropertyAccessorElement element = types.objectType.element.getGetter(name);
-    return (element != null && !element.isStatic);
-  }
-
-  bool _isObjectMethod(String name) {
-    MethodElement element = types.objectType.element.getMethod(name);
-    return (element != null && !element.isStatic);
-  }
-
-  bool isObjectProperty(String name) {
-    return _isObjectGetter(name) || _isObjectMethod(name);
+  /// Return true if this is one of the methods/properties on all Dart Objects
+  /// (toString, hashCode, noSuchMethod, runtimeType).
+  ///
+  /// Operator == is excluded, as it is handled as part of the equality binary
+  /// operator.
+  bool isObjectMember(String name) {
+    // We could look these up on Object, but we have hard coded runtime helpers
+    // so it's not really providing any benefit.
+    switch (name) {
+      case 'hashCode':
+      case 'toString':
+      case 'noSuchMethod':
+      case 'runtimeType':
+        return true;
+    }
+    return false;
   }
 
   // TODO(leafp): Various analyzer pieces computed similar things.