Skip to content

Commit

Permalink
When converting let/const statements to var statements, convert destr…
Browse files Browse the repository at this point in the history
…ucturing too, rather than just single-name declarations.

This fixes some (but not all) of the issues we've seen with CoalesceVariableNames running on ES6 code.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=169167128
  • Loading branch information
tbreisacher authored and blickly committed Sep 19, 2017
1 parent f12f10d commit ad1dd6f
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 9 deletions.
21 changes: 12 additions & 9 deletions src/com/google/javascript/jscomp/CoalesceVariableNames.java
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,8 @@ public void visit(NodeTraversal t, Node n, Node parent) {
n.setString(coalescedVar.name);
compiler.reportChangeToEnclosingScope(n);

if (NodeUtil.isNameDeclaration(parent)) {
if (NodeUtil.isNameDeclaration(parent)
|| NodeUtil.getEnclosingType(n, Token.DESTRUCTURING_LHS) != null) {
makeDeclarationVar(coalescedVar);
removeVarDeclaration(n);
}
Expand Down Expand Up @@ -233,7 +234,9 @@ public void visit(NodeTraversal t, Node n, Node parent) {
n.setString(pseudoName);
compiler.reportChangeToEnclosingScope(n);

if (!vNode.getValue().equals(coalescedVar) && NodeUtil.isNameDeclaration(parent)) {
if (!vNode.getValue().equals(coalescedVar)
&& (NodeUtil.isNameDeclaration(parent)
|| NodeUtil.getEnclosingType(n, Token.DESTRUCTURING_LHS) != null)) {
makeDeclarationVar(coalescedVar);
removeVarDeclaration(n);
}
Expand Down Expand Up @@ -396,12 +399,12 @@ boolean connectIfCrossed(UndiGraph<Var, Void> interferenceGraph) {
* Tries to remove variable declaration if the variable has been coalesced with another variable
* that has already been declared. Any lets or consts are redeclared as vars because at this point
* in the compilation, the code is normalized, so we can safely hoist variables without worrying
* about shaddowing.
* about shadowing.
*
* @param name name node of the variable being coalesced
*/
private static void removeVarDeclaration(Node name) {
Node var = name.getParent();
Node var = NodeUtil.getEnclosingNode(name, NodeUtil.isNameDeclaration);
Node parent = var.getParent();

if (!var.isVar()) {
Expand Down Expand Up @@ -443,11 +446,11 @@ private static void removeVarDeclaration(Node name) {
* Because the code has already been normalized by the time this pass runs, we can safely
* redeclare any let and const coalesced variables as vars
*/
public static void makeDeclarationVar(Var coalescedName) {
Node coalesceVarParent = coalescedName.getParentNode();
if (coalesceVarParent.isLet() || coalesceVarParent.isConst()) {
coalesceVarParent.setToken(Token.VAR);
checkState(coalesceVarParent.isVar());
private static void makeDeclarationVar(Var coalescedName) {
if (coalescedName.isLet() || coalescedName.isConst()) {
Node declNode =
NodeUtil.getEnclosingNode(coalescedName.getParentNode(), NodeUtil.isNameDeclaration);
declNode.setToken(Token.VAR);
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/com/google/javascript/jscomp/NodeUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -2363,6 +2363,13 @@ public static boolean isNameDeclaration(Node n) {
return n != null && (n.isVar() || n.isLet() || n.isConst());
}

static final Predicate<Node> isNameDeclaration = new Predicate<Node>() {
@Override
public boolean apply(Node n) {
return isNameDeclaration(n);
}
};

/**
* @param n The node
* @return True if {@code n} is a VAR, LET or CONST that contains a
Expand Down
30 changes: 30 additions & 0 deletions test/com/google/javascript/jscomp/CoalesceVariableNamesTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,36 @@ public void testFunctionNameReuse() {
inFunction("function x() {} var y = 1; y; x = 1; x");
}

public void testBug65688660() {
test(
LINE_JOINER.join(
"function f(param) {",
" if (true) {",
" const b1 = [];",
" for (const [key, value] of []) {}",
" }",
" if (true) {",
" const b2 = [];",
" for (const kv of []) {",
" const key2 = kv.key;",
" }",
" }",
"}"),
LINE_JOINER.join(
"function f(param) {",
" if (true) {",
" param = [];",
" for (var [key, value] of []) {}",
" }",
" if (true) {",
" key = [];",
" for (const kv of []) {",
" key = kv.key;",
" }",
" }",
"}"));
}

public void testBug1401831() {
// Verify that we don't wrongly merge "opt_a2" and "i" without considering
// arguments[0] aliasing it.
Expand Down

0 comments on commit ad1dd6f

Please sign in to comment.