Skip to content

Commit

Permalink
Back off less on conditional let/const declarations in LiveVariablesA…
Browse files Browse the repository at this point in the history 
…nalysis

Given a conditionally executed var statement (e.g. for (var x of someArr) {}), it is possible that 'x' was previously assigned to, so 'x' is possibly live prior to the for loop. Normalize does not guarantee that a 'var' is never assigned to before the first 'var' statement.

However, given a conditional let/const declaration, it's safe to say that the variable was 'dead' prior to its declaration because it cannot be assigned referenced in the temporal dead zone.

This improves optimizations during CoalesceVariableNames.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=228183336
  • Loading branch information
@lauraharker
lauraharker committed Jan 7, 2019
1 parent 157b3b7 commit 078c3c8
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 43 deletions.
33 changes: 18 additions & 15 deletions src/com/google/javascript/jscomp/LiveVariablesAnalysis.java
View file
Expand Up @@ -257,17 +257,15 @@ private void computeGenKill(Node n, BitSet gen, BitSet kill, boolean conditional
case FOR_OF:
case FOR_IN:
{
// for (x in y) {...}
Node lhs = n.getFirstChild();
if (NodeUtil.isNameDeclaration(lhs)) {
// for (var x in y) {...}
lhs = lhs.getLastChild();
}

// Note that the LHS may never be assigned to or evaluated, like in:
// for-in and for-of initializers are different than vanilla for loops because the
// initializer may never be assigned to/evaluated. For example, `x` is never written in:
// for (x in []) {}
// so should not be killed.
computeGenKill(lhs, gen, kill, conditional);
// So lvalues in the initializer are not 'killed' (with the exception of let/const)
Node lhs = n.getFirstChild();
if (!lhs.isName()) { // a simple name is an lvalue, ignore it
computeGenKill(lhs, gen, kill, /* conditional= */ true);
}

// rhs is executed only once so we don't go into it every loop.
return;
Expand All @@ -280,20 +278,25 @@ private void computeGenKill(Node n, BitSet gen, BitSet kill, boolean conditional
if (c.isName()) {
if (c.hasChildren()) {
computeGenKill(c.getFirstChild(), gen, kill, conditional);
if (!conditional) {
addToSetIfLocal(c, kill);
}
}
if ((!conditional && c.hasChildren()) || n.isLet() || n.isConst()) {
// treat all let/cont declarations as 'kills', since the variable was
// previously in the temporal dead zone.
// var statements are only kills if a) they are unconditional and b) there is a rhs
addToSetIfLocal(c, kill);
}
} else {
checkState(c.isDestructuringLhs(), c);
if (!conditional) {
if (!conditional || n.isLet() || n.isConst()) {
Iterable<Node> allVars = NodeUtil.findLhsNodesInNode(c);
for (Node lhsNode : allVars) {
addToSetIfLocal(lhsNode, kill);
}
}
computeGenKill(c.getFirstChild(), gen, kill, conditional);
computeGenKill(c.getSecondChild(), gen, kill, conditional);
if (c.hasTwoChildren()) {
computeGenKill(c.getSecondChild(), gen, kill, conditional);
}
}
}
return;
Expand All @@ -315,7 +318,7 @@ private void computeGenKill(Node n, BitSet gen, BitSet kill, boolean conditional
case NAME:
if (isArgumentsName(n)) {
markAllParametersEscaped();
} else if (!NodeUtil.isLhsByDestructuring(n)) {
} else if (!NodeUtil.isLhsByDestructuring(n)) {
// Only add names in destructuring patterns if they're not lvalues.
// e.g. "x" in "const {foo = x} = obj;"
addToSetIfLocal(n, gen);
Expand Down
7 changes: 3 additions & 4 deletions test/com/google/javascript/jscomp/CoalesceVariableNamesTest.java
View file
Expand Up @@ -331,8 +331,8 @@ public void testBug65688660() {
" }",
" if (true) {",
" param = [];",
" for (const kv of []) {",
" param = kv.key;",
" for (param of []) {",
" param = param.key;",
" }",
" }",
"}"));
Expand Down Expand Up @@ -525,8 +525,7 @@ public void testConstDestructuringDeclInForOf_dropsConst() {

@Test
public void testConstDestructuringInForOfCoalescedWithUseInBlock() {
// TODO(b/121276933): coalesce `x` and `y`
inFunction("var x = 1; for (let [y] of iter) { y }");
inFunction("var x = 1; for (let [y] of iter) { y }", "var x = 1; for ([x] of iter) { x }");
}

@Test
Expand Down
14 changes: 7 additions & 7 deletions test/com/google/javascript/jscomp/IntegrationTest.java
View file
Expand Up @@ -238,8 +238,8 @@ public void testBug65688660() {
" }",
" if (true) {",
" param = [];",
" for (const kv of []) {",
" param = kv.key;",
" for (param of []) {",
" param = param.key;",
" }",
" }",
"}"));
Expand Down Expand Up @@ -268,15 +268,15 @@ public void testBug65688660_pseudoNames() {
" }",
"}"),
LINE_JOINER.join(
"function f(b1_b2_key2_param) {",
"function f(b1_b2_key2_kv_param) {",
" if (true) {",
" b1_b2_key2_param = [];",
" b1_b2_key2_kv_param = [];",
" for (const [key, value] of []) {}",
" }",
" if (true) {",
" b1_b2_key2_param = [];",
" for (const kv of []) {",
" b1_b2_key2_param = kv.key;",
" b1_b2_key2_kv_param = [];",
" for (b1_b2_key2_kv_param of []) {",
" b1_b2_key2_kv_param = b1_b2_key2_kv_param.key;",
" }",
" }",
"}"));
Expand Down
82 changes: 65 additions & 17 deletions test/com/google/javascript/jscomp/LiveVariablesAnalysisTest.java
View file
Expand Up @@ -102,6 +102,23 @@ public void testConditions() {
assertLiveBeforeX("var a,b;a();X:while(a&&(a=b)){}a()", "a");
}

@Test
public void testConditionsWithLexicalDecl() {
// x is conditionally declared, then used
assertNotLiveBeforeDecl("try { let x = 0; x; } catch (e) {}", "x");
assertNotLiveBeforeDecl("var a; if (a) { const x = 0; x; }", "x");
assertLiveAfterDecl("var a; if (a) { let x = 0; x; }", "x");

// x is conditionally declared, but still unused
assertNotLiveBeforeDecl("var a; if (a) { let x = 0; }", "x");
assertNotLiveBeforeDecl("var a; if (a) { const x = 0; }", "x");
assertNotLiveAfterDecl("var a; if (a) { let x = 0; }", "x");

// x is reassigned after declaration; not live
assertNotLiveAfterDecl("var a; if (a) { let x = 0; x = 1; x; }", "x");
assertNotLiveBeforeX("var a; if (a) { let x = 0; X: x = 1; x; }", "x");
}

@Test
public void testArrays() {
assertLiveBeforeX("var a;X:a[1]", "a");
Expand Down Expand Up @@ -170,9 +187,11 @@ public void testForLoops() {

// It should be live within the loop even if it is not used.
assertLiveBeforeX("var a,b;for(a=0;a<9;a++){X:1}", "a");
assertLiveAfterX("var a,b;for(a in b){X:b};", "a");
// For-In should serve as a gen as well.
assertLiveBeforeX("var a,b; X:for(a in b){ }", "a");

assertNotLiveAfterX("var a,b;for(a in b){X:b};", "a");
assertLiveAfterX("var a,b;for(a in b){X:b}; a", "a");
assertNotLiveBeforeX("var a,b; X:for(a in b){ }", "a");
assertLiveBeforeX("var a,b; X:for(a in b){ } a", "a");

// "a in b" should kill "a" before it.
// Can't prove this unless we have branched backward DFA.
Expand All @@ -188,16 +207,36 @@ public void testForLoops() {

@Test
public void testForOfLoopsVar() {
assertLiveBeforeX("var a; for (a of [1, 2, 3]) {X:{}}", "a");
assertLiveAfterX("for (var a of [1, 2, 3]) {X:{}}", "a");
// the final 'a' might still be '0', if 'iter' is empty, so 'var a = 0;' is live
assertLiveBeforeX("var a = 0; X: for (a of iter) {{}} a", "a");
// `a` is not live because it is never read
assertNotLiveBeforeX("var a; X: for (a of iter) {{}}", "a");

// The following case is overly conservative, treating 'a' as live before X only because
// the pass cannot determine that:
// `a` is read in the loop if and only if `a` is killed in the loop initializer.
assertLiveBeforeX("var a; X: for (a of iter) { a; }", "a");
assertLiveAfterX("for (var a of [1, 2, 3]) {X:{} a; } ", "a");
assertLiveBeforeX("var a,b; for (var y of a = [0, 1, 2]) { X:a[y] }", "a");
assertLiveBeforeX("let x = 3; X:for (var [y = x] of arr) { y; }", "x");
}

@Test
public void testForOfLoopsDestructuring() {
assertLiveBeforeX("var key, value; X:for ([key, value] of arr) {value;} value;", "value");
assertLiveBeforeX("let x = 3; X:for (var [y = x] of arr) { y; }", "x");
assertLiveBeforeX("for (let [key, value] in arr) { X: key; value; }", "key");
public void testForOfLoopsLetAndConst() {
assertNotLiveBeforeX("X: for (let a of iter) { a }", "a");
assertNotLiveBeforeX("X: for (const a of iter) { a }", "a");
assertNotLiveBeforeX("X: for (let [a] of iter) { a }", "a");
assertLiveAfterX("for (let a of iter) { X: {} a; }", "a");
assertLiveAfterX("for (const a of iter) { X: {} a; }", "a");
assertLiveAfterX("for (let [a] of iter) { X: {} a; }", "a");
}

@Test
public void testForOfLoopsInitializerIsConditional() {
assertLiveBeforeX("var a; X: for ([a] of []) {} a", "a");
// 'b = 0' does not kill b because it is only conditionally executed
assertLiveBeforeX("var a, b; X: for ({[b = 0]: a} of []) {} b", "b");
assertLiveBeforeX("var a = {}, b; X: for (a[b = 0] of []) {} b", "b");
}

@Test
Expand Down Expand Up @@ -285,12 +324,25 @@ public void testForInAssignment() {
}

@Test
public void testExceptionThrowingAssignments() {
public void testExceptionThrowingAssignmentsWithVar() {
// the following to cases consider 'a' live before X only because the pass does not realize that
// 'a' is read only if '(var) a = foo()' is executed.
assertLiveBeforeX("try{var a; X:a=foo();a} catch(e) {e()}", "a");

assertLiveBeforeX("try{X:var a=foo();a} catch(e) {e()}", "a");
assertLiveBeforeX("try{X:var a=foo()} catch(e) {e(a)}", "a");
}

@Test
public void testExceptionThrowingAssignmentsWithLet() {
// following case is overly conservative, a is read only if 'a = foo()' runs
// so 'let a;' is dead
assertLiveBeforeX("try {let a; X:a=foo(); a} catch(e) {e()}", "a");
assertNotLiveBeforeDecl("try {let a=foo(); a} catch(e) {e()}", "a");
assertNotLiveBeforeX("try {X: 0; let [a]=foo(); a} catch(e) {e()}", "a");
assertNotLiveBeforeDecl("try {let a=foo()} catch(e) {e(a)}", "a");
}

@Test
public void testInnerFunctions() {
assertLiveBeforeX("function a() {}; X: a()", "a");
Expand Down Expand Up @@ -353,10 +405,6 @@ public void testSimpleLet() {
assertNotLiveBeforeX("let a,b;X:a=1;b(a)", "a");
assertNotLiveAfterX("let a,b;X:b(a);b()", "a");
assertLiveBeforeX("let a,b;X:b();b=1;a()", "b");

// let initialized afterX
assertLiveAfterX("X:a();let a;a()", "a");
assertNotLiveAfterX("X:a();let a=1;a()", "a");
}

@Test
Expand Down Expand Up @@ -418,23 +466,23 @@ public void testComplicatedDeclaration() {
private void assertLiveBeforeX(String src, String var) {
FlowState<LiveVariablesAnalysis.LiveVariableLattice> state = getFlowStateAtX(src);
assertWithMessage(src + " should contain a label 'X:'").that(state).isNotNull();
assertWithMessage("Variable" + var + " should be live before X")
assertWithMessage("Variable " + var + " should be live before X")
.that(state.getIn().isLive(liveness.getVarIndex(var)))
.isTrue();
}

private void assertLiveAfterX(String src, String var) {
FlowState<LiveVariablesAnalysis.LiveVariableLattice> state = getFlowStateAtX(src);
assertWithMessage("Label X should be in the input program.").that(state).isNotNull();
assertWithMessage("Variable" + var + " should be live after X")
assertWithMessage("Variable " + var + " should be live after X")
.that(state.getOut().isLive(liveness.getVarIndex(var)))
.isTrue();
}

private void assertNotLiveAfterX(String src, String var) {
FlowState<LiveVariablesAnalysis.LiveVariableLattice> state = getFlowStateAtX(src);
assertWithMessage("Label X should be in the input program.").that(state).isNotNull();
assertWithMessage("Variable" + var + " should not be live after X")
assertWithMessage("Variable " + var + " should not be live after X")
.that(state.getOut().isLive(liveness.getVarIndex(var)))
.isFalse();
}
Expand Down

0 comments on commit 078c3c8

Please sign in to comment.