From 078c3c8437b0d255cd788b1591f22722f07600c9 Mon Sep 17 00:00:00 2001 From: lharker Date: Mon, 7 Jan 2019 09:50:36 -0800 Subject: [PATCH] Back off less on conditional let/const declarations in LiveVariablesAnalysis 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 --- .../jscomp/LiveVariablesAnalysis.java | 33 ++++---- .../jscomp/CoalesceVariableNamesTest.java | 7 +- .../javascript/jscomp/IntegrationTest.java | 14 ++-- .../jscomp/LiveVariablesAnalysisTest.java | 82 +++++++++++++++---- 4 files changed, 93 insertions(+), 43 deletions(-) diff --git a/src/com/google/javascript/jscomp/LiveVariablesAnalysis.java b/src/com/google/javascript/jscomp/LiveVariablesAnalysis.java index da83ce15b6c..c25ce249f0d 100644 --- a/src/com/google/javascript/jscomp/LiveVariablesAnalysis.java +++ b/src/com/google/javascript/jscomp/LiveVariablesAnalysis.java @@ -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; @@ -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 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; @@ -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); diff --git a/test/com/google/javascript/jscomp/CoalesceVariableNamesTest.java b/test/com/google/javascript/jscomp/CoalesceVariableNamesTest.java index 503dea05760..83b98f90e40 100644 --- a/test/com/google/javascript/jscomp/CoalesceVariableNamesTest.java +++ b/test/com/google/javascript/jscomp/CoalesceVariableNamesTest.java @@ -331,8 +331,8 @@ public void testBug65688660() { " }", " if (true) {", " param = [];", - " for (const kv of []) {", - " param = kv.key;", + " for (param of []) {", + " param = param.key;", " }", " }", "}")); @@ -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 diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index 2571b199104..cee069ec8cb 100644 --- a/test/com/google/javascript/jscomp/IntegrationTest.java +++ b/test/com/google/javascript/jscomp/IntegrationTest.java @@ -238,8 +238,8 @@ public void testBug65688660() { " }", " if (true) {", " param = [];", - " for (const kv of []) {", - " param = kv.key;", + " for (param of []) {", + " param = param.key;", " }", " }", "}")); @@ -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;", " }", " }", "}")); diff --git a/test/com/google/javascript/jscomp/LiveVariablesAnalysisTest.java b/test/com/google/javascript/jscomp/LiveVariablesAnalysisTest.java index 4a4e9d99f8e..f3026b9d7b9 100644 --- a/test/com/google/javascript/jscomp/LiveVariablesAnalysisTest.java +++ b/test/com/google/javascript/jscomp/LiveVariablesAnalysisTest.java @@ -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"); @@ -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. @@ -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 @@ -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"); @@ -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 @@ -418,7 +466,7 @@ public void testComplicatedDeclaration() { private void assertLiveBeforeX(String src, String var) { FlowState 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(); } @@ -426,7 +474,7 @@ private void assertLiveBeforeX(String src, String var) { private void assertLiveAfterX(String src, String var) { FlowState 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(); } @@ -434,7 +482,7 @@ private void assertLiveAfterX(String src, String var) { private void assertNotLiveAfterX(String src, String var) { FlowState 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(); }