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

tests(ast-spec): make PunctuatorTokenToText more type-safe #3529

Merged
merged 1 commit into from Jun 21, 2021

Conversation

MichaelDeBoey
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey commented Jun 14, 2021

Follow-up of #3496

We should somehow test that all possible values of PunctuationSyntaxKind are definitely included.
If not, this should error out somehow.

I first tried doing

export const PunctuatorTokenToText: Record<PunctuationSyntaxKind, string> = {
  // ...
}

and this errored out (as expected) because SyntaxKind.BacktickToken isn't includes, but is included in PunctuationSyntaxKind.
Not including the type makes it also of type string again, which will fail #3461 & #3462.

This however made it a value instead of a type and had the implication that SyntaxKind.BarBarEqualsToken, SyntaxKind.AmpersandAmpersandEqualsToken & SyntaxKind.QuestionQuestionEqualsToken couldn't be added as they'll only be added to PunctuationSyntaxKind in TS 4.4 (they're currently included on main).

In TS 4.4 the new SyntaxKind.HashToken should also be included, so I think it would be nice to have it somehow error out if we don't include that when updating our TS version.

@bradzacher @JamesHenry Any idea how I can make this error out when some values aren't included or how I should write a test that can fail when a new value isn't included here?

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @MichaelDeBoey!

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
Copy link

nx-cloud bot commented Jun 14, 2021

Nx Cloud Report

CI ran the following commands for commit 5ef6efb. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=build --all --parallel
#000000 nx run-many --target=typecheck --all --parallel

Sent with 💌 from NxCloud.

@MichaelDeBoey
Copy link
Contributor Author

@bradzacher @JamesHenry Do you have any idea how I can make this error out as explained in the above message?

Would love to work on this one further, but don't see a solution right away.

@bradzacher bradzacher added the repo maintenance things to do with maintenance of the repo, and not with code/docs label Jun 20, 2021
@bradzacher
Copy link
Member

can you do something similar to what I did in this?

import type { AST_NODE_TYPES } from '../src/ast-node-types';
import type { Node } from '../src/unions/Node';
type GetKeys<T extends AST_NODE_TYPES> = keyof Extract<Node, { type: T }>;
type AllKeys = {
readonly [T in AST_NODE_TYPES]: GetKeys<T>;
};
type TakesString<T extends Record<string, string>> = T;
// @ts-expect-error: purposely unused
type _Test =
// forcing the test onto a new line so it isn't covered by the expect error
// If there are any enum members that don't have a corresponding TSESTree.Node, then this line will error with "Type 'string | number | symbol' is not assignable to type 'string'."
TakesString<AllKeys> | void;

@codecov
Copy link

codecov bot commented Jun 20, 2021

Codecov Report

Merging #3529 (e2e76a7) into master (44f9c07) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head e2e76a7 differs from pull request most recent head 5ef6efb. Consider uploading reports for the commit 5ef6efb to get more accurate results

@@            Coverage Diff             @@
##           master    #3529      +/-   ##
==========================================
- Coverage   92.64%   92.63%   -0.01%     
==========================================
  Files         326      325       -1     
  Lines       11253    11236      -17     
  Branches     3171     3168       -3     
==========================================
- Hits        10425    10409      -16     
+ Misses        368      367       -1     
  Partials      460      460              
Flag Coverage Δ
unittest 92.63% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...ackages/experimental-utils/src/ts-eslint/Linter.ts 100.00% <0.00%> (ø)
...ages/eslint-plugin/src/rules/prefer-regexp-exec.ts 100.00% <0.00%> (ø)
...ges/experimental-utils/src/ts-eslint/RuleTester.ts 100.00% <0.00%> (ø)
packages/eslint-plugin/src/util/escapeRegExp.ts

@MichaelDeBoey
Copy link
Contributor Author

@bradzacher I don't see directly how I can use the mentioned test in my use-case tbh. 🤔

A bit more context (or even the test if you like) would be welcome.

@bradzacher
Copy link
Member

 // @ts-expect-error: purposely unused
type _AllKeys = {
  // no error as all PunctuationSyntaxKind are covered
  readonly [T in PunctuationSyntaxKind]: PunctuatorTokenToText[T]; 
};


// example showing if we miss one of the PunctuationSyntaxKind

interface AccidentallyIncomplete {
  [SyntaxKind.CaretEqualsToken]: '^=';
}

// @ts-expect-error: purposely unused
type _AllKeysError = {
  readonly [T in PunctuationSyntaxKind]: AccidentallyIncomplete[T]; 
//                                       ^^^^^^^^^^^^^^^^^^^^^^^^^ error: Type 'T' cannot be used to index type 'AccidentallyIncomplete'.
}

repl

@MichaelDeBoey
Copy link
Contributor Author

@bradzacher The problem is, it should already error out as BacktickToken is commented out and should be included in the interface. 🤔

@bradzacher
Copy link
Member

i forgot to delete it when I copy pasted - but delete the extends and that will fix it.

@MichaelDeBoey MichaelDeBoey marked this pull request as ready for review June 20, 2021 23:54
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for thsi!

@bradzacher bradzacher changed the title fix(ast-spec): make PunctuatorTokenToText more type-safe tests(ast-spec): make PunctuatorTokenToText more type-safe Jun 21, 2021
@bradzacher bradzacher merged commit abda961 into typescript-eslint:master Jun 21, 2021
@MichaelDeBoey MichaelDeBoey deleted the patch-16 branch June 21, 2021 08:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants