From 60273d843425ef8ce98d10dfc5890b1a58f9a47d Mon Sep 17 00:00:00 2001 From: lillymliu Date: Tue, 25 Jul 2017 14:21:07 -0700 Subject: [PATCH] Have PeepholeRemoveDeadCode not hoist block scoped variables. Changed tryMergeBlock in NodeUtil to take a boolean on whether it should always merge or not. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=163122312 --- .../jscomp/ClosureRewriteModule.java | 2 +- .../google/javascript/jscomp/NodeUtil.java | 33 +++++++++++- .../jscomp/PeepholeRemoveDeadCode.java | 5 +- .../javascript/jscomp/RenameLabels.java | 2 +- .../javascript/jscomp/ScopedAliases.java | 2 +- .../jscomp/TransformAMDToCJSModule.java | 2 +- .../jscomp/WhitespaceWrapGoogModules.java | 2 +- .../javascript/jscomp/NodeUtilTest.java | 41 +++++++++++--- .../jscomp/PeepholeRemoveDeadCodeTest.java | 54 ++++++++++++++++--- .../javascript/jscomp/RenameLabelsTest.java | 2 +- 10 files changed, 123 insertions(+), 22 deletions(-) diff --git a/src/com/google/javascript/jscomp/ClosureRewriteModule.java b/src/com/google/javascript/jscomp/ClosureRewriteModule.java index 44cd4cdbd89..71ad3eda0e9 100644 --- a/src/com/google/javascript/jscomp/ClosureRewriteModule.java +++ b/src/com/google/javascript/jscomp/ClosureRewriteModule.java @@ -1398,7 +1398,7 @@ void updateModuleBody(Node moduleBody) { moduleBody.isModuleBody() && moduleBody.getParent().getBooleanProp(Node.GOOG_MODULE), moduleBody); moduleBody.setToken(Token.BLOCK); - NodeUtil.tryMergeBlock(moduleBody); + NodeUtil.tryMergeBlock(moduleBody, true); updateEndModule(); popScript(); diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index ddc3db2ed27..0894d4cf248 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -2826,12 +2826,13 @@ static void maybeAddFinally(Node tryNode) { * Merge a block with its parent block. * @return Whether the block was removed. */ - public static boolean tryMergeBlock(Node block) { + public static boolean tryMergeBlock(Node block, boolean alwaysMerge) { checkState(block.isNormalBlock()); Node parent = block.getParent(); + boolean canMerge = alwaysMerge || canMergeBlock(block); // Try to remove the block if its parent is a block/script or if its // parent is label and it has exactly one child. - if (isStatementBlock(parent)) { + if (isStatementBlock(parent) && canMerge) { Node previous = block; while (block.hasChildren()) { Node child = block.removeFirstChild(); @@ -2845,6 +2846,34 @@ public static boolean tryMergeBlock(Node block) { } } + /** + * A check inside a block to see if there are const, let, class, or function declarations + * to be safe and not hoist them into the upper block. + * @return Whether the block can be removed + */ + public static boolean canMergeBlock(Node block) { + for (Node c = block.getFirstChild(); c != null; c = c.getNext()) { + switch (c.getToken()) { + case LABEL: + if (canMergeBlock(c)){ + continue; + } else { + return false; + } + + case CONST: + case LET: + case CLASS: + case FUNCTION: + return false; + + default: + continue; + } + } + return true; + } + /** * @param node A node * @return Whether the call is a NEW or CALL node. diff --git a/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java b/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java index 33ebc6ff3c0..5a47b9cdcf6 100644 --- a/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java +++ b/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java @@ -593,7 +593,7 @@ Node tryOptimizeBlock(Node n) { } // Try to remove the block. - if (NodeUtil.tryMergeBlock(n)) { + if (NodeUtil.tryMergeBlock(n, false)) { reportCodeChange(); return null; } @@ -602,7 +602,8 @@ Node tryOptimizeBlock(Node n) { } /** - * Some nodes unremovable node don't have side-effects. + * Some nodes that are unremovable don't have side effects so they aren't caught by + * mayHaveSideEffects */ private static boolean isUnremovableNode(Node n) { return (n.isNormalBlock() && n.isSyntheticBlock()) || n.isScript(); diff --git a/src/com/google/javascript/jscomp/RenameLabels.java b/src/com/google/javascript/jscomp/RenameLabels.java index 98dc5d03b8e..5f5809fa6ff 100644 --- a/src/com/google/javascript/jscomp/RenameLabels.java +++ b/src/com/google/javascript/jscomp/RenameLabels.java @@ -249,7 +249,7 @@ private void visitLabel(NodeTraversal t, Node node, Node parent) { node.removeChild(newChild); parent.replaceChild(node, newChild); if (newChild.isNormalBlock()) { - NodeUtil.tryMergeBlock(newChild); + NodeUtil.tryMergeBlock(newChild, false); } if (markChanges) { t.reportCodeChange(); diff --git a/src/com/google/javascript/jscomp/ScopedAliases.java b/src/com/google/javascript/jscomp/ScopedAliases.java index d118bf9cbe4..a12c1a7f390 100644 --- a/src/com/google/javascript/jscomp/ScopedAliases.java +++ b/src/com/google/javascript/jscomp/ScopedAliases.java @@ -191,7 +191,7 @@ public void hotSwapScript(Node root, Node originalRoot) { expressionWithScopeCall.replaceWith(scopeClosureBlock); NodeUtil.markFunctionsDeleted(expressionWithScopeCall, compiler); compiler.reportChangeToEnclosingScope(scopeClosureBlock); - NodeUtil.tryMergeBlock(scopeClosureBlock); + NodeUtil.tryMergeBlock(scopeClosureBlock, false); } } } diff --git a/src/com/google/javascript/jscomp/TransformAMDToCJSModule.java b/src/com/google/javascript/jscomp/TransformAMDToCJSModule.java index 84cac765c74..607eb505659 100644 --- a/src/com/google/javascript/jscomp/TransformAMDToCJSModule.java +++ b/src/com/google/javascript/jscomp/TransformAMDToCJSModule.java @@ -259,7 +259,7 @@ private void moveCallbackContentToTopLevel(Node defineParent, Node script, script.addChildBefore(callbackBlock, before); } script.addChildToBack(callbackBlock); - NodeUtil.tryMergeBlock(callbackBlock); + NodeUtil.tryMergeBlock(callbackBlock, false); } } diff --git a/src/com/google/javascript/jscomp/WhitespaceWrapGoogModules.java b/src/com/google/javascript/jscomp/WhitespaceWrapGoogModules.java index 451aacdea0f..11f397489fb 100644 --- a/src/com/google/javascript/jscomp/WhitespaceWrapGoogModules.java +++ b/src/com/google/javascript/jscomp/WhitespaceWrapGoogModules.java @@ -48,7 +48,7 @@ public void hotSwapScript(Node scriptRoot, Node originalRoot) { } Node moduleBody = scriptRoot.getFirstChild(); moduleBody.setToken(Token.BLOCK); - NodeUtil.tryMergeBlock(moduleBody); + NodeUtil.tryMergeBlock(moduleBody, true); compiler.reportChangeToEnclosingScope(scriptRoot); // As per ClosureBundler: diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index e4274baecb3..5327a18869b 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -1141,30 +1141,30 @@ public void testReplaceDeclarationName() { replaceDeclChild("const x =1, y = 2, z = 3, w = 4;", 1, "const x = 1; {} const z = 3, w = 4;"); } - public void testMergeBlock1() { + public void testTryMergeBlock1() { // Test removing the initializer. Node actual = parse("{{a();b();}}"); Node parentBlock = actual.getFirstChild(); Node childBlock = parentBlock.getFirstChild(); - assertTrue(NodeUtil.tryMergeBlock(childBlock)); + assertTrue(NodeUtil.tryMergeBlock(childBlock, false)); String expected = "{a();b();}"; String difference = parse(expected).checkTreeEquals(actual); assertNull("Nodes do not match:\n" + difference, difference); } - public void testMergeBlock2() { + public void testTryMergeBlock2() { // Test removing the initializer. Node actual = parse("foo:{a();}"); Node parentLabel = actual.getFirstChild(); Node childBlock = parentLabel.getLastChild(); - assertFalse(NodeUtil.tryMergeBlock(childBlock)); + assertFalse(NodeUtil.tryMergeBlock(childBlock, false)); } - public void testMergeBlock3() { + public void testTryMergeBlock3() { // Test removing the initializer. String code = "foo:{a();boo()}"; Node actual = parse("foo:{a();boo()}"); @@ -1172,12 +1172,41 @@ public void testMergeBlock3() { Node parentLabel = actual.getFirstChild(); Node childBlock = parentLabel.getLastChild(); - assertFalse(NodeUtil.tryMergeBlock(childBlock)); + assertFalse(NodeUtil.tryMergeBlock(childBlock, false)); String expected = code; String difference = parse(expected).checkTreeEquals(actual); assertNull("Nodes do not match:\n" + difference, difference); } + public void testTryMergeBlock4() { + Node actual = parse("{const module$exports$Foo=class{}}"); + String expected = "const module$exports$Foo=class{}"; + + Node block = actual.getFirstChild(); + + assertTrue(NodeUtil.tryMergeBlock(block, true)); + String difference = parse(expected).checkTreeEquals(actual); + assertNull("Nodes do not match:\n" + difference, difference); + } + + public void testCanMergeBlock1() { + Node actual = parse("{a(); let x;}"); + + Node block = actual.getFirstChild(); + + assertFalse(NodeUtil.canMergeBlock(block)); + } + + public void testCanMergeBlock2() { + Node actual = parse("{a(); f(); var x; {const y = 2;}}"); + + Node parentBlock = actual.getFirstChild(); + Node childBlock = parentBlock.getLastChild(); + + assertTrue(NodeUtil.canMergeBlock(parentBlock)); + assertFalse(NodeUtil.canMergeBlock(childBlock)); + } + public void testGetSourceName() { Node n = new Node(Token.BLOCK); Node parent = new Node(Token.BLOCK, n); diff --git a/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java b/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java index 45380393cfb..746285b61ed 100644 --- a/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java +++ b/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java @@ -88,6 +88,7 @@ public void testFoldBlock() { fold("{'hi'}", ""); fold("{x==3}", ""); + fold("{`hello ${foo}`}", ""); fold("{ (function(){x++}) }", ""); foldSame("function f(){return;}"); fold("function f(){return 3;}", "function f(){return 3}"); @@ -99,6 +100,20 @@ public void testFoldBlock() { fold("while(x()){x()}", "while(x())x()"); fold("for(x=0;x<100;x++){x}", "for(x=0;x<100;x++);"); fold("for(x in y){x}", "for(x in y);"); + fold("for (x of y) {x}", "for(x of y);"); + + // Block with declarations + foldSame("{let x}"); + foldSame("function f() {let x}"); + foldSame("{const x = 1}"); + foldSame("{x = 2; y = 4; let z;}"); + fold("{'hi'; let x;}", "{let x}"); + fold("{x = 4; {let y}}", "x = 4; {let y}"); + foldSame("{function f() {} } {function f() {}}"); + foldSame("{class C {}} {class C {}}"); + foldSame("{label: let x}"); + fold("{label: var x}", "label: var x"); + foldSame("{label: var x; let y;}"); } /** Try to remove spurious blocks with multiple children */ @@ -106,8 +121,10 @@ public void testFoldBlocksWithManyChildren() { fold("function f() { if (false) {} }", "function f(){}"); fold("function f() { { if (false) {} if (true) {} {} } }", "function f(){}"); - test("{var x; var y; var z; function f() { { var a; { var b; } } } }", - "var x;var y;var z;function f(){var a;var b}"); + fold("{var x; var y; var z; function f() { { var a; { var b; } } } }", + "{var x;var y;var z;function f(){var a;var b} }"); + fold("{var x; var y; var z; { { var a; { var b; } } } }", + "var x;var y;var z; var a;var b"); } public void testIf() { @@ -187,6 +204,13 @@ public void testVarLifting() { // More var lifting tests in PeepholeIntegrationTests } + public void testLetConstLifting(){ + fold("if(true) {const x = 1}", "{const x = 1}"); + fold("if(false) {const x = 1}", ""); + fold("if(true) {let x}", "{let x}"); + fold("if(false) {let x}", ""); + } + public void testFoldUselessWhile() { fold("while(false) { foo() }", ""); @@ -210,6 +234,8 @@ public void testFoldUselessFor() { fold("for(;true;) foo() ", "for(;;) foo() "); foldSame("for(;;) foo()"); fold("for(;false;) { var a = 0; }", "var a"); + fold("for(;false;) { const a = 0; }", ""); + fold("for(;false;) { let a = 0; }", ""); // Make sure it plays nice with minimizing fold("for(;false;) { foo(); continue }", ""); @@ -277,6 +303,7 @@ public void testRemoveUselessOps2() { // Uncalled function expressions are removed fold("(function () {});", ""); fold("(function f() {});", ""); + fold("(function* f() {})", ""); // ... including any code they contain. fold("(function () {foo();});", ""); @@ -537,6 +564,18 @@ public void testOptimizeSwitchWithLabellessBreak() { " case 21: throw 'x';", " default : console.log('good');", "}")); + + test( + LINE_JOINER.join( + "let x = 1;", + "switch('x') {", + " case 'x': let x = 2; break;", + "}" + ), + LINE_JOINER.join( + "let x = 1;", + "{let x = 2}" + )); } public void testOptimizeSwitchWithLabelledBreak() { @@ -574,20 +613,23 @@ public void testOptimizeSwitchWithReturn() { "}"), "function f() { return 1; }"); - // TODO(moz): This is to show that the current optimization might break if the input has block - // scoped declarations. We will need to fix the logic for removing blocks to account for this. test( LINE_JOINER.join( "function f() {", " let x = 1;", " switch('x') {", - " case 'x': { let x = 2; return 3; }", + " case 'x': { let x = 2; } return 3;", " case 'y': return 4;", " }", "}"), - "function f() { let x = 1; let x = 2; return 3; }"); + LINE_JOINER.join( + "function f() {", + " let x = 1;", + " { let x = 2; } return 3; ", + "}")); } + public void testOptimizeSwitchWithThrow() { test( LINE_JOINER.join( diff --git a/test/com/google/javascript/jscomp/RenameLabelsTest.java b/test/com/google/javascript/jscomp/RenameLabelsTest.java index 78cd36289b2..ad75ff7043f 100644 --- a/test/com/google/javascript/jscomp/RenameLabelsTest.java +++ b/test/com/google/javascript/jscomp/RenameLabelsTest.java @@ -45,7 +45,7 @@ public void testRenameInFunction() { "}" + "}" + "}", - "function x(){function goo(){a:{ a(); break a; }}}"); + "function x(){{function goo(){a:{ a(); break a; }}}}"); test("function x() { " + "Foo:{ " +