Skip to content

Fix panic in syncmap on loading nil #2056

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 11, 2025
+48 −2
Merged

Fix panic in syncmap on loading nil #2056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
036536b
fix syncmap panic on nil
haoqixu Nov 11, 2025
0f96095
fix lint
haoqixu Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions internal/collections/syncmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ type SyncMap[K comparable, V any] struct {

func (s *SyncMap[K, V]) Load(key K) (value V, ok bool) {
val, ok := s.m.Load(key)
if !ok {
if !ok || val == nil {
Copy link
Member

@jakebailey jakebailey Nov 11, 2025

Choose a reason for hiding this comment

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

I don't understand when this would happen. If the map holds V, why would we ever have stored a nil except when V is any and therefore the conversion should succeed?

Copy link
Contributor Author

@haoqixu haoqixu Nov 11, 2025

Choose a reason for hiding this comment

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

The panic occurs only if V is an interface, and we store a nil into the map and then load it. For example, cachedvfs holds a statCache whose type is collections.SyncMap[string, vfs.FileInfo]. If the implementation of its underlying fs returns a nil literal in its Stat, that nil will cause a panic when we load it from the map. 🤔

Copy link
Contributor Author

@haoqixu haoqixu Nov 11, 2025

Choose a reason for hiding this comment

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

If the implementation of its underlying fs returns a nil literal in its Stat, that nil will cause a panic when we load it from the map.

It looks like this: https://go.dev/play/p/AmDx9AdDj6h

Copy link
Member

@jakebailey jakebailey Nov 11, 2025

Choose a reason for hiding this comment

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

Darn, ok.

return value, ok
}
return val.(V), true
Expand All @@ -25,6 +25,10 @@ func (s *SyncMap[K, V]) Store(key K, value V) {

func (s *SyncMap[K, V]) LoadOrStore(key K, value V) (actual V, loaded bool) {
actualAny, loaded := s.m.LoadOrStore(key, value)
if actualAny == nil {
return actual, loaded
}

return actualAny.(V), loaded
}

Expand All @@ -38,7 +42,17 @@ func (s *SyncMap[K, V]) Clear() {

func (s *SyncMap[K, V]) Range(f func(key K, value V) bool) {
s.m.Range(func(key, value any) bool {
return f(key.(K), value.(V))
var k K
if key != nil {
k = key.(K)
}

var v V
if value != nil {
v = value.(V)
}

return f(k, v)
})
}

Expand Down
32 changes: 32 additions & 0 deletions internal/collections/syncmap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package collections_test

import (
"testing"

"github.com/microsoft/typescript-go/internal/collections"
"gotest.tools/v3/assert"
)

func TestSyncMapWithNil(t *testing.T) {
t.Parallel()

var m collections.SyncMap[string, any]

got1, ok := m.Load("foo")
assert.Assert(t, !ok)
assert.Equal(t, got1, nil)

m.Store("foo", nil)

got2, ok := m.Load("foo")
assert.Assert(t, ok)
assert.Equal(t, got2, nil)

too, loaded := m.LoadOrStore("too", nil)
assert.Assert(t, !loaded)
assert.Equal(t, too, nil)

m.Range(func(k string, v any) bool {
return true
})
}