UnreachableCodeElimination: Avoid traversing unreachable statements

When traversing a block of statements, skip traversal of all statements
following the first one we find to be unreachable and remove them all.

This avoids unnecessary work traversing the individual statements that we
already know will not ever execute.

This change also fixes a bug that caused unreachable `do {} while (condition)`
statements to be left behind. This could cause an undeclared variable reference
if the condition referred to a `let` or `const` variable whose declaration was
also removed as unreachable.

That bug was the original motivation for this change.

PiperOrigin-RevId: 565691433

Author: brad4d

src/com/google/javascript/jscomp/UnreachableCodeElimination.java

@@ -23,6 +23,8 @@
 import com.google.javascript.jscomp.graph.DiGraph.DiGraphNode;
 import com.google.javascript.jscomp.graph.GraphReachability;
 import com.google.javascript.rhino.Node;
+import java.util.ArrayDeque;
+import java.util.Deque;
 import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -83,29 +85,70 @@ public void enterChangedScopeRoot(AbstractCompiler compiler, Node root) {
   private class EliminationPass implements NodeTraversal.Callback {
     private final ControlFlowGraph<Node> cfg;
 
+    /**
+     * Keep track of nodes that contain a sequence of statements.
+     *
+     * <p>As soon as we find one statement is unreachable, we can skip traversing the rest.
+     */
+    private final Deque<StatementSequenceParentContext> statementSequenceParentContextStack =
+        new ArrayDeque<>();
+
     private EliminationPass(ControlFlowGraph<Node> cfg) {
       this.cfg = cfg;
     }
 
     @Override
     public boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent) {
-      if (parent == null) {
-        return true;
-      } else if (n.isExport()) {
+      if (n.isExport()) {
         // TODO(b/129564961): We should be exploring EXPORTs. We don't because their descendants
         // have side-effects that `AstAnalyzer.mayHaveSideEffects` doesn't recognize. Since this
         // pass currently runs after exports are removed anyway, this isn't yet an issue.
         return false;
-      } else if (parent.isFunction()) {
-        // We only want to traverse the name of a function.
-        return n.isFirstChildOf(parent);
+      } else if (n.isFunction()) {
+        // Do not descend into function scopes, because they won't be included in our
+        // current CFG.
+        return false;
+      }
+
+      StatementSequenceParentContext statementSequenceParentContext =
+          statementSequenceParentContextStack.peek();
+      if (statementSequenceParentContext != null
+          && statementSequenceParentContext.statementParentNode == parent) {
+        // We're looking at a statement node in the current statement parent
+        if (statementSequenceParentContext.firstUnreachableStatementNode != null) {
+          // A previous statement is unreachable, so there's no point looking at this one.
+          return false;
+        }
+        if (isDefinitelyUnreachable(n)) {
+          statementSequenceParentContext.firstUnreachableStatementNode = n;
+          return false;
+        }
+      }
+
+      if (isStatementSequenceParent(n)) {
+        statementSequenceParentContextStack.push(new StatementSequenceParentContext(n));
       }
 
       return true;
     }
 
     @Override
     public void visit(NodeTraversal t, Node n, Node parent) {
+      StatementSequenceParentContext statementSequenceParentContext =
+          statementSequenceParentContextStack.peek();
+      if (statementSequenceParentContext != null
+          && statementSequenceParentContext.statementParentNode == n) {
+        // We're now visiting the statement parent, itself.
+        statementSequenceParentContextStack.pop();
+        Node unreachableStatementNode =
+            statementSequenceParentContext.firstUnreachableStatementNode;
+        while (unreachableStatementNode != null) {
+          final Node nextStatement = unreachableStatementNode.getNext();
+          removeStatementNode(unreachableStatementNode);
+          unreachableStatementNode = nextStatement;
+        }
+        return;
+      }
       if (parent == null || n.isFunction() || n.isScript()) {
         return;
       }
@@ -121,6 +164,32 @@ public void visit(NodeTraversal t, Node n, Node parent) {
       tryRemoveUnconditionalBranching(n);
     }
 
+    private boolean isDefinitelyUnreachable(Node n) {
+      DiGraphNode<Node, Branch> gNode = getCfgNodeForStatement(n);
+      if (gNode == null) {
+        // Not in CFG.
+        // We may have traversed into a scope not covered by the CFG,
+        // or maybe just looking at a node the CFG doesn't consider part of the control flow.
+        return false;
+      }
+      return gNode.getAnnotation() != GraphReachability.REACHABLE;
+    }
+
+    private DiGraphNode<Node, Branch> getCfgNodeForStatement(Node statement) {
+      switch (statement.getToken()) {
+        case DO:
+          // CFG flows first into the statement within the do {} while ();
+          // So we should consider that CFG node to represent the whole statement.
+          return cfg.getNode(statement.getFirstChild());
+        case LABEL:
+          // A LABEL is never actually executed, so get what it labels.
+          // We use recursion because it is possible to label a label.
+          return getCfgNodeForStatement(statement.getLastChild());
+        default:
+          return cfg.getNode(statement);
+      }
+    }
+
     /**
      * Tries to remove n if it is an unconditional branch node (break, continue, or return) and the
      * target of n is the same as the follow of n.
@@ -179,7 +248,7 @@ private void tryRemoveUnconditionalBranching(Node n) {
             Node fallThrough = computeFollowing(n);
             Node nextCfgNode = outEdges.get(0).getDestination().getValue();
             if (nextCfgNode == fallThrough && !inFinally(n.getParent(), n)) {
-              removeNode(n);
+              logicallyRemoveNode(n);
             }
           }
           break;
@@ -265,10 +334,18 @@ private void removeDeadExprStatementSafely(Node n) {
         return;
       }
 
-      removeNode(n);
+      logicallyRemoveNode(n);
     }
 
-    private void removeNode(Node n) {
+    /**
+     * Logically, put possibly not actually, remove a node.
+     *
+     * <p>This method uses {@code NodeUtil.removeChild()} which has a lot of logic to handle
+     * attempts to remove nodes that are structurally required by the AST. It will make a change
+     * that has the behavior of the node being removed, even though what actually is done to the AST
+     * may not be simple removal of the node.
+     */
+    private void logicallyRemoveNode(Node n) {
       codeChanged = true;
       NodeUtil.redeclareVarsInsideBranch(n);
       compiler.reportChangeToEnclosingScope(n);
@@ -279,4 +356,42 @@ private void removeNode(Node n) {
       NodeUtil.markFunctionsDeleted(n, compiler);
     }
   }
+
+  /**
+   * Remove a statement that is part of a sequence of statements.
+   *
+   * <p>Unlike {@code logicallyRemoveNode()}, this method will always remove the node.
+   */
+  private void removeStatementNode(Node statementNode) {
+    codeChanged = true;
+    NodeUtil.redeclareVarsInsideBranch(statementNode);
+    compiler.reportChangeToEnclosingScope(statementNode);
+    if (logger.isLoggable(Level.FINE)) {
+      logger.fine("Removing " + statementNode);
+    }
+    // Since we know we have a statement within a statement sequence here, simply detaching it is
+    // always safe.
+    statementNode.detach();
+    NodeUtil.markFunctionsDeleted(statementNode, compiler);
+  }
+
+  /** Is {@code n} a {@code Node} that has a sequence of statements as its children? */
+  private static boolean isStatementSequenceParent(Node n) {
+    // A LABEL is a statement parent, but only for a single statement.
+    // For historical reasons, the second child of a TRY is a BLOCK with a single CATCH child.
+    // We don't want to treat the CATCH as if it were a statement.
+    return NodeUtil.isStatementParent(n) && !n.isLabel() && !NodeUtil.isTryCatchNodeContainer(n);
+  }
+
+  /** One of these is created for each node whose children are a sequence of statements. */
+  private static class StatementSequenceParentContext {
+    final Node statementParentNode;
+
+    /** Set non-null only if we discover that some statements are unreachable. */
+    Node firstUnreachableStatementNode = null;
+
+    public StatementSequenceParentContext(Node statementParentNode) {
+      this.statementParentNode = statementParentNode;
+    }
+  }
 }

test/com/google/javascript/jscomp/UnreachableCodeEliminationTest.java

@@ -39,6 +39,26 @@ public void setUp() throws Exception {
     enableComputeSideEffects();
   }
 
+  @Test
+  public void testLabeledBlocks() {
+    test(
+        lines(
+            "function b(m) {", //
+            " return m;",
+            " label: {",
+            "   START('debug');",
+            "   label2: {",
+            "     alert('Shouldnt be here' + m);",
+            "   }",
+            "   END('debug');",
+            "  }",
+            "}"),
+        lines(
+            "function b(m) {", //
+            "  return m;",
+            "}"));
+  }
+
   @Test
   public void testDoNotRemoveDeclarationOfUsedVariable() {
     test(
@@ -54,10 +74,6 @@ public void testDoNotRemoveDeclarationOfUsedVariable() {
         lines(
             "var f = function() {", //
             "  return 1;",
-            "  do {",
-            // TODO: b/297246830 - This is broken behavior.
-            // Either delete all references to `b` or keep its declaration
-            "  } while (b);",
             "};"));
   }
 
@@ -403,7 +419,7 @@ public void testRemoveDo() {
     // for-loops.
     test(
         "for (; 1;) { break; do { print(1); break } while(1) }",
-        "for (; 1;) { break; do {} while(1) }");
+        "for (; 1;) { break;                                 }");
   }
 
   @Test
@@ -747,9 +763,13 @@ public void testIssue4177428_multifinally() {
 
   @Test
   public void testIssue5215541_deadVarDeclar() {
-    testSame("throw 1; var x");
+    test(
+        "       throw 1; var x;", //
+        "var x; throw 1;       ");
     testSame("throw 1; function x() {}");
-    testSame("throw 1; var x; var y;");
+    test(
+        "throw 1; var x; var y;                ", //
+        "                var y; var x; throw 1;");
     test(
         "       throw 1; var x = foo", //
         "var x; throw 1");