Skip to content

Commit 5cce946

Browse files
SukkaWSukkaW
authored
perf: improve rule no-cycle using strongly connected components (#111)
1 parent fe3121a commit 5cce946

File tree

8 files changed

+312
-1
lines changed

8 files changed

+312
-1
lines changed

‎.changeset/silent-pumas-sell.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"eslint-plugin-import-x": patch
3+
---
4+
5+
Drastically improve `no-cycle`'s performance by skipping unnecessary BFSes using [Tarjan's SCC](https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm).

‎package.json

+1
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
"eslint": "^8.56.0 || ^9.0.0-0"
4949
},
5050
"dependencies": {
51+
"@rtsao/scc": "^1.1.0",
5152
"@typescript-eslint/utils": "^7.4.0",
5253
"debug": "^4.3.4",
5354
"doctrine": "^3.0.0",

‎src/rules/no-cycle.ts

+15
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import type { DeclarationMetadata, ModuleOptions } from '../utils'
66
import {
77
ExportMap,
8+
StronglyConnectedComponents,
89
isExternalModule,
910
createRule,
1011
moduleVisitor,
@@ -88,6 +89,8 @@ export = createRule<[Options?], MessageId>({
8889
isExternalModule(name, resolve(name, context)!, context)
8990
: () => false
9091

92+
const scc = StronglyConnectedComponents.get(filename, context)
93+
9194
return {
9295
...moduleVisitor(function checkSourceValue(sourceNode, importer) {
9396
if (ignoreModule(sourceNode.value)) {
@@ -127,6 +130,18 @@ export = createRule<[Options?], MessageId>({
127130
return // no-self-import territory
128131
}
129132

133+
/* If we're in the same Strongly Connected Component,
134+
* Then there exists a path from each node in the SCC to every other node in the SCC,
135+
* Then there exists at least one path from them to us and from us to them,
136+
* Then we have a cycle between us.
137+
*/
138+
if (scc) {
139+
const hasDependencyCycle = scc[filename] === scc[imported.path]
140+
if (!hasDependencyCycle) {
141+
return
142+
}
143+
}
144+
130145
const untraversed: Traverser[] = [{ mget: () => imported, route: [] }]
131146

132147
function detectCycle({ mget, route }: Traverser) {

‎src/utils/export-map.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1094,7 +1094,7 @@ export function recursivePatternCapture(
10941094
* don't hold full context object in memory, just grab what we need.
10951095
* also calculate a cacheKey, where parts of the cacheKey hash are memoized
10961096
*/
1097-
function childContext(
1097+
export function childContext(
10981098
path: string,
10991099
context: RuleContext | ChildContext,
11001100
): ChildContext {

‎src/utils/index.ts

+1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ export * from './pkg-dir'
1717
export * from './pkg-up'
1818
export * from './read-pkg-up'
1919
export * from './resolve'
20+
export * from './scc'
2021
export * from './static-require'
2122
export * from './unambiguous'
2223
export * from './visit'

‎src/utils/scc.ts

+91
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
import calculateScc from '@rtsao/scc'
2+
3+
import type { ChildContext, RuleContext } from '../types'
4+
5+
import { ExportMap, childContext } from './export-map'
6+
import { resolve } from './resolve'
7+
8+
const cache = new Map<string, Record<string, number>>()
9+
10+
export const StronglyConnectedComponents = {
11+
clearCache() {
12+
cache.clear()
13+
},
14+
15+
get(source: string, context: RuleContext) {
16+
const path = resolve(source, context)
17+
if (path == null) {
18+
return null
19+
}
20+
return StronglyConnectedComponents.for(childContext(path, context))
21+
},
22+
23+
for(context: ChildContext) {
24+
const cacheKey = context.cacheKey
25+
if (cache.has(cacheKey)) {
26+
return cache.get(cacheKey)!
27+
}
28+
const scc = StronglyConnectedComponents.calculate(context)
29+
cache.set(cacheKey, scc)
30+
return scc
31+
},
32+
33+
calculate(context: ChildContext) {
34+
const exportMap = ExportMap.for(context)
35+
const adjacencyList =
36+
StronglyConnectedComponents.exportMapToAdjacencyList(exportMap)
37+
const calculatedScc = calculateScc(adjacencyList)
38+
return StronglyConnectedComponents.calculatedSccToPlainObject(calculatedScc)
39+
},
40+
41+
exportMapToAdjacencyList(initialExportMap: ExportMap | null) {
42+
/** for each dep, what are its direct deps */
43+
const adjacencyList = new Map<string, Set<string>>()
44+
// BFS
45+
function visitNode(exportMap: ExportMap | null) {
46+
if (!exportMap) {
47+
return
48+
}
49+
for (const [importedPath, v] of exportMap.imports.entries()) {
50+
const from = exportMap.path
51+
const to = importedPath
52+
53+
if (!adjacencyList.has(from)) {
54+
adjacencyList.set(from, new Set())
55+
}
56+
57+
const set = adjacencyList.get(from)!
58+
59+
if (set.has(to)) {
60+
continue // prevent endless loop
61+
}
62+
set.add(to)
63+
visitNode(v.getter())
64+
}
65+
}
66+
visitNode(initialExportMap)
67+
// Fill gaps
68+
// eslint-disable-next-line unicorn/no-array-for-each -- Map.forEach, and it is way faster
69+
adjacencyList.forEach(values => {
70+
// eslint-disable-next-line unicorn/no-array-for-each -- Set.forEach
71+
values.forEach(value => {
72+
if (!adjacencyList.has(value)) {
73+
adjacencyList.set(value, new Set())
74+
}
75+
})
76+
})
77+
78+
return adjacencyList
79+
},
80+
81+
calculatedSccToPlainObject(sccs: Array<Set<string>>) {
82+
/** for each key, its SCC's index */
83+
const obj: Record<string, number> = {}
84+
for (const [index, scc] of sccs.entries()) {
85+
for (const node of scc) {
86+
obj[node] = index
87+
}
88+
}
89+
return obj
90+
},
91+
}

‎test/utils/scc.spec.ts

+193
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
// import sinon from 'sinon';
2+
import { testContext } from '../utils'
3+
4+
import {
5+
StronglyConnectedComponents,
6+
ExportMap,
7+
childContext as buildChildContext,
8+
} from 'eslint-plugin-import-x/utils'
9+
10+
function exportMapFixtureBuilder(
11+
path: string,
12+
imports: ExportMap[],
13+
): ExportMap {
14+
return {
15+
path,
16+
imports: new Map(
17+
imports.map(imp => [
18+
imp.path,
19+
{ getter: () => imp, declarations: new Set() },
20+
]),
21+
),
22+
} as ExportMap
23+
}
24+
25+
describe('Strongly Connected Components Builder', () => {
26+
afterEach(() => StronglyConnectedComponents.clearCache())
27+
28+
describe('When getting an SCC', () => {
29+
const source = ''
30+
const ruleContext = testContext({})
31+
const childContext = buildChildContext(source, ruleContext)
32+
33+
describe('Given two files', () => {
34+
describe("When they don't cycle", () => {
35+
it('Should return foreign SCCs', () => {
36+
jest
37+
.spyOn(ExportMap, 'for')
38+
.mockReturnValue(
39+
exportMapFixtureBuilder('foo.js', [
40+
exportMapFixtureBuilder('bar.js', []),
41+
]),
42+
)
43+
const actual = StronglyConnectedComponents.for(childContext)
44+
expect(actual).toEqual({ 'foo.js': 1, 'bar.js': 0 })
45+
})
46+
})
47+
48+
describe.skip('When they do cycle', () => {
49+
it('Should return same SCC', () => {
50+
jest
51+
.spyOn(ExportMap, 'for')
52+
.mockReturnValue(
53+
exportMapFixtureBuilder('foo.js', [
54+
exportMapFixtureBuilder('bar.js', [
55+
exportMapFixtureBuilder('foo.js', []),
56+
]),
57+
]),
58+
)
59+
const actual = StronglyConnectedComponents.get(source, ruleContext)
60+
expect(actual).toEqual({ 'foo.js': 0, 'bar.js': 0 })
61+
})
62+
})
63+
})
64+
65+
describe('Given three files', () => {
66+
describe('When they form a line', () => {
67+
describe('When A -> B -> C', () => {
68+
it('Should return foreign SCCs', () => {
69+
jest
70+
.spyOn(ExportMap, 'for')
71+
.mockReturnValue(
72+
exportMapFixtureBuilder('foo.js', [
73+
exportMapFixtureBuilder('bar.js', [
74+
exportMapFixtureBuilder('buzz.js', []),
75+
]),
76+
]),
77+
)
78+
const actual = StronglyConnectedComponents.for(childContext)
79+
expect(actual).toEqual({ 'foo.js': 2, 'bar.js': 1, 'buzz.js': 0 })
80+
})
81+
})
82+
83+
describe('When A -> B <-> C', () => {
84+
it('Should return 2 SCCs, A on its own', () => {
85+
jest
86+
.spyOn(ExportMap, 'for')
87+
.mockReturnValue(
88+
exportMapFixtureBuilder('foo.js', [
89+
exportMapFixtureBuilder('bar.js', [
90+
exportMapFixtureBuilder('buzz.js', [
91+
exportMapFixtureBuilder('bar.js', []),
92+
]),
93+
]),
94+
]),
95+
)
96+
const actual = StronglyConnectedComponents.for(childContext)
97+
expect(actual).toEqual({ 'foo.js': 1, 'bar.js': 0, 'buzz.js': 0 })
98+
})
99+
})
100+
101+
describe('When A <-> B -> C', () => {
102+
it('Should return 2 SCCs, C on its own', () => {
103+
jest
104+
.spyOn(ExportMap, 'for')
105+
.mockReturnValue(
106+
exportMapFixtureBuilder('foo.js', [
107+
exportMapFixtureBuilder('bar.js', [
108+
exportMapFixtureBuilder('buzz.js', []),
109+
exportMapFixtureBuilder('foo.js', []),
110+
]),
111+
]),
112+
)
113+
const actual = StronglyConnectedComponents.for(childContext)
114+
expect(actual).toEqual({ 'foo.js': 1, 'bar.js': 1, 'buzz.js': 0 })
115+
})
116+
})
117+
118+
describe('When A <-> B <-> C', () => {
119+
it('Should return same SCC', () => {
120+
jest
121+
.spyOn(ExportMap, 'for')
122+
.mockReturnValue(
123+
exportMapFixtureBuilder('foo.js', [
124+
exportMapFixtureBuilder('bar.js', [
125+
exportMapFixtureBuilder('foo.js', []),
126+
exportMapFixtureBuilder('buzz.js', [
127+
exportMapFixtureBuilder('bar.js', []),
128+
]),
129+
]),
130+
]),
131+
)
132+
const actual = StronglyConnectedComponents.for(childContext)
133+
expect(actual).toEqual({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 })
134+
})
135+
})
136+
})
137+
138+
describe('When they form a loop', () => {
139+
it('Should return same SCC', () => {
140+
jest
141+
.spyOn(ExportMap, 'for')
142+
.mockReturnValue(
143+
exportMapFixtureBuilder('foo.js', [
144+
exportMapFixtureBuilder('bar.js', [
145+
exportMapFixtureBuilder('buzz.js', [
146+
exportMapFixtureBuilder('foo.js', []),
147+
]),
148+
]),
149+
]),
150+
)
151+
const actual = StronglyConnectedComponents.for(childContext)
152+
expect(actual).toEqual({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 })
153+
})
154+
})
155+
156+
describe('When they form a Y', () => {
157+
it('Should return 3 distinct SCCs', () => {
158+
jest
159+
.spyOn(ExportMap, 'for')
160+
.mockReturnValue(
161+
exportMapFixtureBuilder('foo.js', [
162+
exportMapFixtureBuilder('bar.js', []),
163+
exportMapFixtureBuilder('buzz.js', []),
164+
]),
165+
)
166+
const actual = StronglyConnectedComponents.for(childContext)
167+
expect(actual).toEqual({ 'foo.js': 2, 'bar.js': 0, 'buzz.js': 1 })
168+
})
169+
})
170+
171+
describe('When they form a Mercedes', () => {
172+
it('Should return 1 SCC', () => {
173+
jest
174+
.spyOn(ExportMap, 'for')
175+
.mockReturnValue(
176+
exportMapFixtureBuilder('foo.js', [
177+
exportMapFixtureBuilder('bar.js', [
178+
exportMapFixtureBuilder('foo.js', []),
179+
exportMapFixtureBuilder('buzz.js', []),
180+
]),
181+
exportMapFixtureBuilder('buzz.js', [
182+
exportMapFixtureBuilder('foo.js', []),
183+
exportMapFixtureBuilder('bar.js', []),
184+
]),
185+
]),
186+
)
187+
const actual = StronglyConnectedComponents.for(childContext)
188+
expect(actual).toEqual({ 'foo.js': 0, 'bar.js': 0, 'buzz.js': 0 })
189+
})
190+
})
191+
})
192+
})
193+
})

‎yarn.lock

+5
Original file line numberDiff line numberDiff line change
@@ -1886,6 +1886,11 @@
18861886
dependencies:
18871887
"@xml-tools/parser" "^1.0.11"
18881888

1889+
"@rtsao/scc@^1.1.0":
1890+
version "1.1.0"
1891+
resolved "https://registry.yarnpkg.com/@rtsao/scc/-/scc-1.1.0.tgz#927dd2fae9bc3361403ac2c7a00c32ddce9ad7e8"
1892+
integrity sha512-zt6OdqaDoOnJ1ZYsCYGt9YmWzDXl4vQdKTyJev62gFhRGKdx7mcT54V9KIjg+d2wi9EXsPvAPKe7i7WjfVWB8g==
1893+
18891894
"@sinclair/typebox@^0.27.8":
18901895
version "0.27.8"
18911896
resolved "https://registry.yarnpkg.com/@sinclair/typebox/-/typebox-0.27.8.tgz#6667fac16c436b5434a387a34dedb013198f6e6e"

0 commit comments

Comments
 (0)