Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Commit a637e1b

Browse files
author
Olivier Chafik
committed
Use a symbol for static length/name (and other Function properties) for 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 .
1 parent e693a00 commit a637e1b

File tree

5 files changed

+109
-28
lines changed

5 files changed

+109
-28
lines changed

‎lib/src/codegen/js_codegen.dart

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
8686
final _moduleItems = <JS.Statement>[];
8787
final _temps = new HashMap<Element, JS.TemporaryId>();
8888
final _qualifiedIds = new List<Tuple2<Element, JS.MaybeQualifiedId>>();
89+
final _topLevelExtensionNames = new Set<String>();
8990

9091
/// The name for the library's exports inside itself.
9192
/// `exports` was chosen as the most similar to ES module patterns.
@@ -168,6 +169,11 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
168169

169170
// Flush any unwritten fields/properties.
170171
_flushLibraryProperties(_moduleItems);
172+
// TODO(ochafik): Refactor to merge this with the dartxNames codepath.
173+
if (_topLevelExtensionNames.isNotEmpty) {
174+
_moduleItems.add(_emitExtensionNamesDeclaration(
175+
_topLevelExtensionNames.map(js.string).toList()));
176+
}
171177

172178
// Mark all qualified names as qualified or not, depending on if they need
173179
// to be loaded lazily or not.
@@ -675,6 +681,13 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
675681
}
676682
}
677683

684+
JS.Statement _emitExtensionNamesDeclaration(List<JS.Expression> names) =>
685+
js.statement('dart.defineExtensionNames(#)',
686+
[new JS.ArrayInitializer(names.toList(), multiline: true)]);
687+
688+
JS.Expression _emitExtensionName(String name) =>
689+
js.call('dartx.#', _propertyName(name));
690+
678691
_isQualifiedPath(JS.Expression node) =>
679692
node is JS.Identifier ||
680693
node is JS.PropertyAccess &&
@@ -719,8 +732,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
719732
}
720733
}
721734
if (dartxNames.isNotEmpty) {
722-
body.add(js.statement('dart.defineExtensionNames(#)',
723-
[new JS.ArrayInitializer(dartxNames, multiline: true)]));
735+
body.add(_emitExtensionNamesDeclaration(dartxNames));
724736
}
725737
}
726738

@@ -2259,7 +2271,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
22592271
isLoaded && (field.isConst || _constField.isFieldInitConstant(field));
22602272

