Fix incremental build bug with parallel edges to the same module

Summary:
When two imports from the same origin module resolve to the same target module, Metro can get a little confused.

Consider the following example:
```
              import './Module/index';
  ┌─────────────────────────────────────────┐
  │                                         ▼
┌───────────┐  import './Module';         ┌──────────────────┐
│ /Entry.js │ ──────────────────────────▶ │ /Module/index.js │
└───────────┘                             └──────────────────┘
```

If, while the app is running, we delete *one* of the imports but not the other, Metro will stop correctly propagating updates of `/Module/index.js` to its "parent", `/Entry.js`. Fast Refresh will **appear** to work (the UI indicator will flash briefly) but the app will reflect the *old* contents of `/Module/index.js`.

This happens because DeltaBundler's data structure for keeping track of a module's inverse dependencies is a set keyed on absolute paths, which doesn't account for parallel edges ( = multiple inverse dependencies from the same origin module).

Here we change this data structure to a `CountingSet` - a modified `Set` that only deletes items when the number of `delete(item)` calls matches the number of `add(item)` calls.

Reviewed By: huntie

Differential Revision: D37194640

fbshipit-source-id: 89c190b0de3ec75b533f2d41a7024f17d31c59b0

Author: motiz88

packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/bytecode-test.js

@@ -11,6 +11,8 @@
 
 'use strict';
 
+import CountingSet from '../../../../lib/CountingSet';
+
 const createModuleIdFactory = require('../../../../lib/createModuleIdFactory');
 const {wrapModule} = require('../bytecode');
 const {compile, validateBytecodeModule} = require('metro-hermes-compiler');
@@ -43,7 +45,7 @@ beforeEach(() => {
       ],
     ]),
     getSource: () => Buffer.from(''),
-    inverseDependencies: new Set(),
+    inverseDependencies: new CountingSet(),
     output: [
       {
         data: {

packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/js-test.js

@@ -11,6 +11,8 @@
 
 'use strict';
 
+import CountingSet from '../../../../lib/CountingSet';
+
 const createModuleIdFactory = require('../../../../lib/createModuleIdFactory');
 const {wrapModule} = require('../js');
 
@@ -36,7 +38,7 @@ beforeEach(() => {
       ],
     ]),
     getSource: () => Buffer.from(''),
-    inverseDependencies: new Set(),
+    inverseDependencies: new CountingSet(),
     output: [
       {
         data: {

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

@@ -17,7 +17,7 @@ Object {
         },
       },
       "getSource": [Function],
-      "inverseDependencies": Set {},
+      "inverseDependencies": Array [],
       "output": Array [
         Object {
           "data": Object {
@@ -54,9 +54,9 @@ Object {
         },
       },
       "getSource": [Function],
-      "inverseDependencies": Set {
+      "inverseDependencies": Array [
         "/bundle",
-      },
+      ],
       "output": Array [
         Object {
           "data": Object {
@@ -72,9 +72,9 @@ Object {
     "/bar" => Object {
       "dependencies": Map {},
       "getSource": [Function],
-      "inverseDependencies": Set {
+      "inverseDependencies": Array [
         "/foo",
-      },
+      ],
       "output": Array [
         Object {
           "data": Object {
@@ -90,9 +90,9 @@ Object {
     "/baz" => Object {
       "dependencies": Map {},
       "getSource": [Function],
-      "inverseDependencies": Set {
+      "inverseDependencies": Array [
         "/foo",
-      },
+      ],
       "output": Array [
         Object {
           "data": Object {
@@ -139,7 +139,7 @@ Object {
         },
       },
       "getSource": [Function],
-      "inverseDependencies": Set {},
+      "inverseDependencies": Array [],
       "output": Array [
         Object {
           "data": Object {

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

@@ -11,6 +11,7 @@
 
 import type {Graph, TransformResultDependency} from '../types.flow';
 
+import CountingSet from '../../lib/CountingSet';
 import nullthrows from 'nullthrows';
 
 const {
@@ -202,7 +203,7 @@ async function traverseDependencies(paths, graph, options) {
   );
   const actualInverseDependencies = new Map();
   for (const [path, module] of graph.dependencies) {
-    actualInverseDependencies.set(path, module.inverseDependencies);
+    actualInverseDependencies.set(path, new Set(module.inverseDependencies));
   }
   expect(actualInverseDependencies).toEqual(expectedInverseDependencies);
 
@@ -314,7 +315,7 @@ it('should populate all the inverse dependencies', async () => {
 
   expect(
     nullthrows(graph.dependencies.get('/bar')).inverseDependencies,
-  ).toEqual(new Set(['/foo', '/bundle']));
+  ).toEqual(new CountingSet(['/foo', '/bundle']));
 });
 
 it('should return an empty result when there are no changes', async () => {
@@ -501,7 +502,7 @@ describe('edge cases', () => {
 
     expect(
       nullthrows(graph.dependencies.get('/foo')).inverseDependencies,
-    ).toEqual(new Set(['/bundle', '/baz']));
+    ).toEqual(new CountingSet(['/baz', '/bundle']));
   });
 
   it('should handle renames correctly', async () => {
@@ -2049,7 +2050,7 @@ describe('reorderGraph', () => {
       getSource: () => Buffer.from('// source'),
       // NOTE: inverseDependencies is traversal state/output, not input, so we
       // don't pre-populate it.
-      inverseDependencies: new Set(),
+      inverseDependencies: new CountingSet(),
     });
 
     const graph = createGraph({
@@ -2168,3 +2169,99 @@ describe('optional dependencies', () => {
     ).rejects.toThrow();
   });
 });
+
+describe('parallel edges', () => {
+  it('add twice w/ same key, build and remove once', async () => {
+    // Create a second edge between /foo and /bar.
+    Actions.addDependency('/foo', '/bar', undefined);
+
+    await initialTraverseDependencies(graph, options);
+
+    const fooDeps = nullthrows(graph.dependencies.get('/foo')).dependencies;
+    const fooDepsResolved = [...fooDeps.values()].map(dep => dep.absolutePath);
+    // We dedupe the dependencies because they have the same `name`.
+    expect(fooDepsResolved).toEqual(['/bar', '/baz']);
+
+    // Remove one of the edges between /foo and /bar (arbitrarily)
+    Actions.removeDependency('/foo', '/bar');
+
+    expect(
+      getPaths(await traverseDependencies([...files], graph, options)),
+    ).toEqual({
+      added: new Set(),
+      modified: new Set(['/foo']),
+      deleted: new Set(),
+    });
+  });
+
+  it('add twice w/ same key, build and remove twice', async () => {
+    // Create a second edge between /foo and /bar.
+    Actions.addDependency('/foo', '/bar', undefined);
+
+    await initialTraverseDependencies(graph, options);
+
+    const fooDeps = nullthrows(graph.dependencies.get('/foo')).dependencies;
+    const fooDepsResolved = [...fooDeps.values()].map(dep => dep.absolutePath);
+    // We dedupe the dependencies because they have the same `name`.
+    expect(fooDepsResolved).toEqual(['/bar', '/baz']);
+
+    // Remove both edges between /foo and /bar
+    Actions.removeDependency('/foo', '/bar');
+    Actions.removeDependency('/foo', '/bar');
+
+    expect(
+      getPaths(await traverseDependencies([...files], graph, options)),
+    ).toEqual({
+      added: new Set(),
+      modified: new Set(['/foo']),
+      deleted: new Set(['/bar']),
+    });
+  });
+
+  it('add twice w/ different keys, build and remove once', async () => {
+    // Create a second edge between /foo and /bar, with a different `name`.
+    Actions.addDependency('/foo', '/bar', undefined, 'bar-second');
+
+    await initialTraverseDependencies(graph, options);
+
+    const fooDeps = nullthrows(graph.dependencies.get('/foo')).dependencies;
+    const fooDepsResolved = [...fooDeps.values()].map(dep => dep.absolutePath);
+    // We don't dedupe the dependencies because they have different `name`s.
+    expect(fooDepsResolved).toEqual(['/bar', '/baz', '/bar']);
+
+    // Remove one of the edges between /foo and /bar (arbitrarily)
+    Actions.removeDependency('/foo', '/bar');
+
+    expect(
+      getPaths(await traverseDependencies([...files], graph, options)),
+    ).toEqual({
+      added: new Set(),
+      modified: new Set(['/foo']),
+      deleted: new Set(),
+    });
+  });
+
+  it('add twice w/ different keys, build and remove twice', async () => {
+    // Create a second edge between /foo and /bar, with a different `name`.
+    Actions.addDependency('/foo', '/bar', undefined, 'bar-second');
+
+    await initialTraverseDependencies(graph, options);
+
+    const fooDeps = nullthrows(graph.dependencies.get('/foo')).dependencies;
+    const fooDepsResolved = [...fooDeps.values()].map(dep => dep.absolutePath);
+    // We don't dedupe the dependencies because they have different `name`s.
+    expect(fooDepsResolved).toEqual(['/bar', '/baz', '/bar']);
+
+    // Remove both edges between /foo and /bar
+    Actions.removeDependency('/foo', '/bar');
+    Actions.removeDependency('/foo', '/bar');
+
+    expect(
+      getPaths(await traverseDependencies([...files], graph, options)),
+    ).toEqual({
+      added: new Set(),
+      modified: new Set(['/foo']),
+      deleted: new Set(['/bar']),
+    });
+  });
+});

packages/metro/src/DeltaBundler/graphOperations.js

@@ -39,6 +39,8 @@ import type {
   TransformResultDependency,
 } from './types.flow';
 
+import CountingSet from '../lib/CountingSet';
+
 const invariant = require('invariant');
 const nullthrows = require('nullthrows');
 
@@ -104,7 +106,7 @@ type Delta = $ReadOnly<{
 
   // A place to temporarily track inverse dependencies for a module while it is
   // being processed but has not been added to `graph.dependencies` yet.
-  earlyInverseDependencies: Map<string, Set<string>>,
+  earlyInverseDependencies: Map<string, CountingSet<string>>,
 }>;
 
 type InternalOptions<T> = $ReadOnly<{
@@ -277,7 +279,8 @@ async function processModule<T>(
   );
 
   const previousModule = graph.dependencies.get(path) || {
-    inverseDependencies: delta.earlyInverseDependencies.get(path) || new Set(),
+    inverseDependencies:
+      delta.earlyInverseDependencies.get(path) || new CountingSet(),
     path,
   };
   const previousDependencies = previousModule.dependencies || new Map();
@@ -397,7 +400,7 @@ async function addDependency<T>(
           delta.added.add(path);
           delta.modified.delete(path);
         }
-        delta.earlyInverseDependencies.set(path, new Set([parentModule.path]));
+        delta.earlyInverseDependencies.set(path, new CountingSet());
 
         options.onDependencyAdd();
         module = await processModule(path, graph, delta, options);

packages/metro/src/DeltaBundler/types.flow.js

@@ -14,6 +14,8 @@ import type {RequireContextParams} from '../ModuleGraph/worker/collectDependenci
 import type {PrivateState} from './graphOperations';
 import type {JsTransformOptions} from 'metro-transform-worker';
 
+import CountingSet from '../lib/CountingSet';
+
 export type MixedOutput = {
   +data: mixed,
   +type: string,
@@ -62,7 +64,7 @@ export type Dependency = {
 
 export type Module<T = MixedOutput> = {
   +dependencies: Map<string, Dependency>,
-  +inverseDependencies: Set<string>,
+  +inverseDependencies: CountingSet<string>,
   +output: $ReadOnlyArray<T>,
   +path: string,
   +getSource: () => Buffer,

packages/metro/src/integration_tests/__tests__/buildGraph-test.js

@@ -10,6 +10,8 @@
 
 'use strict';
 
+import CountingSet from '../../lib/CountingSet';
+
 const Metro = require('../../..');
 const path = require('path');
 
@@ -50,7 +52,7 @@ it('should build the dependency graph', async () => {
   expect(graph.dependencies.get(entryPoint)).toEqual(
     expect.objectContaining({
       path: entryPoint,
-      inverseDependencies: new Set(),
+      inverseDependencies: new CountingSet(),
       output: [
         expect.objectContaining({
           type: 'js/module',

packages/metro/src/lib/CountingSet.js

@@ -0,0 +1,126 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow strict-local
+ * @format
+ */
+
+export interface ReadOnlyCountingSet<T> extends Iterable<T> {
+  has(item: T): boolean;
+  @@iterator(): Iterator<T>;
+  +size: number;
+  count(item: T): number;
+  forEach<ThisT>(
+    callbackFn: (
+      this: ThisT,
+      value: T,
+      key: T,
+      set: ReadOnlyCountingSet<T>,
+    ) => mixed,
+
+    // NOTE: Should be optional, but Flow seems happy to infer undefined here
+    // which is what we want.
+    thisArg: ThisT,
+  ): void;
+}
+
+/**
+ * A Set that only deletes a given item when the number of delete(item) calls
+ * matches the number of add(item) calls. Iteration and `size` are in terms of
+ * *unique* items.
+ */
+export default class CountingSet<T> implements ReadOnlyCountingSet<T> {
+  #map: Map<T, number> = new Map();
+
+  constructor(items?: Iterable<T>) {
+    if (items) {
+      if (items instanceof CountingSet) {
+        this.#map = new Map(items.#map);
+      } else {
+        for (const item of items) {
+          this.add(item);
+        }
+      }
+    }
+  }
+
+  has(item: T): boolean {
+    return this.#map.has(item);
+  }
+
+  add(item: T): void {
+    const newCount = this.count(item) + 1;
+    this.#map.set(item, newCount);
+  }
+
+  delete(item: T): void {
+    const newCount = this.count(item) - 1;
+    if (newCount <= 0) {
+      this.#map.delete(item);
+    } else {
+      this.#map.set(item, newCount);
+    }
+  }
+
+  keys(): Iterator<T> {
+    return this.#map.keys();
+  }
+
+  values(): Iterator<T> {
+    return this.#map.keys();
+  }
+
+  *entries(): Iterator<[T, T]> {
+    for (const item of this) {
+      yield [item, item];
+    }
+  }
+
+  // Iterate over unique entries
+  // $FlowIssue[unsupported-syntax]
+  [Symbol.iterator](): Iterator<T> {
+    return this.values();
+  }
+
+  /*::
+  // For Flow's benefit
+  @@iterator(): Iterator<T> {
+    return this.values();
+  }
+  */
+
+  // Number of unique entries
+  // $FlowIssue[unsafe-getters-setters]
+  get size(): number {
+    return this.#map.size;
+  }
+
+  count(item: T): number {
+    return this.#map.get(item) ?? 0;
+  }
+
+  clear(): void {
+    this.#map.clear();
+  }
+
+  forEach<ThisT>(
+    callbackFn: (this: ThisT, value: T, key: T, set: CountingSet<T>) => mixed,
+    thisArg: ThisT,
+  ): void {
+    for (const item of this) {
+      callbackFn.call(thisArg, item, item, this);
+    }
+  }
+
+  // For Jest purposes. Ideally a custom serializer would be enough, but in
+  // practice there is hardcoded magic for Set in toEqual (etc) that we cannot
+  // extend to custom collection classes. Instead let's assume values are
+  // sortable ( = strings) and make this look like an array with some stable
+  // order.
+  toJSON(): mixed {
+    return [...this].sort();
+  }
+}

packages/metro/src/lib/__tests__/CountingSet-test.js

@@ -0,0 +1,200 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow strict-local
+ * @format
+ * @emails oncall+metro_bundler
+ */
+
+import CountingSet from '../CountingSet';
+
+describe('CountingSet', () => {
+  test('basic add/delete', () => {
+    const set = new CountingSet();
+
+    set.add('a');
+    expect(set.has('a')).toBe(true);
+    expect(set.count('a')).toBe(1);
+    expect(set.size).toBe(1);
+
+    set.delete('a');
+    expect(set.has('a')).toBe(false);
+    expect(set.count('a')).toBe(0);
+    expect(set.size).toBe(0);
+  });
+
+  test('multiple add/delete', () => {
+    const set = new CountingSet();
+
+    set.add('a');
+    set.add('a');
+    expect(set.has('a')).toBe(true);
+    expect(set.count('a')).toBe(2);
+    expect(set.size).toBe(1);
+
+    set.delete('a');
+    expect(set.has('a')).toBe(true);
+    expect(set.count('a')).toBe(1);
+    expect(set.size).toBe(1);
+
+    set.delete('a');
+    expect(set.has('a')).toBe(false);
+    expect(set.count('a')).toBe(0);
+    expect(set.size).toBe(0);
+  });
+
+  test('more deletes than adds', () => {
+    const set = new CountingSet();
+
+    set.add('a');
+    set.delete('a');
+    set.delete('a');
+    expect(set.has('a')).toBe(false);
+    expect(set.count('a')).toBe(0);
+    expect(set.size).toBe(0);
+  });
+
+  test('delete nonexistent value', () => {
+    const set = new CountingSet();
+
+    set.delete('a');
+    expect(set.has('a')).toBe(false);
+    expect(set.count('a')).toBe(0);
+    expect(set.size).toBe(0);
+  });
+
+  test('construct from array', () => {
+    const set = new CountingSet(['a', 'b', 'c', 'a']);
+    expect(set.has('a')).toBe(true);
+    expect(set.has('b')).toBe(true);
+    expect(set.has('c')).toBe(true);
+    expect(set.count('a')).toBe(2);
+    expect(set.count('b')).toBe(1);
+    expect(set.count('c')).toBe(1);
+    expect(set.size).toBe(3);
+  });
+
+  test('construct from Set', () => {
+    const set = new CountingSet(new Set(['a', 'b', 'c']));
+    expect(set.has('a')).toBe(true);
+    expect(set.has('b')).toBe(true);
+    expect(set.has('c')).toBe(true);
+    expect(set.count('a')).toBe(1);
+    expect(set.count('b')).toBe(1);
+    expect(set.count('c')).toBe(1);
+    expect(set.size).toBe(3);
+  });
+
+  test('construct from CountingSet', () => {
+    const originalSet = new CountingSet(['a', 'a', 'b', 'c']);
+    const set = new CountingSet(originalSet);
+    originalSet.clear();
+
+    expect(set.has('a')).toBe(true);
+    expect(set.has('b')).toBe(true);
+    expect(set.has('c')).toBe(true);
+    expect(set.count('a')).toBe(2);
+    expect(set.count('b')).toBe(1);
+    expect(set.count('c')).toBe(1);
+    expect(set.size).toBe(3);
+  });
+
+  test('clear', () => {
+    const set = new CountingSet(['a', 'a', 'b', 'c']);
+
+    set.clear();
+    expect(set.size).toBe(0);
+    expect(set.has('a')).toBe(false);
+    expect(set.count('a')).toBe(0);
+    expect(set.has('b')).toBe(false);
+    expect(set.count('b')).toBe(0);
+    expect(set.has('c')).toBe(false);
+    expect(set.count('c')).toBe(0);
+  });
+
+  test('forEach', () => {
+    const set = new CountingSet(['a', 'a', 'b', 'c']);
+    // TODO: Migrate to callback.mock.contexts when we upgrade to Jest 28
+    const contexts = [];
+    const callback = jest.fn(function captureContext() {
+      contexts.push(this);
+    });
+
+    set.forEach(callback);
+    expect(callback.mock.calls).toEqual([
+      ['a', 'a', set],
+      ['b', 'b', set],
+      ['c', 'c', set],
+    ]);
+    expect(contexts).toEqual([undefined, undefined, undefined]);
+  });
+
+  test('forEach with context', () => {
+    const set = new CountingSet(['a', 'a', 'b', 'c']);
+    // TODO: Migrate to callback.mock.contexts when we upgrade to Jest 28
+    const contexts = [];
+    const callback = jest.fn(function captureContext() {
+      contexts.push(this);
+    });
+
+    const context = {};
+    set.forEach(callback, context);
+    expect(callback.mock.calls).toEqual([
+      ['a', 'a', set],
+      ['b', 'b', set],
+      ['c', 'c', set],
+    ]);
+    expect(contexts).toEqual([context, context, context]);
+  });
+
+  test('spread', () => {
+    const set = new CountingSet();
+
+    set.add('a');
+    set.add('a');
+    set.add('b');
+    set.add('c');
+
+    expect([...set]).toEqual(['a', 'b', 'c']);
+  });
+
+  test('keys()', () => {
+    const set = new CountingSet();
+
+    set.add('a');
+    set.add('a');
+    set.add('b');
+    set.add('c');
+
+    expect([...set.keys()]).toEqual(['a', 'b', 'c']);
+  });
+
+  test('values()', () => {
+    const set = new CountingSet();
+
+    set.add('a');
+    set.add('a');
+    set.add('b');
+    set.add('c');
+
+    expect([...set.values()]).toEqual(['a', 'b', 'c']);
+  });
+
+  test('entries()', () => {
+    const set = new CountingSet();
+
+    set.add('a');
+    set.add('a');
+    set.add('b');
+    set.add('c');
+
+    expect([...set.entries()]).toEqual([
+      ['a', 'a'],
+      ['b', 'b'],
+      ['c', 'c'],
+    ]);
+  });
+});

packages/metro/src/lib/getAppendScripts.js

@@ -12,6 +12,8 @@
 
 import type {Module} from '../DeltaBundler';
 
+import CountingSet from './CountingSet';
+
 const getInlineSourceMappingURL = require('../DeltaBundler/Serializers/helpers/getInlineSourceMappingURL');
 const sourceMapString = require('../DeltaBundler/Serializers/sourceMapString');
 const countLines = require('./countLines');
@@ -56,7 +58,7 @@ function getAppendScripts<T: number | string>(
       path: '$$importBundleNames',
       dependencies: new Map(),
       getSource: (): Buffer => Buffer.from(''),
-      inverseDependencies: new Set(),
+      inverseDependencies: new CountingSet(),
       output: [
         {
           type: 'js/script/virtual',
@@ -82,7 +84,7 @@ function getAppendScripts<T: number | string>(
           path: `require-${path}`,
           dependencies: new Map(),
           getSource: (): Buffer => Buffer.from(''),
-          inverseDependencies: new Set(),
+          inverseDependencies: new CountingSet(),
           output: [
             {
               type: 'js/script/virtual',
@@ -113,7 +115,7 @@ function getAppendScripts<T: number | string>(
       path: 'source-map',
       dependencies: new Map(),
       getSource: (): Buffer => Buffer.from(''),
-      inverseDependencies: new Set(),
+      inverseDependencies: new CountingSet(),
       output: [
         {
           type: 'js/script/virtual',
@@ -133,7 +135,7 @@ function getAppendScripts<T: number | string>(
       path: 'source-url',
       dependencies: new Map(),
       getSource: (): Buffer => Buffer.from(''),
-      inverseDependencies: new Set(),
+      inverseDependencies: new CountingSet(),
       output: [
         {
           type: 'js/script/virtual',

packages/metro/src/lib/getPrependedScripts.js

@@ -15,6 +15,8 @@ import type DeltaBundler, {Module} from '../DeltaBundler';
 import type {TransformInputOptions} from '../DeltaBundler/types.flow';
 import type {ConfigT} from 'metro-config/src/configTypes.flow';
 
+import CountingSet from './CountingSet';
+
 const countLines = require('./countLines');
 const getPreludeCode = require('./getPreludeCode');
 const transformHelpers = require('./transformHelpers');
@@ -88,7 +90,7 @@ function _getPrelude({
   return {
     dependencies: new Map(),
     getSource: (): Buffer => Buffer.from(code),
-    inverseDependencies: new Set(),
+    inverseDependencies: new CountingSet(),
     path: name,
     output: [
       {