Skip to content

Commit

Permalink
perf: improve rule no-cycle using strongly connected components
Browse files Browse the repository at this point in the history
  • Loading branch information
SukkaW committed Jul 11, 2024
1 parent 0f07c36 commit 8211511
Show file tree
Hide file tree
Showing 7 changed files with 266 additions and 1 deletion.
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"eslint": "^8.56.0 || ^9.0.0-0"
},
"dependencies": {
"@rtsao/scc": "^1.1.0",
"@typescript-eslint/utils": "^7.4.0",
"debug": "^4.3.4",
"doctrine": "^3.0.0",
Expand Down
15 changes: 15 additions & 0 deletions src/rules/no-cycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import type { DeclarationMetadata, ModuleOptions } from '../utils'
import {
ExportMap,
StronglyConnectedComponents,
isExternalModule,
createRule,
moduleVisitor,
Expand Down Expand Up @@ -87,6 +88,8 @@ export = createRule<[Options?], MessageId>({
options.ignoreExternal &&
isExternalModule(name, resolve(name, context)!, context)

const scc = StronglyConnectedComponents.get(filename, context);

Check failure on line 92 in src/rules/no-cycle.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

Delete `;`

Check failure on line 92 in src/rules/no-cycle.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

Delete `;`
return {
...moduleVisitor(function checkSourceValue(sourceNode, importer) {
if (ignoreModule(sourceNode.value)) {
Expand Down Expand Up @@ -126,6 +129,18 @@ export = createRule<[Options?], MessageId>({
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.
*/
if (scc) {
const hasDependencyCycle = scc[filename] === scc[imported.path];
if (!hasDependencyCycle) {

Check failure on line 139 in src/rules/no-cycle.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

Delete `;`

Check failure on line 139 in src/rules/no-cycle.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

Delete `;`
return;
}

Check failure on line 141 in src/rules/no-cycle.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

Delete `;`

Check failure on line 141 in src/rules/no-cycle.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

Delete `;`
}

const untraversed: Traverser[] = [{ mget: () => imported, route: [] }]

function detectCycle({ mget, route }: Traverser) {
Expand Down
2 changes: 1 addition & 1 deletion src/utils/export-map.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1083,7 +1083,7 @@ export function recursivePatternCapture(
* don't hold full context object in memory, just grab what we need.
* also calculate a cacheKey, where parts of the cacheKey hash are memoized
*/
function childContext(
export function childContext(
path: string,
context: RuleContext | ChildContext,
): ChildContext {
Expand Down
1 change: 1 addition & 0 deletions src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export * from './pkg-dir'
export * from './pkg-up'
export * from './read-pkg-up'
export * from './resolve'
export * from './scc'
export * from './static-require'
export * from './unambiguous'
export * from './visit'
83 changes: 83 additions & 0 deletions src/utils/scc.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import calculateScc from '@rtsao/scc';

Check failure on line 1 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

There should be at least one empty line between import groups

Check failure on line 1 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

Delete `;`

Check failure on line 1 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

There should be at least one empty line between import groups

Check failure on line 1 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

Delete `;`
import { resolve } from './resolve';

Check failure on line 2 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

Delete `;`

Check failure on line 2 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

Delete `;`
import { ExportMap, childContext } from './export-map';

Check failure on line 3 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

There should be at least one empty line between import groups

Check failure on line 3 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

`./export-map` import should occur before import of `./resolve`

Check failure on line 3 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

Delete `;`

Check failure on line 3 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

There should be at least one empty line between import groups

Check failure on line 3 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

`./export-map` import should occur before import of `./resolve`

Check failure on line 3 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

Delete `;`
import type { ChildContext, RuleContext } from '../types';

Check failure on line 4 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on macos-latest

`../types` type import should occur before import of `./resolve`

Check failure on line 4 in src/utils/scc.ts

View workflow job for this annotation

GitHub Actions / Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest

`../types` type import should occur before import of `./resolve`

let cache = new Map<string, Record<string, number>>();

export class StronglyConnectedComponents {
static clearCache() {
cache.clear()
}

static get(source: string, context: RuleContext) {
const path = resolve(source, context);
if (path == null) { return null; }
return StronglyConnectedComponents.for(childContext(path, context));
}

static for(context: ChildContext) {
const cacheKey = context.cacheKey
if (cache.has(cacheKey)) {
return cache.get(cacheKey)!;
}
const scc = StronglyConnectedComponents.calculate(context);
cache.set(cacheKey, scc);
return scc;
}

static calculate(context: ChildContext) {
const exportMap = ExportMap.for(context);
const adjacencyList = StronglyConnectedComponents.exportMapToAdjacencyList(exportMap);
const calculatedScc = calculateScc(adjacencyList);
return StronglyConnectedComponents.calculatedSccToPlainObject(calculatedScc);
}

static exportMapToAdjacencyList(initialExportMap: ExportMap | null) {
/** for each dep, what are its direct deps */
const adjacencyList = new Map<string, Set<string>>();
// BFS
function visitNode(exportMap: ExportMap | null) {
if (!exportMap) {
return;
}
exportMap.imports.forEach((v, importedPath) => {
const from = exportMap.path;
const to = importedPath;

if (!adjacencyList.has(from)) {
adjacencyList.set(from, new Set());
}

const set = adjacencyList.get(from)!;

if (set.has(to)) {
return; // prevent endless loop
}
set.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;
}

static calculatedSccToPlainObject(sccs: Set<string>[]) {
/** for each key, its SCC's index */
const obj: Record<string, number> = {};
sccs.forEach((scc, index) => {
scc.forEach((node) => {
obj[node] = index;
});
});
return obj;
}
}
160 changes: 160 additions & 0 deletions test/utils/scc.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
// import sinon from 'sinon';
import { StronglyConnectedComponents, ExportMap, childContext as buildChildContext } from 'eslint-plugin-import-x/utils';
import { testContext } from '../utils';

function exportMapFixtureBuilder(path: string, imports: ExportMap[]): ExportMap {
return {
path,
imports: new Map(imports.map((imp) => [imp.path, { getter: () => imp, declarations: new Set() }])),
} as ExportMap;
}

describe('Strongly Connected Components Builder', () => {
afterEach(() => StronglyConnectedComponents.clearCache());

describe('When getting an SCC', () => {
const source = '';
const ruleContext = testContext({});
const childContext = buildChildContext(source, ruleContext);

describe('Given two files', () => {
describe('When they don\'t cycle', () => {
it('Should return foreign SCCs', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [exportMapFixtureBuilder('bar.js', [])]),
);
const actual = StronglyConnectedComponents.for(childContext);
expect(actual).toEqual({ 'foo.js': 1, 'bar.js': 0 });
});
});

describe.skip('When they do cycle', () => {
it('Should return same SCC', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [
exportMapFixtureBuilder('bar.js', [
exportMapFixtureBuilder('foo.js', []),
]),
]),
);
const actual = StronglyConnectedComponents.get(source, ruleContext);
expect(actual).toEqual({ 'foo.js': 0, 'bar.js': 0 });
});
});
});

describe('Given three files', () => {
describe('When they form a line', () => {
describe('When A -> B -> C', () => {
it('Should return foreign SCCs', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [
exportMapFixtureBuilder('bar.js', [
exportMapFixtureBuilder('buzz.js', []),
]),
]),
);
const actual = StronglyConnectedComponents.for(childContext);
expect(actual).toEqual({ 'foo.js': 2, 'bar.js': 1, 'buzz.js': 0 });
});
});

describe('When A -> B <-> C', () => {
it('Should return 2 SCCs, A on its own', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [
exportMapFixtureBuilder('bar.js', [
exportMapFixtureBuilder('buzz.js', [
exportMapFixtureBuilder('bar.js', []),
]),
]),
]),
);
const actual = StronglyConnectedComponents.for(childContext);
expect(actual).toEqual({ 'foo.js': 1, 'bar.js': 0, 'buzz.js': 0 });
});
});

describe('When A <-> B -> C', () => {
it('Should return 2 SCCs, C on its own', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [
exportMapFixtureBuilder('bar.js', [
exportMapFixtureBuilder('buzz.js', []),
exportMapFixtureBuilder('foo.js', []),
]),
]),
);
const actual = StronglyConnectedComponents.for(childContext);
expect(actual).toEqual({ 'foo.js': 1, 'bar.js': 1, 'buzz.js': 0 });
});
});

describe('When A <-> B <-> C', () => {
it('Should return same SCC', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [
exportMapFixtureBuilder('bar.js', [
exportMapFixtureBuilder('foo.js', []),
exportMapFixtureBuilder('buzz.js', [
exportMapFixtureBuilder('bar.js', []),
]),
]),
]),
);
const actual = StronglyConnectedComponents.for(childContext);
expect(actual).toEqual({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 });
});
});
});

describe('When they form a loop', () => {
it('Should return same SCC', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [
exportMapFixtureBuilder('bar.js', [
exportMapFixtureBuilder('buzz.js', [
exportMapFixtureBuilder('foo.js', []),
]),
]),
]),
);
const actual = StronglyConnectedComponents.for(childContext);
expect(actual).toEqual({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 });
});
});

describe('When they form a Y', () => {
it('Should return 3 distinct SCCs', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [
exportMapFixtureBuilder('bar.js', []),
exportMapFixtureBuilder('buzz.js', []),
]),
);
const actual = StronglyConnectedComponents.for(childContext);
expect(actual).toEqual({ 'foo.js': 2, 'bar.js': 0, 'buzz.js': 1 });
});
});

