Ditch nodeData interface in favor of generated accessors - #64063
Ditch nodeData interface in favor of generated accessors#64063Jake Bailey (jakebailey) wants to merge 13 commits into
Conversation
Replace nine nodeData interface methods with generated Kind-switch dispatchers to reduce interface method promotion and itab size.
|
TypeScript Bot (@typescript-bot) perf test this |
There was a problem hiding this comment.
Pull request overview
Replaces nodeData interface dispatch with generated kind-based AST accessors to reduce binary size and metadata overhead.
Changes:
- Generates AST visitor, clone, subtree-fact, and accessor dispatch.
- Updates compiler and language-service consumers for the new representation.
- Adds canonical token-payload validation and adjusts direct node construction.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tools/scripts/tsc/generate-go-ast.ts |
Generates kind-based AST dispatch. |
tools/scripts/tsc/generate-encoder.ts |
Updates generated private-field access. |
tsc/internal/ast/ast.go |
Reworks node storage and common fields. |
tsc/internal/ast/ast_generated.go |
Contains generated factories and dispatch. |
tsc/internal/ast/flow.go |
Adapts synthetic flow-node construction. |
tsc/internal/ast/token_test.go |
Tests canonical token payloads. |
tsc/internal/ast/utilities.go |
Updates type-only checks. |
tsc/internal/api/encoder/encoder.go |
Updates source-file position access. |
tsc/internal/api/encoder/encoder_generated.go |
Updates generated text access. |
tsc/internal/checker/grammarchecks.go |
Uses common node accessors. |
tsc/internal/checker/nodebuilder_hover.go |
Updates optional-token access. |
tsc/internal/ls/autoimport/fix.go |
Updates type-only import access. |
tsc/internal/ls/completions.go |
Updates source-file access. |
tsc/internal/ls/lsutil/organizeimports.go |
Updates import classification. |
tsc/internal/ls/organizeimports.go |
Updates type-only grouping. |
tsc/internal/ls/signaturehelp.go |
Updates type-argument access. |
tsc/internal/parser/reparser.go |
Updates mutable typed-node casts. |
tsc/internal/printer/printer_test.go |
Adapts transformed source-file casts. |
tsc/internal/scanner/scanner_test.go |
Adapts node initialization. |
tsc/internal/transformers/declarations/transform.go |
Updates syntax-list traversal. |
tsc/internal/transformers/estransforms/optionalchain.go |
Updates element-access casting. |
tsc/internal/transformers/moduletransforms/commonjsmodule.go |
Updates common expression/name access. |
tsc/internal/transformers/tstransforms/legacydecorators.go |
Updates decorator access. |
tsc/internal/transformers/tstransforms/runtimesyntax.go |
Updates parameter-list access. |
tsc/internal/transformers/tstransforms/typeeraser.go |
Updates type-only import access. |
Files not reviewed (1)
- tsc/internal/api/encoder/encoder_generated.go: Generated file
Suppressed comments (1)
tsc/internal/ast/flow.go:74
- This node likewise stores
*FlowReduceLabelDataunderKindUnknown, while the generatedCloneand subtree-facts switches assert that everyKindUnknownpayload is*Token. Those methods now panic for reduce-label nodes; previously dynamic dispatch reachedNodeDefault. Use a distinct kind or explicitly handle this synthetic payload in dispatch, with a regression test.
return newNode(KindUnknown, node.AsNode(), node, NodeFactoryHooks{})
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
lspComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 26 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- tsc/internal/api/encoder/encoder_generated.go: Generated file
- tsc/internal/ast/kind_stringer_generated.go: Generated file
|
TypeScript Bot (@typescript-bot) perf test this faster |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Gabriela Araujo Britto (gabritto)
left a comment
There was a problem hiding this comment.
This looks good to me, and like I said, better for the upcoming extra type checking, but not sure other people have opinions.
|
This should be perf neutral but trying one more time TypeScript Bot (@typescript-bot) perf test this faster |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hm, there's something to this, I think, I need to investigate. |
This ditches the dynamic dispatch with
nodeDatain place of generated kind switches, like I did withForEachChild..text.gopclntab.go.typetsc/internal/astIt also makes compiling the ast package 5% faster.