Fix crash when a call signature's type parameter cannot be reused

[Nicolaev Eduard (nikeedw)]

Supersedes #4842. That PR was closed over my CLA refusal — a decision I have since reversed: I read the CLA in full and signed it there (@microsoft-github-policy-service agree, comment). Per the CLA's own §2 the agreement covers prior submissions as well, so both that PR's content and this one are licensed under its terms. I could not reopen #4842 (no permission after a maintainer close), hence this fresh PR from the same branch, unchanged. Apologies for the churn — the refusal was my mistake.

Fixes #4748.

The crash

The PseudoTypeKindSingleCallSignature branch of pseudoTypeToNode appends the result of reuseNode into a call signature's type parameter list without checking it:

for _, tp := range d.TypeParameters {
    res = append(res, b.reuseNode(tp.AsNode()))
}
typeParams = b.f.NewNodeList(res)

reuseNode returns nil whenever the recovery boundary in tryReuseExistingNodeHelper fails. The nil is stored in the NodeList and survives all the way to the printer, which dereferences the list's last element while deciding whether to write a trailing comma:

ast.(*Node).End                                 ast.go:193
ast.(*NodeList).HasTrailingComma                ast.go:142
printer.(*Printer).hasTrailingComma             printer.go:4774
printer.(*Printer).emitListRange                printer.go:4755
printer.(*Printer).emitTypeParameters           printer.go:1506
printer.(*Printer).emitFunctionType             printer.go:1928
...
compiler.(*emitter).emitDeclarationFile         emitter.go:269

Every other caller of reuseNode copes with the nil — reuseTypeNode in particular reports an inference fallback and re-serializes the node from the checker. This one call site does not.

When it happens

Four things have to line up:

1. A generic arrow-function property in an object literal, so the pseudochecker classifies it as SingleCallSignature and tries to reuse the original type parameter nodes.
2. The type parameter's constraint references a binding that cannot be named from the file being emitted.
3. Rewriting that constraint has to fail as well. Normally reuse does not fail here, it rewrites — drop the unique symbol from the test case below and you get a perfectly good <K extends "count" | "name">. It only returns nil when the rewrite itself reaches something unnameable.
4. The member has to be inlined structurally into another file's declaration. A single-file version does not reproduce, because the local is nameable in its own .d.ts.

The original report came from a mobx-state-tree codebase, where all four fall out of ordinary usage: a generic <Key extends keyof typeof self> setter declared inside .actions(self => ({ ... })), MST's unique symbol brands, and types.compose pulling models in across files.

Why it only reproduces on the incremental path

For a noEmit or declaration: false project the declaration printer runs in exactly one place — computing d.ts shape signatures. In affectedFilesHandler.updateShapeSignature:

if !file.IsDeclarationFile && !useFileVersionAsSignature {
    update.signature = h.computeDtsSignature(file)
}

A cold run takes the useFileVersionAsSignature: true path and hashes the file text, so nothing is ever printed. A warm run computes the real d.ts signature for each affected file, prints the malformed tree, and crashes.

A direct --declaration --emitDeclarationOnly build hides the bug from the other side: files in this shape always carry declaration diagnostics (TS2527 / TS4023), and their emit is skipped. So the malformed node list is built on every run; only the signature path ever prints it.

The crash is not limited to noEmit. On main the repro below also crashes with a plain JS-emitting incremental build and with composite: true.

The fix

Fall back to serializing the type parameter from the checker, mirroring what reuseTypeNode already does for type nodes. typeParameterToDeclaration always returns a node, so the list can no longer contain a nil.

Test

internal/execute/tsctests gains a two-file incremental scenario under TestTscDeclarationEmit. Without the fix it panics; with it the baseline shows the signature being computed correctly:

export declare const merged: {
    setField: <K extends "count" | "name" | unique symbol>(key: K, value: ({
        name: string;
        count: number;
        [brand]: boolean;
    })[K]) => void;
};

Both declaration diagnostics (TS2527 and TS4023) are still reported, so the only behavioural change is that a crash becomes a correctly serialized type parameter.

Verification

• npx hereby build, npx hereby lint (0 issues), npx hereby format (no changes), npx hereby test — all pass.
• The full baseline corpus, including internal/testrunner against the TypeScript submodule, produces no changed baselines other than the new one added here.
• Also confirmed against the private codebase from Panic: nil pointer in NodeList.HasTrailingComma during incremental rebuild (build-mode declaration printer) — 7.0.2 and current nightly #4748: the warm incremental run that reliably crashed now completes cleanly.

AI assistance disclosure

Per CONTRIBUTING.md: this patch was authored with the help of Claude Code. This is not a queue-driven contribution — #4748 is my own bug report against my own codebase, and I drove this investigation myself. I have read and understand the change, and I will be the one handling review feedback.

[Nicolaev Eduard (nikeedw)]

One observation that may be useful for judging the "symptom vs root cause" question I raised earlier — this crash site has been fixed once before, and the two bugs have exactly the same shape.

#3467 (April, hover path) had identical bottom frames: ast.(*Node).End → NodeList.HasTrailingComma → printer.hasTrailingComma → emitListRange. It was closed by #3485, which fixed it like this (nodebuilder_hover.go):

 	} else if t.symbol != nil && b.ch.IsSymbolAccessibleByFlags(t.symbol, b.ctx.enclosingDeclaration, flags) {
 		reference = b.symbolToExpression(t.symbol, ast.SymbolFlagsType)
+	} else if t.symbol != nil && t.symbol.Name == ast.InternalSymbolNameClass {
+		reference = b.f.NewIdentifier(b.getNameOfSymbolAsWritten(t.symbol))
 	}

i.e. one more branch so that a specific producer stops yielding a nil that ends up inside a NodeList.

