Per-scope change detection validation in the unit tests is broken (the older

all or nothing validation works correctly). Several issues:
* There was a logic mismatch with regard to which Nodes should be checked and
SCRIPT nodes were being ignored.
* The validation was only run if the PhaseOptimizer set but CompilerTestCase
does use PhaseOptimizer by default (some all but a few don't)
* The validation was comparing the state before the setup passes ran (es6
transpilation, normalization, etc) so a unit test might pass even if it didn't
mark a pass as changed.
* The validate logic, in unit tests, was not checking scopes not marked as changed but only
 validated that marked nodes had been changed (the latter is less likely to be
 incorrect).
* The validation logic was ignoring Node annotations when comparing node for
changes which causes some false negative results.

In addition to the validation logic fixes, this simplifies the Compiler/PhaseOptimizer interaction with regard to change tracking and makes way for future changes.

The validation is now turned off by default.  The next
change will turn it on by default with appropriate fixes.

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

Author: concavelenz

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

@@ -265,6 +265,12 @@ LifeCycleStage getLifeCycleStage() {
   /** Let the PhaseOptimizer know which scope a pass is currently analyzing */
   abstract void setScope(Node n);
 
+  /** A monotonically increasing value to identify a change */
+  abstract int getChangeStamp();
+
+  /** Called to indicate that the current change stamp has been used */
+  abstract void incrementChangeStamp();
+
   /** Returns the root of the source tree, ignoring externs */
   abstract Node getJsRoot();
 

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

@@ -262,6 +262,19 @@ public SourceFile apply(String filename) {
 
   private static final Joiner pathJoiner = Joiner.on(File.separator);
 
+  // TODO(johnlenz): remove "currentScope".
+  // Used as a shortcut for change tracking.  This is the current scope being
+  // visited by the "current" NodeTraversal.  This can't be thread safe so
+  // we should move it into the NodeTraversal and require explicit changed
+  // nodes elsewhere so we aren't blocked from doing this elsewhere.
+  private Node currentScope = null;
+
+  // Starts at 0, increases as "interesting" things happen.
+  // Nothing happens at time START_TIME, the first pass starts at time 1.
+  // The correctness of scope-change tracking relies on Node/getIntProp
+  // returning 0 if the custom attribute on a node hasn't been set.
+  private int changeStamp = 1;
+
   /**
    * Creates a Compiler that reports errors and warnings to its logger.
    */
@@ -2293,13 +2306,6 @@ void removeChangeHandler(CodeChangeHandler handler) {
     codeChangeHandlers.remove(handler);
   }
 
-  @Override
-  void setScope(Node n) {
-    if (phaseOptimizer != null) {
-      phaseOptimizer.setScope(n);
-    }
-  }
-
   Node getExternsRoot() {
     return externsRoot;
   }
@@ -2309,6 +2315,47 @@ Node getJsRoot() {
     return jsRoot;
   }
 
+  /**
+   * Some tests don't want to call the compiler "wholesale," they may not want
+   * to call check and/or optimize. With this method, tests can execute custom
+   * optimization loops.
+   */
+  @VisibleForTesting
+  void setPhaseOptimizer(PhaseOptimizer po) {
+    this.phaseOptimizer = po;
+  }
+
+  @Override
+  public int getChangeStamp() {
+    return changeStamp;
+  }
+
+  @Override
+  public void incrementChangeStamp() {
+    changeStamp++;
+  }
+
+  @Override
+  void setScope(Node n) {
+    currentScope = (n.isFunction() || n.isScript()) ? n : getEnclosingChangeScope(n);
+  }
+
+  private Node getEnclosingChangeScope(Node n) {
+    while (n.getParent() != null) {
+      n = n.getParent();
+      if (n.isFunction() || n.isScript()) {
+        return n;
+      }
+    }
+    return n;
+  }
+
+  private void recordChange(Node n) {
+    n.setChangeTime(changeStamp);
+    // Every code change happens at a different time
+    changeStamp++;
+  }
+
   @Override
   boolean hasScopeChanged(Node n) {
     if (phaseOptimizer == null) {
@@ -2318,29 +2365,23 @@ boolean hasScopeChanged(Node n) {
   }
 
   @Override
-  void reportChangeToEnclosingScope(Node n) {
-    if (phaseOptimizer != null) {
-      phaseOptimizer.reportChangeToEnclosingScope(n);
-      phaseOptimizer.startCrossScopeReporting();
-      reportCodeChange();
-      phaseOptimizer.endCrossScopeReporting();
-    } else {
-      reportCodeChange();
+  public void reportCodeChange() {
+    // TODO(johnlenz): if this is called with a null scope we need to invalidate everything
+    // but this isn't done, so we need to make this illegal or record this as having
+    // invalidated everything.
+    if (currentScope != null) {
+      recordChange(currentScope);
+      notifyChangeHandlers();
     }
   }
 
-  /**
-   * Some tests don't want to call the compiler "wholesale," they may not want
-   * to call check and/or optimize. With this method, tests can execute custom
-   * optimization loops.
-   */
-  @VisibleForTesting
-  void setPhaseOptimizer(PhaseOptimizer po) {
-    this.phaseOptimizer = po;
+  @Override
+  void reportChangeToEnclosingScope(Node n) {
+    recordChange(getEnclosingChangeScope(n));
+    notifyChangeHandlers();
   }
 
-  @Override
-  public void reportCodeChange() {
+  private void notifyChangeHandlers() {
     for (CodeChangeHandler handler : codeChangeHandlers) {
       handler.reportChange();
     }

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

@@ -4720,7 +4720,8 @@ public static Map<Node, Node> mapMainToClone(Node main, Node clone) {
   }
 
   private static void mtocHelper(Map<Node, Node> map, Node main, Node clone) {
-    if (main.isFunction()) {
+    // TODO(johnlenz): determine if MODULE_BODY is useful here.
+    if (main.isFunction() || main.isScript()) {
       map.put(main, clone);
     }
     Node mchild = main.getFirstChild();
@@ -4733,8 +4734,10 @@ private static void mtocHelper(Map<Node, Node> map, Node main, Node clone) {
   }
 
   /** Checks that the scope roots marked as changed have indeed changed */
-  public static void verifyScopeChanges(Map<Node, Node> map, Node main,
-      boolean verifyUnchangedNodes) {
+  public static void verifyScopeChanges(
+      String passName, Map<Node, Node> map, Node main) {
+    final String passNameMsg = passName.isEmpty() ? "" : passName + ": ";
+
     // compiler is passed only to call compiler.toSource during debugging to see
     // mismatches in scopes
 
@@ -4744,25 +4747,28 @@ public static void verifyScopeChanges(Map<Node, Node> map, Node main,
     // If verifyUnchangedNodes is true, we are comparing the ASTs before & after
     // a pass. Check all scope roots.
     final Map<Node, Node> mtoc = map;
-    final boolean checkUnchanged = verifyUnchangedNodes;
     Node clone = mtoc.get(main);
     if (main.getChangeTime() > clone.getChangeTime()) {
       Preconditions.checkState(!isEquivalentToExcludingFunctions(main, clone));
-    } else if (checkUnchanged) {
+    } else {
       Preconditions.checkState(isEquivalentToExcludingFunctions(main, clone));
     }
     visitPreOrder(main,
         new Visitor() {
           @Override
           public void visit(Node n) {
-            if (n.isFunction() && mtoc.containsKey(n)) {
+            if ((n.isScript() || n.isFunction()) && mtoc.containsKey(n)) {
               Node clone = mtoc.get(n);
               if (n.getChangeTime() > clone.getChangeTime()) {
                 Preconditions.checkState(
-                    !isEquivalentToExcludingFunctions(n, clone));
-              } else if (checkUnchanged) {
+                    !isEquivalentToExcludingFunctions(n, clone),
+                    "%sunchanged scope marked as changed",
+                    passNameMsg);
+              } else {
                 Preconditions.checkState(
-                    isEquivalentToExcludingFunctions(n, clone));
+                    isEquivalentToExcludingFunctions(n, clone),
+                    "%schange scope not marked as changed",
+                    passNameMsg);
               }
             }
           }
@@ -4803,7 +4809,7 @@ private static boolean isEquivalentToExcludingFunctions(
     if (thisNode == null || thatNode == null) {
       return thisNode == null && thatNode == null;
     }
-    if (!thisNode.isEquivalentToShallow(thatNode)) {
+    if (!thisNode.isEquivalentWithSideEffectsToShallow(thatNode)) {
       return false;
     }
     if (thisNode.getChildCount() != thatNode.getChildCount()) {

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

@@ -56,19 +56,11 @@ class PhaseOptimizer implements CompilerPass {
   private NamedPass currentPass;
   // For each pass, remember the time at the end of the pass's last run.
   private Map<NamedPass, Integer> lastRuns;
-  private Node currentScope;
-  // Starts at 0, increases as "interesting" things happen.
-  // Nothing happens at time START_TIME, the first pass starts at time 1.
-  // The correctness of scope-change tracking relies on Node/getIntProp
-  // returning 0 if the custom attribute on a node hasn't been set.
-  private int timestamp;
   // The time of the last change made to the program by any pass.
   private int lastChange;
   private static final int START_TIME = 0;
   private final Node jsRoot;
-  // Compiler/reportChangeToScope must call reportCodeChange to update all
-  // change handlers. This flag prevents double update in ScopedChangeHandler.
-  private boolean crossScopeReporting;
+
   private final boolean useSizeHeuristicToStopOptimizationLoop;
 
   // Used for sanity checks between loopable passes
@@ -130,8 +122,7 @@ enum State {
     this.passes = new ArrayList<>();
     this.progressRange = range;
     this.inLoop = false;
-    this.crossScopeReporting = false;
-    this.timestamp = this.lastChange = START_TIME;
+    this.lastChange = START_TIME;
     this.useSizeHeuristicToStopOptimizationLoop =
         comp.getOptions().useSizeHeuristicToStopOptimizationLoop;
   }
@@ -198,6 +189,7 @@ void setSanityCheck(PassFactory sanityCheck) {
   }
 
   private void setSanityCheckState() {
+    // TODO(johnlenz): change this to always validate. See b/37164291
     if (inLoop) {
       lastAst = jsRoot.cloneTree();
       mtoc = NodeUtil.mapMainToClone(jsRoot, lastAst);
@@ -242,8 +234,8 @@ public void process(Node externs, Node root) {
 
   private void maybePrintAstHashcodes(String passName, Node root) {
     if (printAstHashcodes) {
-      String hashCodeMsg = "AST hashCode after " + passName + ": " +
-          compiler.toSource(root).hashCode();
+      String hashCodeMsg = "AST hashCode after " + passName + ": "
+          + compiler.toSource(root).hashCode();
       System.err.println(hashCodeMsg);
       compiler.addToDebugLog(hashCodeMsg);
     }
@@ -252,7 +244,7 @@ private void maybePrintAstHashcodes(String passName, Node root) {
   /**
    * Runs the sanity check if it is available.
    */
-  private void maybeSanityCheck(Node externs, Node root) {
+  private void maybeSanityCheck(String passName, Node externs, Node root) {
     if (sanityCheck != null) {
       sanityCheck.create(compiler).process(externs, root);
       // The cross-module passes are loopable and ran together, but do not
@@ -261,7 +253,7 @@ private void maybeSanityCheck(Node externs, Node root) {
       if (inLoop
           && !currentPass.name.equals(Compiler.CROSS_MODULE_CODE_MOTION_NAME)
           && !currentPass.name.equals(Compiler.CROSS_MODULE_METHOD_MOTION_NAME)) {
-        NodeUtil.verifyScopeChanges(mtoc, jsRoot, true);
+        NodeUtil.verifyScopeChanges(passName, mtoc, jsRoot);
       }
     }
   }
@@ -320,7 +312,7 @@ public void process(Node externs, Node root) {
           tracker.recordPassStop(name, traceRuntime);
         }
         maybePrintAstHashcodes(name, root);
-        maybeSanityCheck(externs, root);
+        maybeSanityCheck(name, externs, root);
       } catch (IllegalStateException e) {
         // TODO(johnlenz): Remove this once the normalization checks report
         // errors instead of exceptions.
@@ -334,14 +326,6 @@ public String toString() {
     }
   }
 
-  void setScope(Node n) {
-    // NodeTraversal causes setScope calls outside loops; ignore them.
-    if (inLoop) {
-      // Find the top-level node in the scope.
-      currentScope = n.isFunction() ? n : getEnclosingScope(n);
-    }
-  }
-
   boolean hasScopeChanged(Node n) {
     // Outside loops we don't track changed scopes, so we visit them all.
     if (!inLoop) {
@@ -353,65 +337,26 @@ boolean hasScopeChanged(Node n) {
         || n.getChangeTime() > timeOfLastRun;
   }
 
-  private Node getEnclosingScope(Node n) {
-    while (n.getParent() != null) {
-      n = n.getParent();
-      if (n.isFunction() || n.isScript()) {
-        return n;
-      }
-    }
-    return n;
-  }
-
-  void reportChangeToEnclosingScope(Node n) {
-    lastChange = timestamp;
-    getEnclosingScope(n).setChangeTime(timestamp);
-    // Every code change happens at a different time
-    timestamp++;
-  }
-
-  /**
-   * Records that the currently-running pass may report cross-scope changes.
-   * When this happens, we don't want to falsely report the current scope as
-   * changed when reportChangeToScope is called from Compiler.
-   */
-  void startCrossScopeReporting() {
-    crossScopeReporting = true;
-  }
-
-  /** The currently-running pass won't report cross-scope changes. */
-  void endCrossScopeReporting() {
-    crossScopeReporting = false;
-  }
-
   /**
    * A change handler that marks scopes as changed when reportChange is called.
    */
   private class ScopedChangeHandler implements CodeChangeHandler {
     private int lastCodeChangeQuery;
 
     ScopedChangeHandler() {
-      this.lastCodeChangeQuery = timestamp;
+      this.lastCodeChangeQuery = compiler.getChangeStamp();
     }
 
     @Override
     public void reportChange() {
-      if (crossScopeReporting) {
-        // This call was caused by Compiler/reportChangeToEnclosingScope,
-        // do nothing.
-        return;
-      }
-      lastChange = timestamp;
-      currentScope.setChangeTime(timestamp);
-      // Every code change happens at a different time
-      timestamp++;
+      lastChange = compiler.getChangeStamp();
     }
 
     private boolean hasCodeChangedSinceLastCall() {
       boolean result = lastChange > lastCodeChangeQuery;
-      lastCodeChangeQuery = timestamp;
+      lastCodeChangeQuery = compiler.getChangeStamp();
       // The next call to the method will happen at a different time
-      timestamp++;
+      compiler.incrementChangeStamp();
       return result;
     }
   }
@@ -449,7 +394,11 @@ public void process(Node externs, Node root) {
       // Set up function-change tracking
       scopeHandler = new ScopedChangeHandler();
       compiler.addChangeHandler(scopeHandler);
-      setScope(root);
+
+      // TODO(johnlenz): It is unclear why "setScope" is called here. Try to remove
+      // this.
+      compiler.setScope(root);
+
       // lastRuns is initialized before each loop. This way, when a pass is run
       // in the 2nd loop for the 1st time, it looks at all scopes.
       lastRuns = new HashMap<>();
@@ -487,11 +436,11 @@ public void process(Node externs, Node root) {
                     && !runInPrevIter.contains(pass))
                 || (state == State.RUN_PASSES_THAT_CHANGED_STH_IN_PREV_ITER
                         && madeChanges.contains(pass))) {
-              timestamp++;
+              compiler.incrementChangeStamp();
               currentPass = pass;
               pass.process(externs, root);
               runInPrevIter.add(pass);
-              lastRuns.put(pass, timestamp);
+              lastRuns.put(pass, compiler.getChangeStamp());
               if (hasHaltingErrors()) {
                 return;
               } else if (scopeHandler.hasCodeChangedSinceLastCall()) {