Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: remove partial type-information program#6066

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bradzacher merged 7 commits intov6fromremove-partial-type-info-parse
Jan 23, 2023

Conversation

bradzacher
Copy link
Member

BREAKING CHANGE:
[utils] setsprogram = null forgetParserServices(context, true).
[typescript-estree] disallowserrorOnTypeScriptSyntacticAndSemanticIssues ifproject 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. noparserOptions.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 withts.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 thegetParserServices 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 thesemantic-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 allowserrorOnTypeScriptSyntacticAndSemanticIssues if we have full type info.
    • This is because TS doesn't compute a lot of diagnostics without a program!

JoshuaKGoldberg reacted with rocket emoji
@bradzacherbradzacher added the breaking changeThis change will require a new major version to be released labelNov 23, 2022
@bradzacherbradzacher added this to the6.0.0 milestoneNov 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 onhttps://opencollective.com/typescript-eslint.

@nx-cloud
Copy link

nx-cloudbot commentedNov 23, 2022
edited
Loading

☁️ Nx Cloud Report

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

@bradzacherbradzacherforce-pushed theremove-partial-type-info-parse branch fromf66f1e1 to5ab70f1CompareNovember 30, 2022 06:54
@bradzacherbradzacher marked this pull request as ready for reviewDecember 2, 2022 02:14
@codecov
Copy link

codecovbot commentedDec 2, 2022
edited
Loading

Codecov Report

Merging#6066 (526c1d1) intov6 (dde6861) willdecrease coverage by0.02%.
The diff coverage is70.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
FlagCoverage Δ
unittest91.64% <70.00%> (-0.02%)⬇️

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

Impacted FilesCoverage Δ
...-estree/src/create-program/createDefaultProgram.ts78.26% <ø> (ø)
...estree/src/create-program/createIsolatedProgram.ts64.00% <ø> (-12.00%)⬇️
...-estree/src/create-program/createProjectProgram.ts93.47% <ø> (ø)
...ges/typescript-estree/src/create-program/shared.ts83.33% <ø> (ø)
...t-estree/src/create-program/useProvidedPrograms.ts84.84% <ø> (ø)
...ckages/utils/src/eslint-utils/getParserServices.ts10.00% <0.00%> (ø)
...escript-estree/src/semantic-or-syntactic-errors.ts86.66% <75.00%> (ø)
...eslint-plugin/src/rules/consistent-type-exports.ts97.70% <100.00%> (ø)
...kages/eslint-plugin/src/rules/naming-convention.ts81.86% <100.00%> (ø)
...ript-estree/src/create-program/createSourceFile.ts90.00% <100.00%> (+12.22%)⬆️
... and2 more

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg 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.

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelDec 2, 2022
@bradzacherbradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party labelDec 15, 2022
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg 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 theimports all available rule modules unit test failure is fixed in thev6 branch.

@bradzacherbradzacher merged commit7fc062a intov6Jan 23, 2023
@bradzacherbradzacher deleted the remove-partial-type-info-parse branchJanuary 23, 2023 08:03
@bradzacher
Copy link
MemberAuthor

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 intohttps://www.speedscope.app/ I can see thatgetProgramAndAST 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

JoshuaKGoldberg reacted with rocket emoji

@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJan 31, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
breaking changeThis change will require a new major version to be released
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

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

[8]ページ先頭

©2009-2025 Movatter.jp