Skip to content

Unify type assertion nodes #6099

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
JoshuaKGoldberg opened this issue Nov 26, 2022 Discussed in #6021 · 4 comments
Closed

Unify type assertion nodes #6099

JoshuaKGoldberg opened this issue Nov 26, 2022 Discussed in #6021 · 4 comments
Labels
AST PRs and Issues about the AST structure breaking change This change will require a new major version to be released package: typescript-estree Issues related to @typescript-eslint/typescript-estree wontfix This will not be worked on

Comments

@JoshuaKGoldberg
Copy link
Member

Discussed in #6021

Originally posted by bradzacher May 31, 2020
There are two styles for type-assertions:

  • as assertions (x as type)
  • "angle bracket" assertions (<type>x)

Currently they produce two different, but almost identical ASTs:

const x = 1 as 1;
{
  "type": "TSAsExpression",
  "expression": { /* ... */ },
  "typeAnnotation": { /* ... */ }
}

and

const y = <1>1;
{
  "type": "TSTypeAssertion",
  "expression": { /* ... */ },
  "typeAnnotation": { /* ... */ }
}

I don't think that there's a reason why these should be separate AST nodes. I think that they're separate nodes because they have different code representations, but I don't know if that's something that matters from the POV of the AST. Similar to the discussion that's going on around the ESTree representation of optional chaining - there is a difference between the AST and the CST.

From the perspective of ESLint, having two nodes causes pains when writing lint rules, as authors have to remember to add both nodes to selectors or if checks. Often they don't, which means rules don't have proper support for TS syntax.

I propose that we unify the two nodes with the following AST:

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

Consider this issue a re-opening of #2142.
@JoshuaKGoldberg JoshuaKGoldberg added package: typescript-estree Issues related to @typescript-eslint/typescript-estree breaking change This change will require a new major version to be released has pr there is a PR raised to close this AST PRs and Issues about the AST structure accepting prs Go ahead, send a pull request that resolves this issue labels Nov 26, 2022
@bradzacher
Copy link
Member

Worth noting that now we have TSSatisfiesExpression as well - so we have 3 nodes for assertions now!

@Josh-Cena
Copy link
Member

TSSatisfiesExpression should definitely be a separate thing from the other two—it has entirely different semantics from an "assertion". But the TSTypeAssertion amalgamation makes some sense to me.

@JoshuaKGoldberg JoshuaKGoldberg changed the title [RFC] unify type assertion nodes Unify type assertion nodes Nov 26, 2022
@bradzacher
Copy link
Member

It's worth noting that the AST is there to represent the syntax, not the semantics of the code.

The original issue suggested aligning the nodes based on the semantics (as assertion being the same as an angle bracket assertion), but was suggested to try and make it easier for lint rule authors.

From a pure ast perspective - satisfies and as should really be the same node with kind: 'as' | 'satisfies', and angle bracket assertions exist as their own node.
But it just easier overall to keep things separate so that old rules don't accidentally match satisfies expressions.

At this point I think almost everyone has moved away from angle bracket assertions, so it's becoming less relevant to merge things.

@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0 milestone Nov 27, 2022
@JoshuaKGoldberg
Copy link
Member Author

... so it's becoming less relevant to merge things.

Yeah I don't think this is really necessary anymore. I can't recall anybody external to us asking for it, and it's not completely cut-and-dry which one to go for. I'll just close this issue for now.

Someone should yell & make noise if there's a compelling reason to make this change!

@JoshuaKGoldberg JoshuaKGoldberg removed this from the 6.0.0 milestone Dec 24, 2022
@JoshuaKGoldberg JoshuaKGoldberg added wontfix This will not be worked on and removed has pr there is a PR raised to close this accepting prs Go ahead, send a pull request that resolves this issue labels Dec 24, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
AST PRs and Issues about the AST structure breaking change This change will require a new major version to be released package: typescript-estree Issues related to @typescript-eslint/typescript-estree wontfix This will not be worked on
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@JoshuaKGoldberg @bradzacher @Josh-Cena