From 98a0991aa248216fb904cc88d11aa9070ccb6249 Mon Sep 17 00:00:00 2001 From: Ivan Rubinson Date: Mon, 8 Apr 2024 20:50:56 +0300 Subject: [PATCH] [New] [Refactor] `no-cycle`: use scc algorithm to optimize; add `skipErrorMessagePath` for faster error messages --- CHANGELOG.md | 2 + package.json | 1 + src/rules/no-cycle.js | 21 +++++ src/scc.js | 86 +++++++++++++++++ tests/src/rules/no-cycle.js | 28 +++++- tests/src/scc.js | 179 ++++++++++++++++++++++++++++++++++++ 6 files changed, 316 insertions(+), 1 deletion(-) create mode 100644 src/scc.js create mode 100644 tests/src/scc.js diff --git a/CHANGELOG.md b/CHANGELOG.md index b1d2a3425..0ba781c23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange ### Fixed - [`no-extraneous-dependencies`]: allow wrong path ([#3012], thanks [@chabb]) +- [`no-cycle`]: use scc algorithm to optimize ([#2998], thanks [@soryy708]) ### Changed - [Docs] `no-extraneous-dependencies`: Make glob pattern description more explicit ([#2944], thanks [@mulztob]) @@ -1123,6 +1124,7 @@ for info on changes for earlier releases. [#3012]: https://github.com/import-js/eslint-plugin-import/pull/3012 [#3011]: https://github.com/import-js/eslint-plugin-import/pull/3011 [#3004]: https://github.com/import-js/eslint-plugin-import/pull/3004 +[#2998]: https://github.com/import-js/eslint-plugin-import/pull/2998 [#2991]: https://github.com/import-js/eslint-plugin-import/pull/2991 [#2989]: https://github.com/import-js/eslint-plugin-import/pull/2989 [#2987]: https://github.com/import-js/eslint-plugin-import/pull/2987 diff --git a/package.json b/package.json index de0055a09..2aaa78653 100644 --- a/package.json +++ b/package.json @@ -105,6 +105,7 @@ "eslint": "^2 || ^3 || ^4 || ^5 || ^6 || ^7.2.0 || ^8" }, "dependencies": { + "@rtsao/scc": "^1.1.0", "array-includes": "^3.1.8", "array.prototype.findlastindex": "^1.2.5", "array.prototype.flat": "^1.3.2", diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index e65ff11a4..be8c288dd 100644 --- a/src/rules/no-cycle.js +++ b/src/rules/no-cycle.js @@ -5,6 +5,7 @@ import resolve from 'eslint-module-utils/resolve'; import ExportMapBuilder from '../exportMap/builder'; +import StronglyConnectedComponentsBuilder from '../scc'; import { isExternalModule } from '../core/importType'; import moduleVisitor, { makeOptionsSchema } from 'eslint-module-utils/moduleVisitor'; import docsUrl from '../docsUrl'; @@ -47,6 +48,11 @@ module.exports = { type: 'boolean', default: false, }, + disableScc: { + description: 'When true, don\'t calculate a strongly-connected-components graph. SCC is used to reduce the time-complexity of cycle detection, but adds overhead.', + type: 'boolean', + default: false, + }, })], }, @@ -62,6 +68,8 @@ module.exports = { context, ); + const scc = options.disableScc ? {} : StronglyConnectedComponentsBuilder.get(myPath, context); + function checkSourceValue(sourceNode, importer) { if (ignoreModule(sourceNode.value)) { return; // ignore external modules @@ -98,6 +106,16 @@ module.exports = { return; // no-self-import territory } + /* If we're in the same Strongly Connected Component, + * Then there exists a path from each node in the SCC to every other node in the SCC, + * Then there exists at least one path from them to us and from us to them, + * Then we have a cycle between us. + */ + const hasDependencyCycle = options.disableScc || scc[myPath] === scc[imported.path]; + if (!hasDependencyCycle) { + return; + } + const untraversed = [{ mget: () => imported, route: [] }]; function detectCycle({ mget, route }) { const m = mget(); @@ -106,6 +124,9 @@ module.exports = { traversed.add(m.path); for (const [path, { getter, declarations }] of m.imports) { + // If we're in different SCCs, we can't have a circular dependency + if (!options.disableScc && scc[myPath] !== scc[path]) { continue; } + if (traversed.has(path)) { continue; } const toTraverse = [...declarations].filter(({ source, isOnlyImportingTypes }) => !ignoreModule(source.value) // Ignore only type imports diff --git a/src/scc.js b/src/scc.js new file mode 100644 index 000000000..44c818bbe --- /dev/null +++ b/src/scc.js @@ -0,0 +1,86 @@ +import calculateScc from '@rtsao/scc'; +import { hashObject } from 'eslint-module-utils/hash'; +import resolve from 'eslint-module-utils/resolve'; +import ExportMapBuilder from './exportMap/builder'; +import childContext from './exportMap/childContext'; + +let cache = new Map(); + +export default class StronglyConnectedComponentsBuilder { + static clearCache() { + cache = new Map(); + } + + static get(source, context) { + const path = resolve(source, context); + if (path == null) { return null; } + return StronglyConnectedComponentsBuilder.for(childContext(path, context)); + } + + static for(context) { + const cacheKey = context.cacheKey || hashObject(context).digest('hex'); + if (cache.has(cacheKey)) { + return cache.get(cacheKey); + } + const scc = StronglyConnectedComponentsBuilder.calculate(context); + cache.set(cacheKey, scc); + return scc; + } + + static calculate(context) { + const exportMap = ExportMapBuilder.for(context); + const adjacencyList = this.exportMapToAdjacencyList(exportMap); + const calculatedScc = calculateScc(adjacencyList); + return StronglyConnectedComponentsBuilder.calculatedSccToPlainObject(calculatedScc); + } + + /** @returns {Map>} for each dep, what are its direct deps */ + static exportMapToAdjacencyList(initialExportMap) { + const adjacencyList = new Map(); + // BFS + function visitNode(exportMap) { + if (!exportMap) { + return; + } + exportMap.imports.forEach((v, importedPath) => { + const from = exportMap.path; + const to = importedPath; + + // Ignore type-only imports, because we care only about SCCs of value imports + const toTraverse = [...v.declarations].filter(({ isOnlyImportingTypes }) => !isOnlyImportingTypes); + if (toTraverse.length === 0) { return; } + + if (!adjacencyList.has(from)) { + adjacencyList.set(from, new Set()); + } + + if (adjacencyList.get(from).has(to)) { + return; // prevent endless loop + } + adjacencyList.get(from).add(to); + visitNode(v.getter()); + }); + } + visitNode(initialExportMap); + // Fill gaps + adjacencyList.forEach((values) => { + values.forEach((value) => { + if (!adjacencyList.has(value)) { + adjacencyList.set(value, new Set()); + } + }); + }); + return adjacencyList; + } + + /** @returns {Record} for each key, its SCC's index */ + static calculatedSccToPlainObject(sccs) { + const obj = {}; + sccs.forEach((scc, index) => { + scc.forEach((node) => { + obj[node] = index; + }); + }); + return obj; + } +} diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js index d2adbf61f..efc0fb6eb 100644 --- a/tests/src/rules/no-cycle.js +++ b/tests/src/rules/no-cycle.js @@ -17,7 +17,7 @@ const testVersion = (specifier, t) => _testVersion(specifier, () => Object.assig const testDialects = ['es6']; -ruleTester.run('no-cycle', rule, { +const cases = { valid: [].concat( // this rule doesn't care if the cycle length is 0 test({ code: 'import foo from "./foo.js"' }), @@ -290,4 +290,30 @@ ruleTester.run('no-cycle', rule, { ], }), ), +}; + +ruleTester.run('no-cycle', rule, { + valid: flatMap(cases.valid, (testCase) => [ + testCase, + { + ...testCase, + code: `${testCase.code} // disableScc=true`, + options: [{ + ...testCase.options && testCase.options[0] || {}, + disableScc: true, + }], + }, + ]), + + invalid: flatMap(cases.invalid, (testCase) => [ + testCase, + { + ...testCase, + code: `${testCase.code} // disableScc=true`, + options: [{ + ...testCase.options && testCase.options[0] || {}, + disableScc: true, + }], + }, + ]), }); diff --git a/tests/src/scc.js b/tests/src/scc.js new file mode 100644 index 000000000..376b783ce --- /dev/null +++ b/tests/src/scc.js @@ -0,0 +1,179 @@ +import sinon from 'sinon'; +import { expect } from 'chai'; +import StronglyConnectedComponentsBuilder from '../../src/scc'; +import ExportMapBuilder from '../../src/exportMap/builder'; + +function exportMapFixtureBuilder(path, imports, isOnlyImportingTypes = false) { + return { + path, + imports: new Map(imports.map((imp) => [imp.path, { getter: () => imp, declarations: [{ isOnlyImportingTypes }] }])), + }; +} + +describe('Strongly Connected Components Builder', () => { + afterEach(() => ExportMapBuilder.for.restore()); + afterEach(() => StronglyConnectedComponentsBuilder.clearCache()); + + describe('When getting an SCC', () => { + const source = ''; + const context = { + settings: {}, + parserOptions: {}, + parserPath: '', + }; + + describe('Given two files', () => { + describe('When they don\'t value-cycle', () => { + it('Should return foreign SCCs', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [exportMapFixtureBuilder('bar.js', [])]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 1, 'bar.js': 0 }); + }); + }); + + describe('When they do value-cycle', () => { + it('Should return same SCC', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', [ + exportMapFixtureBuilder('foo.js', [exportMapFixtureBuilder('bar.js', [])]), + ]), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 0, 'bar.js': 0 }); + }); + }); + + describe('When they type-cycle', () => { + it('Should return foreign SCCs', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', [ + exportMapFixtureBuilder('foo.js', []), + ], true), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 1, 'bar.js': 0 }); + }); + }); + }); + + describe('Given three files', () => { + describe('When they form a line', () => { + describe('When A -> B -> C', () => { + it('Should return foreign SCCs', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', [ + exportMapFixtureBuilder('buzz.js', []), + ]), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 2, 'bar.js': 1, 'buzz.js': 0 }); + }); + }); + + describe('When A -> B <-> C', () => { + it('Should return 2 SCCs, A on its own', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', [ + exportMapFixtureBuilder('buzz.js', [ + exportMapFixtureBuilder('bar.js', []), + ]), + ]), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 1, 'bar.js': 0, 'buzz.js': 0 }); + }); + }); + + describe('When A <-> B -> C', () => { + it('Should return 2 SCCs, C on its own', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', [ + exportMapFixtureBuilder('buzz.js', []), + exportMapFixtureBuilder('foo.js', []), + ]), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 1, 'bar.js': 1, 'buzz.js': 0 }); + }); + }); + + describe('When A <-> B <-> C', () => { + it('Should return same SCC', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', [ + exportMapFixtureBuilder('foo.js', []), + exportMapFixtureBuilder('buzz.js', [ + exportMapFixtureBuilder('bar.js', []), + ]), + ]), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 }); + }); + }); + }); + + describe('When they form a loop', () => { + it('Should return same SCC', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', [ + exportMapFixtureBuilder('buzz.js', [ + exportMapFixtureBuilder('foo.js', []), + ]), + ]), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 }); + }); + }); + + describe('When they form a Y', () => { + it('Should return 3 distinct SCCs', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', []), + exportMapFixtureBuilder('buzz.js', []), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 2, 'bar.js': 0, 'buzz.js': 1 }); + }); + }); + + describe('When they form a Mercedes', () => { + it('Should return 1 SCC', () => { + sinon.stub(ExportMapBuilder, 'for').returns( + exportMapFixtureBuilder('foo.js', [ + exportMapFixtureBuilder('bar.js', [ + exportMapFixtureBuilder('foo.js', []), + exportMapFixtureBuilder('buzz.js', []), + ]), + exportMapFixtureBuilder('buzz.js', [ + exportMapFixtureBuilder('foo.js', []), + exportMapFixtureBuilder('bar.js', []), + ]), + ]), + ); + const actual = StronglyConnectedComponentsBuilder.for(source, context); + expect(actual).to.deep.equal({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 }); + }); + }); + }); + }); +});