Skip to content

Commit

Permalink
Fix ES6RewriteBlockScopedDeclaration to generate normalized FOR loop …
Browse files Browse the repository at this point in the history 
…initializer

The pass currently generates var in the initializer part of FOR loops. This makes the AST unnormalized. Fixing this to preserve normalization brings us closer to moving the pass post normalize.

PiperOrigin-RevId: 565963249
  • Loading branch information
@rishipal @copybara-github
rishipal authored and copybara-github committed Sep 16, 2023
1 parent ec5741d commit 5e53a60
Show file tree
Hide file tree
Showing 2 changed files with 178 additions and 19 deletions.
105 changes: 90 additions & 15 deletions src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.Map;
Expand Down Expand Up @@ -154,36 +153,109 @@ private static void maybeAddConstJSDoc(Node srcDeclaration, Node srcName, Node d
}
}

/**
* Given a declarationList of let/const declarations, convert all declared names to their own,
* separate (normalized) var declarations.
*
* <p>"const i = 0, j = 0;" becomes "/\*\* @const *\/ var i = 0; /\** @const *\/ var j = 0;"
*
* <p>If the declarationList of let/const declarations is in a FOR intializer, moves those into
* separate declarations outside the FOR loop (for maintaining normalization).
*
* <p>TODO: b/197349249 - We won't have any declaration lists here when this pass runs post
* normalize.
*/
private void handleDeclarationList(Node declarationList, Node parent) {
// Normalize: "const i = 0, j = 0;" becomes "/** @const */ var i = 0; /** @const */ var j = 0;"
while (declarationList.hasMoreThanOneChild()) {
if (declarationList.isVar() && declarationList.hasOneChild()) {
// This declaration list is already handled and we can safely return. This happens because
// this method is also called by {@code replaceDeclarationWithProperty} where it gets
// repeatedly called for each name in the declarationList. After the first call
// to this (for the first name), this declarationList would no longer be {@code Token.CONST}
// (i.e. it would've become a separate var with an {@code /** const */} annotation).
return;
}
if (parent.isVanillaFor()) {
handleDeclarationListInForInitializer(declarationList, parent);
return;
}
// convert all names to their own, separate (normalized) declarations
while (declarationList.hasChildren()) {
Node name = declarationList.getLastChild();
Node newDeclaration = IR.var(name.detach()).srcref(declarationList);
/*
* This method gets called multiple times by {@code replaceDeclarationWithProperty}. In the
* first call, it splits the declarationList into individual /\** @const *\/ var declarations.
* i.e. `const a,b` --> `/\** @const *\/ var a; /\** @const *\/ var b;`
*
* <p>Then it gets invoked for each individual var declaration. once for each name in the
* original declarationList). However, after the first call to this (for the first name), this
* declarationList would no longer be {@code Token.CONST} (i.e. it would've become a separate
* var with an {@code /*\* const *\/} annotation).
*
* <p>Previously, this method rewrote those individual var declarations again into new var
* declarations. Without copying over the JSDoc, those newly created var declarations did not
* contain `@const`, and produce this:
*
* <p>`/\** @const *\/ var a; /\** @const *\/ var b;` -->`var a; var b;`
*
* <p>Now, even though the "redundant rewriting" is handled by early-returning from this
* method whenever this method gets called with a non-declaration var list, it's still
* important that the individual var declarations created by the first rewriting contain the
* annotations from original declaration list. Hence we propagate JSDoc from declarationList
* into the individual var declarations.
*/
extractInlineJSDoc(declarationList, name, newDeclaration);
maybeAddConstJSDoc(declarationList, name, newDeclaration);
newDeclaration.insertAfter(declarationList);
compiler.reportChangeToEnclosingScope(parent);
}
maybeAddConstJSDoc(declarationList, declarationList.getFirstChild(), declarationList);
declarationList.setToken(Token.VAR);

// declarationList has no children left. Remove.
declarationList.detach();
compiler.reportChangeToEnclosingScope(parent);
}

private void handleDeclarationListInForInitializer(Node declarationList, Node parent) {
checkState(parent.isVanillaFor());
// if the declarationList is in a FOR initializer, move it outside
Node insertSpot = getInsertSpotBeforeLoop(parent);
// convert all names to their own, separate (normalized) declarations
while (declarationList.hasChildren()) {
Node name = declarationList.getLastChild();
Node newDeclaration = IR.var(name.detach()).srcref(declarationList);
extractInlineJSDoc(declarationList, name, newDeclaration);
maybeAddConstJSDoc(declarationList, name, newDeclaration);
// generate normalized var initializer (i.e. outside FOR)
newDeclaration.insertBefore(insertSpot);
insertSpot = newDeclaration;
compiler.reportChangeToEnclosingScope(parent);
}
// make FOR initializer empty
Node empty = astFactory.createEmpty().srcref(declarationList);
declarationList.replaceWith(empty);
compiler.reportChangeToEnclosingScope(empty);
}

private void addNodeBeforeLoop(Node newNode, Node loopNode) {
Node insertSpot = getInsertSpotBeforeLoop(loopNode);
newNode.insertBefore(insertSpot);
compiler.reportChangeToEnclosingScope(newNode);
}

private Node getInsertSpotBeforeLoop(Node loopNode) {
Node insertSpot = loopNode;
while (insertSpot.getParent().isLabel()) {
insertSpot = insertSpot.getParent();
}
newNode.insertBefore(insertSpot);
compiler.reportChangeToEnclosingScope(newNode);
return insertSpot;
}

private void rewriteDeclsToVars() {
if (!letConsts.isEmpty()) {
for (Node n : letConsts) {
if (n.isConst()) {
handleDeclarationList(n, n.getParent());
}
n.setToken(Token.VAR);
compiler.reportChangeToEnclosingScope(n);
// for both lets and consts we want to split the declaration lists when converting them to
// vars (to maintain normalization)
handleDeclarationList(n, n.getParent());
}
}
}
Expand Down Expand Up @@ -416,11 +488,14 @@ private void replaceDeclarationWithProperty(
LoopObject loopObject, String newPropertyName, Node reference) {
Node declaration = reference.getParent();
Node grandParent = declaration.getParent();
// If the declaration contains multiple declared variables, split it apart.
handleDeclarationList(declaration, grandParent);
// Record that the let / const declaration statement has been turned into one or more
// Record that the let / const declaration statement will get turned into one or more
// var statements by handleDeclarationList(), so we won't try to change it again later.
letConsts.remove(declaration);
// If the declaration contains multiple declared variables, split it apart.
// NOTE: This call could be made for each declarationList once, rather than each name in that
// list
handleDeclarationList(declaration, grandParent);

// The variable we're working with may have been moved to a new var statement.
declaration = reference.getParent();
if (reference.hasChildren()) {
Expand Down
92 changes: 88 additions & 4 deletions test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,9 @@ public void testLetShadowingTranspileOnly() {
@Test
public void testLetShadowingWithMultivariateDeclaration() {
Sources srcs = srcs("var x, y; for (let x, y;;) {}");
// normalized var decls outside the for loop
Expected expected =
expected("var x, y; for (var x$jscomp$1 = void 0, y$jscomp$1 = void 0;;) {}");
expected("var x, y; var x$jscomp$1 = void 0; var y$jscomp$1 = void 0; for (;;) {}");
test(srcs, expected);
}

Expand Down Expand Up @@ -392,6 +393,88 @@ public void testRenameConflict() {
test(srcs, expected);
}

@Test
public void testForLetInitializer_getsNormalized() {
Sources srcs =
srcs(
lines(
"function f() {",
" return function foo() {",
" for (let i = 0; i < 10; i++) {",
" }",
" };",
"}"));

Expected expected =
expected(
lines(
"function f() {",
" return function foo() {",
" var i = 0;", // normalized
" for (; i < 10; i++) {",
" }",
" };",
"}"));

test(srcs, expected);
}

@Test
public void testForLetInitializer_declarationList_getsNormalized() {
Sources srcs =
srcs(
lines(
"function f() {",
" return function foo() {",
" for (let i = 0, y = 0; i < 10; i++) {",
" }",
" };",
"}"));

Expected expected =
expected(
lines(
"function f() {",
" return function foo() {",
" var i = 0; var y=0;", // normalized
" for (; i < 10; i++) {",
" }",
" };",
"}"));

test(srcs, expected);
}

@Test
public void testsForVarInitializer_staysNormalized() {
Sources srcs =
srcs(
lines(
"function f() {",
" return function foo() {",
" var i = 0;",
" for ( ;i < 10; i++) {", // originally normalized
" }",
" };",
"}"));

testSame(srcs);
}

@Test
public void testsForVarInitializer_unchanged() {
Sources srcs =
srcs(
lines(
"function f() {",
" return function foo() {",
" for (var i = 0; i < 10; i++) {", // originally unnormalized
" }",
" };",
"}"));
testSame(srcs);
}

@Test
public void testForLoop() {
Sources srcs =
Expand All @@ -412,7 +495,8 @@ public void testForLoop() {
"function use(x) {}",
"function f() {",
" /** @const */ var y = 0;",
" for (var x$jscomp$1 = 0; x$jscomp$1 < 10; x$jscomp$1++) {",
" var x$jscomp$1 = 0;",
" for (; x$jscomp$1 < 10; x$jscomp$1++) {",
" /** @const */ var y$jscomp$1 = x$jscomp$1 * 2;",
" /** @const */ var z = y$jscomp$1;",
" }",
Expand Down Expand Up @@ -445,11 +529,11 @@ public void testForLoop() {
loopClosureTest(srcs, expected);

srcs = srcs("for (let i = 0;;) { let i; }");
expected = expected("for (var i = 0;;) { var i$jscomp$1 = void 0; }");
expected = expected("var i = 0; for (;;) { var i$jscomp$1 = void 0; }");
loopClosureTest(srcs, expected);

srcs = srcs("for (let i = 0;;) {} let i;");
expected = expected("for (var i$jscomp$1 = 0;;) {} var i;");
expected = expected("var i$jscomp$1 = 0; for (;;) {} var i;");
loopClosureTest(srcs, expected);

test(
Expand Down

0 comments on commit 5e53a60

Please sign in to comment.