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

lib/src/codegen/js_codegen.dart

Index: lib/src/codegen/js_codegen.dart
diff --git a/lib/src/codegen/js_codegen.dart b/lib/src/codegen/js_codegen.dart
index 36e1f829f9faea7db0d69c705eb92a89378fcfbd..1cc53aba4e8aed2659ed6a7b9dcdffe931e21861 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -86,6 +86,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   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.
@@ -159,6 +160,10 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
 
     // 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.
@@ -658,6 +663,13 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     }
   }
 
+  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.
@@ -681,8 +693,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
         }
       }
       if (dartxNames.isNotEmpty) {
-        body.add(js.statement('dart.defineExtensionNames(#)',
-            [new JS.ArrayInitializer(dartxNames, multiline: true)]));
+        body.add(_emitExtensionNamesDeclaration(dartxNames));
       }
     }
 
@@ -3391,8 +3402,19 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       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(
@@ -3419,7 +3441,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     if (allowExtensions &&
         _extensionTypes.contains(baseType.element) &&
         !_isObjectProperty(name)) {
-      return js.call('dartx.#', _propertyName(name));
+      return _emitExtensionName(name);
     }
 
     return _propertyName(name);