Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize no-cycle rule using strongly connected components #2998

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.7",
"array.prototype.findlastindex": "^1.2.4",
"array.prototype.flat": "^1.3.2",
Expand Down
16 changes: 16 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 @@ -62,6 +63,8 @@
context,
);

const scc = StronglyConnectedComponentsBuilder.get(myPath, context);
ljharb marked this conversation as resolved.
Show resolved Hide resolved

function checkSourceValue(sourceNode, importer) {
if (ignoreModule(sourceNode.value)) {
return; // ignore external modules
Expand Down Expand Up @@ -98,6 +101,16 @@
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 = scc[myPath] === scc[imported.path];
if (!hasDependencyCycle) {
return;

Check warning on line 111 in src/rules/no-cycle.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-cycle.js#L111

Added line #L111 was not covered by tests
}

const untraversed = [{ mget: () => imported, route: [] }];
function detectCycle({ mget, route }) {
const m = mget();
Expand All @@ -106,6 +119,9 @@
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 (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();
ljharb marked this conversation as resolved.
Show resolved Hide resolved
}

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());
ljharb marked this conversation as resolved.
Show resolved Hide resolved
}

if (adjacencyList.get(from).has(to)) {
return; // prevent endless loop

Check warning on line 58 in src/scc.js

View check run for this annotation

Codecov / codecov/patch

src/scc.js#L58

Added line #L58 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this line can’t happen, there’s no loop to prevent, so should this check be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that it shouldn't happen doesn't mean that it can't.
I don't remember why I put it in here, either I actually experienced an endless loop (in which case it can happen), or it protects against regressions on code mutation

}
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;
}
}
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', []),
]),
]),
);
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 });
});
});
});
});
});