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

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

Closed
wants to merge 17 commits into from

Conversation

armano2
Copy link
Member

@armano2 armano2 commented Feb 20, 2021

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

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
Conflicts:
	packages/types/src/ts-estree.ts
	packages/typescript-estree/src/convert.ts
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
Member Author

@armano2 armano2 Feb 22, 2021

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.
Labels
package: typescript-estree Issues related to @typescript-eslint/typescript-estree refactor PRs that refactor code only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants