Cache one nodebuilder per emit resolver, make emit resolver emit context scoped - #64649
Wesley Wigham (weswigham) wants to merge 11 commits into
Conversation
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: 5
Open (5)
emitContextcreates a sharp edge because the new public API accepts… · Newchecker.GetEmitResolver(emitContext)now constructs a new resolver each call, and… · Newchecker.GetEmitResolver(emitContext)now constructs a new resolver each call, and… · Newchecker.GetEmitResolver(emitContext)now constructs a new resolver each call, and… · NewEmitResolver.CreateLiteralConstValuenow owns parsing via its boundemitContext… · New
What changed in this PR
This PR refactors emit resolver creation to be EmitContext-aware, updates related interfaces, and adjusts transformer/checker call sites to use the new API while sharing resolver link state via Checker.
Changes:
- Update
GetEmitResolverAPIs to accept anEmitContextand removeEmitContextparameters fromEmitResolvernode-construction methods. - Centralize resolver link stores on
Checker(EmitResolverLinks) and adjustEmitResolverto use them. - Update transformers, compiler emit host, language service, and tests to pass/create an
EmitContextwhen requesting an emit resolver.
| File | Description |
|---|---|
| tsc/internal/transformers/tstransforms/importelision_test.go | Updates tests to create an EmitContext and pass it into GetEmitResolver. |
| tsc/internal/transformers/declarations/util.go | Switches helper functions from DeclarationEmitHost to printer.EmitResolver for flag checks. |
| tsc/internal/transformers/declarations/transform.go | Routes flag checks and type construction through tx.resolver (context-bound) and updates resolver method calls. |
| tsc/internal/printer/emitresolver.go | Changes EmitResolver interface to no longer take EmitContext for node construction methods. |
| tsc/internal/printer/emithost.go | Updates EmitHost.GetEmitResolver signature to require an EmitContext. |
| tsc/internal/ls/findallreferences.go | Creates an EmitContext when requesting an emit resolver for visibility checks. |
| tsc/internal/compiler/emitter.go | Passes emitContext into host.GetEmitResolver. |
| tsc/internal/compiler/emitHost.go | Reworks emit host to produce emit resolvers via a function taking EmitContext. |
| tsc/internal/checker/symbolaccessibility.go | Switches to getDiagnosticsEmitResolver() for diagnostics-layer checks. |
| tsc/internal/checker/nodebuilderimpl.go | Switches to getDiagnosticsEmitResolver() for helper visibility/undefined checks. |
| tsc/internal/checker/exports.go | Switches to getDiagnosticsEmitResolver() for implicit undefined check. |
| tsc/internal/checker/emitresolver.go | Makes EmitResolver context-bound, caches a node builder per resolver, and moves link stores to Checker. |
| tsc/internal/checker/checker.go | Adds EmitResolverLinks, introduces GetEmitResolver(emitContext), and renames cached resolver accessor to getDiagnosticsEmitResolver(). |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| var type_, initializer *ast.Node | ||
| if ast.IsPrimitiveLiteralValue(unwrapParenthesizedExpression(expression), true) { | ||
| initializer = tx.resolver.CreateLiteralConstValue(tx.EmitContext(), tx.EmitContext().ParseNode(assignment), tx.tracker) | ||
| initializer = tx.resolver.CreateLiteralConstValue(tx.EmitContext().ParseNode(assignment), tx.tracker) |
…mit contexts so they have finite lifetimes
| import ( | ||
| "context" | ||
| "sync" | ||
| "weak" |
There was a problem hiding this comment.
Is this strictly required? This will lock us out of tinygo for sure...
There was a problem hiding this comment.
What, the weak map? Yeah, we don't wanna leak emit contexts - since those retain node factories which in turn retain nodes (unless explicitly cleared). The cache has to be weak - or we have to break the contract of emit hosts managing emit contexts and shove emit host knowledge into the emit context just to manage a cache, which is.... bad.
You could always not cache but then every caller needs to be mindful of the lifetime, rather than letting the GC handle it.
There was a problem hiding this comment.
GC's always been bad to us anyway, so I made ownership and freeing of emit contexts explicit now instead of having a cache - no more global cache, callers should either take one or create one for long duration tasks.
What this means in practice is that since we make one emitHost per thread per file, that emitHost now makes one EmitResolver during the course of emitting that file (shared for both declaration and js emit), which was made with one EmitContext used throughout the whole process. Since the same checker is used for multiple files, the resolver still needs to use the checker lock... but nothing else should need any threading stuff.
…ion up to operation bounds (or remove entirely)
…context from the resolver now that it owns one
Fixes #64625