fix(typescript-estree): don't allow single-run unless we're in type-aware linting mode #5893
Conversation
…ware linting mode
|
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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Sometimes the tiny fixes that take the longest time to figure out :-) thanks for diving into this one! |
Codecov Report
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| // single-run implies type-aware linting - no projects means we can't be in single-run mode | ||
| options?.project == null || |
There was a problem hiding this comment.
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:
parseForESLint(code, options?): can be called with justcode?parseAndGenerateServices(code, options): passesparserOptionsto...createParseSettings(code, options): passesoptionstoinferSingleRun
|
@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 Unfortunately this bug meant that if a user used type-aware linting, then the non-type-aware 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! |
|
That explanation makes sense (thanks!) but I don't see how it follows through to this PR. Specifically this idea:
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 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
|
There was a problem hiding this comment.
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.
PR Checklist
allowAutomaticSingleRunInferencefailure #5880Overview
This was an absolute doozy to debug!
eslint-plugin-importdoes its own out-of-band parsing for some of its lint rules (likeimport/no-cycleorimport/namespace). It does this by reusing the config originally passed to ESLint for the currently linted file - namely theparserandparserOptionsconfig 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_modulesfiles).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
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 unfortunatelyinferSingleRundidn't consider the case where we aren't doing type-aware linting for a file. So the bug would be:eslint-plugin-importsees A depends on B -> out-of-band parse BparseAndGenerateServicesCalls[B] = 1parseAndGenerateServicesCalls[B] = 2parseSettings.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!