Fix - removing an async dependency may incorrectly free its target

Summary:
Metro's core `Graph` maintains a `CountingSet` of inverse "eager" dependencies for each module - zero inverse dependencies triggers a module's removal from the graph.

Currently, this is not decremented with on dependency removals consistently with the way it is incremented. In particular, removing an async dependency (in combination with `options.lazy`) removes an inverse dependency, even though the reverse does not add one.

This leads to incorrect deletion of module B from the graph when module A has both sync and async dependencies on B, and the async dependency is removed.

```
* **[Fix]**: Fix Fast Refresh edge case where removing an async dependency could incorrectly remove modules from the graph.
```

Reviewed By: motiz88

Differential Revision: D50309038

fbshipit-source-id: 4a44703ac31fc400736c5666a51ca26783a34d19

Author: robhogan

packages/metro/src/DeltaBundler/Graph.js

@@ -485,16 +485,19 @@ export class Graph<T = MixedOutput> {
       return;
     }
 
+    const module = this.dependencies.get(absolutePath);
+
     if (options.lazy && dependency.data.data.asyncType != null) {
       this._decrementImportBundleReference(dependency, parentModule);
+    } else if (module) {
+      // Decrement inverseDependencies only if the dependency is not async,
+      // mirroring the increment conditions in _addDependency.
+      module.inverseDependencies.delete(parentModule.path);
     }
 
-    const module = this.dependencies.get(absolutePath);
-
     if (!module) {
       return;
     }
-    module.inverseDependencies.delete(parentModule.path);
     if (
       module.inverseDependencies.size > 0 ||
       this.entryPoints.has(absolutePath)

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

@@ -103,9 +103,10 @@ const Actions = {
       name?: string,
       data?: DependencyDataInput,
     } = {},
-  ) {
-    Actions.addInferredDependency(path, dependencyPath, options);
+  ): string {
+    const key = Actions.addInferredDependency(path, dependencyPath, options);
     files.add(path);
+    return key;
   },
 
   addInferredDependency(
@@ -120,15 +121,15 @@ const Actions = {
       name?: string,
       data?: DependencyDataInput,
     } = {},
-  ): void {
+  ): string {
     if (!mockedDependencies.has(path)) {
       Actions.createFile(path);
     }
     const deps = getMockDependency(path);
     const depName = name ?? dependencyPath.replace('/', '');
     const key = require('crypto')
       .createHash('sha1')
-      .update(depName)
+      .update([depName, data?.asyncType ?? '(null)'].join('\0'))
       .digest('base64');
     const dep = {
       name: depName,
@@ -149,11 +150,16 @@ const Actions = {
 
     mockedDependencyTree.set(path, deps);
     mockedDependencies.add(dependencyPath);
+    return key;
   },
 
   removeDependency(path: string, dependencyPath: string) {
     Actions.removeInferredDependency(path, dependencyPath);
+    files.add(path);
+  },
 
+  removeDependencyByKey(path: string, key: string) {
+    Actions.removeInferredDependencyByKey(path, key);
     files.add(path);
   },
 
@@ -166,6 +172,15 @@ const Actions = {
       mockedDependencyTree.set(path, deps);
     }
   },
+
+  removeInferredDependencyByKey(path: string, key: string) {
+    const deps = nullthrows(mockedDependencyTree.get(path));
+    const index = deps.findIndex(({data}) => data.key === key);
+    if (index !== -1) {
+      deps.splice(index, 1);
+      mockedDependencyTree.set(path, deps);
+    }
+  },
 };
 
 function deferred(
@@ -1545,6 +1560,48 @@ describe('edge cases', () => {
       });
     });
 
+    it('removing an async dependency preserves existing sync dependency', async () => {
+      const asyncDependencyKey = Actions.addDependency('/bundle', '/foo', {
+        data: {
+          asyncType: 'async',
+        },
+      });
+      /*
+      ┌─────────┐ ───▶ ┌──────┐     ┌──────┐
+      │ /bundle │      │ /foo │ ──▶ │ /bar │
+      └─────────┘ ···▶ └──────┘     └──────┘
+                          │
+                          │
+                          ▼
+                       ┌──────┐
+                       │ /baz │
+                       └──────┘
+      */
+      await graph.initialTraverseDependencies(localOptions);
+      files.clear();
+      Actions.removeDependencyByKey('/bundle', asyncDependencyKey);
+
+      // The synchronous dependency remains, /foo should not be removed.
+      /*
+      ┌─────────┐     ┌──────┐     ┌──────┐
+      │ /bundle │ ──▶ │ /foo │ ──▶ │ /bar │
+      └─────────┘     └──────┘     └──────┘
+                        │
+                        │
+                        ▼
+                      ┌──────┐
+                      │ /baz │
+                      └──────┘
+      */
+      expect(
+        getPaths(await graph.traverseDependencies([...files], localOptions)),
+      ).toEqual({
+        added: new Set([]),
+        deleted: new Set([]),
+        modified: new Set(['/bundle']),
+      });
+    });
+
     it('changing a sync dependency to async is a deletion', async () => {
       /*
       ┌─────────┐     ┌──────┐     ┌──────┐

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

@@ -5,12 +5,12 @@ TestGraph {
   "dependencies": Map {
     "/bundle" => Object {
       "dependencies": Map {
-        "C+7Hteo/D9vJXQ3UfzxbwnXaijM=" => Object {
+        "LB7P4TKrvfdUdViBXGaVopqz7Os=" => Object {
           "absolutePath": "/foo",
           "data": Object {
             "data": Object {
               "asyncType": null,
-              "key": "C+7Hteo/D9vJXQ3UfzxbwnXaijM=",
+              "key": "LB7P4TKrvfdUdViBXGaVopqz7Os=",
               "locs": Array [],
             },
             "name": "foo",
@@ -34,23 +34,23 @@ TestGraph {
     },
     "/foo" => Object {
       "dependencies": Map {
-        "Ys23Ag/5IOWqZCw9QGaVDdHwH00=" => Object {
+        "W+de6an7x9bzpev84O0W/hS4K8U=" => Object {
           "absolutePath": "/bar",
           "data": Object {
             "data": Object {
               "asyncType": null,
-              "key": "Ys23Ag/5IOWqZCw9QGaVDdHwH00=",
+              "key": "W+de6an7x9bzpev84O0W/hS4K8U=",
               "locs": Array [],
             },
             "name": "bar",
           },
         },
-        "u+lgol6jEdIdQGaek98gA7qbkKI=" => Object {
+        "x6e9Oz1JO0QPfIBBjUad2qqGFjI=" => Object {
           "absolutePath": "/baz",
           "data": Object {
             "data": Object {
               "asyncType": null,
-              "key": "u+lgol6jEdIdQGaek98gA7qbkKI=",
+              "key": "x6e9Oz1JO0QPfIBBjUad2qqGFjI=",
               "locs": Array [],
             },
             "name": "baz",
@@ -132,12 +132,12 @@ TestGraph {
   "dependencies": Map {
     "/bundle" => Object {
       "dependencies": Map {
-        "C+7Hteo/D9vJXQ3UfzxbwnXaijM=" => Object {
+        "LB7P4TKrvfdUdViBXGaVopqz7Os=" => Object {
           "absolutePath": "/foo",
           "data": Object {
             "data": Object {
               "asyncType": null,
-              "key": "C+7Hteo/D9vJXQ3UfzxbwnXaijM=",
+              "key": "LB7P4TKrvfdUdViBXGaVopqz7Os=",
               "locs": Array [],
             },
             "name": "foo",