Skip defaults when printing BindingName #1025

muglug · 2025-06-03T18:11:56Z

This should fix #1012

This fills in a TODO to match code in checker.ts, and fixes a bunch of test discrepancies.

PR also adds more bounds checks to comment parsing to to match the JS implementation.

text.charCodeAt(pos) returns NaN when the pos is out of range, which means the big switch falls through and returns, whereas calling text[pos:] panics in existing code when pos is out of range.

jakebailey · 2025-06-03T18:13:35Z

Can you add a repro to https://github.com/microsoft/typescript-go/tree/main/testdata/tests/cases/compiler?

muglug · 2025-06-03T18:58:11Z

Yes, though now I'm determined to track down the original bug (actually in the error printing step)

muglug · 2025-06-03T19:29:03Z

@microsoft-github-policy-service agree

internal/printer/printer.go

This reverts commit 37177c8.

jakebailey · 2025-06-04T00:47:05Z

FWIW you probably want to retitle the PR 😄

jakebailey · 2025-06-04T06:20:56Z

Also fixes a data race in LinkStore.

Where is that in the PR?

muglug · 2025-06-04T12:24:27Z

I removed it — I was able to repro the race locally, but then I added a missing switch that I had omitted and the race disappeared.

The race prevention code is pretty simple but also not necessary, at least for this PR (I believe).

internal/checker/nodebuilderimpl.go

Co-authored-by: Jake Bailey <[email protected]>

internal/checker/nodebuilderimpl.go

jakebailey

Great work, much appreciated!

jakebailey · 2025-06-04T19:52:59Z

Ugh, I guess the nil check is a thing? Hmmm

jakebailey · 2025-06-04T20:00:45Z

The context being nil I think is a bug in the PR, not something that needs a check. What I'm seeing in the debugger is that in the initial frame. ctx is set, but when we recurse, the next time it gets to the cloning function, it's nil.

jakebailey · 2025-06-04T20:14:51Z

Ugh, #1001 broke it, it's the thing intializeClosures was doing to "recapture the context", even though I haven't been able to figure out why that matters.

jakebailey · 2025-06-04T20:31:37Z

The problem was that nodeBuilderImpl was returned by value, so the bound funcs actually never worked in that func.

jakebailey

Approving but since I pushed last it I'll have to get someone else to +1 it

muglug · 2025-06-04T21:40:32Z

Thanks for your pointers!

jakebailey · 2025-06-04T21:41:24Z

Pun intended!

scottbessler · 2025-06-05T21:13:12Z

With this fix in place I'm still seeing a similar panic:

panic: runtime error: slice bounds out of range [20017:2169]

goroutine 3919 [running]:
github.com/microsoft/typescript-go/internal/scanner.SkipTriviaEx({0x14000150900, 0x879}, 0x1401f3d7a70?, 0xbc?)
        /usr/local/repos/typescript-go/internal/scanner/scanner.go:2100 +0x490
github.com/microsoft/typescript-go/internal/scanner.SkipTrivia(...)
        /usr/local/repos/typescript-go/internal/scanner/scanner.go:2085
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitSourceMapsBeforeNode(0x1402005fe00, 0x1401f3d7a70)
        /usr/local/repos/typescript-go/internal/printer/printer.go:5470 +0x118
