Use a symbol for static length/name (conflicts with Function properties in ES5: cannot redefine Fun…

lib/src/codegen/js_codegen.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.codegen.js_codegen;
 
 import 'dart:collection' show HashSet, HashMap, SplayTreeSet;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/ast.dart' hide ConstantEvaluator;
@@ skipping 68 matching lines @@
   JS.TemporaryId _asyncStarController;
 
   /// Imported libraries, and the temporaries used to refer to them.
   final _imports = new Map<LibraryElement, JS.TemporaryId>();
   final _exports = new Set<String>();
   final _properties = <FunctionDeclaration>[];
   final _privateNames = new HashMap<String, JS.TemporaryId>();
   final _moduleItems = <JS.Statement>[];
   final _temps = new HashMap<Element, JS.TemporaryId>();
   final _qualifiedIds = new List<Tuple2<Element, JS.MaybeQualifiedId>>();
+  final _topLevelExtensionNames = new Set<String>();
 
   /// The name for the library's exports inside itself.
   /// `exports` was chosen as the most similar to ES module patterns.
   final _dartxVar = new JS.Identifier('dartx');
   final _exportsVar = new JS.TemporaryId('exports');
   final _runtimeLibVar = new JS.Identifier('dart');
   final _namedArgTemp = new JS.TemporaryId('opts');
 
   final TypeProvider _types;
 
@@ skipping 53 matching lines @@
         if (decl is TopLevelVariableDeclaration) {
           visitTopLevelVariableDeclaration(decl);
         } else {
           _loader.loadDeclaration(decl, decl.element);
         }
       }
     }
 
     // Flush any unwritten fields/properties.
     _flushLibraryProperties(_moduleItems);
+    if (_topLevelExtensionNames.isNotEmpty) {

[Jennifer Messerly, 2016/02/02 00:22:56]

I don't think this should be needed at the top-level.

Static methods/fields should only appear in a class, right? If it's not in a
class, then we don't need to worry about shadowing "Function".

It would be good to merge this in with the "dartxNames" code, that defines
extension members for a class.

It might need some light refactoring (given that extension members are currently
all or nothing for a class), but there's probably a clean way to factor it.

For example, we could rename this field to _extensionNames, initialize it when
we start generating a class, and then use it in _finishClassMembers. Some
benefits--I don't think we'll need to collect the names like it currently does,
and maybe "allowExtensions" bool can go away. 

[ochafik, 2016/02/03 19:58:49]

Great point! Leaving todo for follow up refactoring as you suggested

+      _moduleItems.add(_emitExtensionNamesDeclaration(
+          _topLevelExtensionNames.map(js.string).toList()));
+    }
 
     // Mark all qualified names as qualified or not, depending on if they need
     // to be loaded lazily or not.
     for (var elementIdPairs in _qualifiedIds) {
       var element = elementIdPairs.e0;
       var id = elementIdPairs.e1;
       id.setQualified(!_loader.isLoaded(element));
     }
 
     var moduleBuilder = new ModuleBuilder(options.moduleFormat);
@@ skipping 479 matching lines @@
   JS.Expression _instantiateAnnotation(Annotation node) {
     var element = node.element;
     if (element is ConstructorElement) {
       return _emitInstanceCreationExpression(element, element.returnType,
           node.constructorName, node.arguments, true);
     } else {
       return _visit(node.name);
     }
   }
 
