eslint-plugin-import does its own out-of-band parsing for some of its lint rules (likeimport/no-cycle orimport/namespace). It does this by reusing the config originally passed to ESLint for the currently linted file - namely theparser andparserOptions 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 (likenode_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 doeslint --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 in2ee81df

	/**
	* 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 toeslint-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 unfortunatelyinferSingleRun didn't consider the case where we aren't doing type-aware linting for a file. So the bug would be:

lint file A
parse file A
1. create a single-run program for the tsconfig
eslint-plugin-import sees A depends on B -> out-of-band parse B
1. parseAndGenerateServicesCalls[B] = 1
lint file B
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!

fix(typescript-estree): don't allow single-run unless we're in type-a…

ea882a3

…ware linting mode

bradzacher added the bugSomething isn't working label

Oct 26, 2022

bradzacher requested a review fromJoshuaKGoldberg

October 26, 2022 04:15

Copy link

Contributor

typescript-eslintbot commentedOct 26, 2022

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 visitorsper day.

Copy link

nx-cloudbot commentedOct 26, 2022•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit27ef439. 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

Sent with 💌 fromNxCloud.

Copy link

netlifybot commentedOct 26, 2022•
edited
Loading

✅ Deploy Preview fortypescript-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 yourNetlify site settings.

bradzacher mentioned this pull request

Oct 26, 2022

"no-cycle" affecting type resolution between monorepo packagesimport-js/eslint-plugin-import#2496

Closed

Copy link

ljharb commentedOct 26, 2022

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

Copy link

codecovbot commentedOct 26, 2022•
edited
Loading

Codecov Report

Merging#5893 (ea882a3) intomain (d806bda) willdecrease coverage by0.01%.
The diff coverage is50.00%.

❗ Current headea882a3 differs from pull request most recent head27ef439. Consider uploading reports for the commit27ef439 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 reviewed

Oct 26, 2022

View reviewed changes

packages/typescript-estree/src/parseSettings/inferSingleRun.ts

Comment on lines +19 to +20

		// single-run implies type-aware linting - no projects means we can't be in single-run mode
		options?.project == null \|\|

Copy link

Member

JoshuaKGoldbergOct 26, 2022

Choose a reason for hiding this comment

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

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

Looking through the stack:

parseForESLint(code, options?): can be called with justcode?
parseAndGenerateServices(code, options): passesparserOptions to...
createParseSettings(code, options): passesoptions toinferSingleRun

Copy link

MemberAuthor

bradzacher commentedOct 26, 2022

@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 ats.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-awareeslint-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!

Copy link

Member

JoshuaKGoldberg commentedOct 26, 2022

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?

Copy link

MemberAuthor

bradzacher commentedOct 26, 2022

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, buteslint-plugin-import is parsing sans-projects (it doesdelete 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:

the counter bump -
typescript-eslint/packages/typescript-estree/src/parser.ts
Line 193 in2ee81df
if(parseSettings.singleRun&&options.filePath){
the isolated program switch -
typescript-eslint/packages/typescript-estree/src/parser.ts
Line 199 in2ee81df
parseSettings.singleRun&&

bradzacher requested a review fromJoshuaKGoldberg

October 28, 2022 12:14

JoshuaKGoldberg approved these changes

Oct 29, 2022

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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, howsingleRun is used- and debugging through locally clears it up a bunch.