-
Notifications
You must be signed in to change notification settings - Fork 659
Fix NodeBuilder.ctxStack management #1252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix NodeBuilder.ctxStack management #1252
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems correct to me, good find.
In managing
NodeBuilder
's context stack (internal/checker/nodebuilder.go#L20-L50) we have essentially:This is incorrect --
enterContext
assumes the current context is the top of the stack, butpopContext
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:
From printf-debugging, the sequence of operations at issue is effectively:
reverseMappedStack
reverseMappedStack
-- 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.