fix #552, Object members on native types

lib/src/compiler/code_generator.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.
 
 import 'dart:collection' show HashMap, HashSet;
 import 'dart:math' show min, max;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart' show Token, TokenType;
@@ skipping 206 matching lines @@
       // TODO(jmesserly): find a cleaner design for this.
       if (_isDartRuntime(library)) {
         items.add(new JS.ExportDeclaration(
             js.call('const # = Object.create(null)', [_dartxVar])));
       }
     }
 
     // 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.
     // Order will be changed as needed so the resulting code can execute.
     // This is done by forward declaring items.
     compilationUnits.forEach(visitCompilationUnit);
 
     // Declare imports
     _finishImports(items);
 
@@ skipping 69 matching lines @@
         imports.add(new JS.NameSpecifier(_runtimeLibVar));
         imports.add(new JS.NameSpecifier(_dartxVar));
       }
       items.add(new JS.ImportDeclaration(
           namedImports: imports, from: js.string(module, "'")));
     });
   }
 
   /// 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) {
           map[field.element] = field;
         }
       } else {

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

Is it worth skipping items already in runtimeHelpers?

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

Hmmm. We actually can't as we need the Element -> Node mapping to emit them. But
your observation is good, I don't think there's any reason for me to populate
runtimeHelpers in this method. Let me pull that code out of this function, I
think that may be what's confusing things.

         map[declaration.element] = declaration;
       }
     }
-    return map;
   }
 
   void _emitModuleItem(AstNode node) {
     // TODO(jmesserly): ideally we could do this at a smaller granularity.
     // We'll need to be consistent about when we're generating functions, and
     // only run this on the outermost function.
     inferNullableTypes(node);
 
     var code = _visit(node);
     if (code != null) _moduleItems.add(code);
@@ skipping 610 matching lines @@
   JS.Method _emitSuperAccessorWrapper(MethodDeclaration method,
       InterfaceType type, List<ClassElement> superclasses) {
     var methodElement = method.element as PropertyAccessorElement;
     var field = methodElement.variable;
     if (!field.isSynthetic) return null;
     var propertyOverrideResult = checkForPropertyOverride(
         methodElement.variable, superclasses, _extensionTypes);
 
     // 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)
         return null;
       var fn = js.call('function(value) { super[#] = value; }', [name]);
       return new JS.Method(name, fn, isSetter: true);
     } else {
       // Generate a getter
       if (field.getter != null || !propertyOverrideResult.foundGetter)
         return null;
@@ skipping 133 matching lines @@
 
   /// If a concrete class implements one of our extensions, we might need to
   /// add forwarders.
   void _defineExtensionMembers(List<ExecutableElement> extensions,
       JS.Expression className, List<JS.Statement> body) {
     // If a concrete class implements one of our extensions, we might need to
     // add forwarders.
     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,
         new JS.ArrayInitializer(methodNames, multiline: methodNames.length > 4)
       ]));
     }
   }
 
   /// Emit the signature on the class recording the runtime type information
   void _emitClassSignature(
@@ skipping 17 matching lines @@
     for (MethodDeclaration node in methods) {
       if (!(node.isSetter || node.isGetter || node.isAbstract)) {
         var name = node.name.name;
         var element = node.element;
         var inheritedElement =
             classElem.lookUpInheritedConcreteMethod(name, currentLibrary);
         if (inheritedElement != null && inheritedElement.type == element.type) {
           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));
         if (node.isStatic) {
           tStatics.add(property);
           sNames.add(memberName);
         } else {
           tMethods.add(property);
         }
       }
@@ skipping 35 matching lines @@
   /// Ensure `dartx.` symbols we will use are present.
   void _initExtensionSymbols(
       ClassElement classElem,
       List<MethodDeclaration> methods,
       List<FieldDeclaration> fields,
       List<JS.Statement> body) {
     if (_extensionTypes.hasNativeSubtype(classElem.type)) {
       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) {
         if (!f.isStatic) {
           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));
             }
           }
         }
       }
       if (dartxNames.isNotEmpty) {
         body.add(js.statement('dart.defineExtensionNames(#)',
             [new JS.ArrayInitializer(dartxNames, multiline: true)]));
       }
     }
   }
 
   List<ExecutableElement> _extensionsToImplement(ClassElement element) {
     var members = <ExecutableElement>[];
     if (_extensionTypes.isNativeClass(element)) return members;
 
     // Collect all extension types we implement.
     var type = element.type;
     var types = _extensionTypes.collectNativeInterfaces(element);
     if (types.isEmpty) return members;
 
     // Collect all possible extension method names.
     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.)

       }
     }
 
     // Collect all of extension methods this type implements.
     for (var m in [type.methods, type.accessors].expand((e) => e)) {
       if (!m.isStatic && !m.isAbstract && extensionMembers.contains(m.name)) {
         members.add(m);
       }
     }
     return members;
@@ skipping 413 matching lines @@
         // call sites, but it's cleaner to instead transform the operator method.
         fn = _alwaysReturnLastParameter(fn);
       }
 
       fn = _makeGenericFunction(fn);
     }
 
     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,
             isStatic: node.isStatic),
         node,
         node.element);
   }
 
   /// Transform the function so the last parameter is always returned.
   ///
