Refactor AST change tracking to correct inaccuracies introduced when …

…SCRIPT became a change scope root. SCRIPT nodes were not being set as the "change scope" when a function scope was pop'd from the scope stack if there was not a block scope between the script and the function. Script nodes are only on the scope stack during some unit tests, when the traversal is started from a SCRIPT rather than a ROOT, during normal operation SCRIPT are not semantic scope roots.

Disable a few tests that no longer pass with these changes.

Also introduce an alternate version that doesn't depend on compiler state in "NodeTraversal#reportCodeChange" that can be used by passes that can be run in multiple threads at the same time.

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

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

@@ -138,6 +138,18 @@ static enum MostRecentTypechecker {
    */
   public abstract void reportCodeChange();
 
+  /**
+   * Passes that make modifications in a scope that is different than the Compiler.currentScope use
+   * this (eg, InlineVariables and many others)
+   */
+  abstract void reportChangeToEnclosingScope(Node n);
+
+  /**
+   * Make modifications in a scope that is different than the Compiler.currentScope use
+   * this (eg, InlineVariables and many others)
+   */
+  abstract void reportChangeToChangeScope(Node changeScopeRoot);
+
   /**
    * Logs a message under a central logger.
    */
@@ -263,7 +275,7 @@ LifeCycleStage getLifeCycleStage() {
   abstract void removeChangeHandler(CodeChangeHandler handler);
 
   /** Let the PhaseOptimizer know which scope a pass is currently analyzing */
-  abstract void setScope(Node n);
+  abstract void setChangeScope(Node n);
 
   /** A monotonically increasing value to identify a change */
   abstract int getChangeStamp();
@@ -277,12 +289,6 @@ LifeCycleStage getLifeCycleStage() {
   /** True iff a function changed since the last time a pass was run */
   abstract boolean hasScopeChanged(Node n);
 
-  /**
-   * Passes that make modifications in a scope that is different than the Compiler.currentScope use
-   * this (eg, InlineVariables and many others)
-   */
-  abstract void reportChangeToEnclosingScope(Node n);
-
   /**
    * Represents the different contexts for which the compiler could have
    * distinct configurations.

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

@@ -270,7 +270,7 @@ public SourceFile apply(String filename) {
   // 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;
+  private Node currentChangeScope = null;
 
   // Starts at 0, increases as "interesting" things happen.
   // Nothing happens at time START_TIME, the first pass starts at time 1.
@@ -2389,8 +2389,8 @@ public void incrementChangeStamp() {
   }
 
   @Override
-  void setScope(Node n) {
-    currentScope = (n.isFunction() || n.isScript()) ? n : getEnclosingChangeScope(n);
+  void setChangeScope(Node newChangeScopeRoot) {
+    currentChangeScope = newChangeScopeRoot;
   }
 
   private Node getEnclosingChangeScope(Node n) {
@@ -2433,10 +2433,18 @@ 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();
+    if (currentChangeScope != null) {
+      Preconditions.checkState(currentChangeScope.isScript() || currentChangeScope.isFunction());
+      recordChange(currentChangeScope);
     }
+    notifyChangeHandlers();
+  }
+
+  @Override
+  public void reportChangeToChangeScope(Node changeScopeRoot) {
+    Preconditions.checkState(changeScopeRoot.isScript() || changeScopeRoot.isFunction());
+    recordChange(changeScopeRoot);
+    notifyChangeHandlers();
   }
 
   @Override

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

@@ -38,6 +38,12 @@ public class NodeTraversal {
   /** Contains the current node*/
   private Node curNode;
 
+  /** The current script being visited */
+  private Node curScript;
+
+  /** The change scope for the current node being visiteds */
+  private Node currentChangeScope;
+
   /**
    * Stack containing the Scopes that have been created. The Scope objects
    * are lazily created; so the {@code scopeRoots} stack contains the
@@ -269,7 +275,6 @@ public NodeTraversal(AbstractCompiler compiler, Callback cb,
       this.scopeCallback = (ScopedCallback) cb;
     }
     this.compiler = compiler;
-    setInputId(null, "");
     this.scopeCreator = scopeCreator;
     this.useBlockScope = scopeCreator.hasBlockScope();
   }
@@ -304,7 +309,7 @@ private String formatNodeContext(String label, Node n) {
    */
   public void traverse(Node root) {
     try {
-      setInputId(NodeUtil.getInputId(root), "");
+      initTraversal(root);
       curNode = root;
       pushScope(root);
       // null parent ensures that the shallow callbacks will traverse root
@@ -320,7 +325,7 @@ void traverseRoots(Node externs, Node root) {
       Node scopeRoot = externs.getParent();
       Preconditions.checkNotNull(scopeRoot);
 
-      setInputId(NodeUtil.getInputId(scopeRoot), "");
+      initTraversal(scopeRoot);
       curNode = scopeRoot;
       pushScope(scopeRoot);
 
@@ -360,7 +365,7 @@ private String formatNodePosition(Node n) {
   void traverseWithScope(Node root, Scope s) {
     Preconditions.checkState(s.isGlobal() || s.isModuleScope());
     try {
-      setInputId(null, "");
+      initTraversal(root);
       curNode = root;
       pushScope(s);
       traverseBranch(root, null);
@@ -376,6 +381,7 @@ void traverseWithScope(Node root, Scope s) {
    */
   void traverseAtScope(Scope s) {
     Node n = s.getRootNode();
+    initTraversal(n);
     curNode = n;
     Deque<Scope> parentScopes = new ArrayDeque<>();
     Scope temp = s.getParent();
@@ -387,11 +393,6 @@ void traverseAtScope(Scope s) {
       pushScope(parentScopes.pop(), true);
     }
     if (n.isFunction()) {
-      // We need to do some extra magic to make sure that the scope doesn't
-      // get re-created when we dive into the function.
-      if (inputId == null) {
-        setInputId(NodeUtil.getInputId(n), getSourceName(n));
-      }
       pushScope(s);
 
       Node args = n.getSecondChild();
@@ -401,9 +402,6 @@ void traverseAtScope(Scope s) {
 
       popScope();
     } else if (n.isNormalBlock()) {
-      if (inputId == null) {
-        setInputId(NodeUtil.getInputId(n), getSourceName(n));
-      }
       pushScope(s);
 
       // traverseBranch is not called here to avoid re-creating the block scope.
@@ -428,9 +426,7 @@ public void traverseFunctionOutOfBand(Node node, Scope scope) {
     Preconditions.checkNotNull(scope);
     Preconditions.checkState(node.isFunction());
     Preconditions.checkState(scope.getRootNode() != null);
-    if (inputId == null) {
-      setInputId(NodeUtil.getInputId(node), getSourceName(node));
-    }
+    initTraversal(node);
     curNode = node.getParent();
     pushScope(scope, true /* quietly */);
     traverseBranch(node, curNode);
@@ -449,9 +445,7 @@ public void traverseFunctionOutOfBand(Node node, Scope scope) {
    */
   void traverseInnerNode(Node node, Node parent, Scope refinedScope) {
     Preconditions.checkNotNull(parent);
-    if (inputId == null) {
-      setInputId(NodeUtil.getInputId(node), getSourceName(node));
-    }
+    initTraversal(node);
     if (refinedScope != null && getScope() != refinedScope) {
       curNode = node;
       pushScope(refinedScope);
@@ -541,8 +535,8 @@ public Node getCurrentNode() {
    * doesn't know which scope changed. We keep track of the current scope by
    * calling Compiler.setScope inside pushScope and popScope.
    * The automatic tracking can be wrong in rare cases when a pass changes scope
-   * w/out causing a call to pushScope or popScope. It's very hard to find the
-   * places where this happens unless a bug is triggered.
+   * w/out causing a call to pushScope or popScope.
+   *
    * Passes that do cross-scope modifications call
    * Compiler.reportChangeToEnclosingScope(Node n).
    */
@@ -609,14 +603,44 @@ public static void traverseRootsTyped(
     t.traverseRoots(externs, root);
   }
 
+  private void handleScript(Node n, Node parent) {
+    setChangeScope(n);
+    curScript = n;
+    setInputId(n.getInputId(), getSourceName(n));
+
+    curNode = n;
+    if (callback.shouldTraverse(this, n, parent)) {
+      traverseChildren(n);
+      curNode = n;
+      callback.visit(this, n, parent);
+    }
+    curScript = null;
+    setChangeScope(null);
+  }
+
+  private void handleFunction(Node n, Node parent) {
+    Node changeScope = this.currentChangeScope;
+    setChangeScope(n);
+    curNode = n;
+    if (callback.shouldTraverse(this, n, parent)) {
+      traverseFunction(n, parent);
+      curNode = n;
+      callback.visit(this, n, parent);
+    }
+    setChangeScope(changeScope);
+  }
+
   /**
    * Traverses a branch.
    */
   private void traverseBranch(Node n, Node parent) {
     Token type = n.getToken();
     if (type == Token.SCRIPT) {
-      setInputId(n.getInputId(), getSourceName(n));
-      compiler.setScope(n);
+      handleScript(n, parent);
+      return;
+    } else if (type == Token.FUNCTION) {
+      handleFunction(n, parent);
+      return;
     }
 
     curNode = n;
@@ -738,7 +762,6 @@ public Node getEnclosingFunction() {
 
   /** Sets the given node as the current scope and pushes the relevant frames on the CFG stacks. */
   private void recordScopeRoot(Node node) {
-    compiler.setScope(node);
     if (NodeUtil.isValidCfgRoot(node)) {
       cfgs.push(node);
     }
@@ -792,10 +815,6 @@ private void popScope(boolean quietly) {
     if (NodeUtil.isValidCfgRoot(scopeRoot)) {
       cfgs.pop();
     }
-    Node newScopeRoot = getScopeRoot();
-    if (newScopeRoot != null) {
-      compiler.setScope(newScopeRoot);
-    }
   }
 
   /** Gets the current scope. */
@@ -927,11 +946,51 @@ public void report(Node n, DiagnosticType diagnosticType,
     compiler.report(error);
   }
 
+  public void reportCodeChange() {
+    Node changeScope = this.currentChangeScope;
+    Preconditions.checkState(changeScope != null && NodeUtil.isChangeScopeRoot(changeScope));
+    compiler.reportChangeToChangeScope(changeScope);
+  }
+
+  public void reportCodeChange(Node n) {
+    compiler.reportChangeToEnclosingScope(n);
+  }
+
   private static String getSourceName(Node n) {
     String name = n.getSourceFileName();
     return nullToEmpty(name);
   }
 
+  /**
+   * @param n The current change scope, should be null when the traversal is complete.
+   */
+  private void setChangeScope(Node n) {
+    this.currentChangeScope = n;
+    // TODO(johnlenz): the compiler is a bad place to store this value
+    // multiple traversals can interfer with each other
+    // (even on the same thread).
+    compiler.setChangeScope(n);
+  }
+
+  private Node getEnclosingScript(Node n) {
+    while (n != null && !n.isScript()) {
+      n = n.getParent();
+    }
+    return n;
+  }
+
+  private void initTraversal(Node traversalRoot) {
+    Node changeScope = NodeUtil.getEnclosingChangeScopeRoot(traversalRoot);
+    setChangeScope(changeScope);
+    Node script = getEnclosingScript(changeScope);
+    if (script != null) {
+      setInputId(script.getInputId(), script.getSourceFileName());
+    } else {
+      setInputId(null, "");
+    }
+    this.curScript = script;
+  }
+
   private void setInputId(InputId id, String sourceName) {
     inputId = id;
     this.sourceName = sourceName;

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

@@ -4707,6 +4707,26 @@ static boolean isNaN(Node n) {
             && n.getLastChild().getDouble() == 0);
   }
 
+  /**
+   * A change scope does not directly correspond to a language scope but is an internal
+   * grouping of changes.
+   *
+   * @return Whether the node represents a change scope root.
+   */
+  static boolean isChangeScopeRoot(Node n) {
+    return (n.isScript() || n.isFunction());
+  }
+
+  /**
+   * @return the change scope root
+   */
+  static Node getEnclosingChangeScopeRoot(Node n) {
+    while (n != null && !isChangeScopeRoot(n)) {
+      n = n.getParent();
+    }
+    return n;
+  }
+
   /**
    * Given an AST and its copy, map the root node of each scope of main to the
    * corresponding root node of clone
@@ -4739,34 +4759,38 @@ public static void verifyScopeChanges(
     final String passNameMsg = passName.isEmpty() ? "" : passName + ": ";
 
     Node clone = mtoc.get(main);
-    if (main.getChangeTime() > clone.getChangeTime()) {
-      Preconditions.checkState(!isEquivalentToExcludingFunctions(main, clone));
-    } else {
-      Preconditions.checkState(isEquivalentToExcludingFunctions(main, clone));
-    }
+    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);
-              if (n.getChangeTime() > clone.getChangeTime()) {
-                Preconditions.checkState(
-                    !isEquivalentToExcludingFunctions(n, clone),
-                    "%sunchanged scope marked as changed",
-                    passNameMsg);
-              } else {
-                Preconditions.checkState(
-                    isEquivalentToExcludingFunctions(n, clone),
-                    "%schanged scope not marked as changed",
-                    passNameMsg);
-              }
+              verifyNodeChange(passNameMsg, n, clone);
             }
           }
         },
         Predicates.<Node>alwaysTrue());
   }
 
+  static void verifyNodeChange(final String passNameMsg, Node n, Node clone) {
+    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.
     final int[] wrappedSize = {0};