From b6db7497f5a303e1b821db9029acff4cf4c1c2e9 Mon Sep 17 00:00:00 2001 From: Volker Braun Date: Fri, 15 Jan 2021 17:13:11 +0100 Subject: [PATCH 1/3] Separate out the dependency graph construction into a separate function --- index.js | 103 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 62 insertions(+), 41 deletions(-) diff --git a/index.js b/index.js index 32c7be9..daa4383 100644 --- a/index.js +++ b/index.js @@ -24,18 +24,11 @@ class CircularDependencyPlugin { if (plugin.options.onStart) { plugin.options.onStart({ compilation }); } - for (let module of modules) { - const shouldSkip = ( - module.resource == null || - plugin.options.exclude.test(module.resource) || - !plugin.options.include.test(module.resource) - ) - // skip the module if it matches the exclude pattern - if (shouldSkip) { - continue - } - let maybeCyclicalPathsList = this.isCyclic(module, module, {}, compilation) + const [vertices, arrow] = webpackDependencyGraph(compilation, modules, plugin, this.options); + + for (let module of vertices) { + let maybeCyclicalPathsList = this.isCyclic(module, module, {}, arrow) if (maybeCyclicalPathsList) { // allow consumers to override all behavior with onDetected if (plugin.options.onDetected) { @@ -67,7 +60,7 @@ class CircularDependencyPlugin { }) } - isCyclic(initialModule, currentModule, seenModules, compilation) { + isCyclic(initialModule, currentModule, seenModules, arrow) { let cwd = this.options.cwd // Add the current module to the seen modules cache @@ -80,34 +73,7 @@ class CircularDependencyPlugin { } // Iterate over the current modules dependencies - for (let dependency of currentModule.dependencies) { - if ( - dependency.constructor && - dependency.constructor.name === 'CommonJsSelfReferenceDependency' - ) { - continue - } - - let depModule = null - if (compilation.moduleGraph) { - // handle getting a module for webpack 5 - depModule = compilation.moduleGraph.getModule(dependency) - } else { - // handle getting a module for webpack 4 - depModule = dependency.module - } - - if (!depModule) { continue } - // ignore dependencies that don't have an associated resource - if (!depModule.resource) { continue } - // ignore dependencies that are resolved asynchronously - if (this.options.allowAsyncCycles && dependency.weak) { continue } - // the dependency was resolved to the current module due to how webpack internals - // setup dependencies like CommonJsSelfReferenceDependency and ModuleDecoratorDependency - if (currentModule === depModule) { - continue - } - + for (let depModule of arrow(currentModule)) { if (depModule.debugId in seenModules) { if (depModule.debugId === initialModule.debugId) { // Initial module has a circular dependency @@ -120,7 +86,7 @@ class CircularDependencyPlugin { continue } - let maybeCyclicalPathsList = this.isCyclic(initialModule, depModule, seenModules, compilation) + let maybeCyclicalPathsList = this.isCyclic(initialModule, depModule, seenModules, arrow) if (maybeCyclicalPathsList) { maybeCyclicalPathsList.unshift(path.relative(cwd, currentModule.resource)) return maybeCyclicalPathsList @@ -131,4 +97,59 @@ class CircularDependencyPlugin { } } + +/** + * Construct the dependency (directed) graph for the given plugin options + * + * Returns the graph as a pair [vertices, arrow] where + * - vertices is an array containing all vertices, and + * - arrow is a function mapping vertices to the array of dependencies, that is, + * the head vertex for each graph edge whose tail is the given vertex. + */ +function webpackDependencyGraph(compilation, modules, plugin, options) { + + // vertices of the dependency graph are the modules + const vertices = modules.filter((module) => + module.resource != null && + !plugin.options.exclude.test(module.resource) && + plugin.options.include.test(module.resource) + ); + + // arrow function for the dependency graph + const arrow = (module) => module.dependencies + .filter((dependency) => { + // ignore CommonJsSelfReferenceDependency + if (dependency.constructor && + dependency.constructor.name === 'CommonJsSelfReferenceDependency') { + return false; + } + // ignore dependencies that are resolved asynchronously + if (options.allowAsyncCycles && dependency.weak) { return false } + return true; + }) + .map((dependency) => { + // map webpack dependency to module + if (compilation.moduleGraph) { + // handle getting a module for webpack 5 + return compilation.moduleGraph.getModule(dependency) + } else { + // handle getting a module for webpack 4 + return dependency.module + } + }) + .filter((depModule) => { + if (!depModule) { return false } + // ignore dependencies that don't have an associated resource + if (!depModule.resource) { return false } + // the dependency was resolved to the current module due to how webpack internals + // setup dependencies like CommonJsSelfReferenceDependency and ModuleDecoratorDependency + if (module === depModule) { + return false + } + return true; + }); + + return [vertices, arrow]; +} + module.exports = CircularDependencyPlugin From 8a7e1a61f7c40223d39ad0ab9e624f2efe0c768a Mon Sep 17 00:00:00 2001 From: Volker Braun Date: Fri, 15 Jan 2021 17:35:41 +0100 Subject: [PATCH 2/3] Refactor listing of cycles into a separate function --- index.js | 130 ++++++++++++++++++++++++++++++++----------------------- 1 file changed, 75 insertions(+), 55 deletions(-) diff --git a/index.js b/index.js index daa4383..9362be7 100644 --- a/index.js +++ b/index.js @@ -27,81 +27,45 @@ class CircularDependencyPlugin { const [vertices, arrow] = webpackDependencyGraph(compilation, modules, plugin, this.options); - for (let module of vertices) { - let maybeCyclicalPathsList = this.isCyclic(module, module, {}, arrow) - if (maybeCyclicalPathsList) { - // allow consumers to override all behavior with onDetected - if (plugin.options.onDetected) { - try { - plugin.options.onDetected({ - module: module, - paths: maybeCyclicalPathsList, - compilation: compilation - }) - } catch(err) { - compilation.errors.push(err) - } - continue + cycleDetector(vertices, arrow, (cycle) => { + // print modules as paths in error messages + const cyclicalPaths = cycle.map( + (module) => path.relative(cwd, module.resource)); + // allow consumers to override all behavior with onDetected + if (plugin.options.onDetected) { + try { + plugin.options.onDetected({ + module: module, + paths: cyclicalPaths, + compilation: compilation + }) + } catch(err) { + compilation.errors.push(err) } - + } else { // mark warnings or errors on webpack compilation - let error = new Error(BASE_ERROR.concat(maybeCyclicalPathsList.join(' -> '))) + let error = new Error(BASE_ERROR.concat(cyclicalPaths.join(' -> '))) if (plugin.options.failOnError) { compilation.errors.push(error) } else { compilation.warnings.push(error) } } - } + }); + if (plugin.options.onEnd) { plugin.options.onEnd({ compilation }); } }) }) } - - isCyclic(initialModule, currentModule, seenModules, arrow) { - let cwd = this.options.cwd - - // Add the current module to the seen modules cache - seenModules[currentModule.debugId] = true - - // If the modules aren't associated to resources - // it's not possible to display how they are cyclical - if (!currentModule.resource || !initialModule.resource) { - return false - } - - // Iterate over the current modules dependencies - for (let depModule of arrow(currentModule)) { - if (depModule.debugId in seenModules) { - if (depModule.debugId === initialModule.debugId) { - // Initial module has a circular dependency - return [ - path.relative(cwd, currentModule.resource), - path.relative(cwd, depModule.resource) - ] - } - // Found a cycle, but not for this module - continue - } - - let maybeCyclicalPathsList = this.isCyclic(initialModule, depModule, seenModules, arrow) - if (maybeCyclicalPathsList) { - maybeCyclicalPathsList.unshift(path.relative(cwd, currentModule.resource)) - return maybeCyclicalPathsList - } - } - - return false - } } /** * Construct the dependency (directed) graph for the given plugin options * - * Returns the graph as a pair [vertices, arrow] where + * Returns the graph as a pair (vertices, arrow) where * - vertices is an array containing all vertices, and * - arrow is a function mapping vertices to the array of dependencies, that is, * the head vertex for each graph edge whose tail is the given vertex. @@ -152,4 +116,60 @@ function webpackDependencyGraph(compilation, modules, plugin, options) { return [vertices, arrow]; } + +/** + * Call the callback with all cycles in the directed graph defined by (vertices, arrow) + * + * The graph is acyclic iff the callback is not called. + */ +function cycleDetector(vertices, arrow, cycleCallback) { + /** + * This is the old implementation which has performance issues. In the future, it might + * be replaced with a different implementation. Keep it for now so none of the unit tests change. + */ + for (let module of vertices) { + let cycle = findModuleCycleAt(module, module, {}, arrow) + if (cycle) { + cycleCallback(cycle); + } + } +} + + +// part of the cycleDetector implementation +function findModuleCycleAt(initialModule, currentModule, seenModules, arrow) { + + // Add the current module to the seen modules cache + seenModules[currentModule.debugId] = true + + // If the modules aren't associated to resources + // it's not possible to display how they are cyclical + if (!currentModule.resource || !initialModule.resource) { + return false + } + + // Iterate over the current modules dependencies + for (let depModule of arrow(currentModule)) { + if (depModule.debugId in seenModules) { + if (depModule.debugId === initialModule.debugId) { + // Initial module has a circular dependency + return [ + currentModule, + depModule, + ] + } + // Found a cycle, but not for this module + continue + } + + let maybeCyclicalPathsList = findModuleCycleAt(initialModule, depModule, seenModules, arrow) + if (maybeCyclicalPathsList) { + maybeCyclicalPathsList.unshift(currentModule); + return maybeCyclicalPathsList; + } + } + return false +} + + module.exports = CircularDependencyPlugin From 2e782f046771e8b483f9f2d46090b2c60165eaff Mon Sep 17 00:00:00 2001 From: Volker Braun Date: Fri, 15 Jan 2021 19:38:06 +0100 Subject: [PATCH 3/3] Use the fast depth-first search to stop early if there are no cycles --- __tests__/is-acyclic.test.js | 112 +++++++++++++++++++++++++++++++++++ index.js | 19 +++--- is-acyclic.js | 69 +++++++++++++++++++++ 3 files changed, 193 insertions(+), 7 deletions(-) create mode 100644 __tests__/is-acyclic.test.js create mode 100644 is-acyclic.js diff --git a/__tests__/is-acyclic.test.js b/__tests__/is-acyclic.test.js new file mode 100644 index 0000000..8122f8b --- /dev/null +++ b/__tests__/is-acyclic.test.js @@ -0,0 +1,112 @@ +let { isAcyclic, depthFirstIterator } = require('../is-acyclic'); + + +function makeGraph(adjacencyList) { + const vertices = adjacencyList.map((v) => v[0]); + const arrow = (vertex) => { + for (const adj of adjacencyList) + if (adj[0] === vertex) + return adj[1]; + throw new Error('invalid vertex: ' + vertex); + } + return { + vertices, + arrow, + } +} + + +describe('makeGraph', () => { + const graph = makeGraph([ + ['u', ['v', 'x']], + ['v', ['y']], + ['w', ['y', 'z']], + ['x', ['v']], + ['y', ['x']], + ['z', ['z']], + ]); + + it('constructs vertices', () => { + expect(graph.vertices).toEqual(['u', 'v', 'w', 'x', 'y', 'z']) + }); + + it('constructs arrow function', () => { + expect(graph.arrow('u')).toEqual(['v', 'x']) + expect(graph.arrow('v')).toEqual(['y']) + expect(graph.arrow('w')).toEqual(['y', 'z']) + expect(graph.arrow('x')).toEqual(['v']) + expect(graph.arrow('y')).toEqual(['x']) + expect(graph.arrow('z')).toEqual(['z']) + }); +}) + + +describe('depthFirstIterator', () => { + + const graph = makeGraph([ + ['u', ['v', 'x']], + ['v', ['y']], + ['w', ['y', 'z']], + ['x', ['v']], + ['y', ['x']], + ['z', ['z']], + ]); + + it('traverses all vertices', () => { + vertices = new Set(); + depthFirstIterator( + graph, + (vertex, adjacent) => vertices.add(vertex), + (u, v) => {}, + ) + expect(vertices).toEqual(new Set(graph.vertices)); + }); + + it('follows this path', () => { + const path = []; + depthFirstIterator( + graph, + (vertex, adjacent) => path.push({ vertex, adjacent }), + (u, v) => path.push({ backEdge: [u, v] }), + ) + expect(path).toEqual([ + { vertex: 'u', adjacent: ['v', 'x'] }, + { vertex: 'v', adjacent: ['y'] }, + { vertex: 'y', adjacent: ['x'] }, + { vertex: 'x', adjacent: ['v'] }, + { backEdge: ['x', 'v'] }, + { vertex: 'w', adjacent: ['y', 'z'] }, + { vertex: 'z', adjacent: ['z'] }, + { backEdge: ['z', 'z'] }, + ]); + }); +}); + + +describe('isAcyclic', () => { + + it('finds no cycle in tree', () => { + // note: adjacent vertices are alphabetically higher => tree graph + const graph = makeGraph([ + ['u', ['v', 'x']], + ['v', ['y']], + ['w', ['y', 'z']], + ['x', []], + ['y', []], + ['z', []], + ]); + expect(isAcyclic(graph)).toBe(true); + }); + + it('finds cycle in complicated graph', () => { + const graph = makeGraph([ + ['u', ['v', 'x']], + ['v', ['y']], + ['w', ['y', 'z']], + ['x', ['v']], + ['y', ['x']], + ['z', ['z']], + ]); + expect(isAcyclic(graph)).toBe(false); + }); +}); diff --git a/index.js b/index.js index 9362be7..a2c8470 100644 --- a/index.js +++ b/index.js @@ -2,6 +2,7 @@ let path = require('path') let extend = require('util')._extend let BASE_ERROR = 'Circular dependency detected:\r\n' let PluginTitle = 'CircularDependencyPlugin' +let { isAcyclic } = require('./is-acyclic'); class CircularDependencyPlugin { constructor(options) { @@ -25,9 +26,9 @@ class CircularDependencyPlugin { plugin.options.onStart({ compilation }); } - const [vertices, arrow] = webpackDependencyGraph(compilation, modules, plugin, this.options); + const graph = webpackDependencyGraph(compilation, modules, plugin, this.options); - cycleDetector(vertices, arrow, (cycle) => { + cycleDetector(graph, (cycle) => { // print modules as paths in error messages const cyclicalPaths = cycle.map( (module) => path.relative(cwd, module.resource)); @@ -65,7 +66,7 @@ class CircularDependencyPlugin { /** * Construct the dependency (directed) graph for the given plugin options * - * Returns the graph as a pair (vertices, arrow) where + * Returns the graph as a object { vertices, arrow } where * - vertices is an array containing all vertices, and * - arrow is a function mapping vertices to the array of dependencies, that is, * the head vertex for each graph edge whose tail is the given vertex. @@ -113,7 +114,7 @@ function webpackDependencyGraph(compilation, modules, plugin, options) { return true; }); - return [vertices, arrow]; + return { vertices, arrow }; } @@ -122,13 +123,17 @@ function webpackDependencyGraph(compilation, modules, plugin, options) { * * The graph is acyclic iff the callback is not called. */ -function cycleDetector(vertices, arrow, cycleCallback) { +function cycleDetector(graph, cycleCallback) { + // checking that there are no cycles is much faster than actually listing cycles + if (isAcyclic(graph)) + return; + /** * This is the old implementation which has performance issues. In the future, it might * be replaced with a different implementation. Keep it for now so none of the unit tests change. */ - for (let module of vertices) { - let cycle = findModuleCycleAt(module, module, {}, arrow) + for (let module of graph.vertices) { + let cycle = findModuleCycleAt(module, module, {}, graph.arrow) if (cycle) { cycleCallback(cycle); } diff --git a/is-acyclic.js b/is-acyclic.js new file mode 100644 index 0000000..00a0127 --- /dev/null +++ b/is-acyclic.js @@ -0,0 +1,69 @@ + + +/** + * Test whether a directed graph is acyclic + * + * The graph is represented as a object { vertices, arrow } where + * - vertices is an array containing all vertices, and + * - arrow is a function mapping a tail vertex to the array of head vertices, that is, + * the vertices at the head of each graph edge whose tail is the given vertex. + * + * For example: + * + * x <- y <- z + * + * vertices == [x, y, z] (order does not matter) + * arrow(x) == [] + * arrow(y) == [x] + * arrow(z) == [y] + * + * See https://stackoverflow.com/questions/261573/best-algorithm-for-detecting-cycles-in-a-directed-graph + */ +function isAcyclic(graph) { + let isAcyclic = true; + depthFirstIterator( + graph, + () => {}, + (backEdge) => isAcyclic = false); + return isAcyclic; +} + + +/** + * Depth-first traversal of the graph + * + * The visitor function is called with vertex, adjacent vertices + * The backEdge function is called with head, tail of a back edge + */ +function depthFirstIterator(graph, visitorFn, backEdgeFn) { + const discovered = new Set(); + const finished = new Set(); + for (const vertex of graph.vertices) { + if (!(discovered.has(vertex) || finished.has(vertex))) + depthFirstVisitor(vertex, discovered, finished, graph, visitorFn, backEdgeFn) + } +} + + +function depthFirstVisitor(vertex, discovered, finished, graph, visitorFn, backEdgeFn) { + discovered.add(vertex) + const adjacent = graph.arrow(vertex); // the adjacent vertices in the direction of the edges + visitorFn(vertex, adjacent); + + for (const v of adjacent) { + if (discovered.has(v)) { + backEdgeFn(vertex, v); + } else { + if (!finished.has(v)) + depthFirstVisitor(v, discovered, finished, graph, visitorFn, backEdgeFn) + } + } + discovered.delete(vertex) + finished.add(vertex) +} + + +module.exports = { + isAcyclic, + depthFirstIterator, +}