Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses several crashers/incorrect assumptions in the Go printer / Go decoder and the TypeScript @typescript/api RemoteNode implementation, primarily around empty/omitted child NodeLists in the binary AST protocol.
Changes:
- Add nil-guard in Go printer’s
canEmitSimpleArrowHeadand a regression test for ArrowFunctions whoseParameterslist is omitted/nil. - Update Go decoder to synthesize an empty
ModuleBlock.StatementsNodeList when omitted, and decode JSX fragment tokens as dedicated node types. - Fix
RemoteNode.getNamedChildfor single-child nodes with zero encoded children, add a JS regression test, and add several missing child-property accessors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/printer/printer.go | Avoid nil deref in canEmitSimpleArrowHead when parameters is nil. |
| internal/printer/printer_test.go | Adds regression test ensuring printing an ArrowFunction with nil Parameters doesn’t panic. |
| internal/api/encoder/decoder.go | Synthesizes empty statements for ModuleBlock; decodes JSX fragment tokens as their own node types. |
| internal/api/encoder/decoder_test.go | Adds decode regression test for an empty namespace body (ModuleBlock with empty statements). |
| _packages/api/src/node/node.ts | Prevent getNamedChild from reading a non-child at index+1 when there are zero children; adds some missing getters. |
| _packages/api/test/encoder.test.ts | Adds regression test for JsxAttributes with empty properties NodeList; imports RemoteNodeList for traversal. |
You can also share your feedback on Copilot code review. Take the survey.
internal/api/encoder/decoder.go
Outdated
| } | ||
| return d.factory.NewModuleBlock(stmts), nil | ||
| case ast.KindCaseBlock: | ||
| return d.factory.NewCaseBlock(d.singleNodeListChild(childIndices)), nil |
There was a problem hiding this comment.
This is definitely plausible, but I'd want to know what the parser does 🥴
There was a problem hiding this comment.
Yeah, the parser always produces a non-nil NodeList for CaseBlock.Clause. Have also fixed other similar cases
| // JSX fragment tokens (must be their own types, not Token, for the printer) | ||
| case ast.KindJsxOpeningFragment: | ||
| return d.factory.NewJsxOpeningFragment(), nil | ||
| case ast.KindJsxClosingFragment: | ||
| return d.factory.NewJsxClosingFragment(), nil | ||
|
|
_packages/api/test/encoder.test.ts
Outdated
| const declarations = declList.declarations! as unknown as RemoteNodeList; | ||
| const varDecl = declarations.at(0)!; |
There was a problem hiding this comment.
@navya9singh you need to fix this return type at the source in node.ts get declarations()
|
@copilot format |
internal/printer/printer.go
Outdated
| func canEmitSimpleArrowHead(parentNode *ast.Node, parameters *ast.ParameterList) bool { | ||
| // only arrow functions with a single parameter may have simple arrow head | ||
| if !ast.IsArrowFunction(parentNode) || len(parameters.Nodes) != 1 { | ||
| if parameters == nil || !ast.IsArrowFunction(parentNode) || len(parameters.Nodes) != 1 { |
There was a problem hiding this comment.
One nil NodeList printer crash was fixed in the decoder; the other here. We want the decoder to produce ASTs that are as similar as possible to the parser. The fact that this check wasn't necessary before suggests that the decoder is doing something wrong. I am too scared to ask whether the parser itself is internally consistent about whether to use nil or empty NodeLists, but the least we can do is match whatever the parser is already doing. This should be handled in the decoder instead.
There was a problem hiding this comment.
The decoder now produces a non-nil empty NodeList instead of nil, which matches the parser's nil vs non-nil semantics.
internal/printer/printer_test.go
Outdated
| emittestutil.CheckEmit(t, nil, file.AsSourceFile(), "() => ({}.a);") | ||
| } | ||
|
|
||
| func TestArrowFunctionNilParameters(t *testing.T) { |
There was a problem hiding this comment.
This test should be deleted per my other comment. An API test that constructs something like this and prints it would be better.
…o navyasingh/apiBugFix
…o navyasingh/apiBugFix
Discovered 3 bugs while testing that copilot fixed:
The Corsa API returns empty arrays [] for properties like signature.typeParameters where strada returned undefined. In JavaScript, !![] evaluates to true, so the
check !!functionSignature.typeParameters at line 1915 of extractSymbol.ts was incorrectly truthy for non-generic functions. This caused an early return that left
the FunctionType variableType set, which then sent an ArrayType node with a nil elementType to the Go printer — resulting in a nil pointer dereference panic. Fixed
by changing to functionSignature.typeParameters && functionSignature.typeParameters.length > 0.
In @typescript/api's RemoteNode class, the getNamedChild method's single-child path blindly read the node at index + 1 without verifying it was actually a child of
the current node. When the Go encoder skips encoding empty NodeLists (e.g., JsxAttributes with no attributes), the node has zero children — but getNamedChild would
read whatever unrelated node happened to be at index + 1, then throw "Expected only one child" because that node's next pointer was non-zero. Fixed by adding a
hasChildren() guard that checks the parent pointer before accessing index + 1, returning undefined when the node has no children.
When the JS encoder sends a synthetic ArrowFunction node (e.g., inside an object literal PropertyAssignment) to the Go printer, the Parameters field can be nil
because the Go encoder skips encoding empty NodeLists. The printer's canEmitSimpleArrowHead function at printer.go:1513 accessed parameters.Nodes without a nil
check, causing a panic. Fixed by adding parameters == nil guard to the condition, returning false (i.e., not a simple arrow head) when parameters is nil.