Skip to content

Commit

Permalink
Handle multivariate let/const declaration renaming during transpilation
Browse files Browse the repository at this point in the history 
Note: The Es6SplitVariableDeclarations pass normalizes multivariate declarations with at least one destructuring pattern. I didn't just add let/const declaration normalization to that pass because it doesn't handle declarations outside of a statement block. (e.g. in a for-loop header or label).

Fixes #2969

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=200105402
  • Loading branch information
@lauraharker @blickly
lauraharker authored and blickly committed Jun 11, 2018
1 parent c09332c commit b3146ab
Show file tree
Hide file tree
Showing 3 changed files with 149 additions and 75 deletions.
75 changes: 46 additions & 29 deletions src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java
View file
Expand Up @@ -81,42 +81,20 @@ public void visit(NodeTraversal t, Node n, Node parent) {
if (!n.hasChildren() || !NodeUtil.isBlockScopedDeclaration(n.getFirstChild())) {
return;
}

Scope scope = t.getScope();
Node nameNode = n.getFirstChild();
// NOTE: This pass depends on for-of being transpiled away before it runs.
checkState(parent == null || !parent.isForOf(), parent);
// NOTE: This pass depends on classes being transpiled away before it runs.
checkState(!n.isClass(), n);
if (!n.isFunction()
&& !nameNode.hasChildren()
&& (parent == null || !parent.isForIn())
&& !n.isCatch()
&& inLoop(n)) {
Node undefined = createUndefinedNode().srcref(nameNode);
nameNode.addChildToFront(undefined);
compiler.reportChangeToEnclosingScope(undefined);
}

String oldName = nameNode.getString();
if (n.isLet() || n.isConst()) {
letConsts.add(n);
}
Scope hoistScope = scope.getClosestHoistScope();
if (scope != hoistScope) {
String newName = oldName;
if (hoistScope.hasSlot(oldName) || undeclaredNames.contains(oldName)) {
do {
newName = oldName + "$" + compiler.getUniqueNameIdSupplier().get();
} while (hoistScope.hasSlot(newName));
nameNode.setString(newName);
compiler.reportChangeToEnclosingScope(nameNode);
Node scopeRoot = scope.getRootNode();
renameTable.put(scopeRoot, oldName, newName);
if (NodeUtil.isNameDeclaration(n)) {
for (Node nameNode : n.children()) {
visitBlockScopedName(t, n, nameNode);
}
Var oldVar = scope.getVar(oldName);
scope.undeclare(oldVar);
hoistScope.declare(newName, nameNode, oldVar.input);
} else {
// NOTE: This pass depends on class declarations having been transpiled away
checkState(n.isFunction() || n.isCatch(), "Unexpected declaration node: %s", n);
visitBlockScopedName(t, n, n.getFirstChild());
}
}

Expand Down Expand Up @@ -152,6 +130,44 @@ private boolean getShouldAddTypesOnNewAstNodes() {
return compiler.hasTypeCheckingRun();
}

/**
* Renames block-scoped declarations that shadow a variable in an outer scope
*
* <p>Also normalizes declarations with no initializer in a loop to be initialized to undefined.
*/
private void visitBlockScopedName(NodeTraversal t, Node decl, Node nameNode) {
Scope scope = t.getScope();
Node parent = decl.getParent();
// Normalize "let x;" to "let x = undefined;" if in a loop, since we later convert x
// to be $jscomp$loop$0.x and want to reset the property to undefined every loop iteration.
if ((decl.isLet() || decl.isConst())
&& !nameNode.hasChildren()
&& (parent == null || !parent.isForIn())
&& inLoop(decl)) {
Node undefined = createUndefinedNode().srcref(nameNode);
nameNode.addChildToFront(undefined);
compiler.reportChangeToEnclosingScope(undefined);
}

String oldName = nameNode.getString();
Scope hoistScope = scope.getClosestHoistScope();
if (scope != hoistScope) {
String newName = oldName;
if (hoistScope.hasSlot(oldName) || undeclaredNames.contains(oldName)) {
do {
newName = oldName + "$" + compiler.getUniqueNameIdSupplier().get();
} while (hoistScope.hasSlot(newName));
nameNode.setString(newName);
compiler.reportChangeToEnclosingScope(nameNode);
Node scopeRoot = scope.getRootNode();
renameTable.put(scopeRoot, oldName, newName);
}
Var oldVar = scope.getVar(oldName);
scope.undeclare(oldVar);
hoistScope.declare(newName, nameNode, oldVar.input);
}
}

/**
* Whether n is inside a loop. If n is inside a function which is inside a loop, we do not
* consider it to be inside a loop.
Expand Down Expand Up @@ -184,6 +200,7 @@ private static void maybeAddConstJSDoc(Node srcDeclaration, Node srcParent, Node
Node destDeclaration) {
if (srcDeclaration.isConst()
// Don't add @const for the left side of a for/in. If we do we get warnings from the NTI.
// TODO(lharker): Check if this condition is still necessary, since NTI is deleted
&& !(srcParent.isForIn() && srcDeclaration == srcParent.getFirstChild())) {
extractInlineJSDoc(srcDeclaration, srcName, destDeclaration);
JSDocInfoBuilder builder = JSDocInfoBuilder.maybeCopyFrom(destDeclaration.getJSDocInfo());
Expand Down
119 changes: 73 additions & 46 deletions test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java
View file
Expand Up @@ -156,6 +156,29 @@ public void testLetShadowing() {
"}"));
}

public void testLetShadowingWithMultivariateDeclaration() {
test(
lines(
"{", // preserve newline
" let x, y;",
"}",
"{",
" let x, y;",
" alert(y);",
"}"),
lines(
"{", // preserve newline
" var x, y;",
"}",
"{",
" var x$0, y$1;",
" alert(y$1);",
"}"));
test(
"var x, y; for (let x, y;;) {}",
"var x, y; for (var x$0 = undefined, y$1 = undefined;;) {}");
}

public void testNonUniqueLet() {
test(
lines(
Expand Down Expand Up @@ -818,15 +841,15 @@ public void testLoopClosureCommaInBody() {
lines(
"/** @const */ var arr = [];",
"var j = 0;",
"var $jscomp$loop$1 = {};",
"var $jscomp$loop$2 = {};",
"var i = 0;",
"for (; i < 10; $jscomp$loop$1 = {i$0: $jscomp$loop$1.i$0,",
" j: $jscomp$loop$1.j}, i++) {",
" $jscomp$loop$1.i$0 = undefined;",
" $jscomp$loop$1.j = 0;",
" arr.push((function($jscomp$loop$1) {",
" return function() { return $jscomp$loop$1.i$0 + $jscomp$loop$1.j; };",
" })($jscomp$loop$1));",
"for (; i < 10; $jscomp$loop$2 = {i$0: $jscomp$loop$2.i$0,",
" j$1: $jscomp$loop$2.j$1}, i++) {",
" $jscomp$loop$2.i$0 = undefined;",
" $jscomp$loop$2.j$1 = 0;",
" arr.push((function($jscomp$loop$2) {",
" return function() { return $jscomp$loop$2.i$0 + $jscomp$loop$2.j$1; };",
" })($jscomp$loop$2));",
"}"));
}

