fix(typescript-estree): don't allow single-run unless we're in type-aware linting mode

[bradzacher]

PR Checklist

• Addresses an existing open issue: fixes Bug: Reproducible allowAutomaticSingleRunInference failure #5880
  • fixes "no-cycle" affecting type resolution between monorepo packages import-js/eslint-plugin-import#2496
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

This was an absolute doozy to debug!

eslint-plugin-import does its own out-of-band parsing for some of its lint rules (like import/no-cycle or import/namespace). It does this by reusing the config originally passed to ESLint for the currently linted file - namely the parser and parserOptions config items.

This parsing is always forced to be in non-type-aware mode thanks to some hard-coding I added to their tooling way way back (https://github.com/import-js/eslint-plugin-import/blob/c3d14cb920bdc6d277134973d37364db22c3a8b8/utils/parse.js#L79-L84). This was necessary for performance and because they often parse files not covered by a tsconfig (like node_modules files).

Back to the issue at hand. When we added the single-run optimisation, we had to add a fallback state for when you do eslint --fix - a state where we would fall out of the optimised path to handle the changed, fixed file contents with a dynamic program. The logic is simple - it just increments a counter if we're doing single-run linting, and if that counter is > 1, then we use the slow path:

typescript-eslint/packages/typescript-estree/src/parser.ts

Lines 185 to 203 in 2ee81df

	/**
	* If we are in singleRun mode but the parseAndGenerateServices() function has been called more than once for the current file,
	* it must mean that we are in the middle of an ESLint automated fix cycle (in which parsing can be performed up to an additional
	* 10 times in order to apply all possible fixes for the file).
	*
	* In this scenario we cannot rely upon the singleRun AOT compiled programs because the SourceFiles will not contain the source
	* with the latest fixes applied. Therefore we fallback to creating the quickest possible isolated program from the updated source.
	*/
	if (parseSettings.singleRun && options.filePath) {
	parseAndGenerateServicesCalls[options.filePath] =
	(parseAndGenerateServicesCalls[options.filePath] || 0) + 1;
	}
	const { ast, program } =
	parseSettings.singleRun &&
	options.filePath &&
	parseAndGenerateServicesCalls[options.filePath] > 1
	? createIsolatedProgram(parseSettings)
	: getProgramAndAST(parseSettings, shouldProvideParserServices)!;

This slow path uses a (slightly dodgy) "isolated" program for the file that's light-weight, slightly incorrect, but relatively fast to create (faster than rebuilding the programs for the tsconfig from the ground up!). Using a "dodgy" program is okay because the fixed file's contents won't ever influence the types of any other file in the workspace (by definition fixers must be safe and not break the build!).

So what is the bug?
Due to eslint-plugin-import's out-of-band parsing it is possible for a file to be parsed multiple times with the second time being the eslint parse. This is an issue because unfortunately inferSingleRun didn't consider the case where we aren't doing type-aware linting for a file. So the bug would be:

1. lint file A
2. parse file A
   1. create a single-run program for the tsconfig
3. eslint-plugin-import sees A depends on B -> out-of-band parse B
   1. parseAndGenerateServicesCalls[B] = 1
4. lint file B
5. parse file B
   1. parseAndGenerateServicesCalls[B] = 2
   2. parseSettings.singleRun === true && parseAndGenerateServicesCalls[B] > 1, so create an isolated program for that file and use that instead of the true program for the file from (2,i)

The fix is simple - ensure we can never enter single run mode unless we're doing type-aware linting!

I don't know a good way to test this, but I confirmed that this fixes the problem by patching this change into the repro repos provided in both issues and it fixed the bug!

[typescript-eslint]

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 27ef439. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 46 targets

• Node 18 - nx test @typescript-eslint/eslint-plugin
• Node 14 - nx test @typescript-eslint/eslint-plugin --coverage=false
• Node 14 - nx test @typescript-eslint/typescript-estree --coverage=false
• nx run-many --target=typecheck --all --parallel
• Node 18 - nx test @typescript-eslint/typescript-estree
• Node 18 - nx test @typescript-eslint/type-utils
• nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/visitor-keys
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/utils
• Node 18 - nx test @typescript-eslint/scope-manager
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx test @typescript-eslint/scope-manager --coverage=false
• Node 14 - nx test @typescript-eslint/parser --coverage=false
• Node 18 - nx test @typescript-eslint/eslint-plugin-internal
• nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx test @typescript-eslint/eslint-plugin-tslint --coverage=false
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/parser
• Node 14 - nx test @typescript-eslint/utils --coverage=false
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx test @typescript-eslint/type-utils --coverage=false
• Node 14 - nx test @typescript-eslint/eslint-plugin-internal --coverage=false
• Node 18 - nx test @typescript-eslint/eslint-plugin-tslint
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/ast-spec
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx test @typescript-eslint/visitor-keys --coverage=false
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx test @typescript-eslint/ast-spec --coverage=false
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• nx run-many --target=build --all --parallel --exclude website --exclude website
• nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14 - nx run-many --target=build --all --parallel --exclude website --exclude website
• nx run-many --target=build --all --parallel --exclude website --exclude website

---

Sent with 💌 from NxCloud.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	27ef439
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/635d26cef38ed100084a71bc
😎 Deploy Preview	https://deploy-preview-5893--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[ljharb]

Sometimes the tiny fixes that take the longest time to figure out :-) thanks for diving into this one!

