feat: merge AST structure of TSAsExpression with TSTypeAssertion #3005

armano2 · 2021-02-06T05:48:50Z

This PR aims to merge AST structure of TSAsExpression with TSTypeAssertion

This is BREAKING CHANGE

New AST structure

interface TSTypeAssertion extends Node {
  type: AST_NODE_TYPES.TSTypeAssertion;
  typeAnnotation: TypeNode;
  expression: Expression;
  kind: 'as' | 'angle-bracket';
}

TODO:

Add unit tests for indent rule
Add unit tests for keyword-spacing rule
Add unit tests for no-var-requires rule

fixes #2142

typescript-eslint · 2021-02-06T05:48:54Z

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.

armano2 · 2021-02-06T05:56:44Z

packages/scope-manager/src/referencer/Referencer.ts

+    if (node.kind === 'as') {
+      this.visit(node.expression);
+      this.visitType(node.typeAnnotation);
+    } else {
+      this.visitType(node.typeAnnotation);
+      this.visit(node.expression);
+    }


while doing this change i noticed that order of creating references was not correct

codecov · 2021-02-06T06:01:55Z

Codecov Report

Merging #3005 (16a89b1) into master (5e2a993) will decrease coverage by 0.01%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##           master    #3005      +/-   ##
==========================================
- Coverage   92.87%   92.85%   -0.02%     
==========================================
  Files         314      314              
  Lines       10670    10671       +1     
  Branches     3026     3029       +3     
==========================================
- Hits         9910     9909       -1     
  Misses        344      344              
- Partials      416      418       +2

Flag	Coverage Δ
unittest	`92.85% <76.47%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...nt-plugin/src/rules/indent-new-do-not-use/index.ts	`98.13% <ø> (ø)`
...ackages/eslint-plugin/src/rules/keyword-spacing.ts	`81.25% <0.00%> (-11.61%)`	⬇️
...ackages/eslint-plugin/src/rules/no-var-requires.ts	`88.88% <ø> (ø)`
...ackages/eslint-plugin/src/rules/prefer-as-const.ts	`100.00% <ø> (ø)`
packages/eslint-plugin/src/rules/unbound-method.ts	`93.10% <ø> (ø)`
...ges/experimental-utils/src/ast-utils/predicates.ts	`37.50% <0.00%> (+0.65%)`	⬆️
...pt-estree/src/ts-estree/estree-to-ts-node-types.ts	`100.00% <ø> (ø)`
packages/visitor-keys/src/visitor-keys.ts	`100.00% <ø> (ø)`
packages/eslint-plugin/src/rules/indent.ts	`89.13% <80.00%> (-1.35%)`	⬇️
...int-plugin/src/rules/consistent-type-assertions.ts	`84.84% <100.00%> (-1.64%)`	⬇️
... and 4 more

packages/eslint-plugin/src/rules/no-var-requires.ts

armano2 · 2021-02-06T06:21:35Z

packages/eslint-plugin/src/rules/indent.ts

+          node.type === AST_NODE_TYPES.TSTypeAssertion &&
+          node.kind === 'as'


there is no unit case for type assertion with brackets

armano2 · 2021-02-24T19:49:10Z

i implemented this change but I'm unsure if this is actually good

bradzacher

i implemented this change but I'm unsure if this is actually good

What do you mean?
Do you mean you don't like having one node represent the two different types?

bradzacher · 2021-02-24T20:22:51Z

packages/eslint-plugin/src/rules/keyword-spacing.ts

+      TSTypeAssertion(node): void {
+        if (node.kind !== 'as') {
+          return;
+        }


could do this via a selector

Suggested change

TSTypeAssertion(node): void {

if (node.kind !== 'as') {

return;

}

'TSTypeAssertion[kind = "as"]'(node): void {

we could also add this as a typed selector to packages/types/src/ast-node-types.ts as it's probably going to be a more common usecase

armano2 · 2021-02-24T21:40:15Z

What do you mean?
Do you mean you don't like having one node represent the two different types?

yes, I'm not sure if this is a good change, for sure its more convenient

THIS IS BREAKING CHANGE

armano2 added breaking change This change will require a new major version to be released AST PRs and Issues about the AST structure labels Feb 6, 2021

armano2 requested a review from bradzacher February 6, 2021 05:51

armano2 added the DO NOT MERGE PRs which should not be merged yet label Feb 6, 2021

armano2 changed the title ~~feat: merge ast structure of as expression with type assertion~~ feat: merge AST structure of as expression with type assertion Feb 6, 2021

armano2 changed the title ~~feat: merge AST structure of as expression with type assertion~~ feat: merge AST structure of TSAsExpression with TSTypeAssertion Feb 6, 2021

armano2 added enhancement New feature or request and removed DO NOT MERGE PRs which should not be merged yet labels Feb 6, 2021

armano2 commented Feb 6, 2021

View reviewed changes

packages/eslint-plugin/src/rules/no-var-requires.ts Outdated Show resolved Hide resolved

armano2 commented Feb 6, 2021

View reviewed changes

armano2 added this to the 5.0.0 milestone Feb 19, 2021

bradzacher reviewed Feb 24, 2021

View reviewed changes

feat: merge ast structure of as expression with type assertion

16a89b1

THIS IS BREAKING CHANGE

armano2 force-pushed the feat/remove-ts-as-expression branch from 8233a9b to 16a89b1 Compare February 27, 2021 17:34

bradzacher removed this from the 5.0.0 milestone Aug 21, 2021

bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Mar 4, 2022

bradzacher closed this Mar 4, 2022

bradzacher deleted the feat/remove-ts-as-expression branch March 4, 2022 07:40

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: merge AST structure of TSAsExpression with TSTypeAssertion #3005

feat: merge AST structure of TSAsExpression with TSTypeAssertion #3005

armano2 commented Feb 6, 2021 •

edited

Loading

typescript-eslint bot commented Feb 6, 2021

armano2 Feb 6, 2021 •

edited

Loading

codecov bot commented Feb 6, 2021 •

edited

Loading

armano2 Feb 6, 2021

armano2 commented Feb 24, 2021

bradzacher left a comment

bradzacher Feb 24, 2021

bradzacher Feb 24, 2021

armano2 commented Feb 24, 2021

		node.type === AST_NODE_TYPES.TSTypeAssertion &&
		node.kind === 'as'

feat: merge AST structure of TSAsExpression with TSTypeAssertion #3005

feat: merge AST structure of TSAsExpression with TSTypeAssertion #3005

Conversation

armano2 commented Feb 6, 2021 • edited Loading

New AST structure

TODO:

typescript-eslint bot commented Feb 6, 2021

armano2 Feb 6, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2021 • edited Loading

Codecov Report

armano2 Feb 6, 2021

Choose a reason for hiding this comment

armano2 commented Feb 24, 2021

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher Feb 24, 2021

Choose a reason for hiding this comment

bradzacher Feb 24, 2021

Choose a reason for hiding this comment

armano2 commented Feb 24, 2021

armano2 commented Feb 6, 2021 •

edited

Loading

armano2 Feb 6, 2021 •

edited

Loading

codecov bot commented Feb 6, 2021 •

edited

Loading