Use one cached nodebuilder for potentially circular internal ops, just like strada

[Wesley Wigham (weswigham)]

Turns out it's not just for perf, it's also functional and is what enables circularity bails.

Fixes #3805.

[Copilot]

Pull request overview

Caches a single shared NodeBuilder instance (typeToStringNodebuilder) for Checker.getNodeBuilder() calls, matching the strada (TypeScript) reference implementation. This is not merely a performance optimization—reusing the same nodebuilder enables its internal circularity tracking to bail out on recursive type-to-string scenarios (e.g. satisfies typeof f), fixing the stack overflow in #3805. Baselines update across many quickInfo verbosity tests because the cached builder now produces the expected "compact" forms.

Changes:

• Add typeToStringNodebuilder *NodeBuilder field to Checker.
• Make getNodeBuilder() lazily create and reuse a single instance.
• Add a compiler test plus update fourslash/quickInfo baselines.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/checker/checker.go	Adds cached typeToStringNodebuilder field on Checker.
internal/checker/nodebuilder.go	getNodeBuilder returns a cached NodeBuilder rather than allocating each call.
testdata/tests/cases/compiler/noTypeToStringStackOverflow.ts	New minimal repro for issue #3805.
testdata/baselines/reference/compiler/noTypeToStringStackOverflow.*	New baselines for the repro test (types/symbols/errors).
testdata/baselines/reference/fourslash/quickInfo/*.baseline	Updated quickInfo baselines reflecting the consistent cached-nodebuilder output.

[Wesley Wigham (weswigham)]

Note that it's fine for the LS ops that use this to get a fresh builder with an identifier map to do so because any circularities from type lookup-caused diagnostic-generation during node building will dive back into the non-ex entrypoint and use the cached builder, which'll catch and bail on the circularity after another go-around.

Truth be told, it'd be way simpler if diagnostic calculation was more totally decoupled from type evaluation (something we do sometimes with deferred checking), so the later could be safely used without fear of reentrancy in more cases (since diagnostic calculation may need to dip into node building but type evaluation shouldn't and is needed for reliable type printback), but this is what strada does to work around the problem and it works, so... yeah.

[Anders Hejlsberg (ahejlsberg)]

Yes, decoupling type resolution from type checking has long been on our wish list. But given that the code paths are typically the same, it is really tempting to just have one and save the duplication/complexity of having two.

[Wesley Wigham (weswigham)]

We do have two phases already, we just don't reliably do all diagnostics in the later - we mix most into the forward pass just because it's simplest to - like you said, calculation and checking are a hair apart for most node kinds. We do have many cases where we do the actual diagnostics later on in the checkNodeDeferred pass (often because we need all finalized signature resolution to be done so we can use all those cached types for the errors).

🤷‍♂️ Just something to think about if we wanna do a big logical cleanup/separation at some point. Caching is also funky tristates throughout the checker because type eval cache state is tied up in diagnostic cache state.

[Jake Bailey (jakebailey)]

I thought we had undone this because it caused way too much memory retention?

(I'd have to find the PR)

[Wesley Wigham (weswigham)]

The only long-term thing the node builder should retain is cached built nodes to avoid doing the work of transforming the same type multiple times. It's a classic memory vs speed tradeoff.

[Anders Hejlsberg (ahejlsberg)]

Wesley Wigham (@weswigham) I'm not 100% understanding how this PR causes the other baseline changes where structural types change into type aliases. Would you mind explaining?

[Wesley Wigham (weswigham)]

We're finding cached types in the node builder from previous invocations and reusing the cached resultant nodes, is all. Whiiiich, looks like is also a bug. When expandable hover is on in strada we bust the cache but in corsa we don't right now, lemme fix that quick.

[Jake Bailey (jakebailey)]

That feels really spooky...

[Wesley Wigham (weswigham)]

Transforming a type into a node given the same context and node builder flags should produce the same output every time - makes sense to cache it. 🤷‍♂️

[Anders Hejlsberg (ahejlsberg)]

We're finding cached types in the node builder from previous invocations and reusing the cached resultant nodes, is all.

But why does that produce different output than without the cache? I mean, the baselines are different.

[Wesley Wigham (weswigham)]

But why does that produce different output than without the cache?

Because the quickinfo request includes expandable hover configuration in the node builder context that produces a different node than was set in the cached result - which is why strada busts the cache on expandable hover requests and always generates a new node. Really, all that hover context configuration should just be part of the cache key, to avoid totally uncached calls.

[Wesley Wigham (weswigham)]

There we go, no more expandable hover quickinfo chages by doing the same thing strada does and skipping the cache when expandable hover is on. We could probably do better, but I don't think we've needed to (the node builder has caching for highly degenerate declaration emit cases - 5MB types have a lot of repeated parts usually - it matters way less for quickinfo I imagine).

[Anders Hejlsberg (ahejlsberg)]

I'm good with this, but want to hear if Jake Bailey (@jakebailey) found out if we undid the node builder sharing for a reason?

[Jake Bailey (jakebailey)]

I'm referring to #1289, which fixed #988. This PR effectively undoes that PR, so I would be pretty worried about this causing issues again.

Especially without #3435 which would serve to mitigate this problem slightly?

[Wesley Wigham (weswigham)]

Eh, if the memory is actually still a problem, we probably need to look into why it's an issue in corsa but not strada and mitigate along that axis instead then, 'cause as we've discovered - there's a functional, not just performance, reason to only be using one node builder during checker ops.

From our discussion before in the prior PR, the observed memory usage seems to be because the nodes we build are just forever tied to the lifetime of the underlying EmitContext's Factory, rather than simply gc collectable; but! That seems like an excuse now - we know we hold onto a ton of nodes because of the cache in visitAndTransformType, which should be true across corsa and strada - so why would corsa hold on to so much more? Or why would it build so much more? Is it all traffic in uncached node kinds, like simple string keyword nodes that we make one billion instances of and hold onto forever now that they're arena allocated? Should the factory we use in the node builder simply not be arena-allocated so the nodes are free to be GC'd if they're uncached? Questions we can choose to return to, I suppose.

[Anders Hejlsberg (ahejlsberg)]

I have a feeling that the underlying issue in #988 was something else, particularly since the excessively deep type instantiation errors are gone as was commented later in the thread. A lot has changed in the last 10 months.

[Wesley Wigham (weswigham)]

Jake Bailey (@jakebailey) If you can still find a project with a memory issue, I have this commit which does what I suggested back in that other PR and allows arenas to be released so objects formerly within them can be independently GC'd, which, barring GC quirkiness I can't predict, should free memory in the same way making new factories did, but allow for retaining things in caches still. (And if that's insufficient, then we really would have to look into what cached stuff is for some reason different.)

Hard to justify/work with without some kinda repeatable test case, though!

[Jake Bailey (jakebailey)]

I guess we'll find out...