Skip to content

Commit

Permalink
[New] [Refactor] no-cycle: use scc algorithm to optimize; add `skip…
Browse files Browse the repository at this point in the history
…ErrorMessagePath` for faster error messages
  • Loading branch information
soryy708 authored and ljharb committed Apr 8, 2024
1 parent 19dbc33 commit 98a0991
Show file tree
Hide file tree
Showing 6 changed files with 316 additions and 1 deletion.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
21 changes: 21 additions & 0 deletions src/rules/no-cycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
},
})],
},

Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand Down
86 changes: 86 additions & 0 deletions src/scc.js
Original file line number Diff line number Diff line change
@@ -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<string, Set<string>>} 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<string, number>} for each key, its SCC's index */
static calculatedSccToPlainObject(sccs) {
const obj = {};
sccs.forEach((scc, index) => {
scc.forEach((node) => {
obj[node] = index;
});
});
return obj;
}
}
28 changes: 27 additions & 1 deletion tests/src/rules/no-cycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"' }),
Expand Down Expand Up @@ -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,
}],
},
]),
});
179 changes: 179 additions & 0 deletions tests/src/scc.js
Original file line number Diff line number Diff line change
@@ -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 });
});
});
});
});
});

0 comments on commit 98a0991

Please sign in to comment.