Skip to content

Commit 7391394

Browse files
brad4dYannic
brad4d
authored and
Yannic
committed
Always hoist vars declared in for-loops when transpiling generators.
Fixes google#2910 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=196136758
1 parent bfd4623 commit 7391394

File tree

2 files changed

+143
-26
lines changed

2 files changed

+143
-26
lines changed

‎src/com/google/javascript/jscomp/Es6RewriteGenerators.java

Lines changed: 94 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2049,9 +2049,13 @@ public void visit(NodeTraversal t, Node n, Node parent) {
20492049
} else if (n.isName() && n.getString().equals("arguments")) {
20502050
visitArguments(n);
20512051
} else if (n.isVar()) {
2052-
// don't transpile var in "for (var i = 0; ; )"
2053-
if (!(parent.isVanillaFor() || parent.isForIn()) || parent.getFirstChild() != n) {
2054-
visitVar(n);
2052+
if (parent.isVanillaFor()) {
2053+
visitVanillaForLoopVar(n);
2054+
} else if (parent.isForIn()) {
2055+
visitForInLoopVar(n);
2056+
} else {
2057+
// NOTE: for-of loops are transpiled away before this pass
2058+
visitVarStatement(n);
20552059
}
20562060
} // else no changes need to be made
20572061
}
@@ -2130,9 +2134,86 @@ void visitArguments(Node n) {
21302134
* a = "test", b = i + 5;
21312135
* </pre>
21322136
*/
2133-
void visitVar(Node varStatement) {
2137+
void visitVarStatement(Node varStatement) {
2138+
Node commaExpression = extractAssignmentsToCommaExpression(varStatement);
2139+
if (commaExpression == null) {
2140+
varStatement.detach();
2141+
} else {
2142+
varStatement.replaceWith(IR.exprResult(commaExpression));
2143+
}
2144+
// Move declaration without initial values to just before the program method definition.
2145+
hoistNode(varStatement);
2146+
}
2147+
2148+
/**
2149+
* Hoists {@code var} declarations in vanilla for loops into the closure containing the
2150+
* generator to preserve their state across multiple invocation of state machine program.
2151+
*
2152+
* <p>
2153+
*
2154+
* <pre>
2155+
* for (var a = "test", b = i + 5; ... ; )
2156+
* </pre>
2157+
*
2158+
* is transpiled to:
2159+
*
2160+
* <pre>
2161+
* var a, b;
2162+
* for (a = "test", b = i + 5; ...; )
2163+
* </pre>
2164+
*/
2165+
private void visitVanillaForLoopVar(Node varDeclaration) {
2166+
Node commaExpression = extractAssignmentsToCommaExpression(varDeclaration);
2167+
if (commaExpression == null) {
2168+
// `for (var x; ` becomes `for (; `
2169+
varDeclaration.replaceWith(IR.empty());
2170+
} else {
2171+
// `for (var i = 0, j = 0; `... becomes `for (i = 0, j = 0; `...
2172+
varDeclaration.replaceWith(commaExpression);
2173+
}
2174+
// Move declaration without initial values to just before the program method definition.
2175+
hoistNode(varDeclaration);
2176+
}
2177+
2178+
/**
2179+
* Hoists {@code var} declarations in for-in loops into the closure containing the
2180+
* generator to preserve their state across multiple invocation of state machine program.
2181+
*
2182+
* <p>
2183+
*
2184+
* <pre>
2185+
* for (var a in obj)
2186+
* </pre>
2187+
*
2188+
* is transpiled to:
2189+
*
2190+
* <pre>
2191+
* var a;
2192+
* for (a in obj))
2193+
* </pre>
2194+
*/
2195+
private void visitForInLoopVar(Node varDeclaration) {
2196+
// `for (var varName in ` ...
2197+
Node varName = varDeclaration.getOnlyChild();
2198+
checkState(!varName.hasChildren(), varName);
2199+
Node clonedVarName = varName.cloneNode().setJSDocInfo(null);
2200+
// becomes `for (varName in ` ...
2201+
varDeclaration.replaceWith(clonedVarName);
2202+
// Move declaration without initial values to just before the program method definition.
2203+
hoistNode(varDeclaration);
2204+
}
2205+
2206+
/**
2207+
* Removes all initializers from a var declaration and returns them as a single expression
2208+
* of comma-separated assignments or null if there aren't any initializers.
2209+
*
2210+
* @param varDeclaration VAR node
2211+
* @return null or expression node (e.g. `varName1 = 1, varName2 = y`)
2212+
*/
2213+
@Nullable
2214+
private Node extractAssignmentsToCommaExpression(Node varDeclaration) {
21342215
ArrayList<Node> assignments = new ArrayList<>();
2135-
for (Node varName : varStatement.children()) {
2216+
for (Node varName : varDeclaration.children()) {
21362217
if (varName.hasChildren()) {
21372218
Node copiedVarName = varName.cloneNode().setJSDocInfo(null);
21382219
Node assign =
@@ -2142,22 +2223,15 @@ void visitVar(Node varStatement) {
21422223
assignments.add(assign);
21432224
}
21442225
}
2145-
if (assignments.isEmpty()) {
2146-
varStatement.detach();
2147-
} else {
2148-
Node commaExpression = null;
2149-
for (Node assignment : assignments) {
2150-
commaExpression =
2151-
commaExpression == null
2152-
? assignment
2153-
: withType(IR.comma(commaExpression, assignment), assignment.getJSType())
2154-
.useSourceInfoFrom(assignment);
2155-
}
2156-
varStatement.replaceWith(IR.exprResult(commaExpression));
2226+
Node commaExpression = null;
2227+
for (Node assignment : assignments) {
2228+
commaExpression =
2229+
commaExpression == null
2230+
? assignment
2231+
: withType(IR.comma(commaExpression, assignment), assignment.getJSType())
2232+
.useSourceInfoFrom(assignment);
21572233
}
2158-
// Place original var statement with initial values removed to just before
2159-
// the program method definition.
2160-
hoistNode(varStatement);
2234+
return commaExpression;
21612235
}
21622236
}
21632237

‎test/com/google/javascript/jscomp/Es6RewriteGeneratorsTest.java

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,37 @@ public void testSimpleGenerator() {
226226
"return $jscomp$generator$context.yield(i, 0);"));
227227
}
228228

229+
public void testForLoopWithExtraVarDeclaration() {
230+
test(
231+
lines(
232+
"function *gen() {",
233+
" var i = 2;",
234+
" yield i;",
235+
" use(i);",
236+
" for (var i = 0; i < 3; i++) {",
237+
" use(i);",
238+
" }",
239+
"}"),
240+
lines(
241+
"function gen(){",
242+
" var i;",
243+
// TODO(bradfordcsmith): avoid duplicate var declarations
244+
// It does no real harm at the moment since the normalize pass will clean this up later.
245+
" var i;",
246+
" return $jscomp.generator.createGenerator(",
247+
" gen,",
248+
" function($jscomp$generator$context) {",
249+
" if ($jscomp$generator$context.nextAddress==1) {",
250+
" i=2;",
251+
" return $jscomp$generator$context.yield(i,2);",
252+
" }",
253+
" use(i);",
254+
" for (i = 0; i < 3 ; i++) use(i);",
255+
" $jscomp$generator$context.jumpToEnd();",
256+
" })",
257+
"}"));
258+
}
259+
229260
public void testUnreachableCodeGeneration() {
230261
rewriteGeneratorBody(
231262
"if (i) return 1; else return 2;",
@@ -278,28 +309,39 @@ public void testNestedGenerator() {
278309
public void testForLoops() {
279310
rewriteGeneratorBodyWithVars(
280311
"var i = 0; for (var j = 0; j < 10; j++) { i += j; }",
281-
"var i;",
312+
"var i; var j;",
282313
lines(
283314
"i = 0;",
284-
"for (var j = 0; j < 10; j++) { i += j; }",
315+
"for (j = 0; j < 10; j++) { i += j; }",
285316
"$jscomp$generator$context.jumpToEnd();"));
286317

287318
rewriteGeneratorBodyWithVars(
288319
"var i = 0; for (var j = yield; j < 10; j++) { i += j; }",
289-
"var i;",
320+
"var i; var j;",
290321
lines(
291322
"if ($jscomp$generator$context.nextAddress == 1) {",
292323
" i = 0;",
293324
" return $jscomp$generator$context.yield(undefined, 2);",
294325
"}",
295-
"for (var j = $jscomp$generator$context.yieldResult; j < 10; j++) { i += j; }",
326+
"for (j = $jscomp$generator$context.yieldResult; j < 10; j++) { i += j; }",
296327
"$jscomp$generator$context.jumpToEnd();"));
297328

298329
rewriteGeneratorBody(
299330
"for (;;) { yield 1; }",
300331
lines(
301332
" return $jscomp$generator$context.yield(1, 1);"));
302333

334+
rewriteGeneratorBodyWithVars(
335+
"for (var yieldResult; yieldResult === undefined; yieldResult = yield 1) {}",
336+
"var yieldResult;",
337+
lines(
338+
" if ($jscomp$generator$context.nextAddress == 1) {",
339+
" if (!(yieldResult === undefined)) return $jscomp$generator$context.jumpTo(0);",
340+
" return $jscomp$generator$context.yield(1,5);",
341+
" }",
342+
" yieldResult = $jscomp$generator$context.yieldResult;",
343+
" return $jscomp$generator$context.jumpTo(1);"));
344+
303345
rewriteGeneratorBodyWithVars(
304346
"for (var j = 0; j < 10; j++) { yield j; }",
305347
"var j;",
@@ -1043,13 +1085,14 @@ public void testNoTranslate() {
10431085
}
10441086

10451087
public void testForIn() {
1046-
rewriteGeneratorBody(
1088+
rewriteGeneratorBodyWithVars(
10471089
"for (var i in yield) { }",
1090+
"var i;",
10481091
lines(
10491092
"if ($jscomp$generator$context.nextAddress == 1) {",
10501093
" return $jscomp$generator$context.yield(undefined, 2);",
10511094
"}",
1052-
"for (var i in $jscomp$generator$context.yieldResult) { }",
1095+
"for (i in $jscomp$generator$context.yieldResult) { }",
10531096
"$jscomp$generator$context.jumpToEnd();"));
10541097

10551098

0 commit comments

Comments
 (0)