github.com/microsoft/typescript-go/internal/printer.(*Printer).enterNode(0x1402005fe00, 0x1401f3d7a70)
        /usr/local/repos/typescript-go/internal/printer/printer.go:5694 +0x50
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitBindingIdentifier(0x1402005fe00, 0x1401f3d7a70)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1140 +0xd4
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitBindingName(0x1402005fe00?, 0x140214de230?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1191 +0x48
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitParameterName(...)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1413
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitParameter(0x1402005fe00, 0x140214de230)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1421 +0x84
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitParameterNode(0x1402005fe00?, 0x1401fca71f8?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1433 +0x34
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitListItems(0x1402005fe00, 0x1035271f0, 0x140214a87e0, {0x1400741fd38, 0x1, 0x12af4c098?}, 0xa10, 0x0, {0xffffffff, 0xffffffff})
        /usr/local/repos/typescript-go/internal/printer/printer.go:4676 +0x3b8
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitListRange(0x1402005fe00, 0x1035271f0, 0x140214a87e0, 0x14010b920c0, 0xa10, 0x14010b7b340?, 0x1401fca7308?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:4473 +0x324
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitList(0x1402005fe00, 0x1035271f0, 0x140214a87e0, 0x14010b920c0, 0xa10)
        /usr/local/repos/typescript-go/internal/printer/printer.go:4418 +0x8c
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitParameters(0x1402005fe00?, 0x140214a87e0?, 0x14010b920c0?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1485 +0x5c
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitFunctionType(0x1402005fe00, 0x140214a87e0)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1867 +0x17c
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitTypeNode(0x1402005fe00, 0x140214a87e0, 0x1?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:2230 +0x210
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitTypeNodePreservingExtends(...)
        /usr/local/repos/typescript-go/internal/printer/printer.go:2192
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitReturnType(0x1402005fe00, 0x140214a87e0)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1856 +0xe0
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitFunctionType(0x1402005fe00, 0x140214a8850)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1869 +0x1b0
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitTypeNode(0x1402005fe00, 0x140214a8850, 0x1fca7548?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:2230 +0x210
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitTypeNodePreservingExtends(...)
        /usr/local/repos/typescript-go/internal/printer/printer.go:2192
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitTypeNodeOutsideExtends(...)
        /usr/local/repos/typescript-go/internal/printer/printer.go:2186
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitTypeAnnotation(0x1402005fe00, 0x140214a8850)
        /usr/local/repos/typescript-go/internal/printer/printer.go:1469 +0x8c
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitVariableDeclaration(0x1402005fe00, 0x140214a51e0)
        /usr/local/repos/typescript-go/internal/printer/printer.go:3437 +0x64
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitVariableDeclarationNode(0x1402005fe00?, 0x1401fca75f8?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:3444 +0x34
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitListItems(0x1402005fe00, 0x103527238, 0x14010b92100, {0x1400741fe60, 0x1, 0x80?}, 0x210, 0x0, {0x68, 0xbc})
        /usr/local/repos/typescript-go/internal/printer/printer.go:4676 +0x3b8
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitListRange(0x1402005fe00, 0x103527238, 0x14010b92100, 0x14012fcdb20, 0x210, 0x10330b0a8?, 0x1?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:4473 +0x324
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitList(0x1402005fe00, 0x103527238, 0x14010b92100, 0x14012fcdb20, 0x210)
        /usr/local/repos/typescript-go/internal/printer/printer.go:4418 +0x8c
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitVariableDeclarationList(0x1402005fe00, 0x14010b92100)
        /usr/local/repos/typescript-go/internal/printer/printer.go:3464 +0x1fc
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitVariableStatement(0x1402005fe00, 0x14020b5d5e0)
        /usr/local/repos/typescript-go/internal/printer/printer.go:3152 +0x6c
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitStatement(0x1401f081180?, 0x1401fca7848?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:3903 +0x98
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitListItems(0x1402005fe00, 0x1035271f8, 0x14020fa7348, {0x14020fcaba0, 0xa, 0x102f9c998?}, 0x1, 0x0, {0x0, 0x878})
        /usr/local/repos/typescript-go/internal/printer/printer.go:4676 +0x3b8
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitListRange(0x1402005fe00, 0x1035271f8, 0x14020fa7348, 0x1401c30f1c0, 0x1, 0x14000b3f8c8?, 0x1401fca7938?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:4473 +0x324
github.com/microsoft/typescript-go/internal/printer.(*Printer).emitSourceFile(0x1402005fe00, 0x14020fa7348)
        /usr/local/repos/typescript-go/internal/printer/printer.go:4395 +0x1d8
github.com/microsoft/typescript-go/internal/printer.(*Printer).Write(0x1402005fe00, 0x14020fa7348, 0x14009364900?, {0x10353f890, 0x1401f081180}, 0x1?)
        /usr/local/repos/typescript-go/internal/printer/printer.go:4925 +0xcf4
github.com/microsoft/typescript-go/internal/compiler.(*emitter).printSourceFile(0x1401ee7f9e0, {0x14009364900, 0x56}, {0x14009364960, 0x5a}, 0x14020fa7348, 0x1402005fe00, 0x1)
        /usr/local/repos/typescript-go/internal/compiler/emitter.go:158 +0x1e8
github.com/microsoft/typescript-go/internal/compiler.(*emitter).emitDeclarationFile(0x1401ee7f9e0, 0x14000b3f8c8, {0x14009364900, 0x56}, {0x14009364960, 0x5a})
        /usr/local/repos/typescript-go/internal/compiler/emitter.go:127 +0x498
github.com/microsoft/typescript-go/internal/compiler.(*emitter).emit(0x1401ee7f9e0)
        /usr/local/repos/typescript-go/internal/compiler/emitter.go:43 +0x58
github.com/microsoft/typescript-go/internal/compiler.(*Program).Emit.func2()
        /usr/local/repos/typescript-go/internal/compiler/program.go:839 +0x140
github.com/microsoft/typescript-go/internal/core.(*parallelWorkGroup).Queue.func1()
        /usr/local/repos/typescript-go/internal/core/workgroup.go:39 +0x5c
created by github.com/microsoft/typescript-go/internal/core.(*parallelWorkGroup).Queue in goroutine 1
        /usr/local/repos/typescript-go/internal/core/workgroup.go:37 +0x84

I will try and track down a repro, but thought it would be useful to report straight away. git bisect points at the same commit being the source of the issue: 6fc5cb8.

muglug · 2025-06-05T22:02:17Z

yeah a repro is necessary to fix — happy to look when you have one

muglug · 2025-06-06T12:58:15Z

Another quick note that my PR deliberately did not add an out-of-range check at the top of SkipTriviaEx that exists (implicitly) in the original JS, because a panic there is a sign that logic elsewhere in the system is incorrect.

jakebailey reviewed Jun 3, 2025

View reviewed changes

internal/printer/printer.go Outdated Show resolved Hide resolved

muglug added 4 commits June 3, 2025 20:22

Add a couple of bounds checks

2d750e2

Add loop-start bounds check

6fa549c

Add code to detect incorrect position information

f25baca

Revert "Add code to detect incorrect position information"

edc757a

This reverts commit 37177c8.

muglug force-pushed the muglug-no-crash branch 2 times, most recently from 930b6c1 to 05627ce Compare June 4, 2025 00:30

muglug added 2 commits June 3, 2025 20:43

Improve test expectations

7377cdb

Fill in TODO

a702ed6

muglug force-pushed the muglug-no-crash branch from 05627ce to a702ed6 Compare June 4, 2025 00:43

muglug changed the title ~~Add more bounds checks to comment parsing~~ Skip defaults when printing BindingName Jun 4, 2025

Add missing conditionals from JS source

2f4112b

muglug force-pushed the muglug-no-crash branch from 5bf0270 to 2f4112b Compare June 4, 2025 04:05

jakebailey reviewed Jun 4, 2025

View reviewed changes

internal/checker/nodebuilderimpl.go Outdated Show resolved Hide resolved

internal/checker/nodebuilderimpl.go Outdated Show resolved Hide resolved

internal/checker/nodebuilderimpl.go Outdated Show resolved Hide resolved

muglug and others added 3 commits June 4, 2025 14:26

Update internal/checker/nodebuilderimpl.go

eb3d156

Co-authored-by: Jake Bailey <[email protected]>

Remove nil check

7f5d9d2

Simplify function

741a83b

jakebailey reviewed Jun 4, 2025

View reviewed changes

internal/checker/nodebuilderimpl.go Outdated Show resolved Hide resolved

muglug added 2 commits June 4, 2025 15:19

Merge remote-tracking branch 'upstream/main' into muglug-no-crash

f3bb008

Move visitor into field

4746875

jakebailey approved these changes Jun 4, 2025

View reviewed changes

Fix nodeBuilderImpl method binding bug

88eb8b9

jakebailey approved these changes Jun 4, 2025

View reviewed changes

jakebailey enabled auto-merge June 4, 2025 20:38

sheetalkamat approved these changes Jun 4, 2025

View reviewed changes

jakebailey added this pull request to the merge queue Jun 4, 2025

Merged via the queue into microsoft:main with commit 14e44b1 Jun 4, 2025
23 checks passed

muglug deleted the muglug-no-crash branch June 4, 2025 21:23

Skip defaults when printing BindingName #1025

Skip defaults when printing BindingName #1025

Uh oh!

Conversation

muglug commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakebailey commented Jun 3, 2025

Uh oh!

muglug commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muglug commented Jun 3, 2025

Uh oh!

Uh oh!

jakebailey commented Jun 4, 2025

Uh oh!

jakebailey commented Jun 4, 2025

Uh oh!

muglug commented Jun 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Jun 4, 2025

Uh oh!

jakebailey commented Jun 4, 2025

Uh oh!

jakebailey commented Jun 4, 2025

Uh oh!

jakebailey commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

muglug commented Jun 4, 2025

Uh oh!

jakebailey commented Jun 4, 2025

Uh oh!

scottbessler commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muglug commented Jun 5, 2025

Uh oh!

muglug commented Jun 6, 2025

Uh oh!

Uh oh!

muglug commented Jun 3, 2025 •

edited

Loading

muglug commented Jun 3, 2025 •

edited

Loading

jakebailey commented Jun 4, 2025 •

edited

Loading

scottbessler commented Jun 5, 2025 •

edited

Loading