Refactor change verification into a class so we can track additional …

…metadata (such as the current change number) in preparation for improving new node tagging.

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

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

@@ -0,0 +1,169 @@
+/*
+ * Copyright 2017 The Closure Compiler Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.google.javascript.jscomp;
+
+import com.google.common.base.Preconditions;
+import com.google.common.base.Predicates;
+import com.google.javascript.jscomp.NodeUtil.Visitor;
+import com.google.javascript.rhino.Node;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A Class to assist in AST change tracking verification.  To validate a "snapshot" is taken
+ * before and "checkRecordedChanges" at the desired check point.
+ */
+public class ChangeVerifier {
+  private final AbstractCompiler compiler;
+  private final Map<Node, Node> map = new HashMap<>();
+  private int snapshotChange;
+
+  ChangeVerifier(AbstractCompiler compiler) {
+    this.compiler = compiler;
+  }
+
+  ChangeVerifier snapshot(Node root) {
+    // remove any existing snapshot data.
+    map.clear();
+    snapshotChange = compiler.getChangeStamp();
+
+    Node snapshot = root.cloneTree();
+    associateClones(root, snapshot);
+    return this;
+  }
+
+  void checkRecordedChanges(Node current) {
+    checkRecordedChanges("", current);
+  }
+
+  void checkRecordedChanges(String passName, Node root) {
+    verifyScopeChangesHaveBeenRecorded(passName, root);
+  }
+
+  /**
+   * Given an AST and its copy, map the root node of each scope of main to the
+   * corresponding root node of clone
+   */
+  private void associateClones(Node n, Node snapshot) {
+    // TODO(johnlenz): determine if MODULE_BODY is useful here.
+    if (NodeUtil.isChangeScopeRoot(n)) {
+      map.put(n, snapshot);
+    }
+
+    Node child = n.getFirstChild();
+    Node snapshotChild = snapshot.getFirstChild();
+    while (child != null) {
+      associateClones(child, snapshotChild);
+      child = child.getNext();
+      snapshotChild = snapshotChild.getNext();
+    }
+  }
+
+  /** Checks that the scope roots marked as changed have indeed changed */
+  private void verifyScopeChangesHaveBeenRecorded(
+      String passName, Node root) {
+    final String passNameMsg = passName.isEmpty() ? "" : passName + ": ";
+
+    NodeUtil.visitPreOrder(root,
+        new Visitor() {
+          @Override
+          public void visit(Node n) {
+            if (n.isRoot()) {
+              verifyRoot(n);
+            } else if (NodeUtil.isChangeScopeRoot(n)) {
+              Node clone = map.get(n);
+              if (clone == null) {
+                verifyNewNode(n);
+              } else {
+                verifyNodeChange(passNameMsg, n, clone);
+              }
+            }
+          }
+        },
+        Predicates.<Node>alwaysTrue());
+  }
+
+  private void verifyNewNode(Node n) {
+    // TODO(johnlenz): Verify the new nodes are properly tagged.
+  }
+
+  private void verifyRoot(Node root) {
+    Preconditions.checkState(root.isRoot());
+    if (root.getChangeTime() != 0) {
+      throw new IllegalStateException("Root nodes should never be marked as changed.");
+    }
+  }
+
+  private void verifyNodeChange(final String passNameMsg, Node n, Node snapshot) {
+    if (n.isRoot()) {
+      return;
+    }
+    if (n.getChangeTime() > snapshot.getChangeTime()) {
+      if (isEquivalentToExcludingFunctions(n, snapshot)) {
+        throw new IllegalStateException(
+            passNameMsg + "unchanged scope marked as changed: " + n.toStringTree());
+      }
+    } else {
+      if (!isEquivalentToExcludingFunctions(n, snapshot)) {
+        throw new IllegalStateException(
+            passNameMsg + "change scope not marked as changed: " + n.toStringTree());
+      }
+    }
+  }
+
+  /**
+   * @return Whether the two node are equivalent while ignoring
+   * differences any descendant functions differences.
+   */
+  private static boolean isEquivalentToExcludingFunctions(
+      Node thisNode, Node thatNode) {
+    if (thisNode == null || thatNode == null) {
+      return thisNode == null && thatNode == null;
+    }
+    if (!thisNode.isEquivalentWithSideEffectsToShallow(thatNode)) {
+      return false;
+    }
+    if (thisNode.getChildCount() != thatNode.getChildCount()) {
+      return false;
+    }
+    Node thisChild = thisNode.getFirstChild();
+    Node thatChild = thatNode.getFirstChild();
+    while (thisChild != null && thatChild != null) {
+      if (thisChild.isFunction() || thisChild.isScript()) {
+        // Don't compare function expression name, parameters or bodies.
+        // But do check that that the node is there.
+        if (thatChild.getToken() != thisChild.getToken()) {
+          return false;
+        }
+        // Only compare function names for function declarations (not function expressions)
+        // as they change the outer scope definition.
+        if (thisChild.isFunction() && NodeUtil.isFunctionDeclaration(thisChild)) {
+          String thisName = thisChild.getFirstChild().getString();
+          String thatName = thatChild.getFirstChild().getString();
+          if (!thisName.equals(thatName)) {
+            return false;
+          }
+        }
+      } else if (!isEquivalentToExcludingFunctions(thisChild, thatChild)) {
+        return false;
+      }
+      thisChild = thisChild.getNext();
+      thatChild = thatChild.getNext();
+    }
+    return true;
+  }
+}
+

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