Expand Down Expand Up @@ -1242,47 +1265,51 @@ public void testFunctionsInLoop() {

// https://github.com/google/closure-compiler/issues/1557
public void testNormalizeDeclarations() {
test(lines(
"while(true) {",
" let x, y;",
" let f = function() {",
" x = 1;",
" y = 2;",
" }",
"}"),
test(
lines(
"var $jscomp$loop$0 = {};",
"while(true) {",
" $jscomp$loop$0.x = undefined;",
" var f = function($jscomp$loop$0) {",
" return function() {",
" $jscomp$loop$0.x = 1;",
" $jscomp$loop$0.y = 2;",
" }",
" }($jscomp$loop$0);",
" $jscomp$loop$0 = {x: $jscomp$loop$0.x, y: $jscomp$loop$0.y};",
"}"));
"while(true) {",
" let x, y;",
" let f = function() {",
" x = 1;",
" y = 2;",
" }",
"}"),
lines(
"var $jscomp$loop$0 = {};",
"while(true) {",
" $jscomp$loop$0.x = undefined;",
" $jscomp$loop$0.y = undefined;",
" var f = function($jscomp$loop$0) {",
" return function() {",
" $jscomp$loop$0.x = 1;",
" $jscomp$loop$0.y = 2;",
" }",
" }($jscomp$loop$0);",
" $jscomp$loop$0 = {x: $jscomp$loop$0.x, y: $jscomp$loop$0.y};",
"}"));

test(lines(
"while(true) {",
" let x, y;",
" let f = function() {",
" y = 2;",
" x = 1;",
" }",
"}"),
test(
lines(
"var $jscomp$loop$0 = {};",
"while(true) {",
" $jscomp$loop$0.x = undefined;",
" var f = function($jscomp$loop$0) {",
" return function() {",
" $jscomp$loop$0.y = 2;",
" $jscomp$loop$0.x = 1;",
" }",
" }($jscomp$loop$0);",
" $jscomp$loop$0 = {y: $jscomp$loop$0.y, x: $jscomp$loop$0.x};",
"}"));
"while(true) {",
" let x, y;",
" let f = function() {",
" y = 2;",
" x = 1;",
" }",
"}"),
lines(
"var $jscomp$loop$0 = {};",
"while(true) {",
" $jscomp$loop$0.x = undefined;",
" $jscomp$loop$0.y = undefined;",
" var f = function($jscomp$loop$0) {",
" return function() {",
" $jscomp$loop$0.y = 2;",
" $jscomp$loop$0.x = 1;",
" }",
" }($jscomp$loop$0);",
" $jscomp$loop$0 = {y: $jscomp$loop$0.y, x: $jscomp$loop$0.x};",
"}"));
}

public void testTypeAnnotationsOnLetConst() {
Expand Down
30 changes: 30 additions & 0 deletions test/com/google/javascript/jscomp/runtime_tests/let_const_rename_test.js
View file
Expand Up @@ -386,3 +386,33 @@ function testNestedContinueDoesNotBreakClosures() {
// i skipped because it was after h and in the same word
assertEquals('bcdefgjkl', letters);
}

function testLetWithSameName_multivariateDeclaration() {
// See https://github.com/google/closure-compiler/issues/2969
{
let x = 1, y = 2;
assertEquals(1, x);
assertEquals(2, y);
}
{
let x = 3, y = 4;
assertEquals(3, x);
assertEquals(4, y);
}
}

function testLetInLoopClosure_multivariateDeclaration() {
for (let i = 0; i < 3; i++) {
let x, y;
// Verify that x and y are always undefined here, even though they are
// reassigned later in the loop.
assertUndefined(x);
assertUndefined(y);
function f() {
x = y = 3;
}
f();
assertEquals(3, x);
assertEquals(3, y);
}
}

0 comments on commit b3146ab

Please sign in to comment.