Use a symbol for static length/name (and other Function properties) f…

…or Closure, to avoid ES5->ES6 lowering bug (google/closure-compiler#1460). This is part of the overall "simple closure" effort (issue #312) BUG= [email protected] Review URL: https://codereview.chromium.org/1630963003 .
dart-lang · Feb 3, 2016 · 84043ff · 84043ff
1 parent f7f21ca
commit 84043ff
Show file tree

Hide file tree

Showing 5 changed files with 109 additions and 28 deletions.
diff --git a/pkg/dev_compiler/lib/src/codegen/js_codegen.dart b/pkg/dev_compiler/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.
@@ -168,6 +169,11 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
 
     // Flush any unwritten fields/properties.
     _flushLibraryProperties(_moduleItems);
+    // TODO(ochafik): Refactor to merge this with the dartxNames codepath.
+    if (_topLevelExtensionNames.isNotEmpty) {
+      _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.
@@ -675,6 +681,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));
+
   _isQualifiedPath(JS.Expression node) =>
       node is JS.Identifier ||
       node is JS.PropertyAccess &&
@@ -719,8 +732,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));
       }
     }
 
@@ -2259,7 +2271,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
         isLoaded && (field.isConst || _constField.isFieldInitConstant(field));
 
     var fieldName = field.name.name;
