Skip to content

Commit

Permalink
Better support for let/const in FlowSensitiveInlineVariables
Browse files Browse the repository at this point in the history 
Also adds a unit test demonstrating incorrect behavior with block scopes: the pass sometimes moves let/const variables out of the scope in which they are defined.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=185917144
  • Loading branch information
@lauraharker @brad4d
lauraharker authored and brad4d committed Feb 16, 2018
1 parent ec2d978 commit bd0a38b
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 5 deletions.
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java
View file
Expand Up @@ -258,7 +258,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {

// Make sure that the name node is purely a read.
if ((NodeUtil.isAssignmentOp(parent) && parent.getFirstChild() == n)
|| parent.isVar()
|| NodeUtil.isNameDeclaration(parent)
|| parent.isInc()
|| parent.isDec()
|| parent.isParamList()
Expand Down
11 changes: 9 additions & 2 deletions src/com/google/javascript/jscomp/MaybeReachingVariableUse.java
View file
Expand Up @@ -207,14 +207,20 @@ private void computeMayUse(
return;

case FOR_IN:
case FOR_OF:
// for(x in y) {...}
Node lhs = n.getFirstChild();
Node rhs = lhs.getNext();
if (lhs.isVar()) {
if (NodeUtil.isNameDeclaration(lhs)) {
lhs = lhs.getLastChild(); // for(var x in y) {...}
if (lhs.isDestructuringLhs()) {
lhs = lhs.getFirstChild(); // for (let [x] of obj) {...}
}
}
if (lhs.isName() && !conditional) {
removeFromUseIfLocal(lhs.getString(), output);
} else if (lhs.isDestructuringPattern()) {
computeMayUse(lhs, cfgNode, output, true);
}
computeMayUse(rhs, cfgNode, output, conditional);
return;
Expand All @@ -232,7 +238,8 @@ private void computeMayUse(
return;

case VAR:
// TODO(b/73123594): this should also handle LET/CONST
case LET:
case CONST:
Node varName = n.getFirstChild();
checkState(n.hasChildren(), "AST should be normalized", n);

Expand Down
4 changes: 3 additions & 1 deletion src/com/google/javascript/jscomp/MustBeReachingVariableDef.java
View file
Expand Up @@ -258,13 +258,15 @@ private void computeMustDef(
return;

case FOR_IN:
case FOR_OF:
// for(x in y) {...}
Node lhs = n.getFirstChild();
Node rhs = lhs.getNext();
if (lhs.isVar()) {
if (NodeUtil.isNameDeclaration(lhs)) {
lhs = lhs.getLastChild(); // for(var x in y) {...}
}
if (lhs.isName()) {
// TODO(lharker): This doesn't seem right - given for (x in y), the value set to x isn't y
addToDefIfLocal(lhs.getString(), cfgNode, rhs, output);
}
return;
Expand Down
58 changes: 57 additions & 1 deletion test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java
View file
Expand Up @@ -68,6 +68,19 @@ public void testSimpleVar() {
inline("var x = 1; x = x + 1", "var x; x = 1 + 1");
}

public void testSimpleLet() {
inline("let x = 1; print(x)", "let x; print(1)");
inline("let x = 1; x", "let x; 1");
inline("let x = 1; let a = x", "let x; let a = 1");
inline("let x = 1; x = x + 1", "let x; x = 1 + 1");
}

public void testSimpleConst() {
inline("const x = 1; print(x)", "const x = undefined; print(1)");
inline("const x = 1; x", "const x = undefined; 1");
inline("const x = 1; const a = x", "const x = undefined; const a = 1");
}

public void testSimpleForIn() {
inline("var a,b,x = a in b; x",
"var a,b,x; a in b");
Expand Down Expand Up @@ -477,7 +490,15 @@ public void testForIn() {
noInline("var x; var y = {}; for(x in y){}");
noInline("var x; var y = {}; var z; for(x in z = y){print(z)}");
noInline("var x; var y = {}; var z; for(x in y){print(z)}");
}

public void testForInDestructuring() {
noInline("var x = 1, y = [], z; for ({z = x} in y) {}");
noInline("var x = 1, y = [], z; for ([z = x] in y) {}");
noInline("var x = 1, y = [], z; print(x); for ({z = x} in y) {}");
noInline("var x = 1, y = [], z; print(x); for ([z = x] in y) {}");
noInline("var x = 1, y = [], z; print(x); for (let {z = x} in y) {}");
noInline("var x = 1, y = [], z; print(x); for (const {z = x} in y) {}");
}

public void testNotOkToSkipCheckPathBetweenNodes() {
Expand Down Expand Up @@ -568,7 +589,7 @@ public void testIssue794b() {
"return x;");
}

public void testVarAssinInsideHookIssue965() {
public void testVarAssignInsideHookIssue965() {
noInline("var i = 0; return 1 ? (i = 5) : 0, i;");
noInline("var i = 0; return (1 ? (i = 5) : 0) ? i : 0;");
noInline("var i = 0; return (1 ? (i = 5) : 0) || i;");
Expand Down Expand Up @@ -706,6 +727,30 @@ public void testBlockScoping() {
"return(a)"));
}

public void testBlockScoping_shouldntInline() {
// TODO(b/73123594): fix this test. It adds a reference to a block-scoped let outside its block.
inline(
lines(
"var JSCompiler_inline_result;",
"{",
" let a = 1;",
" if (3 < 4) {",
" a = 2;",
" }",
" JSCompiler_inline_result = a;",
"}",
"alert(JSCompiler_inline_result);"),
lines(
"var JSCompiler_inline_result;",
"{",
" let a = 1;",
" if (3 < 4) {",
" a = 2;",
" }",
"}",
"alert(a);")); // a is not defined here! This is wrong.
}

public void testInlineInGenerators() {
test(
lines(
Expand All @@ -729,6 +774,17 @@ public void testNoInlineForOf() {
noInline("var x = 1; var n = {}; for(x of n) {}");
}

public void testForOfDestructuring() {
noInline("var x = 1, y = [], z; for ({z = x} of y) {}");
noInline("var x = 1, y = [], z; for ([z = x] of y) {}");

noInline("var x = 1, y = [], z; print(x); for ({z = x} of y) {}");
noInline("var x = 1, y = [], z; print(x); for ([z = x] of y) {}");

noInline("var x = 1, y = [], z; print(x); for (let [z = x] of y) {}");
noInline("var x = 1, y = [], z; print(x); for (const [z = x] of y) {}");
}

public void testTemplateStrings() {
inline("var name = 'Foo'; `Hello ${name}`",
"var name; `Hello ${'Foo'}`");
Expand Down
14 changes: 14 additions & 0 deletions test/com/google/javascript/jscomp/MaybeReachingVariableUseTest.java
View file
Expand Up @@ -110,6 +110,20 @@ public void testForIn() {
assertNotMatch("D: var x = [], foo; U: for (x in foo) { }");
assertNotMatch("D: var x = [], foo; for (x in foo) { U:x }");
assertMatch("var x = [], foo; D: for (x in foo) { U:x }");
assertMatch("var foo; D: for (let x in foo) { U:x }");
assertMatch("var foo; D: for (const x in foo) { U:x }");
assertMatch("D: var x = 1, foo; U: x; U: for (let [z = x] in foo) {}");
assertMatch("D: var x = 1, foo; U: x; for (let [x] in foo) {}");
}

public void testForOf() {
assertNotMatch("D: var x = [], foo; U: for (x of foo) { }");
assertNotMatch("D: var x = [], foo; for (x of foo) { U:x }");
assertMatch("var x = [], foo; D: for (x of foo) { U:x }");
assertMatch("var foo; D: for (let x of foo) { U:x }");
assertMatch("var foo; D: for (const x of foo) { U:x }");
assertMatch("D: var x = 1, foo; U: x; U: for (let [z = x] of foo) {}");
assertMatch("D: var x = 1, foo; U: x; for (let [x] of foo) {}");
}

public void testTryCatch() {
Expand Down

0 comments on commit bd0a38b

Please sign in to comment.