+  JS.Statement _emitExtensionNamesDeclaration(List<JS.Expression> names) =>
+      js.statement('dart.defineExtensionNames(#)',
+          [new JS.ArrayInitializer(names.toList(), multiline: true)]);
+
+  JS.Expression _emitExtensionName(String name) =>
+      js.call('dartx.#', _propertyName(name));
+
   /// Emit class members that need to come after the class declaration, such
   /// as static fields. See [_emitClassMethods] for things that are emitted
   /// inside the ES6 `class { ... }` node.
   JS.Statement _finishClassMembers(
       ClassElement classElem,
       JS.ClassExpression cls,
       List<ConstructorDeclaration> ctors,
       List<FieldDeclaration> fields,
       List<FieldDeclaration> staticFields,
       List<MethodDeclaration> methods,
       List<Annotation> metadata,
       String jsPeerName) {
     var name = classElem.name;
     var body = <JS.Statement>[];
 
     if (_extensionTypes.contains(classElem)) {
       var dartxNames = <JS.Expression>[];
       for (var m in methods) {
         if (!m.isAbstract && !m.isStatic && m.element.isPublic) {
           dartxNames.add(_elementMemberName(m.element, allowExtensions: false));
         }
       }
       if (dartxNames.isNotEmpty) {
-        body.add(js.statement('dart.defineExtensionNames(#)',
-            [new JS.ArrayInitializer(dartxNames, multiline: true)]));
+        body.add(_emitExtensionNamesDeclaration(dartxNames));
       }
     }
 
     body.add(new JS.ClassDeclaration(cls));
 
     // TODO(jmesserly): we should really just extend native Array.
     if (jsPeerName != null && classElem.typeParameters.isNotEmpty) {
       body.add(js.statement('dart.setBaseClass(#, dart.global.#);',
           [classElem.name, _propertyName(jsPeerName)]));
     }
@@ skipping 2688 matching lines @@
   /// 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}) {
-    // Static members skip the rename steps.
-    if (isStatic) return _propertyName(name);
+    // Static members skip the rename steps, except for Function properties.
+    if (isStatic) {
+      if (options.closure) {
+        // Avoid colliding with Function properties, which might be desirable
+        // in general but is critical to work around a Closure ES6->ES5 bug
+        // (https://github.com/google/closure-compiler/issues/1460).
+        if (invalidStaticFieldName(name, closureCompiler: true)) {

[Jennifer Messerly, 2016/01/28 20:03:36]

I still don't understand why were are using extension method Symbols.

In Chrome, right now, I run this JavaScript code:

    class Foo {
      static name() { return 42; }
    }
    console.log(Foo.name()); // 42

If I try the same thing in Dart, it won't work. I'll have to know about
"Foo[dartx.name]()"

If it helps, I can write up a CL to show the alternate approach? Maybe we should
discuss next design meeting?

[Jennifer Messerly, 2016/02/02 00:22:56]

We chatted about this, decided extension method symbols are okay for now.  (if
we look at statics the same as instance properties. In the future with Dart
metaclasses, that might be the case.)

So what I'd like to see here -- I feel like the "closureCompiler" part of this
workaround applies to certain names only.  I think this may be the right
approach:

if (isStatic) {
  // Avoid colliding with names on Function that are disallowed in ES6,
  // or where we need to work around transpilers.
  if (invalidStaticName(name, options)) {
    return _emitExtensionName(name);
  }
}

then in invalidStaticName, check for "closure" mode for the particular fields
we're worried about.

[ochafik, 2016/02/03 19:58:49]

Done.

[ochafik, 2016/02/03 19:58:49]

Acknowledged.

+          _topLevelExtensionNames.add(name);
+          return _emitExtensionName(name);
+        }
+      }
+      return _propertyName(name);
+    }
 
     if (name.startsWith('_')) {
       return _privateNames.putIfAbsent(
           name, () => _initSymbol(new JS.TemporaryId(name)) as JS.TemporaryId);
     }
 
     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 &&
         _extensionTypes.contains(baseType.element) &&
         !_isObjectProperty(name)) {
-      return js.call('dartx.#', _propertyName(name));
+      return _emitExtensionName(name);
     }
 
     return _propertyName(name);
   }
 
   bool _externalOrNative(node) =>
       node.externalKeyword != null || _functionBody(node) is NativeFunctionBody;
 
   FunctionBody _functionBody(node) =>
       node is FunctionDeclaration ? node.functionExpression.body : node.body;
@@ skipping 177 matching lines @@
 
 /// A special kind of element created by the compiler, signifying a temporary
 /// variable. These objects use instance equality, and should be shared
 /// everywhere in the tree where they are treated as the same variable.
 class TemporaryVariableElement extends LocalVariableElementImpl {
   TemporaryVariableElement.forNode(Identifier name) : super.forNode(name);
 
   int get hashCode => identityHashCode(this);
   bool operator ==(Object other) => identical(this, other);
 }