22612273
var fieldName = field.name.name;
2262-
if (eagerInit && !JS.invalidStaticFieldName(fieldName)) {
2274+
if (eagerInit && !JS.invalidStaticFieldName(fieldName, options)) {
22632275
return annotateVariable(
22642276
js.statement('#.# = #;', [
22652277
classElem.name,
@@ -2319,7 +2331,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
23192331
field.element);
23202332
}
23212333

2322-
if (eagerInit && !JS.invalidStaticFieldName(fieldName)) {
2334+
if (eagerInit && !JS.invalidStaticFieldName(fieldName, options)) {
23232335
return annotateVariable(
23242336
js.statement('# = #;', [_visit(field.name), jsInit]), field.element);
23252337
}
@@ -3393,8 +3405,17 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
33933405
bool unary: false,
33943406
bool isStatic: false,
33953407
bool allowExtensions: true}) {
3396-
// Static members skip the rename steps.
3397-
if (isStatic) return _propertyName(name);
3408+
// Static members skip the rename steps, except for Function properties.
3409+
if (isStatic) {
3410+
// Avoid colliding with names on Function that are disallowed in ES6,
3411+
// or where we need to work around transpilers.
3412+
if (invalidStaticFieldName(name, options)) {
3413+
_topLevelExtensionNames.add(name);
3414+
return _emitExtensionName(name);
3415+
} else {
3416+
return _propertyName(name);
3417+
}
3418+
}
33983419

33993420
if (name.startsWith('_')) {
34003421
return _privateNames.putIfAbsent(
@@ -3421,7 +3442,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
34213442
if (allowExtensions &&
34223443
_extensionTypes.contains(baseType.element) &&
34233444
!_isObjectProperty(name)) {
3424-
return js.call('dartx.#', _propertyName(name));
3445+
return _emitExtensionName(name);
34253446
}
34263447

34273448
return _propertyName(name);

‎lib/src/codegen/js_names.dart

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import 'dart:collection';
66

77
import '../js/js_ast.dart';
8+
import '../options.dart';
89

910
/// Unique instance for temporary variables. Will be renamed consistently
1011
/// across the entire file. Different instances will be named differently
@@ -283,14 +284,20 @@ bool invalidVariableName(String keyword, {bool strictMode: true}) {
283284
return false;
284285
}
285286

286-
/// Returns true for invalid static field names in strict mode.
287+
/// Returns true for invalid static field names in strict mode or for some
288+
/// transpilers (e.g. when doing ES6->ES5 lowering with the Closure Compiler).
287289
/// In particular, "caller" "callee" and "arguments" cannot be used.
288-
bool invalidStaticFieldName(String name) {
290+
bool invalidStaticFieldName(String name, CodegenOptions options) {
289291
switch (name) {
290292
case "arguments":
291293
case "caller":
292294
case "callee":
293295
return true;
296+
// Workarounds for Closure:
297+
// (see https://github.com/google/closure-compiler/issues/1460)
298+
case "name":
299+
case "length":
300+
return options.closure;
294301
}
295302
return false;
296303
}

‎test/codegen/closure.dart

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
library test;
22
import 'dart:js';
33

4-
54
typedef void Callback({int i});
65

76
class Foo<T> {
@@ -15,7 +14,7 @@ class Foo<T> {
1514
factory Foo.build() => new Foo(1, null);
1615

1716
untyped_method(a, b) {}
18-
17+
1918
T pass(T t) => t;
2019

2120
String typed_method(
@@ -29,6 +28,15 @@ class Foo<T> {
2928

3029
static named_params(a, {b, c}) {}
3130

31+
// Avoid colliding with Function.name & Function.length, as Closure fails to
32+
// lower these to ES6 (https://github.com/google/closure-compiler/issues/1460)
33+
static name() => 'Foo.name()';
34+
static length() => 'Foo.length()';
35+
36+
static arguments() => 'Foo.arguments()';
37+
static caller() => 'Foo.caller()';
38+
static callee() => 'Foo.callee()';
39+
3240
nullary_method() {}
3341

3442
function_params(int f(x, [y]), g(x, {String y, z}), Callback cb) {
@@ -52,8 +60,14 @@ class Baz extends Foo<int> with Bar {
5260
Baz(int i) : super(i, 123);
5361
}
5462

55-
void main(args) {}
63+
void main(args) {
64+
print(Foo.name());
65+
print(Foo.length());
66+
print(Foo.arguments());
67+
print(Foo.caller());
68+
print(Foo.callee());
69+
}
5670

5771
const String some_top_level_constant = "abc";
5872
final String some_top_level_final = "abc";
59-
String some_top_level_var = "abc";
73+
String some_top_level_var = "abc";

‎test/codegen/expect/closure.js

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,21 @@ dart_library.library('closure', null, /* Imports */[
6767
let b = opts && 'b' in opts ? opts.b : null;
6868
let c = opts && 'c' in opts ? opts.c : null;
6969
}
70+
static [dartx.name]() {
71+
return 'Foo.name()';
72+
}
73+
static [dartx.length]() {
74+
return 'Foo.length()';
75+
}
76+
static [dartx.arguments]() {
77+
return 'Foo.arguments()';
78+
}
79+
static [dartx.caller]() {
80+
return 'Foo.caller()';
81+
}
82+
static [dartx.callee]() {
83+
return 'Foo.callee()';
84+
}
7085
nullary_method() {}
7186
/**
7287
* @param {function(?, ?=):?number} f
@@ -104,8 +119,15 @@ dart_library.library('closure', null, /* Imports */[
104119
nullary_method: [dart.dynamic, []],
105120
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]]
106121
}),
107-
statics: () => ({named_params: [dart.dynamic, [dart.dynamic], {b: dart.dynamic, c: dart.dynamic}]}),
108-
names: ['named_params']
122+
statics: () => ({
123+
named_params: [dart.dynamic, [dart.dynamic], {b: dart.dynamic, c: dart.dynamic}],
124+
[dartx.name]: [dart.dynamic, []],
125+
[dartx.length]: [dart.dynamic, []],
126+
[dartx.arguments]: [dart.dynamic, []],
127+
[dartx.caller]: [dart.dynamic, []],
128+
[dartx.callee]: [dart.dynamic, []]
129+
}),
130+
names: ['named_params', dartx.name, dartx.length, dartx.arguments, dartx.caller, dartx.callee]
109131
});
110132
/** @final {string} */
111133
Foo.some_static_constant = "abc";
@@ -129,6 +151,11 @@ dart_library.library('closure', null, /* Imports */[
129151
});
130152
/** @param {?} args */
131153
function main(args) {
154+
core.print(Foo[dartx.name]());
155+
core.print(Foo[dartx.length]());
156+
core.print(Foo[dartx.arguments]());
157+
core.print(Foo[dartx.caller]());
158+
core.print(Foo[dartx.callee]());
132159
}
133160
dart.fn(main, dart.void, [dart.dynamic]);
134161
/** @final {string} */
@@ -137,6 +164,13 @@ dart_library.library('closure', null, /* Imports */[
137164
exports.some_top_level_final = "abc";
138165
/** @type {string} */
139166
exports.some_top_level_var = "abc";
167+
dart.defineExtensionNames([
168+
"name",
169+
"length",
170+
"arguments",
171+
"caller",
172+
"callee"
173+
]);
140174
// Exports:
141175
exports.Callback = Callback;
142176
exports.Foo$ = Foo$;

‎test/codegen/expect/names.js

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,40 +20,45 @@ dart_library.library('names', null, /* Imports */[
2020
}
2121
dart.fn(_foo);
2222
class Frame extends core.Object {
23-
caller(arguments$) {
23+
[dartx.caller](arguments$) {
2424
this.arguments = arguments$;
2525
}
26-
static callee() {
26+
static [dartx.callee]() {
2727
return null;
2828
}
2929
}
30-
dart.defineNamedConstructor(Frame, 'caller');
30+
dart.defineNamedConstructor(Frame, dartx.caller);
3131
dart.setSignature(Frame, {
32-
constructors: () => ({caller: [Frame, [core.List]]}),
33-
statics: () => ({callee: [dart.dynamic, []]}),
34-
names: ['callee']
32+
constructors: () => ({[dartx.caller]: [Frame, [core.List]]}),
33+
statics: () => ({[dartx.callee]: [dart.dynamic, []]}),
34+
names: [dartx.callee]
3535
});
3636
class Frame2 extends core.Object {}
3737
dart.defineLazyProperties(Frame2, {
38-
get caller() {
38+
get [dartx.caller]() {
3939
return 100;
4040
},
41-
set caller(_) {},
42-
get arguments() {
41+
set [dartx.caller](_) {},
42+
get [dartx.arguments]() {
4343
return 200;
4444
},
45-
set arguments(_) {}
45+
set [dartx.arguments](_) {}
4646
});
4747
function main() {
4848
core.print(exports.exports);
4949
core.print(new Foo()[_foo$]());
5050
core.print(_foo());
51-
core.print(new Frame.caller([1, 2, 3]));
52-
let eval$ = Frame.callee;
51+
core.print(new Frame[dartx.caller]([1, 2, 3]));
52+
let eval$ = Frame[dartx.callee];
5353
core.print(eval$);
54-
core.print(dart.notNull(Frame2.caller) + dart.notNull(Frame2.arguments));
54+
core.print(dart.notNull(Frame2[dartx.caller]) + dart.notNull(Frame2[dartx.arguments]));
5555
}
5656
dart.fn(main);
57+
dart.defineExtensionNames([
58+
"caller",
59+
"callee",
60+
"arguments"
61+
]);
5762
// Exports:
5863
exports.Foo = Foo;
5964
exports.Frame = Frame;

0 commit comments

Comments
 (0)