describe('When they form a Mercedes', () => {
it('Should return 1 SCC', () => {
jest.spyOn(ExportMap, 'for').mockReturnValue(
exportMapFixtureBuilder('foo.js', [
exportMapFixtureBuilder('bar.js', [
exportMapFixtureBuilder('foo.js', []),
exportMapFixtureBuilder('buzz.js', []),
]),
exportMapFixtureBuilder('buzz.js', [
exportMapFixtureBuilder('foo.js', []),
exportMapFixtureBuilder('bar.js', []),
]),
]),
);
const actual = StronglyConnectedComponents.for(childContext);
expect(actual).toEqual({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 });
});
});
});
});
});
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1886,6 +1886,11 @@
dependencies:
"@xml-tools/parser" "^1.0.11"

"@rtsao/scc@^1.1.0":
version "1.1.0"
resolved "https://registry.yarnpkg.com/@rtsao/scc/-/scc-1.1.0.tgz#927dd2fae9bc3361403ac2c7a00c32ddce9ad7e8"
integrity sha512-zt6OdqaDoOnJ1ZYsCYGt9YmWzDXl4vQdKTyJev62gFhRGKdx7mcT54V9KIjg+d2wi9EXsPvAPKe7i7WjfVWB8g==

"@sinclair/typebox@^0.27.8":
version "0.27.8"
resolved "https://registry.yarnpkg.com/@sinclair/typebox/-/typebox-0.27.8.tgz#6667fac16c436b5434a387a34dedb013198f6e6e"
Expand Down

0 comments on commit 8211511

Please sign in to comment.