optimize no-cycle rule using strongly connected components

[soryy708]

Lets make the no-cycle rule faster by not running many unnecessary BFSes.

For each dependency graph (aka ExportMap) we can run Tarjan's SCC once (which is a derivative of DFS = O(n))

That saves us a lot of work because we run a linear-complexity algorithm once, as opposed to for each linted file (which turned us O(n^2))

#2937

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/chai@3.5.0	None	+4	427 kB	chaijs
npm/enhanced-resolve@0.9.1	Transitive: environment, filesystem	+3	113 kB	sokra
npm/find-root@1.1.0	filesystem	0	5.27 kB	jsdnxx
npm/interpret@1.4.0	None	0	14.9 kB	phated
npm/resolve@2.0.0-next.5	environment, filesystem	+2	152 kB	ljharb
npm/webpack@5.94.0	Transitive: environment, eval, filesystem, shell, unsafe	+75	18.1 MB	evilebottnawi, jhnns, sokra, ...1 more

🚮 Removed packages: npm/@angular-eslint/template-parser@13.5.0), npm/@typescript-eslint/parser@5.62.0), npm/babel-cli@6.26.0), npm/babel-eslint@10.1.0), npm/babel-plugin-module-resolver@2.7.1), npm/babel-preset-airbnb@2.6.0), npm/babel-preset-flow@6.23.0), npm/chai@4.5.0), npm/cross-env@4.0.0), npm/escope@3.6.0), npm/eslint-doc-generator@1.7.1), npm/eslint-import-resolver-typescript@1.1.1), npm/eslint-plugin-eslint-plugin@2.3.0), npm/eslint-plugin-json@2.1.2), npm/find-babel-config@=1.2.0), npm/fs-copy-file-sync@1.1.1), npm/in-publish@2.0.1), npm/jackspeak@=2.1.1), npm/jsonc-parser@=3.2.0), npm/linklocal@2.8.2), npm/lodash.isarray@4.0.0), npm/markdownlint-cli@0.35.0), npm/npm-which@3.0.1), npm/redux@3.7.2), npm/rimraf@2.7.1), npm/safe-publish-latest@2.0.0), npm/sinon@2.4.1)

View full report↗︎

[socket-security]

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

[soryy708]

Why is socket-security failing this?
The changes in the dependencies it found don't make sense.
The library I added @rtsao/scc has no dependencies at all.
Here's their package.json: https://github.com/rtsao/scc/blob/1120edc92040b0ca748ab62a882a98565c85deed/package.json

[ljharb]

Good question. I think it's that this is the first new PR it's been run on since i installed it?

[codecov]

Codecov Report

Attention: Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.91%. Comparing base (bdff75d) to head (55c2741).

Files	Patch %	Lines
src/rules/no-cycle.js	80.00%	1 Missing ⚠️
src/scc.js	97.87%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2998       +/-   ##
===========================================
+ Coverage   85.33%   95.91%   +10.57%     
===========================================
  Files          78       79        +1     
  Lines        3300     3352       +52     
  Branches     1160     1171       +11     
===========================================
+ Hits         2816     3215      +399     
+ Misses        484      137      -347     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ljharb]

Looks great! I'll look at failing tests next

[pumano]

would love to see that optimization!

[jonahallibone]

Can't wait for this to be merged!

[pumano]

@ljharb any news on this bro?

[soryy708]

There's a pathology with SCC right now:

• It traverses ignored modules (isExternalModule), which slows it down

[ljharb]

I see two uncovered lines - can we add test cases to cover them?

Otherwise, this LGTM assuming all existing tests pass!

[soryy708]

About the uncovered lines,

I see them in codecov. The 1st is early return marked "no-self-import territory", so unrelated to my changes.

The other is early return when files don't have the same SCC.
Unfortunately it can never happen in the tests since ExportMap is null for some reason, so both files are in an undefined scc.

[soryy708]

After all these changes, I decided to measure performance impact.
I ran lint on a big TypeScript codebase with many circular dependencies, and looked at total command execution time (as reported by yarn).

Without SCC (disableScc: true):

367 seconds (6:07 minutes)

With SCC:

133 seconds (2:13 minutes)

(I made sure it isn't using eslint cache)

So we're talking about a 63% decrease in run time.

[JounQin]

Notice that we discovered performance downgrade at un-ts#113 when we ported this "optimization", I don't quite sure whether there were some improvements after we ported.

cc @SukkaW

[soryy708]

Notice that we discovered performance downgrade at un-ts#113 when we ported this "optimization", I don't quite sure whether there were some improvements after we ported.

cc @SukkaW

Thanks, I have already seen this. What I did about it from when it was ported to the community fork:

• Simplified things by ripping out "skip error message path" option. This means less things can go wrong
• Changed SCC to differentiate between value and type imports
• Changed BFS to look at SCC
• Added option to disable SCC via config

In summary, this PR diverged significantly from the port discussed, in a way which should improve performance, invalidating prior benchmarks. A workaround has been built in to fall-back on in case of performance regression, without rolling-back.

[SukkaW]

Notice that we discovered performance downgrade at un-ts#113 when we ported this "optimization", I don't quite sure whether there were some improvements after we ported.
cc @SukkaW

Thanks, I have already seen this. What I did about it from when it was ported to the community fork:

• Simplified things by ripping out "skip error message path" option. This means less things can go wrong
• Changed SCC to differentiate between value and type imports
• Changed BFS to look at SCC
• Added option to disable SCC via config

In summary, this PR diverged significantly from the port discussed, in a way which should improve performance, invalidating prior benchmarks. A workaround has been built in to fall-back on in case of performance regression, without rolling-back.

It was only after backporting the PR that I realized this was still an early experiment and a PoC.

I'm considering using @newdash/graphlib (which powers cycle-import-check) to reimplement the no-cycle rule. With an existing graph library, we can effortlessly identify any "circle" within the module graph. However, without appending extra information, we lose track of the AST node.