From 98b21b030e0dec4a430c8a8dd0a5290266ebeb9c Mon Sep 17 00:00:00 2001 From: Olivier Chafik Date: Wed, 3 Feb 2016 20:35:59 +0000 Subject: [PATCH] Revert "Use a symbol for static length/name (and other Function properties) for Closure, to avoid ES5->ES6 lowering bug (https://github.com/google/closure-compiler/issues/1460)." This reverts commit a637e1b048487629d0d44620eb41e703531ce18d. --- .../lib/src/codegen/js_codegen.dart | 35 ++++------------- .../lib/src/codegen/js_names.dart | 11 +----- pkg/dev_compiler/test/codegen/closure.dart | 22 ++--------- .../test/codegen/expect/closure.js | 38 +------------------ pkg/dev_compiler/test/codegen/expect/names.js | 31 +++++++-------- 5 files changed, 28 insertions(+), 109 deletions(-) diff --git a/pkg/dev_compiler/lib/src/codegen/js_codegen.dart b/pkg/dev_compiler/lib/src/codegen/js_codegen.dart index fb1f0a6adad1..2b3fddff52f2 100644 --- a/pkg/dev_compiler/lib/src/codegen/js_codegen.dart +++ b/pkg/dev_compiler/lib/src/codegen/js_codegen.dart @@ -86,7 +86,6 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator { final _moduleItems = []; final _temps = new HashMap(); final _qualifiedIds = new List>(); - final _topLevelExtensionNames = new Set(); /// The name for the library's exports inside itself. /// `exports` was chosen as the most similar to ES module patterns. @@ -169,11 +168,6 @@ 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. @@ -681,13 +675,6 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator { } } - JS.Statement _emitExtensionNamesDeclaration(List 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 && @@ -732,7 +719,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator { } } if (dartxNames.isNotEmpty) { - body.add(_emitExtensionNamesDeclaration(dartxNames)); + body.add(js.statement('dart.defineExtensionNames(#)', + [new JS.ArrayInitializer(dartxNames, multiline: true)])); } } @@ -2271,7 +2259,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator { isLoaded && (field.isConst || _constField.isFieldInitConstant(field)); var fieldName = field.name.name; - if (eagerInit && !JS.invalidStaticFieldName(fieldName, options)) { + if (eagerInit && !JS.invalidStaticFieldName(fieldName)) { return annotateVariable( js.statement('#.# = #;', [ classElem.name, @@ -2331,7 +2319,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator { field.element); } - if (eagerInit && !JS.invalidStaticFieldName(fieldName, options)) { + if (eagerInit && !JS.invalidStaticFieldName(fieldName)) { return annotateVariable( js.statement('# = #;', [_visit(field.name), jsInit]), field.element); } @@ -3405,17 +3393,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator { bool unary: false, bool isStatic: false, bool allowExtensions: true}) { - // 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); - } - } + // Static members skip the rename steps. + if (isStatic) return _propertyName(name); if (name.startsWith('_')) { return _privateNames.putIfAbsent( @@ -3442,7 +3421,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator { if (allowExtensions && _extensionTypes.contains(baseType.element) && !_isObjectProperty(name)) { - return _emitExtensionName(name); + return js.call('dartx.#', _propertyName(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 index 36614c48b0f6..307e52e0b29d 100644 --- a/pkg/dev_compiler/lib/src/codegen/js_names.dart +++ b/pkg/dev_compiler/lib/src/codegen/js_names.dart @@ -5,7 +5,6 @@ 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 @@ -284,20 +283,14 @@ bool invalidVariableName(String keyword, {bool strictMode: true}) { return false; } -/// 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). +/// Returns true for invalid static field names in strict mode. /// In particular, "caller" "callee" and "arguments" cannot be used. -bool invalidStaticFieldName(String name, CodegenOptions options) { +bool invalidStaticFieldName(String name) { 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 index cf2cd1ad8458..d40e8a759476 100644 --- a/pkg/dev_compiler/test/codegen/closure.dart +++ b/pkg/dev_compiler/test/codegen/closure.dart @@ -1,6 +1,7 @@ library test; import 'dart:js'; + typedef void Callback({int i}); class Foo { @@ -14,7 +15,7 @@ class Foo { factory Foo.build() => new Foo(1, null); untyped_method(a, b) {} - + T pass(T t) => t; String typed_method( @@ -28,15 +29,6 @@ class Foo { 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) { @@ -60,14 +52,8 @@ class Baz extends Foo with Bar { Baz(int i) : super(i, 123); } -void main(args) { - print(Foo.name()); - print(Foo.length()); - print(Foo.arguments()); - print(Foo.caller()); - print(Foo.callee()); -} +void main(args) {} 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"; \ No newline at end of file diff --git a/pkg/dev_compiler/test/codegen/expect/closure.js b/pkg/dev_compiler/test/codegen/expect/closure.js index 0fe32fc15265..f8084ab24495 100644 --- a/pkg/dev_compiler/test/codegen/expect/closure.js +++ b/pkg/dev_compiler/test/codegen/expect/closure.js @@ -67,21 +67,6 @@ 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 @@ -119,15 +104,8 @@ 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}], - [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] + statics: () => ({named_params: [dart.dynamic, [dart.dynamic], {b: dart.dynamic, c: dart.dynamic}]}), + names: ['named_params'] }); /** @final {string} */ Foo.some_static_constant = "abc"; @@ -151,11 +129,6 @@ 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} */ @@ -164,13 +137,6 @@ 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 index f3a06082de79..ebe11deb9980 100644 --- a/pkg/dev_compiler/test/codegen/expect/names.js +++ b/pkg/dev_compiler/test/codegen/expect/names.js @@ -20,45 +20,40 @@ dart_library.library('names', null, /* Imports */[ } dart.fn(_foo); class Frame extends core.Object { - [dartx.caller](arguments$) { + caller(arguments$) { this.arguments = arguments$; } - static [dartx.callee]() { + static callee() { return null; } } - dart.defineNamedConstructor(Frame, dartx.caller); + dart.defineNamedConstructor(Frame, 'caller'); dart.setSignature(Frame, { - constructors: () => ({[dartx.caller]: [Frame, [core.List]]}), - statics: () => ({[dartx.callee]: [dart.dynamic, []]}), - names: [dartx.callee] + constructors: () => ({caller: [Frame, [core.List]]}), + statics: () => ({callee: [dart.dynamic, []]}), + names: ['callee'] }); class Frame2 extends core.Object {} dart.defineLazyProperties(Frame2, { - get [dartx.caller]() { + get caller() { return 100; }, - set [dartx.caller](_) {}, - get [dartx.arguments]() { + set caller(_) {}, + get arguments() { return 200; }, - set [dartx.arguments](_) {} + set arguments(_) {} }); function main() { core.print(exports.exports); core.print(new Foo()[_foo$]()); core.print(_foo()); - core.print(new Frame[dartx.caller]([1, 2, 3])); - let eval$ = Frame[dartx.callee]; + core.print(new Frame.caller([1, 2, 3])); + let eval$ = Frame.callee; core.print(eval$); - core.print(dart.notNull(Frame2[dartx.caller]) + dart.notNull(Frame2[dartx.arguments])); + core.print(dart.notNull(Frame2.caller) + dart.notNull(Frame2.arguments)); } dart.fn(main); - dart.defineExtensionNames([ - "caller", - "callee", - "arguments" - ]); // Exports: exports.Foo = Foo; exports.Frame = Frame;