-    if (eagerInit && !JS.invalidStaticFieldName(fieldName)) {
+    if (eagerInit && !JS.invalidStaticFieldName(fieldName, options)) {
       return annotateVariable(
           js.statement('#.# = #;', [
             classElem.name,
@@ -2319,7 +2331,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
           field.element);
     }
 
-    if (eagerInit && !JS.invalidStaticFieldName(fieldName)) {
+    if (eagerInit && !JS.invalidStaticFieldName(fieldName, options)) {
       return annotateVariable(
           js.statement('# = #;', [_visit(field.name), jsInit]), field.element);
     }
@@ -3393,8 +3405,17 @@ 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) {
+      // Avoid colliding with names on Function that are disallowed in ES6,
+      // or where we need to work around transpilers.
+      if (invalidStaticFieldName(name, options)) {
+        _topLevelExtensionNames.add(name);
+        return _emitExtensionName(name);
+      } else {
+        return _propertyName(name);
+      }
+    }
 
     if (name.startsWith('_')) {
       return _privateNames.putIfAbsent(
@@ -3421,7 +3442,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);

diff --git a/pkg/dev_compiler/lib/src/codegen/js_names.dart b/pkg/dev_compiler/lib/src/codegen/js_names.dart
@@ -5,6 +5,7 @@
 import 'dart:collection';
 
 import '../js/js_ast.dart';
+import '../options.dart';
 
 /// Unique instance for temporary variables. Will be renamed consistently
 /// across the entire file. Different instances will be named differently
@@ -283,14 +284,20 @@ bool invalidVariableName(String keyword, {bool strictMode: true}) {
   return false;
 }
 
-/// Returns true for invalid static field names in strict mode.
+/// Returns true for invalid static field names in strict mode or for some
+/// transpilers (e.g. when doing ES6->ES5 lowering with the Closure Compiler).
 /// In particular, "caller" "callee" and "arguments" cannot be used.
-bool invalidStaticFieldName(String name) {
+bool invalidStaticFieldName(String name, CodegenOptions options) {
   switch (name) {
     case "arguments":
     case "caller":
     case "callee":
       return true;
+    // Workarounds for Closure:
+    // (see https://github.com/google/closure-compiler/issues/1460)
+    case "name":
+    case "length":
+      return options.closure;
   }
   return false;
 }
diff --git a/pkg/dev_compiler/test/codegen/closure.dart b/pkg/dev_compiler/test/codegen/closure.dart
@@ -1,7 +1,6 @@
 library test;
 import 'dart:js';
 
-
 typedef void Callback({int i});
 
 class Foo<T> {
@@ -15,7 +14,7 @@ class Foo<T> {
   factory Foo.build() => new Foo(1, null);
 
   untyped_method(a, b) {}
-  
+
   T pass(T t) => t;
 
   String typed_method(
@@ -29,6 +28,15 @@ class Foo<T> {
 
   static named_params(a, {b, c}) {}
 
+  // Avoid colliding with Function.name & Function.length, as Closure fails to
+  // lower these to ES6 (https://github.com/google/closure-compiler/issues/1460)
+  static name() => 'Foo.name()';
+  static length() => 'Foo.length()';
+
+  static arguments() => 'Foo.arguments()';
+  static caller() => 'Foo.caller()';
+  static callee() => 'Foo.callee()';
+
   nullary_method() {}
 
   function_params(int f(x, [y]), g(x, {String y, z}), Callback cb) {
@@ -52,8 +60,14 @@ class Baz extends Foo<int> with Bar {
   Baz(int i) : super(i, 123);
 }
 
-void main(args) {}
+void main(args) {
+  print(Foo.name());
+  print(Foo.length());
+  print(Foo.arguments());
+  print(Foo.caller());
+  print(Foo.callee());
+}
 
 const String some_top_level_constant = "abc";
 final String some_top_level_final = "abc";
-String some_top_level_var = "abc";
+String some_top_level_var = "abc";
diff --git a/pkg/dev_compiler/test/codegen/expect/closure.js b/pkg/dev_compiler/test/codegen/expect/closure.js
@@ -67,6 +67,21 @@ dart_library.library('closure', null, /* Imports */[
         let b = opts && 'b' in opts ? opts.b : null;
         let c = opts && 'c' in opts ? opts.c : null;
       }
+      static [dartx.name]() {
+        return 'Foo.name()';
+      }
+      static [dartx.length]() {
+        return 'Foo.length()';
+      }
+      static [dartx.arguments]() {
+        return 'Foo.arguments()';
+      }
+      static [dartx.caller]() {
+        return 'Foo.caller()';
+      }
+      static [dartx.callee]() {
+        return 'Foo.callee()';
+      }
       nullary_method() {}
       /**
        * @param {function(?, ?=):?number} f
@@ -104,8 +119,15 @@ dart_library.library('closure', null, /* Imports */[
         nullary_method: [dart.dynamic, []],
         function_params: [dart.dynamic, [dart.functionType(core.int, [dart.dynamic], [dart.dynamic]), dart.functionType(dart.dynamic, [dart.dynamic], {y: core.String, z: dart.dynamic}), Callback]]
       }),
-      statics: () => ({named_params: [dart.dynamic, [dart.dynamic], {b: dart.dynamic, c: dart.dynamic}]}),
-      names: ['named_params']
+      statics: () => ({
+        named_params: [dart.dynamic, [dart.dynamic], {b: dart.dynamic, c: dart.dynamic}],
+        [dartx.name]: [dart.dynamic, []],
+        [dartx.length]: [dart.dynamic, []],
+        [dartx.arguments]: [dart.dynamic, []],
+        [dartx.caller]: [dart.dynamic, []],
+        [dartx.callee]: [dart.dynamic, []]
+      }),
+      names: ['named_params', dartx.name, dartx.length, dartx.arguments, dartx.caller, dartx.callee]
     });
     /** @final {string} */
     Foo.some_static_constant = "abc";
@@ -129,6 +151,11 @@ dart_library.library('closure', null, /* Imports */[
   });
   /** @param {?} args */
   function main(args) {
+    core.print(Foo[dartx.name]());
+    core.print(Foo[dartx.length]());
+    core.print(Foo[dartx.arguments]());
+    core.print(Foo[dartx.caller]());
+    core.print(Foo[dartx.callee]());
   }
   dart.fn(main, dart.void, [dart.dynamic]);
   /** @final {string} */
@@ -137,6 +164,13 @@ dart_library.library('closure', null, /* Imports */[
   exports.some_top_level_final = "abc";
   /** @type {string} */
   exports.some_top_level_var = "abc";
+  dart.defineExtensionNames([
+    "name",
+    "length",
+    "arguments",
+    "caller",
+    "callee"
+  ]);
   // Exports:
   exports.Callback = Callback;
   exports.Foo$ = Foo$;

diff --git a/pkg/dev_compiler/test/codegen/expect/names.js b/pkg/dev_compiler/test/codegen/expect/names.js
@@ -20,40 +20,45 @@ dart_library.library('names', null, /* Imports */[
   }
   dart.fn(_foo);
   class Frame extends core.Object {
-    caller(arguments$) {
+    [dartx.caller](arguments$) {
       this.arguments = arguments$;
     }
-    static callee() {
+    static [dartx.callee]() {
       return null;
     }
   }
-  dart.defineNamedConstructor(Frame, 'caller');
+  dart.defineNamedConstructor(Frame, dartx.caller);
   dart.setSignature(Frame, {
-    constructors: () => ({caller: [Frame, [core.List]]}),
-    statics: () => ({callee: [dart.dynamic, []]}),
-    names: ['callee']
+    constructors: () => ({[dartx.caller]: [Frame, [core.List]]}),
+    statics: () => ({[dartx.callee]: [dart.dynamic, []]}),
+    names: [dartx.callee]
   });
   class Frame2 extends core.Object {}
   dart.defineLazyProperties(Frame2, {
-    get caller() {
+    get [dartx.caller]() {
       return 100;
     },
-    set caller(_) {},
-    get arguments() {
+    set [dartx.caller](_) {},
+    get [dartx.arguments]() {
       return 200;
     },
-    set arguments(_) {}
+    set [dartx.arguments](_) {}
   });
   function main() {
     core.print(exports.exports);
     core.print(new Foo()[_foo$]());
     core.print(_foo());
-    core.print(new Frame.caller([1, 2, 3]));
-    let eval$ = Frame.callee;
+    core.print(new Frame[dartx.caller]([1, 2, 3]));
+    let eval$ = Frame[dartx.callee];
     core.print(eval$);
-    core.print(dart.notNull(Frame2.caller) + dart.notNull(Frame2.arguments));
+    core.print(dart.notNull(Frame2[dartx.caller]) + dart.notNull(Frame2[dartx.arguments]));
   }
   dart.fn(main);
+  dart.defineExtensionNames([
+    "caller",
+    "callee",
+    "arguments"
+  ]);
   // Exports:
   exports.Foo = Foo;
   exports.Frame = Frame;