@@ -40,7 +40,6 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumSet;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -4726,68 +4725,6 @@ static Node getEnclosingChangeScopeRoot(Node n) {
     return n;
   }
 
-  /**
-   * Given an AST and its copy, map the root node of each scope of main to the
-   * corresponding root node of clone
-   */
-  public static Map<Node, Node> mapMainToClone(Node main, Node clone) {
-    Preconditions.checkState(main.isEquivalentTo(clone));
-    Map<Node, Node> mtoc = new HashMap<>();
-    mtoc.put(main, clone);
-    mtocHelper(mtoc, main, clone);
-    return mtoc;
-  }
-
-  private static void mtocHelper(Map<Node, Node> map, Node main, Node clone) {
-    // TODO(johnlenz): determine if MODULE_BODY is useful here.
-    if (main.isFunction() || main.isScript()) {
-      map.put(main, clone);
-    }
-    Node mchild = main.getFirstChild();
-    Node cchild = clone.getFirstChild();
-    while (mchild != null) {
-      mtocHelper(map, mchild, cchild);
-      mchild = mchild.getNext();
-      cchild = cchild.getNext();
-    }
-  }
-
-  /** Checks that the scope roots marked as changed have indeed changed */
-  public static void verifyScopeChanges(
-      String passName, final Map<Node, Node> mtoc, Node main) {
-    final String passNameMsg = passName.isEmpty() ? "" : passName + ": ";
-
-    Node clone = mtoc.get(main);
-    verifyNodeChange(passNameMsg, main, clone);
-    visitPreOrder(main,
-        new Visitor() {
-          @Override
-          public void visit(Node n) {
-            if ((n.isScript() || n.isFunction()) && mtoc.containsKey(n)) {
-              Node clone = mtoc.get(n);
-              verifyNodeChange(passNameMsg, n, clone);
-            }
-          }
-        },
-        Predicates.<Node>alwaysTrue());
-  }
-
-  static void verifyNodeChange(final String passNameMsg, Node n, Node clone) {
-    if (n.isRoot() && n.getChangeTime() != 0) {
-      throw new IllegalStateException("Root nodes should never be marked as changed.");
-    }
-    if (n.getChangeTime() > clone.getChangeTime()) {
-      if (isEquivalentToExcludingFunctions(n, clone)) {
-        throw new IllegalStateException(
-            passNameMsg + "unchanged scope marked as changed: " + n.toStringTree());
-      }
-    } else {
-      if (!isEquivalentToExcludingFunctions(n, clone)) {
-        throw new IllegalStateException(
-            passNameMsg + "change scope not marked as changed: " + n.toStringTree());
-      }
-    }
-  }
 
   static int countAstSizeUpToLimit(Node n, final int limit) {
     // Java doesn't allow accessing mutable local variables from another class.
@@ -4813,47 +4750,6 @@ static int countAstSize(Node n) {
     return countAstSizeUpToLimit(n, Integer.MAX_VALUE);
   }
 
-  /**
-   * @return Whether the two node are equivalent while ignoring
-   * differences any descendant functions differences.
-   */
-  private static boolean isEquivalentToExcludingFunctions(
-      Node thisNode, Node thatNode) {
-    if (thisNode == null || thatNode == null) {
-      return thisNode == null && thatNode == null;
-    }
-    if (!thisNode.isEquivalentWithSideEffectsToShallow(thatNode)) {
-      return false;
-    }
-    if (thisNode.getChildCount() != thatNode.getChildCount()) {
-      return false;
-    }
-    Node thisChild = thisNode.getFirstChild();
-    Node thatChild = thatNode.getFirstChild();
-    while (thisChild != null && thatChild != null) {
-      if (thisChild.isFunction() || thisChild.isScript()) {
-        // Don't compare function expression name, parameters or bodies.
-        // But do check that that the node is there.
-        if (thatChild.getToken() != thisChild.getToken()) {
-          return false;
-        }
-        // Only compare function names for function declarations (not function expressions)
-        // as they change the outer scope definition.
-        if (thisChild.isFunction() && NodeUtil.isFunctionDeclaration(thisChild)) {
-          String thisName = thisChild.getFirstChild().getString();
-          String thatName = thatChild.getFirstChild().getString();
-          if (!thisName.equals(thatName)) {
-            return false;
-          }
-        }
-      } else if (!isEquivalentToExcludingFunctions(thisChild, thatChild)) {
-        return false;
-      }
-      thisChild = thisChild.getNext();
-      thatChild = thatChild.getNext();
-    }
-    return true;
-  }
 
   static JSDocInfo createConstantJsDoc() {
     JSDocInfoBuilder builder = new JSDocInfoBuilder(false);

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

@@ -63,9 +63,8 @@ class PhaseOptimizer implements CompilerPass {
 
   private final boolean useSizeHeuristicToStopOptimizationLoop;
 
-  // Used for sanity checks between loopable passes
-  private Node lastAst;
-  private Map<Node, Node> mtoc; // Stands for "main to clone"
+  // Used for sanity checking passes
+  private ChangeVerifier changeVerifier;
 
   /**
    * When processing loopable passes in order, the PhaseOptimizer can be in one
@@ -189,12 +188,7 @@ Loop addFixedPointLoop() {
    */
   void setSanityCheck(PassFactory sanityCheck) {
     this.sanityCheck = sanityCheck;
-    setSanityCheckState();
-  }
-
-  private void setSanityCheckState() {
-    lastAst = jsRoot.cloneTree();
-    mtoc = NodeUtil.mapMainToClone(jsRoot, lastAst);
+    this.changeVerifier = new ChangeVerifier(compiler).snapshot(jsRoot);
   }
 
   /**
@@ -248,7 +242,7 @@ private void maybePrintAstHashcodes(String passName, Node root) {
   private void maybeSanityCheck(String passName, Node externs, Node root) {
     if (sanityCheck != null) {
       sanityCheck.create(compiler).process(externs, root);
-      NodeUtil.verifyScopeChanges(passName, mtoc, jsRoot);
+      changeVerifier.checkRecordedChanges(passName, jsRoot);
     }
   }
 
@@ -275,7 +269,7 @@ public void process(Node externs, Node root) {
       if (sanityCheck != null) {
         // Before running the pass, clone the AST so you can sanity-check the
         // changed AST against the clone after the pass finishes.
-        setSanityCheckState();
+        changeVerifier = new ChangeVerifier(compiler).snapshot(jsRoot);
       }
       if (tracker != null) {
         tracker.recordPassStart(name, factory.isOneTimePass());