Skip to content

refactor(typescript-estree): add type guards for typescript-estree #3078

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 17 commits into from
Closed

refactor(typescript-estree): add type guards for typescript-estree #3078

wants to merge 17 commits into from

Conversation

armano2
Copy link
Collaborator

@armano2 armano2 commented Feb 20, 2021 •
edited
Loading

This PR aims to add proper types to conversion done by typescript-estree

this PR is currently blocked by #2933, #3081

changes from #3081 has been merged to this branch

detected issues with AST so far

LeftHandSideExpression can be a AwaitExpression #3083

const a = ++(await x)

IntrinsicKeyword is not defined in AST #3081

type Uppercase<S extends string> = intrinsic;

PrivateIdentifier is not defined in AST #2933

class x {
 #y
}

TryStatement finalizer can be null #3083

try {} catch {}

RegExpLiteral value can be null if regexp is invalid or unsupported by env #3083

good example in this case can be any proposal that has not been accepted yet
eg. https://github.com/tc39/proposal-regexp-match-indices

const x = /a+(?<Z>z)?/d

JSXOpeningElement attributes can be a JSXSpreadAttribute #3083

const foo = <x {...spread}></x>

TSModuleDeclaration body can be TSModuleDeclaration #3083

// TSModuleDeclaration without body
module "x";
// TSModuleDeclaration -> TSModuleBlock
module x {
}
// TSModuleDeclaration -> TSModuleDeclaration **
module x.x {
}

issues that need additional investigation

Type '(PropertyComputedName | PropertyNonComputedName | TSEmptyBodyFunctionExpression | TSAbstractMethodDefinitionComputedName | ... 4 more ... | SpreadElement)[]' is not assignable to type 'ObjectLiteralElementLike[]'.
  Type 'PropertyComputedName | PropertyNonComputedName | TSEmptyBodyFunctionExpression | TSAbstractMethodDefinitionComputedName | ... 4 more ... | SpreadElement' is not assignable to type 'ObjectLiteralElementLike'.
    Type 'TSEmptyBodyFunctionExpression' is not assignable to type 'ObjectLiteralElementLike'.
      Type 'TSEmptyBodyFunctionExpression' is missing the following properties from type 'MethodDefinitionNonComputedName': key, computed, value, static, kind

Type '(PropertyComputedName | PropertyNonComputedName | RestElement | AssignmentPattern)[]' is not assignable to type '(PropertyComputedName | PropertyNonComputedName | RestElement)[]'.
  Type 'PropertyComputedName | PropertyNonComputedName | RestElement | AssignmentPattern' is not assignable to type 'PropertyComputedName | PropertyNonComputedName | RestElement'.
    Type 'AssignmentPattern' is not assignable to type 'PropertyComputedName | PropertyNonComputedName | RestElement'.
      Property 'argument' is missing in type 'AssignmentPattern' but required in type 'RestElement'.

@armano2 armano2 added the package: typescript-estree Issues related to @typescript-eslint/typescript-estree label Feb 20, 2021
@armano2 armano2 requested a review from bradzacher February 20, 2021 00:39
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @armano2!

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.

@codecov
Copy link

codecov bot commented Feb 20, 2021 •
edited
Loading

Codecov Report

Merging #3078 (8433bbe) into master (509a117) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3078   +/-   ##
=======================================
  Coverage   92.83%   92.83%           
=======================================
  Files         314      314           
  Lines       10672    10675    +3     
  Branches     3027     3027           
=======================================
+ Hits         9907     9910    +3     
  Misses        348      348           
  Partials      417      417
Flag Coverage Δ
unittest 92.83% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...pt-estree/src/ts-estree/estree-to-ts-node-types.ts 100.00% <ø> (ø)
packages/typescript-estree/src/convert.ts 98.37% <100.00%> (+<0.01%) ⬆️
packages/eslint-plugin/src/rules/no-unused-vars.ts 96.66% <0.00%> (-0.03%) ⬇️
packages/visitor-keys/src/visitor-keys.ts 100.00% <0.00%> (ø)
.../src/rules/sort-type-union-intersection-members.ts 92.53% <0.00%> (ø)

@armano2 armano2 marked this pull request as draft February 20, 2021 01:20
@armano2 armano2 changed the title fix: improve TSNode types refactor(typescript-estree): add type guards for typescript-estree Feb 20, 2021
@armano2 armano2 added the refactor PRs that refactor code only label Feb 20, 2021
This was referenced Feb 21, 2021
Merged
Merged
@armano2
Merge branch 'master'
5daa0a5 
Conflicts:
	packages/types/src/ts-estree.ts
	packages/typescript-estree/src/convert.ts
bradzacher
bradzacher reviewed Feb 22, 2021
View reviewed changes
packages/typescript-estree/src/convert.ts Outdated
allowPattern?: boolean,
): any {
allowPattern?: P,
): TSESTreeToTSNodeGuard<typeof node, P> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this works the way you think it will - typescript will not parameterise the passed type of node, meaning this type will be the same as TSESTreeToTSNodeGuard<TSNodeConvertable, P>

Copy link
Collaborator Author

@armano2 armano2 Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

yea i know that this type is not going to work, and it seem that there is no easy way around it, for now i just wanted to validate if we are missing anything in ast, best aproach i can think of is

  private convertJSXTagName<T extends ts.JsxTagNameExpression>(
    node: T,
    parent: ts.Node,
  ): T extends ts.JsxTagNamePropertyAccess
    ? TSESTree.JSXMemberExpression
    : T extends ts.ThisExpression | ts.Identifier
    ? TSESTree.JSXIdentifier
    : TSESTree.JSXMemberExpression | TSESTree.JSXIdentifier;

  private convertJSXTagName(
    node: ts.JsxTagNameExpression,
    parent: ts.Node,
  ): TSESTree.JSXMemberExpression | TSESTree.JSXIdentifier {

different function but same issue

@armano2 armano2 closed this Feb 22, 2021
@armano2 armano2 deleted the refactor/converter-types-1 branch February 22, 2021 05:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher left review comments

Assignees
No one assigned
Labels
package: typescript-estree Issues related to @typescript-eslint/typescript-estree refactor PRs that refactor code only
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@armano2 @bradzacher