From 5d7305e2f3a9f5f4aebc889a452afb03b1db12a7 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Wed, 10 May 2023 08:25:39 -0700 Subject: [PATCH] Fix graph delta bugs when a dependency is added+modified+removed / removed+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 | 42 +++++++++++++++---- .../src/DeltaBundler/__tests__/Graph-test.js | 33 +++++++++++++++ 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/packages/metro/src/DeltaBundler/Graph.js b/packages/metro/src/DeltaBundler/Graph.js index d2f9b1f454..3361c9d759 100644 --- a/packages/metro/src/DeltaBundler/Graph.js +++ b/packages/metro/src/DeltaBundler/Graph.js @@ -174,11 +174,19 @@ export class Graph { 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 { } const modified = new Map>(); - 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 { options: InternalOptions, ): Promise> { 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); diff --git a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js b/packages/metro/src/DeltaBundler/__tests__/Graph-test.js index 79e846b8ec..10b8f07a76 100644 --- a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js +++ b/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');