Fix NodeBuilder.ctxStack management #1252
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In managing `NodeBuilder`'s context stack (internal/checker/nodebuilder.go#L20-L50) we have essentially:
```go
func (b *NodeBuilder) enterContext(...) {
b.impl.ctx = &NodeBuilderContext{...}
b.ctxStack = append(b.ctxStack, b.impl.ctx)
}
func (b *NodeBuilder) popContext() {
b.impl.ctx = b.ctxStack[len(b.ctxStack)-1]
b.ctxStack = b.ctxStack[:len(b.ctxStack)-1]
}
```
This is incorrect -- `enterContext` assumes the current context is the top of the stack, but `popContext` assumes that the stack includes only the non-current contexts, so the top of the stack is the next context to use when we pop.
In this commit I fix the stack to work consistently -- the current context is not on the stack.
This fixes a crash in our codebase. Unfortunately the conditions for the crash are very complex -- they require a particular file-loading order (in the parallel workers) and involve some of our most complex types, so I haven't been able to come up with a repro. The stacktrace is:
```
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).addPropertyToElementList(0x14015cdae00, 0x1425dde0e68, {0x0, 0x0, 0x0})
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2152 +0x6e0
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).createTypeNodesFromResolvedType(0x14015cdae00, 0x14312b65790)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2223 +0x5a0
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).createTypeNodeFromObjectType(0x14015cdae00, 0x14312b65790)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2284 +0x22c
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).createAnonymousTypeNode(0x144b65be7b8?, 0x14312b65790?)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2375 +0x310
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).typeToTypeNode(0x14015cdae00, 0x5109bf37f709a572?)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2917 +0x63c
github.com/microsoft/typescript-go/internal/core.Map[...](...)
/Users/benkraft/src/typescript-go/internal/core/core.go:56
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).mapToTypeNodes(0x14015cdae00, {0x1431014ef70, 0x2, 0x102c2f544?})
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:219 +0xc8
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).typeToTypeNode(0x14015cdae00, 0x0?)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2899 +0x6bc
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).serializeTypeForDeclaration(0x14015cdae00, 0x0, 0x1468314c448?, 0x0?)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:1902 +0x2c4
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).addPropertyToElementList(0x14015cdae00, 0x14683a3b2d8, {0x14683a5c060, 0x3, 0x4})
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2147 +0x554
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).createTypeNodesFromResolvedType(0x14015cdae00, 0x14683a53800)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2223 +0x5a0
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).createTypeNodeFromObjectType(0x14015cdae00, 0x14683a53800)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2284 +0x22c
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).visitAndTransformType(0x14015cdae00, 0x14683a53800, 0x10368c560)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2645 +0x4a8
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).createAnonymousTypeNode(0x14015cdae00, 0x14683a53800)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2371 +0x300
github.com/microsoft/typescript-go/internal/checker.(*nodeBuilderImpl).typeToTypeNode(0x14015cdae00, 0x140abfceff0?)
/Users/benkraft/src/typescript-go/internal/checker/nodebuilderimpl.go:2917 +0x63c
github.com/microsoft/typescript-go/internal/checker.(*NodeBuilder).TypeToTypeNode(0x141e4d55c20, 0x14683a53800, 0x9?, 0x3595840?, 0x1?, {0x0?, 0x0?})
/Users/benkraft/src/typescript-go/internal/checker/nodebuilder.go:163 +0x50
github.com/microsoft/typescript-go/internal/checker.(*Checker).typeToStringEx(0x141e3d7d008, 0x14683a53800, 0x140abfceff0, 0x0)
/Users/benkraft/src/typescript-go/internal/checker/printer.go:185 +0x108
github.com/microsoft/typescript-go/internal/checker.(*Checker).getTypeNamesForErrorDisplay(0x141e3d7d008, 0x14683a53800, 0x1450fef6360)
/Users/benkraft/src/typescript-go/internal/checker/relater.go:1273 +0x48
github.com/microsoft/typescript-go/internal/checker.(*Relater).reportRelationError(0x140c6d48000, 0x0, 0x14683a53800, 0x1450fef6360)
/Users/benkraft/src/typescript-go/internal/checker/relater.go:4670 +0x40
github.com/microsoft/typescript-go/internal/checker.(*Relater).isRelatedToEx(0x140c6d48000, 0x14683a53800, 0x1450fef6360, 0x3, 0x1, 0x0, 0x0)
/Users/benkraft/src/typescript-go/internal/checker/relater.go:2625 +0x3f0
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkTypeRelatedToEx(0x141e3d7d008, 0x14683a53800, 0x1450fef6360, 0x1412ea6f360, 0x140abf9d5d8, 0x0, 0x0)
/Users/benkraft/src/typescript-go/internal/checker/relater.go:373 +0x138
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkTypeRelatedToAndOptionallyElaborate(0x141e3d7d008, 0x14683a53800, 0x1450fef6360, 0x1412ea6f360, 0x140abf9d5d8, 0x140abfceff0, 0x0, 0x0)
/Users/benkraft/src/typescript-go/internal/checker/relater.go:434 +0x9c
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkTypeAssignableToAndOptionallyElaborate(...)
/Users/benkraft/src/typescript-go/internal/checker/relater.go:426
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkVariableLikeDeclaration(0x141e3d7d008, 0x140abf9d5d8)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:5599 +0x300
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkVariableDeclaration(0x141e3d7d008, 0x140abf9d5d8)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:5489 +0x48
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceElementWorker(0x141e3d7d008, 0x140abf9d5d8)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2257 +0x2d8
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceElement(0x141e3d7d008, 0x102e84744?)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2155 +0x50
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceElements(0x141e3d7d008, {0x140abfa0838, 0x1, 0x141e6378f60?})
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2146 +0x34
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkVariableDeclarationList(...)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:5484
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkVariableStatement(0x141e3d7d008, 0x141da06fca8?)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:5480 +0xb8
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceElementWorker(0x141e3d7d008, 0x140a9b517a8)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2227 +0x258
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceElement(0x141e3d7d008, 0x102ee8304?)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2155 +0x50
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceElements(0x141e3d7d008, {0x140abfa0890, 0x3, 0x102eb03a0?})
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2146 +0x34
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkBlock(0x141e3d7d008, 0x140abfb8cc8)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:3587 +0x130
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceElementWorker(0x141e3d7d008, 0x140abfb8cc8)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2225 +0x308
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceElement(0x141e3d7d008, 0x140abfa28c0?)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2155 +0x50
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkFunctionExpressionOrObjectLiteralMethodDeferred(0x141e3d7d008, 0x140abfa28c0)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:9830 +0xbc
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkDeferredNode(0x141e3d7d008, 0x103698fe0?)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2331 +0xd0
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkDeferredNodes-range1(...)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2316
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkDeferredNodes.(*OrderedSet[...]).Keys.func1(...)
/Users/benkraft/src/typescript-go/internal/collections/ordered_map.go:129
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkDeferredNodes(0x141e3d7d008, 0x140ab8777e0?)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2312 +0xc0
github.com/microsoft/typescript-go/internal/checker.(*Checker).checkSourceFile(0x141e3d7d008, {0x1036946e8, 0x103bd3800}, 0x140ab801608)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2119 +0x108
github.com/microsoft/typescript-go/internal/checker.(*Checker).CheckSourceFile(0x141e3d7d008, {0x1036946e8, 0x103bd3800}, 0x140ab801608)
/Users/benkraft/src/typescript-go/internal/checker/checker.go:2107 +0x64
github.com/microsoft/typescript-go/internal/compiler.(*Program).CheckSourceFiles.func1-range1(0x1404d9bbf38?)
/Users/benkraft/src/typescript-go/internal/compiler/program.go:301 +0x4c
github.com/microsoft/typescript-go/internal/compiler.(*checkerPool).Files.func1(0x141e63718f0)
/Users/benkraft/src/typescript-go/internal/compiler/checkerpool.go:82 +0x84
github.com/microsoft/typescript-go/internal/compiler.(*Program).CheckSourceFiles.func1()
/Users/benkraft/src/typescript-go/internal/compiler/program.go:300 +0xdc
github.com/microsoft/typescript-go/internal/core.(*parallelWorkGroup).Queue.func1()
/Users/benkraft/src/typescript-go/internal/core/workgroup.go:39 +0x5c
created by github.com/microsoft/typescript-go/internal/core.(*parallelWorkGroup).Queue in goroutine 1
/Users/benkraft/src/typescript-go/internal/core/workgroup.go:37 +0x84
```
From printf-debugging, the sequence of operations at issue is effectively:
1. push a context
2. push to the `reverseMappedStack`
3. push a context
4. pop the context (actually doesn't pop, due to the bug)
5. pop from the `reverseMappedStack` -- which is now the empty stack from (3) instead of the one we pushed to in (2), panic
Let me know if there's a good test I should add here, or if somemone who better understands what this code is doing can suggest where to look for a test case.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the inconsistent management of NodeBuilder’s context stack to prevent crashes due to an extra push of the current context.
- Removes a duplicate push to the context stack in enterContext.
- Adjusts context management to ensure that popContext retrieves the correct previous context.
Comments suppressed due to low confidence (2)
internal/checker/nodebuilder.go:20
- Consider adding unit tests that simulate nested context operations and concurrent file-loading scenarios. These tests can help ensure that the adjusted ctxStack management handles edge cases correctly.
func (b *NodeBuilder) enterContext(enclosingDeclaration *ast.Node, flags nodebuilder.Flags, internalFlags nodebuilder.InternalFlags, tracker nodebuilder.SymbolTracker) {
internal/checker/nodebuilder.go:21
- Verify that pushing the previous context before creating a new one meets the intended design. The current change intends to ensure the current context does not reside on the stack, which appears correct—double-check that all invocations of popContext correctly assume this invariant.
b.ctxStack = append(b.ctxStack, b.impl.ctx)
jakebailey
approved these changes
Jun 21, 2025
This was referenced Jun 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In managing
NodeBuilder's context stack (internal/checker/nodebuilder.go#L20-L50) we have essentially:This is incorrect --
enterContextassumes the current context is the top of the stack, butpopContextassumes that the stack includes only the non-current contexts, so the top of the stack is the next context to use when we pop.In this commit I fix the stack to work consistently -- the current context is not on the stack.
This fixes a crash in our codebase. Unfortunately the conditions for the crash are very complex -- they require a particular file-loading order (in the parallel workers) and involve some of our most complex types, so I haven't been able to come up with a repro. The stacktrace is:
From printf-debugging, the sequence of operations at issue is effectively:
reverseMappedStackreverseMappedStack-- which is now the empty stack from (3) instead of the one we pushed to in (2), panicLet me know if there's a good test I should add here, or if somemone who better understands what this code is doing can suggest where to look for a repro/test case.