Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove partial type-information program #6066

Merged
merged 7 commits into from Jan 23, 2023

Conversation

bradzacher
Copy link
Member

BREAKING CHANGE:
[utils] sets program = null for getParserServices(context, true).
[typescript-estree] disallows errorOnTypeScriptSyntacticAndSemanticIssues if project not provided

PR Checklist

Overview

A few years ago I experimented with leveraging TS's unused variable diagnostics to power a lint rule. As this diagnostic is "single-file", I added the ability for the parser to generate a program in "isolated" mode (i.e. no parserOptions.project required and types/diagnostics only produced for the single file) as well as the ability for plugins to interact with this program.

However this approach had some major drawbacks:

  1. calculating diagnostics in TS is slow - so the lint rule was slow as there's no way to tell TS to calculate specific diagnostics.
  2. the isolated types are completely useless because in most modules so many types are seeded from imports.

We never bothered going back to cleanup this work (until now).

This PR:

  1. updates the parser so that it only works with ts.SourceFiles if we're not doing type-aware linting.
    • this should have the added bonus of speeding the parser up for non-type-aware runs as we're not going through the heavy process of creating a program!
  2. updates the getParserServices util as appropriate
    • Note: I left the ability to consume parser services without type info because the node maps can be useful in some situations, or sometimes it's useful to consume compiler options if provided.
  3. removes the semantic-diagnostics-enabled test because it's a test that we don't get any value out of - it's just emitting TS errors which might change from version to version
  4. only allows errorOnTypeScriptSyntacticAndSemanticIssues if we have full type info.
    • This is because TS doesn't compute a lot of diagnostics without a program!

@bradzacher bradzacher added the breaking change This change will require a new major version to be released label Nov 23, 2022
@bradzacher bradzacher added this to the 6.0.0 milestone Nov 23, 2022
@typescript-eslint
Copy link
Contributor

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.

@nx-cloud
Copy link

nx-cloud bot commented Nov 23, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 526c1d1. 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 47 targets

Sent with 💌 from NxCloud.

@bradzacher bradzacher marked this pull request as ready for review December 2, 2022 02:14
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #6066 (526c1d1) into v6 (dde6861) will decrease coverage by 0.02%.
The diff coverage is 70.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##               v6    #6066      +/-   ##
==========================================
- Coverage   91.66%   91.64%   -0.02%     
==========================================
  Files         354      354              
  Lines       12199    12200       +1     
  Branches     3581     3582       +1     
==========================================
- Hits        11182    11181       -1     
- Misses        721      722       +1     
- Partials      296      297       +1     
Flag Coverage Δ
unittest 91.64% <70.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...-estree/src/create-program/createDefaultProgram.ts 78.26% <ø> (ø)
...estree/src/create-program/createIsolatedProgram.ts 64.00% <ø> (-12.00%) ⬇️
...-estree/src/create-program/createProjectProgram.ts 93.47% <ø> (ø)
...ges/typescript-estree/src/create-program/shared.ts 83.33% <ø> (ø)
...t-estree/src/create-program/useProvidedPrograms.ts 84.84% <ø> (ø)
...ckages/utils/src/eslint-utils/getParserServices.ts 10.00% <0.00%> (ø)
...escript-estree/src/semantic-or-syntactic-errors.ts 86.66% <75.00%> (ø)
...eslint-plugin/src/rules/consistent-type-exports.ts 97.70% <100.00%> (ø)
...kages/eslint-plugin/src/rules/naming-convention.ts 81.86% <100.00%> (ø)
...ript-estree/src/create-program/createSourceFile.ts 90.00% <100.00%> (+12.22%) ⬆️
... and 2 more

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🔥 Mostly looks fantastic - just requesting changes on the // backwards compatibility check comment.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Dec 2, 2022
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Dec 15, 2022
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

No remaining blockers, and a lot of goodness in this PR. Let's get this merged!! 🙌

@JoshuaKGoldberg
Copy link
Member

Btw @bradzacher the imports all available rule modules unit test failure is fixed in the v6 branch.

@bradzacher bradzacher merged commit 7fc062a into v6 Jan 23, 2023
@bradzacher bradzacher deleted the remove-partial-type-info-parse branch January 23, 2023 08:03
@bradzacher
Copy link
Member Author

For reference I just did a really quick perf test of the before-and-after for this.

$ TSESTREE_SINGLE_RUN=true node --cpu-prof ./node_modules/.bin/eslint --no-eslintrc --parser="@typescript-eslint/parser" --ext=".ts,.tsx" .

Loading the profiles into https://www.speedscope.app/ I can see that getProgramAndAST is about 100ms faster on our codebase.

It doesn't seem like much, but our codebase is relatively small - ~2.6k files for ~207k SLOC in total. For a larger code base I'd expect more of an impact, though it'll likely still be pretty small overall.

For reference the CPU profiles: PROFILES.zip

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change This change will require a new major version to be released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Remove 2 year old backwards compatibility check from getParserServices
2 participants