Replace Node interface with discriminated unions - #64040
Replace Node interface with discriminated unions #64040Arnaud Barré (ArnaudBarre) wants to merge 5 commits into
Conversation
f681238 to
c331a91
Compare
There was a problem hiding this comment.
Pull request overview
Reworks AST typings so Node and major node categories narrow through discriminated unions.
Changes:
- Introduces
NodeBaseas the structural AST root. - Generates concrete unions for
Node, expressions, statements, types, and declarations. - Includes aliased syntax kinds in discriminants.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tools/scripts/tsc/generate-ts-ast.ts |
Generates discriminated AST unions. |
packages/typescript/src/ast/ast.ts |
Introduces the common NodeBase interface. |
packages/typescript/src/ast/ast.generated.ts |
Contains regenerated AST unions and kinds. |
| } | ||
|
|
||
| export interface Node extends ReadonlyTextRange { | ||
| export interface NodeBase extends ReadonlyTextRange { |
There was a problem hiding this comment.
This is obviously a break on purpose, but I know that some consumers like TSESLint do patch in node types for compat reasons, which makes me wonder if the type should be defined as some goofy NodeRegistry interface + keyof typeof
But, I don't think that works for kinds anyway. Would have to cross check
| const allNodesNames = api | ||
| .nodes() | ||
| .map(node => node.name) | ||
| .sort((a, b) => a.localeCompare(b)); | ||
| parts.push(`export type Node = ${allNodesNames.join(" | ")};`); |
There was a problem hiding this comment.
Is this kind of tests expected?
(AI generated from Eli (@lishaduck) exploration)
There was a problem hiding this comment.
Nah, this is copilot running out of ideas 😄
|
|
||
| function isUnaryExpressionKind(kind: SyntaxKind): boolean { | ||
| switch (kind) { | ||
| export function isUnaryExpression(node: Node): node is PrefixUnaryExpression | PostfixUnaryExpression | DeleteExpression | TypeOfExpression | VoidExpression | AwaitExpression | TypeAssertion | LeftHandSideExpression { |
There was a problem hiding this comment.
The current ast.json doesn't trigger an enum for UnaryExpression, this could be changed
There was a problem hiding this comment.
I would think that we'd want this as strong as possible; I think the downstream people like ts-morph and so on have a good handle on what we could be stronger about too?
There was a problem hiding this comment.
Do you want me to do it in this PR?
There are quite some improvements the AI did in this exploration but it feels a bit orthogonal to the goal of the PR. Some of them should probably be considered outside of the "Node as discriminated union"
c7577b0 to
2865838
Compare
Attempt at #56275
https://bsky.app/profile/jakebailey.dev/post/3mt7ufvojqc23