Fix graph delta bugs when a dependency is added+modified+removed / re…

…moved+modified+added within a traversal

Summary:
I came across this edge case while working on module diffing.

Typically:
1. DeltaCalculator aggregates relevant file changes between requests for a delta (e.g., via HMR). (If no client is connected, these changes may accrue over a period of time)
2. When `getDelta` is called, this list of paths is provided to `Graph.traverseDependencies`, which determines the actual graph delta, performing any necessary transformation and dependency traversal.
3. `traverseDependencies` works through these paths sequentially, marking modules as modified and deeply adding/removing dependencies as it goes. It ignores any path that doesn't correspond to a module in the graph.

Previously, a path was added to `delta.modified` if only if it was given in `paths` *and* existed in the graph at the time we came to traverse it as part of the `traverseDependencies` sequence.

## Error cases

This led to issues in a couple of cases:

## Module modified while temporarily NOT part of the graph
For an existing graph with edges `A->B` and `A->C`.
1. The dependency `A->C` is removed, freeing `C` from the graph and adding it to `delta.deleted`. `A` is marked modified.
2. `C` is modified, but traversal skips it because it's not in the graph.
3. The dependency `B->C` is added. `C` is removed from `delta.deleted` and processed. `B` is marked modified.

Result: `{ added: [], modified: [A, B], deleted: [] }` - `C`'s modification is missed.

### Module modified while temporarily part of the graph
For an existing graph with edges `A->B`.
1. The dependency `B->C` is added, processing `C` as a new dependency and adding it to `delta.added`
2. `C` is modified, traversal marks it as modified because it's now part of the graph.
3. The dependency `A->B` is removed. `C` is removed from `delta.added` but remains in `delta.modified`. `A` is marked modified, `B` marked deleted.

Result: `C`'s path remains in `delta.modified` but not `added` or `deleted`, but it's not present in the graph at the end of the traversal. `traverseDependencies` throws, client sees a redbox from `nullthrows` on:
```
modified.set(pathOfC, nullthrows(this.dependencies.get(pathOfC)))
```

## This diff
 - Mark modules as potentially modified when they are **processed**, *either directly or transitively*.
 - Filter modifications to only those from the given `paths` that were present in the graph *before traversal started*. [Building this list feeds nicely into subsequent work on diffing modules.]

Changelog:
```
**[Fix]**: Fix crash on a module added+modified+removed between updates.
**[Fix]**: Fix missed modification on module removed+modified+added between updates.
```

Reviewed By: motiz88

Differential Revision: D45691691

fbshipit-source-id: 4f246582311063650f26678006b6089c778014f4

packages/metro/src/DeltaBundler/Graph.js

@@ -174,11 +174,19 @@ export class Graph<T = MixedOutput> {
 
     const internalOptions = getInternalOptions(options);
 
-    for (const path of paths) {
-      // Start traversing from modules that are already part of the dependency graph.
+    // Record the paths that are part of the dependency graph before we start
+    // traversing - we'll use this to ensure we don't report modules modified
+    // that only exist as part of the graph mid-traversal.
+    const existingPaths = paths.filter(path => this.dependencies.has(path));
+
+    for (const path of existingPaths) {
+      // Traverse over modules that are part of the dependency graph.
+      //
+      // Note: A given path may not be part of the graph *at this time*, in
+      // particular it may have been removed since we started traversing, but
+      // in that case the path will be visited if and when we add it back to
+      // the graph in a subsequent iteration.
       if (this.dependencies.get(path)) {
-        delta.modified.add(path);
-
         await this._traverseDependenciesForSingleFile(
           path,
           delta,
@@ -195,10 +203,23 @@ export class Graph<T = MixedOutput> {
     }
 
     const modified = new Map<string, Module<T>>();
-    for (const path of delta.modified) {
-      // Only report a module as modified if we're not already reporting it as added or deleted.
-      if (!delta.added.has(path) && !delta.deleted.has(path)) {
-        modified.set(path, nullthrows(this.dependencies.get(path)));
+
+    // A path in delta.modified has been processed during this traversal,
+    // but may not actually differ, may be new, or may have been deleted after
+    // processing. The actually-modified modules are the intersection of
+    // delta.modified with the pre-existing paths, minus modules deleted.
+    for (const path of existingPaths) {
+      invariant(
+        !delta.added.has(path),
+        'delta.added has %s, but this path was already in the graph.',
+        path,
+      );
+      if (delta.modified.has(path)) {
+        // Only report a module as modified if we're not already reporting it
+        // as added or deleted.
+        if (!delta.deleted.has(path)) {
+          modified.set(path, nullthrows(this.dependencies.get(path)));
+        }
       }
     }
 
@@ -268,6 +289,11 @@ export class Graph<T = MixedOutput> {
     options: InternalOptions<T>,
   ): Promise<Module<T>> {
     const resolvedContext = this.#resolvedContexts.get(path);
+
+    // Mark any module processed as potentially modified. Once we've finished
+    // traversing we'll filter this set down.
+    delta.modified.add(path);
+
     // Transform the file via the given option.
     // TODO: Unbind the transform method from options
     const result = await options.transform(path, resolvedContext);

packages/metro/src/DeltaBundler/__tests__/Graph-test.js

@@ -675,6 +675,39 @@ describe('edge cases', () => {
     expect(graph.dependencies.get('/baz')).toBe(undefined);
   });
 
+  it('remove a dependency, modify it, and re-add it elsewhere', async () => {
+    await graph.initialTraverseDependencies(options);
+
+    Actions.removeDependency('/foo', '/bar');
+    Actions.modifyFile('/bar');
+    Actions.addDependency('/baz', '/bar');
+
+    expect(
+      getPaths(await graph.traverseDependencies([...files], options)),
+    ).toEqual({
+      added: new Set(),
+      modified: new Set(['/foo', '/bar', '/baz']),
+      deleted: new Set(),
+    });
+  });
+
+  it('Add a dependency, modify it, and remove it', async () => {
+    await graph.initialTraverseDependencies(options);
+
+    Actions.createFile('/quux');
+    Actions.addDependency('/bar', '/quux');
+    Actions.modifyFile('/quux');
+    Actions.removeDependency('/foo', '/bar');
+
+    expect(
+      getPaths(await graph.traverseDependencies([...files], options)),
+    ).toEqual({
+      added: new Set(),
+      modified: new Set(['/foo']),
+      deleted: new Set(['/bar']),
+    });
+  });
+
   it('removes a cyclic dependency but should not remove any dependency', async () => {
     Actions.createFile('/bar1');
     Actions.addDependency('/bar', '/bar1');