@@ skipping 782 matching lines @@
     JS.Expression jsTarget = _visit(target);
     if (DynamicInvoke.get(target)) {
       if (typeArgs != null) {
         return js.call('dart.dgsend(#, #, #, #)',
             [jsTarget, new JS.ArrayInitializer(typeArgs), memberName, args]);
       } else {
         return js.call('dart.dsend(#, #, #)', [jsTarget, memberName, args]);
       }
     }
     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);
 
     if (typeArgs != null) jsTarget = new JS.Call(jsTarget, typeArgs);
 
     if (DynamicInvoke.get(node.methodName)) {
       // This is a dynamic call to a statically known target. For example:
       //     class Foo { Function bar; }
       //     new Foo().bar(); // dynamic call
@@ skipping 332 matching lines @@
       // Static and instance fields are handled elsewhere.
       assert(node.element is TopLevelVariableElement);
       return _emitTopLevelField(node);
     }
 
     var name =
         new JS.Identifier(node.name.name, type: emitTypeRef(node.element.type));
     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
   /// dependencies are safe to refer to while we are initializing the class,
   /// then we can initialize it eagerly:
   ///
   ///     // Baz must be const constructor, and the name "Baz" must be defined
   ///     // by this point.
   ///     Foo.bar = dart.const(new Baz(42));
   ///
@@ skipping 45 matching lines @@
       jsInit = _visitInitializer(field);
       _loader.finishTopLevel(element);
       eagerInit = _loader.isLoaded(element);
     } else {
       // TODO(jmesserly): we're visiting the initializer here, and again
       // later on when we emit lazy fields. That seems busted.
       jsInit = _visitInitializer(field);
       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.
       _eagerTopLevelFields.add(element);
 
       return annotate(
           js.statement('# = #;', [_emitTopLevelName(element), jsInit]),
           field,
           element);
     }
@@ skipping 672 matching lines @@
     }
   }
 
   /// Everything in Dart is an Object and supports the 4 members on Object,
   /// so we have to use a runtime helper to handle values such as `null` and
   /// native types.
   ///
   /// 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;
     }
 
     // Check if the target could be `null`, is dynamic, or may be an extension
     // native type. In all of those cases we need defensive code generation.
     var type = getStaticType(target);
     return isNullable(target) ||
         type.isDynamic ||
         (_extensionTypes.hasNativeSubtype(type) && target is! SuperExpression);
   }
@@ skipping 57 matching lines @@
       // subclasses cannot override x.
       jsTarget = new JS.This();
     }
 
     JS.Expression result;
     if (member != null && member is MethodElement && !isStatic) {
       // Tear-off methods: explicitly bind it.
       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]);
     }
     if (typeArgs == null) {
       return result;
     }
     return js.call('dart.gbind(#, #)', [result, typeArgs]);
   }
 
   /// Emits a generic send, like an operator method.
@@ skipping 447 matching lines @@
   JS.Expression _visitListToBinary(List<Expression> nodes, String operator) {
     if (nodes == null || nodes.isEmpty) return null;
     return new JS.Expression.binary(
         _visitList(nodes) as List<JS.Expression>, operator);
   }
 
   /// Like [_emitMemberName], but for declaration sites.
   ///
   /// 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;
     } else {
       name = e.name;
     }
     return _emitMemberName(name,
         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.
   ///
   /// Private names are generated using ES6 symbols:
   ///
   ///     // At the top of the module:
   ///     let _x = Symbol('_x');
   ///     let _y = Symbol('_y');
   ///     ...
@@ skipping 27 matching lines @@
   /// Unary minus looks like: `x['unary-']()`. Note that [unary] must be passed
   /// for this transformation to happen, otherwise binary minus is assumed.
   ///
   /// Equality is a bit special, it is generated via the Dart `equals` runtime
   /// helper, that checks for null. The user defined method is called '=='.
   ///
   JS.Expression _emitMemberName(String name,
       {DartType type,
       bool unary: false,
       bool isStatic: false,
-      bool allowExtensions: true}) {
+      bool useExtension}) {
     // Static members skip the rename steps.
     if (isStatic) return _propertyName(name);
 
     if (name.startsWith('_')) {
       return _emitPrivateNameSymbol(currentLibrary, name);
     }
 
     if (name == '[]') {
       name = 'get';
     } else if (name == '[]=') {
       name = 'set';
     } else if (name == '-' && unary) {
       name = 'unary-';
     } else if (name == 'constructor' || name == 'prototype') {
       // This uses an illegal (in Dart) character for a member, avoiding the
       // conflict. We could use practically any character for this.
       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) {
     return _privateNames
         .putIfAbsent(library, () => new HashMap())
         .putIfAbsent(name, () {
       var id = new JS.TemporaryId(name);
       _moduleItems.add(
           js.statement('const # = Symbol(#);', [id, js.string(id.name, "'")]));
       return id;
@@ skipping 25 matching lines @@
   }
 
   /// Returns true if this is any kind of object represented by `Number` in JS.
   ///
   /// In practice, this is 4 types: num, int, double, and JSNumber.
   ///
   /// JSNumber is the type that actually "implements" all numbers, hence it's
   /// 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.
   // Share this logic somewhere?
   DartType _getExpectedReturnType(ExecutableElement element) {
     FunctionType functionType = element.type;
     if (functionType == null) {
       return DynamicTypeImpl.instance;
     }
     var type = functionType.returnType;
@@ skipping 79 matching lines @@
 }
 
 bool isLibraryPrefix(Expression node) =>
     node is SimpleIdentifier && node.staticElement is PrefixElement;
 
 LibraryElement _getLibrary(AnalysisContext c, String uri) =>
     c.computeLibraryElement(c.sourceFactory.forUri(uri));
 
 bool _isDartRuntime(LibraryElement l) =>
     l.isInSdk && l.source.uri.toString() == 'dart:_runtime';