Enforce that PureFunctionIdentifier runs over only normalized ASTs.

This is a step towards running `PureFunctionIdentifer` using `OptimizeCalls` which also requires normalization to have run.

Enforcing normalization requires updating tests for passes that depend on pure function identification to also perform normalization. An upshot of this is that some test cases become redundant (and are deleted) when this is enforced.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=241393703

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

@@ -1177,6 +1177,7 @@ private static boolean checkForStateChangeHelper(
         break;
 
       case NAME:
+        // TODO(b/129564961): Consider EXPORT declarations.
         if (n.hasChildren()) {
           // This is the left side of a var/let/const
           return true;
@@ -1616,6 +1617,7 @@ static boolean nodeTypeMayHaveSideEffects(Node n, AbstractCompiler compiler) {
         return NodeUtil.constructorCallHasSideEffects(n);
       case NAME:
         // A variable definition.
+        // TODO(b/129564961): Consider EXPORT declarations.
         return n.hasChildren();
       case REST:
       case SPREAD:

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

@@ -90,7 +90,7 @@ Node optimizeSubtree(Node subtree) {
       case IF:
         return tryFoldIf(subtree);
       case WHILE:
-        return tryFoldWhile(subtree);
+        throw checkNormalization(false, "WHILE");
       case FOR:
         {
           Node condition = NodeUtil.getConditionExpression(subtree);
@@ -755,6 +755,7 @@ Node tryOptimizeBlock(Node n) {
     for (Node c = n.getFirstChild(); c != null; ) {
       Node next = c.getNext();  // save c.next, since 'c' may be removed
       if (!isUnremovableNode(c) && !mayHaveSideEffects(c)) {
+        checkNormalization(!NodeUtil.isFunctionDeclaration(n), "function declaration");
         // TODO(johnlenz): determine what this is actually removing. Candidates
         //    include: EMPTY nodes, control structures without children
         //    (removing infinite loops), empty try blocks.  What else?
@@ -1269,4 +1270,9 @@ private void tryFoldForCondition(Node forCondition) {
       forCondition.replaceWith(IR.empty());
     }
   }
+
+  private static IllegalStateException checkNormalization(boolean condition, String feature) {
+    checkState(condition, "Unexpected %s. AST should be normalized.", feature);
+    return null;
+  }
 }

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

@@ -134,6 +134,7 @@ public PureFunctionIdentifier(
 
   @Override
   public void process(Node externsAst, Node srcAst) {
+    checkState(compiler.getLifeCycleStage().isNormalized());
     checkState(
         externs == null && root == null,
         "PureFunctionIdentifier::process may only be called once per instance.");

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

@@ -19,7 +19,7 @@
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.javascript.jscomp.ControlFlowGraph.Branch;
-import com.google.javascript.jscomp.NodeTraversal.AbstractShallowCallback;
+import com.google.javascript.jscomp.NodeTraversal.Callback;
 import com.google.javascript.jscomp.NodeTraversal.ChangeScopeRootCallback;
 import com.google.javascript.jscomp.graph.DiGraph.DiGraphEdge;
 import com.google.javascript.jscomp.graph.DiGraph.DiGraphNode;
@@ -59,6 +59,8 @@ class UnreachableCodeElimination implements CompilerPass {
 
   @Override
   public void process(Node externs, Node toplevel) {
+    checkState(compiler.getLifeCycleStage().isNormalized());
+
     NodeTraversal.traverseChangedFunctions(compiler, new ChangeScopeRootCallback() {
         @Override
         public void enterChangeScopeRoot(AbstractCompiler compiler, Node root) {
@@ -80,12 +82,30 @@ public void enterChangeScopeRoot(AbstractCompiler compiler, Node root) {
       });
   }
 
-  private class EliminationPass extends AbstractShallowCallback {
+  private class EliminationPass implements Callback {
     private final ControlFlowGraph<Node> cfg;
+
     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()) {
+        // TODO(b/129564961): We should be exploring EXPORTs. We don't because their descendants
+        // have side-effects that `NodeUtil::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);
+      }
+
+      return true;
+    }
+
     @Override
     public void visit(NodeTraversal t, Node n, Node parent) {
       if (parent == null || n.isFunction() || n.isScript()) {

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

@@ -1863,8 +1863,8 @@ public void helperInlineReferenceToFunction(
     final Node expectedRoot = parseExpected(new Compiler(), expectedResult);
 
     Node mainRoot = tree;
-    PureFunctionIdentifier.Driver mark = new PureFunctionIdentifier.Driver(compiler);
-    mark.process(externsRoot, mainRoot);
+    new Normalize(compiler, false).process(externsRoot, mainRoot);
+    new PureFunctionIdentifier.Driver(compiler).process(externsRoot, mainRoot);
 
     Normalize normalize = new Normalize(compiler, false);
     normalize.process(externsRoot, mainRoot);

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

@@ -20,7 +20,6 @@
 import static com.google.javascript.jscomp.CompilerTestCase.lines;
 import static com.google.javascript.rhino.testing.NodeSubject.assertNode;
 
-import com.google.javascript.jscomp.AbstractCompiler.LifeCycleStage;
 import com.google.javascript.jscomp.NodeTraversal.Callback;
 import com.google.javascript.rhino.IR;
 import com.google.javascript.rhino.Node;
@@ -163,12 +162,13 @@ public void testMutateInitializeUninitializedVars1() {
   @Test
   public void testMutateInitializeUninitializedVars2() {
     helperMutate(
-        "function foo(a) {for(var b in c)return a;}; foo(1);",
+        "function foo(a) {var b; for(b in c)return a;}; foo(1);",
         lines(
             "{",
             "  JSCompiler_inline_label_foo_2:",
             "  {",
-            "    for (var b$jscomp$inline_1 in c) {",
+            "    var b$jscomp$inline_1;",
+            "    for (b$jscomp$inline_1 in c) {",
             "      1;",
             "      break JSCompiler_inline_label_foo_2;",
             "    }",
@@ -256,11 +256,11 @@ public void testMutateFunctionDefinitionHoisting() {
             "foo(1);"),
         lines(
             "{",
-            "  var g$jscomp$inline_2 = function(c$jscomp$inline_6) {return c$jscomp$inline_6};",
-            "  var h$jscomp$inline_4 = function(){};",
-            "  var i$jscomp$inline_5 = function(){};",
-            "  var b$jscomp$inline_1 = g$jscomp$inline_2(1);",
-            "  var c$jscomp$inline_3 = i$jscomp$inline_5();",
+            "  var g$jscomp$inline_1 = function(c$jscomp$inline_6) {return c$jscomp$inline_6};",
+            "  var h$jscomp$inline_2 = function(){};",
+            "  var i$jscomp$inline_3 = function(){};",
+            "  var b$jscomp$inline_4 = g$jscomp$inline_1(1);",
+            "  var c$jscomp$inline_5 = i$jscomp$inline_3();",
             "}"),
         "foo",
         null);
@@ -288,13 +288,11 @@ public void helperMutate(String code, String expectedResult, String fnName, Stri
     compiler.externsRoot = new Node(Token.ROOT);
     compiler.jsRoot = IR.root(script);
     compiler.externAndJsRoot = IR.root(compiler.externsRoot, compiler.jsRoot);
-    PureFunctionIdentifier.Driver mark = new PureFunctionIdentifier.Driver(compiler);
-    mark.process(compiler.externsRoot, compiler.jsRoot);
 
-    final Node fnNode = findFunction(script, fnName);
+    new Normalize(compiler, false).process(compiler.externsRoot, compiler.jsRoot);
+    new PureFunctionIdentifier.Driver(compiler).process(compiler.externsRoot, compiler.jsRoot);
 
-    // Fake precondition.
-    compiler.setLifeCycleStage(LifeCycleStage.NORMALIZED);
+    final Node fnNode = findFunction(script, fnName);
 
     // inline tester
     Method tester =

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

@@ -16,6 +16,7 @@
 package com.google.javascript.jscomp;
 
 import com.google.common.collect.ImmutableList;
+import org.junit.Before;
 import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -43,6 +44,15 @@ protected int getNumRepetitions() {
     return 1;
   }
 
+  @Override
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    enableNormalize();
+    enableComputeSideEffects();
+  }
+
   @Test
   public void testRemoveDuplicates() {
     test(

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

@@ -32,6 +32,8 @@ public class PeepholeIntegrationTest extends CompilerTestCase {
   @Before
   public void setUp() throws Exception {
     super.setUp();
+
+    enableNormalize();
     this.late = false;
     this.numRepetitions = 2;
   }
@@ -136,9 +138,6 @@ public void testBug1059649() {
     // ensure that folding blocks with a single var node doesn't explode
     test("if(x){var y=3;}var z=5", "if(x)var y=3;var z=5");
 
-    // With normalization, we no longer have this case.
-    testSame("if(x){var y=3;}else{var y=4;}var z=5");
-    test("while(x){var y=3;}var z=5", "while(x)var y=3;var z=5");
     test("for(var i=0;i<10;i++){var y=3;}var z=5",
          "for(var i=0;i<10;i++)var y=3;var z=5");
     test("for(var i in x){var y=3;}var z=5",
@@ -186,7 +185,7 @@ public void testFoldLogicalOpIntegration() {
 
   @Test
   public void testFoldBitwiseOpStringCompareIntegration() {
-    test("while (-1 | 0) {}", "while (1);");
+    test("for (;-1 | 0;) {}", "for (;;);");
   }
 
   @Test
@@ -206,16 +205,6 @@ public void testBug1438784() {
     test("for(var i=0;i<10;i++)if(x)x.y;", "for(var i=0;i<10;i++);");
   }
 
-  @Test
-  public void testFoldUselessWhileIntegration() {
-    test("while(!true) { foo() }", "");
-    test("while(!false) foo() ", "while(1) foo()");
-    test("while(!void 0) foo()", "while(1) foo()");
-
-    // Make sure proper empty nodes are inserted.
-    test("if(foo())while(false){foo()}else bar()", "foo()||bar()");
-  }
-
   @Test
   public void testFoldUselessForIntegration() {
     test("for(;!true;) { foo() }", "");
@@ -239,17 +228,6 @@ public void testFoldUselessDoIntegration() {
     test("if(foo())do {foo()} while(false) else bar()", "foo()?foo():bar()");
   }
 
-  @Test
-  public void testMinimizeWhileConstantConditionIntegration() {
-    test("while(!false) foo()", "while(1) foo()");
-    test("while(202) foo()", "while(1) foo()");
-    test("while(Infinity) foo()", "while(1) foo()");
-    test("while('text') foo()", "while(1) foo()");
-    test("while([]) foo()", "while(1) foo()");
-    test("while({}) foo()", "while(1) foo()");
-    test("while(/./) foo()", "while(1) foo()");
-  }
-
   @Test
   public void testMinimizeExpr() {
     test("!!true", "");
@@ -286,7 +264,7 @@ public void testBugIssue43() {
 
   @Test
   public void testFoldNegativeBug() {
-    test("while(-3){};", "while(1);");
+    test("for (;-3;){};", "for (;;);");
   }
 
   @Test