This PR is the same move on a different producer:

 	for _, tp := range d.TypeParameters {
-		res = append(res, b.reuseNode(tp.AsNode()))
+		reused := b.reuseNode(tp.AsNode())
+		if reused == nil {
+			// fall back to serializing from the checker
+		}
+		res = append(res, reused)
 	}

Same three-step failure both times: a node-builder path leaves a nil → the nil is stored in a NodeList unchecked → the printer dereferences the list's last element in HasTrailingComma. The crash site itself is unchanged at HEAD — HasTrailingComma guards against an empty list but not against a nil element:

func (list *NodeList) HasTrailingComma() bool {
	if len(list.Nodes) == 0 {
		return false
	}
	last := list.Nodes[len(list.Nodes)-1]
	return last.End() < list.End()
}

So the honest framing is: #3485 closed one producer, this PR closes a second, and nothing prevents a third. There are ~410 NewNodeList(...) call sites; any of them fed a nil by a future builder path will produce the same SIGSEGV three frames away from the actual bug.

If you want to close the class rather than the instance, the structural option is to reject nils at NodeList construction/append time — a check (even debug-only) that panics with the culprit still on the stack. Making HasTrailingComma nil-tolerant would be the wrong direction — it would just push the bad node further into the printer. I kept this PR minimal on purpose, but I'm happy to add such a guard here or in a follow-up if you'd take it; equally fine if you'd rather handle that internally.

[Nicolaev Eduard (nikeedw)]

Follow-up on the NodeList question from my previous comment, this time with measurements instead of speculation. Everything below was run against main @ 86cc476 (this PR's base), go1.26.5, darwin/arm64.

Static survey. reuseNode/reuseTypeNode have 8 call sites outside tests. Six either check the result for nil or carry the fallback internally (nodebuilderimpl.go:2099; the nodecopy.go machinery itself). Two do not:

• pseudotypenodebuilder.go:246 — object-literal method type parameters; same shape as the site this PR patches (line 187).
• pseudotypenodebuilder.go:320 — NewLiteralTypeNode(b.reuseNode(source)); here a nil would go directly into a node constructor rather than a list.

I was not able to reach either one. A method-shorthand analogue of this PR's repro (setField<K extends keyof typeof state>(key, value) {} instead of the arrow property) type-checks cleanly, and per the instrumentation below constructs no nil.

Dynamic survey. I added an env-gated check to NodeFactory.NewNodeList on unpatched main that logs a stack whenever a list is constructed with a nil element, without changing behavior:

if nilCensusFile != "" { // TSGO_NIL_CENSUS
	for i, n := range nodes {
		if n == nil {
			// append stack to the census file; behavior otherwise unchanged
			break
		}
	}
}

Results:

• This PR's two-file repro (arrow form): exactly one nil, constructed by the block at pseudotypenodebuilder.go:184-190, logged before the printer panics. The producer is identified at construction time, three frames before the current crash site.
• The method-shorthand variant: zero nils.
• The full test suite, go test -count=1 ./internal/...: 57 packages pass, zero nil elements constructed anywhere in the corpus.

What this supports, and no more than this. On current main, no test constructs a NodeList containing a nil element. A construction-time assert (for example via debug.Assert in NewNodeList) would therefore not fire on any existing test, and for this bug it would have reported the producing line directly instead of a SIGSEGV in the printer. I have not verified whether it would have caught #3467, and I have no evidence that the two statically-unchecked sites above are reachable — only that nothing I tried reaches them.

I can add that assert to this PR or as a separate follow-up, or leave it with you — whichever you prefer.

[Wesley Wigham (weswigham)]

This should be a compiler test in testdata\tests\cases\compiler and not a full stack test, since it's not concerned with incremental/watch/CLI stuff.

[Nicolaev Eduard (nikeedw)]

I tried that first and verified against the unpatched code — as a compiler test this repro is green, because it never reaches the failing path. The shape necessarily carries declaration diagnostics (TS2527/TS4023), and declaration emit skips printing a file that has them:

declBlocked := len(diags) > 0 && !e.forceEmit && e.emitOnly != EmitOnlyForcedDts

The malformed list is still built — the diagnostics come from that same transform — but only EmitOnlyForcedDts prints in spite of them, and its sole production caller is the incremental d.ts signature computation in affectedfileshandler.go. The compiler runner sets neither forceEmit nor emitOnly, so the same two files under cases/compiler pass with and without the fix, while this scenario panics without it.

Agreed the defect itself is in the node builder/printer, not in anything incremental — incremental is just the only path that prints the result. If there's a diagnostics-free shape that makes type-parameter reuse fail (I went looking via the silent markError(nil) paths in the reuse visitor and couldn't build one), I'll gladly move this to a compiler test; otherwise I'd prefer to keep the incremental regression test, since it's the only harness that reaches the crash.

[Wesley Wigham (weswigham)]

This is a pretty suspect way to get the type from the node - is there a reason a simple ch.getTypeFromTypeNode(tp) doesn't suffice?

[Nicolaev Eduard (nikeedw)]

tp is a TypeParameterDeclaration, not a type node — getTypeFromTypeNodeWorker has no KindTypeParameter case, so getTypeFromTypeNode(tp.AsNode()) returns errorType. I tried it: errorType has no symbol, so it crashes in the node builder instead (typeParameterToName → symbolToName → lookupSymbolChain with a nil symbol), before the printer is even reached. tp.Constraint doesn't fit either — that yields the constraint's type, while typeParameterToDeclaration needs the type parameter itself (it reads the name, constraint and default off it).

The indirection was a fair point though: the declaration's symbol is already bound, so this is now getDeclaredTypeOfTypeParameter(node.Symbol()) — the same spelling getTypeParametersFromDeclaration uses. Pushed in 620995b; baselines unchanged.