Skip to content

Don't reuse emit resolvers cross-file - #63986

Open
Jake Bailey (jakebailey) wants to merge 3 commits into
microsoft:mainfrom
jakebailey:test-race-fix
Open

Don't reuse emit resolvers cross-file#63986
Jake Bailey (jakebailey) wants to merge 3 commits into
microsoft:mainfrom
jakebailey:test-race-fix

Conversation

@jakebailey

Copy link
Copy Markdown
Member

Reusing resolvers appears to cause races, like:

diff --git a/tsc/testdata/baselines/reference/compiler/declarationEmitComputedPropertyNameSymbol2.js b/tsc/testdata/baselines/reference/compiler/declarationEmitComputedPropertyNameSymbol2.js
index dfd39246..fabe2d77 100644
--- a/tsc/testdata/baselines/reference/compiler/declarationEmitComputedPropertyNameSymbol2.js
+++ b/tsc/testdata/baselines/reference/compiler/declarationEmitComputedPropertyNameSymbol2.js
@@ -15,12 +15,24 @@ export const foo = { ...({} as Type) };
 
 
 //// [type.d.ts]
-declare namespace Foo {
-    const sym: unique symbol;
-}
 export type Type = {
     x?: {
         [Foo.sym]: 0;
     };
 };
-export {};
+
+
+!!!! File type.d.ts differs from original emit in noCheck emit
+//// [type.d.ts]
+--- Expected	The full check baseline
++++ Actual	with noCheck set
+@@ -1,5 +1,9 @@
++declare namespace Foo {
++    const sym: unique symbol;
++}
+ export type Type = {
+     x?: {
+         [Foo.sym]: 0;
+     };
+ };
++export {};
\ No newline at end of file

As far as I can tell, there's no perf impact for flipping this around to just make a new one and refactor things a bit.

Copilot AI balanced review requested due to automatic review settings August 24, 2026 18:16
@github-project-automation github-project-automation Bot moved this to Not started in PR Backlog Aug 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Prevents cross-file declaration-emit state leakage by replacing checker-wide resolver reuse with operation-scoped resolvers.

Changes:

  • Creates a fresh emit resolver per emit host.
  • Threads resolver instances through node building and symbol-accessibility checks.
  • Updates language-service and test callers.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tsc/internal/compiler/emitHost.go Creates per-file emit resolvers.
tsc/internal/checker/checker.go Removes cached resolver state.
tsc/internal/checker/emitresolver.go Shares the active resolver with node builders.
tsc/internal/checker/symbolaccessibility.go Accepts explicit resolver instances.
tsc/internal/checker/nodebuilder.go Adds resolver-aware constructors.
tsc/internal/checker/nodebuilderimpl.go Uses the builder’s resolver consistently.
tsc/internal/checker/nodecopy.go Routes accessibility checks through the builder.
tsc/internal/checker/exports.go Uses a fresh resolver for exported checker behavior.
tsc/internal/ls/findallreferences.go Isolates resolver state during reference lookup.
tsc/internal/transformers/tstransforms/importelision_test.go Updates test setup for the new API.

return &emitHost{
program: program,
emitResolver: checker.GetEmitResolver(),
emitResolver: checker.NewEmitResolver(),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have that, it's called "the flaky test I already mentioned"

@DanielRosenwasser

Copy link
Copy Markdown
Member

I noticed that newEmitResolver is

type EmitResolver struct {
	checker                 *Checker
	checkerMu               *sync.Mutex
	isValueAliasDeclaration func(node *ast.Node) bool
	aliasMarkingVisitor     func(node *ast.Node) bool
	referenceResolver       binder.ReferenceResolver
	jsxLinks                core.LinkStore[*ast.Node, JSXLinks]
	declarationLinks        core.LinkStore[*ast.Node, DeclarationLinks]
	declarationFileLinks    core.LinkStore[*ast.Node, DeclarationFileLinks]
}

func newEmitResolver(checker *Checker) *EmitResolver {
	e := &EmitResolver{checker: checker}
	e.isValueAliasDeclaration = e.isValueAliasDeclarationWorker
	e.aliasMarkingVisitor = e.aliasMarkingVisitorWorker
	e.checkerMu = &checker.mu
	return e
}

Does this allocate two closures per call?

  • It seems easy enough to run isValueAliasDeclaration as a loop instead of stashing away a closure.
  • aliasMarkingVisitor is only ever needed as part of PrecalculateDeclarationEmitVisibility, so maybe that could be initialized lazily.

@DanielRosenwasser

Copy link
Copy Markdown
Member

Also, is it worth running a perf test on here just to be safe?

@github-project-automation github-project-automation Bot moved this from Not started to Needs merge in PR Backlog Aug 24, 2026
@jakebailey

Copy link
Copy Markdown
Member Author

@jakebailey

Copy link
Copy Markdown
Member Author

Does this allocate two closures per call?

It does, but we only make one of these per file to emit, so it's not a hotspot.

All of this could use a good refactor, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs merge

Development

Successfully merging this pull request may close these issues.

3 participants