Leverages deletion tracking to address a TODO in change tracking.

Now automatically removes items from the change timeline in the
moment they are deleted.

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

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

@@ -2555,17 +2555,6 @@ public int getChangeStamp() {
   List<Node> getChangedScopeNodesForPass(String passName) {
     List<Node> changedScopeNodes = changeTimeline.getSince(passName);
     changeTimeline.mark(passName);
-
-    // TODO(stalcup): do this better when the change tracking system knows about deleted scopes.
-    if (changedScopeNodes != null) {
-      // Filter out scope nodes that have been removed from the AST.
-      for (Iterator<Node> iterator = changedScopeNodes.iterator(); iterator.hasNext(); ) {
-        if (!NodeUtil.isAttached(iterator.next())) {
-          iterator.remove();
-        }
-      }
-    }
-
     return changedScopeNodes;
   }
 
@@ -2639,9 +2628,10 @@ public void reportChangeToChangeScope(Node changeScopeRoot) {
   }
 
   @Override
-  public void reportFunctionDeleted(Node node) {
-    checkState(node.isFunction());
-    node.setDeleted(true);
+  public void reportFunctionDeleted(Node n) {
+    checkState(n.isFunction());
+    n.setDeleted(true);
+    changeTimeline.remove(n);
   }
 
   @Override

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

@@ -4919,14 +4919,4 @@ static void markFunctionsDeleted(Node node, AbstractCompiler compiler) {
       markFunctionsDeleted(child, compiler);
     }
   }
-
-  static boolean isAttached(Node node) {
-    if (node == null) {
-      return false;
-    }
-    if (node.isRoot()) {
-      return true;
-    }
-    return isAttached(node.getParent());
-  }
 }

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

@@ -98,6 +98,26 @@ void add(T value) {
     headEvent = addEvent(value, eventsByValue, headEvent);
   }
 
+  void remove(T value) {
+    Event<T> event = eventsByValue.remove(value);
+
+    // If the event already exists somewhere else in the history then...
+    if (event != null) {
+      // make the rest of the list not reference it...
+      if (event.nextEvent != null) {
+        event.nextEvent.previousEvent = event.previousEvent;
+      } else {
+        // if it was the head element then back the head reference up one node.
+        headEvent = event.previousEvent;
+      }
+      event.previousEvent.nextEvent = event.nextEvent;
+
+      // make it not reference the rest of the list.
+      event.nextEvent = null;
+      event.previousEvent = null;
+    }
+  }
+
   void mark(String timeName) {
     headEvent = addEvent(new Time(timeName), eventsByTime, headEvent);
   }

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

@@ -1123,19 +1123,28 @@ public void testReportChangeWithScopeSucceeds() {
   }
 
   /** See TimelineTest.java for the many behavior tests that don't make sense to duplicate here. */
-  public void testGetChangedScopeNodesForPass_dropsUnattached() {
+  public void testGetChangedScopeNodesForPass_doesntIncludeDeleted() {
     Compiler compiler = new Compiler();
-    Node attachedFunctionNode = IR.function(IR.name("foo"), IR.paramList(), IR.block());
-    Node unattachedFunctionNode = IR.function(IR.name("foo"), IR.paramList(), IR.block());
-    IR.root(IR.script(attachedFunctionNode));
+    Node function1 = IR.function(IR.name("foo"), IR.paramList(), IR.block());
+    Node function2 = IR.function(IR.name("foo"), IR.paramList(), IR.block());
+    IR.root(IR.script(function1, function2));
 
+    // In the initial state everything is attached and nothing has been marked changed.
     assertThat(compiler.getChangedScopeNodesForPass("FunctionInliner")).isNull();
 
-    compiler.reportChangeToChangeScope(attachedFunctionNode);
-    compiler.reportChangeToChangeScope(unattachedFunctionNode);
+    // Mark both functions changed, then delete function2 and mark it deleted.
+    compiler.reportChangeToChangeScope(function1);
+    compiler.reportChangeToChangeScope(function2);
+    function2.detach();
+    compiler.reportFunctionDeleted(function2);
+
+    // Only function1 will be seen as changed, since deleting something removes it from the changed
+    // items list.
     assertThat(compiler.getChangedScopeNodesForPass("FunctionInliner"))
-        .containsExactly(attachedFunctionNode);
+        .containsExactly(function1);
 
+    // Asking for changes again will now show nothing, since no changes have occurred between these
+    // consecutive "FunctionInliner" markings.
     assertThat(compiler.getChangedScopeNodesForPass("FunctionInliner")).isEmpty();
   }
 

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

@@ -45,6 +45,7 @@ public boolean equals(Object obj) {
 
   private static final Event ATE_GREEN_EGGS = new Event(1);
   private static final Event ATE_HAM = new Event(2);
+  private static final Event ATE_HASHBROWNS = new Event(3);
   private Timeline<Event> timeline;
 
   @Override
@@ -53,6 +54,24 @@ protected void setUp() throws Exception {
     timeline = new Timeline<>();
   }
 
+  public void testRemove_simple() {
+    timeline.mark("Monday");
+    timeline.add(ATE_GREEN_EGGS);
+    timeline.remove(ATE_GREEN_EGGS);
+
+    assertThat(timeline.getSince("Monday")).isEmpty();
+  }
+
+  public void testRemove_maintainsOrder() {
+    timeline.mark("Monday");
+    timeline.add(ATE_GREEN_EGGS);
+    timeline.add(ATE_HAM);
+    timeline.add(ATE_HASHBROWNS);
+    timeline.remove(ATE_HAM);
+
+    assertThat(timeline.getSince("Monday")).containsExactly(ATE_GREEN_EGGS, ATE_HASHBROWNS);
+  }
+
   public void testUnknownTimesReturnNull() {
     assertNull(timeline.getSince("Monday"));
   }