[codecov]

Codecov Report

Merging #5893 (ea882a3) into main (d806bda) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

❗ Current head ea882a3 differs from pull request most recent head 27ef439. Consider uploading reports for the commit 27ef439 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5893      +/-   ##
==========================================
- Coverage   91.36%   91.34%   -0.02%     
==========================================
  Files         365      364       -1     
  Lines       12189    12138      -51     
  Branches     3555     3543      -12     
==========================================
- Hits        11136    11088      -48     
+ Misses        749      748       -1     
+ Partials      304      302       -2     

Flag	Coverage Δ
unittest	91.34% <50.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...escript-estree/src/parseSettings/inferSingleRun.ts	80.00% <50.00%> (-10.91%)	⬇️
...es/eslint-plugin/src/rules/no-invalid-void-type.ts	95.74% <0.00%> (-0.09%)	⬇️
.../src/rules/sort-type-union-intersection-members.ts	90.74% <0.00%> (ø)
.../eslint-plugin/src/rules/sort-type-constituents.ts

[JoshuaKGoldberg]

This I'm confused about. I thought single-run linting can have a lack of project if users don't provide a project in their ESLint config?

Looking through the stack:

1. parseForESLint(code, options?): can be called with just code?
2. parseAndGenerateServices(code, options): passes parserOptions to...
3. createParseSettings(code, options): passes options to inferSingleRun

[bradzacher]

@JoshuaKGoldberg putting it as a top-level comment cos there's some good context I want to be visible

---

In the past for all type-aware linting we created a watch host and a builder program. This api let use calculate types and efficiently respond to changes in an incremental fashion (I.e. TS can recalculate just the changes to the types instead of recreating the entire program from scratch).

In the "one and done" CLI run usecase you don't need to respond to changes because each file is linted exactly once and file contents are (assumed to be) constant.

The problem is that the incremental API has a lot of overhead involved even if there are no changes made.

Single run mode is a "fast path" which relies on the aforementioned assumption that a CLI run is constant - it uses the non-incremental APIs to calculate types. This means we can calculate and fetch types much faster just for CLI runs.

With all that being said - if we aren't doing type-aware linting then none of the above is necessary! We can essentially just create a ts.SourceFile to parse the text, convert the AST, then ditch the SourceFile. It's near-zero cost to do that, unlike creating a program.

Unfortunately this bug meant that if a user used type-aware linting, then the non-type-aware eslint-plugin-import parse would change the program we used to get types to a bad one, and thus cause lint rules to misbehave.

So the fix is to make sure that the single run code that makes type-aware linting fast only ever applies to type-aware linting like it should!

[JoshuaKGoldberg]

That explanation makes sense (thanks!) but I don't see how it follows through to this PR. Specifically this idea:

single-run implies type-aware linting - no projects means we can't be in single-run mode

Where do the projects come from when someone runs ESLint in single-run mode without specifying one?

[bradzacher]

Where do the projects come from when someone runs ESLint in single-run mode without specifying one?

The user is parsing in type-aware mode (with projects) for the main linting, but eslint-plugin-import is parsing sans-projects (it does delete parserOptions.project).

The issue specifically is that if you parse a file sans-project, we shouldn't increment our counter for that file because the counter tracks how many times a file was parsed in single run mode (which we cannot be in without a project).

Right now the counter is incremented because the parser gets into a pseudo-single-run state and some of the single-run code paths fire because all they look for is the flag.

So this fix makes sure we can't get into that pseudo-state by forcing the flag off if we don't have a project.

If you grep the code for .singleRun you'll see the two places we look at the flag without looking for projects:

1. the counter bump -

   typescript-eslint/packages/typescript-estree/src/parser.ts

   Line 193 in 2ee81df

   if (parseSettings.singleRun && options.filePath) {

2. the isolated program switch -

   typescript-eslint/packages/typescript-estree/src/parser.ts

   Line 199 in 2ee81df

   parseSettings.singleRun &&

[JoshuaKGoldberg]

OK I took a deeper look at this locally and I think I understand. Thanks for the thorough explanations @bradzacher!

The combination of your last message -especially the reference to the counter, how singleRun is used- and debugging through locally clears it up a bunch.