diff --git a/lib/atom/README.md b/lib/atom/README.md index 2a188d6d..43365ba3 100644 --- a/lib/atom/README.md +++ b/lib/atom/README.md @@ -14,7 +14,7 @@ The constructors are the module's only top-level members; all mutation happens t ## Types -The cells created above are custom Starlark values. Each is truthy when its value is non-zero / non-empty, hashable (usable as a dict key), and ordered (`==`, `!=`, `<`, `<=`, `>`, `>=` compare by current value against another cell of the same type). `str()` renders as ``, ``, ``. +The cells created above are custom Starlark values. Each is truthy when its value is non-zero / non-empty, and ordered (`==`, `!=`, `<`, `<=`, `>`, `>=` compare by current value against another cell of the same type). Cells are **not hashable** — a mutable cell cannot be a dict/set key (a live-value hash would go stale on mutation and strand the entry); key by `x.get()` when a snapshot key is what you mean. `str()` renders as ``, ``, ``. ### `atom_int` @@ -112,6 +112,7 @@ print(x.get()) - **Type names.** Script-visible type names are `atom_int`, `atom_float`, `atom_string` (as returned by `type()`); the underlying Go types are `AtomicInt`, `AtomicFloat`, `AtomicString`. - **Truthiness.** A cell is falsy when its value is `0` / `0.0` / `""` and truthy otherwise (`bool(new_int(0))` is `False`). -- **Comparison and hashing.** Cells are comparable only against the same cell type and are hashable by current value, so they work as dict keys. Mutating a cell after using it as a key leaves the existing entry under the old hash — treat keyed cells as you would any mutable key. +- **Comparison and hashing.** Cells are comparable only against the same cell type. They are **not hashable**: using a cell as a dict/set key errors with `unhashable type: atom_int` (or `atom_float` / `atom_string`) — a live-value hash goes stale when the cell mutates and the keyed entry becomes unreachable. Key by `x.get()` instead. +- **Freezing.** Once a cell is frozen (Starlark freezes values that cross a module boundary), every mutating method — `set`, `cas`, `add`, `sub`, `inc`, `dec` — errors with `cannot frozen atom_int` (or `atom_float` / `atom_string`); `get`, truthiness, and comparisons still work. - **Range.** `atom_int` is a 64-bit signed integer; `atom_float` is IEEE-754 `float64`. `add`/`sub` wrap or lose precision exactly as the underlying 64-bit types do. - **Float widening.** `atom_float` accepts ints for `set`/`cas`/`add`/`sub` and stores them as floats; `cas` compares with float equality, so seed `old` with the exact stored float. diff --git a/lib/atom/atom.go b/lib/atom/atom.go index 5454cd7b..f294caad 100644 --- a/lib/atom/atom.go +++ b/lib/atom/atom.go @@ -57,7 +57,7 @@ func newInt(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k type AtomicInt struct { val *atomic.Int64 - frozen bool + frozen atomic.Bool } func (a *AtomicInt) String() string { @@ -69,16 +69,18 @@ func (a *AtomicInt) Type() string { } func (a *AtomicInt) Freeze() { - a.frozen = true + a.frozen.Store(true) } func (a *AtomicInt) Truth() starlark.Bool { return a.val.Load() != 0 } +// Hash rejects using a cell as a dict/set key: a cell is mutable and its +// hash tracked the live value, so mutating a keyed cell silently stranded +// the entry under the stale hash (a in d became False after a.set). func (a *AtomicInt) Hash() (uint32, error) { - //return 0, fmt.Errorf("unhashable: %s", a.Type()) - return hashInt64(a.val.Load()), nil + return 0, fmt.Errorf("unhashable type: %s", a.Type()) } func (a *AtomicInt) Attr(name string) (starlark.Value, error) { @@ -115,7 +117,7 @@ var ( type AtomicFloat struct { val *atomic.Float64 - frozen bool + frozen atomic.Bool } func newFloat(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { @@ -135,15 +137,16 @@ func (a *AtomicFloat) Type() string { } func (a *AtomicFloat) Freeze() { - a.frozen = true + a.frozen.Store(true) } func (a *AtomicFloat) Truth() starlark.Bool { return a.val.Load() != 0 } +// Hash rejects keying; see AtomicInt.Hash. func (a *AtomicFloat) Hash() (uint32, error) { - return hashFloat64(a.val.Load()), nil + return 0, fmt.Errorf("unhashable type: %s", a.Type()) } func (a *AtomicFloat) Attr(name string) (starlark.Value, error) { @@ -180,7 +183,7 @@ var ( type AtomicString struct { val *atomic.String - frozen bool + frozen atomic.Bool } func newString(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { @@ -200,15 +203,16 @@ func (a *AtomicString) Type() string { } func (a *AtomicString) Freeze() { - a.frozen = true + a.frozen.Store(true) } func (a *AtomicString) Truth() starlark.Bool { return a.val.Load() != "" } +// Hash rejects keying; see AtomicInt.Hash. func (a *AtomicString) Hash() (uint32, error) { - return hashString(a.val.Load()), nil + return 0, fmt.Errorf("unhashable type: %s", a.Type()) } func (a *AtomicString) Attr(name string) (starlark.Value, error) { diff --git a/lib/atom/atom_test.go b/lib/atom/atom_test.go index 0d37932f..981b0305 100644 --- a/lib/atom/atom_test.go +++ b/lib/atom/atom_test.go @@ -33,10 +33,6 @@ func TestLoadModule_Atom(t *testing.T) { assert.eq(str(x), '') assert.eq(dir(x), ["add", "cas", "dec", "get", "inc", "set", "sub"]) assert.true(not bool(x)) - - m = {} - m[x] = 1 - # assert.eq(m[x], 1) `), }, { @@ -189,10 +185,6 @@ func TestLoadModule_Atom(t *testing.T) { assert.eq(str(x), '') assert.eq(dir(x), ["add", "cas", "get", "set", "sub"]) assert.true(not bool(x)) - - m = {} - m[x] = 1 - # assert.eq(m[x], 1) `), }, { @@ -327,10 +319,6 @@ func TestLoadModule_Atom(t *testing.T) { assert.eq(str(x), '') assert.eq(dir(x), ["cas", "get", "set"]) assert.true(not bool(x)) - - m = {} - m[x] = 1 - # assert.eq(m[x], 1) `), }, { @@ -423,6 +411,197 @@ func TestLoadModule_Atom(t *testing.T) { assert.eq(x.get(), "!!!!!!!!!!") `), }, + // unhashable: a mutable cell must not be usable as a dict/set key — + // its live-value hash goes stale on mutation and the entry is lost + { + name: `int: unhashable as dict key`, + script: itn.HereDoc(` + load('atom', 'new_int') + x = new_int(1) + d = {x: 'a'} + `), + wantErr: "unhashable type: atom_int", + }, + { + name: `float: unhashable as dict key`, + script: itn.HereDoc(` + load('atom', 'new_float') + x = new_float(1.5) + d = {x: 'a'} + `), + wantErr: "unhashable type: atom_float", + }, + { + name: `string: unhashable as dict key`, + script: itn.HereDoc(` + load('atom', 'new_string') + x = new_string("k") + d = {x: 'a'} + `), + wantErr: "unhashable type: atom_string", + }, + { + name: `int: unhashable as set element`, + script: itn.HereDoc(` + load('atom', 'new_int') + s = set([new_int(1)]) + `), + wantErr: "unhashable type: atom_int", + }, + // frozen: every mutating method must reject a frozen cell + { + name: `int: frozen set fails`, + script: itn.HereDoc(` + load('atom', 'new_int') + load('freeze.star', 'freeze') + x = new_int(1) + freeze(x) + x.set(2) + `), + wantErr: "cannot set frozen atom_int", + }, + { + name: `int: frozen cas fails`, + script: itn.HereDoc(` + load('atom', 'new_int') + load('freeze.star', 'freeze') + x = new_int(1) + freeze(x) + x.cas(1, 2) + `), + wantErr: "cannot cas frozen atom_int", + }, + { + name: `int: frozen add fails`, + script: itn.HereDoc(` + load('atom', 'new_int') + load('freeze.star', 'freeze') + x = new_int(1) + freeze(x) + x.add(1) + `), + wantErr: "cannot add frozen atom_int", + }, + { + name: `int: frozen sub fails`, + script: itn.HereDoc(` + load('atom', 'new_int') + load('freeze.star', 'freeze') + x = new_int(1) + freeze(x) + x.sub(1) + `), + wantErr: "cannot sub frozen atom_int", + }, + { + name: `int: frozen inc fails`, + script: itn.HereDoc(` + load('atom', 'new_int') + load('freeze.star', 'freeze') + x = new_int(1) + freeze(x) + x.inc() + `), + wantErr: "cannot inc frozen atom_int", + }, + { + name: `int: frozen dec fails`, + script: itn.HereDoc(` + load('atom', 'new_int') + load('freeze.star', 'freeze') + x = new_int(1) + freeze(x) + x.dec() + `), + wantErr: "cannot dec frozen atom_int", + }, + { + name: `float: frozen set fails`, + script: itn.HereDoc(` + load('atom', 'new_float') + load('freeze.star', 'freeze') + x = new_float(1.5) + freeze(x) + x.set(2.5) + `), + wantErr: "cannot set frozen atom_float", + }, + { + name: `float: frozen cas fails`, + script: itn.HereDoc(` + load('atom', 'new_float') + load('freeze.star', 'freeze') + x = new_float(1.5) + freeze(x) + x.cas(1.5, 2.5) + `), + wantErr: "cannot cas frozen atom_float", + }, + { + name: `float: frozen add fails`, + script: itn.HereDoc(` + load('atom', 'new_float') + load('freeze.star', 'freeze') + x = new_float(1.5) + freeze(x) + x.add(1.0) + `), + wantErr: "cannot add frozen atom_float", + }, + { + name: `float: frozen sub fails`, + script: itn.HereDoc(` + load('atom', 'new_float') + load('freeze.star', 'freeze') + x = new_float(1.5) + freeze(x) + x.sub(1.0) + `), + wantErr: "cannot sub frozen atom_float", + }, + { + name: `string: frozen set fails`, + script: itn.HereDoc(` + load('atom', 'new_string') + load('freeze.star', 'freeze') + x = new_string("a") + freeze(x) + x.set("b") + `), + wantErr: "cannot set frozen atom_string", + }, + { + name: `string: frozen cas fails`, + script: itn.HereDoc(` + load('atom', 'new_string') + load('freeze.star', 'freeze') + x = new_string("a") + freeze(x) + x.cas("a", "b") + `), + wantErr: "cannot cas frozen atom_string", + }, + // frozen cells stay readable and comparable + { + name: `frozen cells still read and compare`, + script: itn.HereDoc(` + load('atom', 'new_int', 'new_float', 'new_string') + load('freeze.star', 'freeze') + i = new_int(7) + f = new_float(2.5) + s = new_string("hi") + freeze(i) + freeze(f) + freeze(s) + assert.eq(i.get(), 7) + assert.eq(f.get(), 2.5) + assert.eq(s.get(), "hi") + assert.true(i == new_int(7)) + assert.true(f < new_float(3.0)) + assert.true(s != new_string("yo")) + assert.true(bool(i)) + `), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/lib/atom/helper.go b/lib/atom/helper.go index 675dcf86..2af9d971 100644 --- a/lib/atom/helper.go +++ b/lib/atom/helper.go @@ -1,10 +1,7 @@ package atom import ( - "encoding/binary" "fmt" - "hash/fnv" - "math" "sort" "go.starlark.net/starlark" @@ -28,46 +25,6 @@ func builtinAttrNames(methods map[string]*starlark.Builtin) []string { return names } -// hashInt64 hashes an int64 value to a uint32 hash value using little-endian byte order -func hashInt64(value int64) uint32 { - // Allocate a byte slice - bytes := make([]byte, 8) - // Convert the int64 value into bytes using little-endian encoding - binary.LittleEndian.PutUint64(bytes, uint64(value)) - // Initialize a new 32-bit FNV-1a hash - h := fnv.New32a() - // Write the bytes to the hasher, and ignore the error returned by Write, as hashing can't really fail here - _, _ = h.Write(bytes) - // Calculate the hash and return it - return h.Sum32() -} - -// hashFloat64 hashes a float64 value to a uint32 hash value -func hashFloat64(value float64) uint32 { - // Convert the float64 value into its binary representation as uint64 - bits := math.Float64bits(value) - // Allocate a byte slice - bytes := make([]byte, 8) - // Convert the uint64 bits into bytes using little-endian encoding - binary.LittleEndian.PutUint64(bytes, bits) - // Initialize a new 32-bit FNV-1a hash - h := fnv.New32a() - // Write the bytes to the hasher, and ignore the error returned by Write, as hashing can't really fail here - _, _ = h.Write(bytes) - // Calculate the hash and return it - return h.Sum32() -} - -// hashString hashes a string value to a uint32 hash value -func hashString(value string) uint32 { - // Initialize a new 32-bit FNV-1a hash - h := fnv.New32a() - // Write the string to the hasher, and ignore the error returned by Write, as hashing can't really fail here - _, _ = h.Write([]byte(value)) - // Calculate the hash and return it - return h.Sum32() -} - // threewayCompare interprets a three-way comparison value cmp (-1, 0, +1) // as a boolean comparison (e.g. x < y). func threewayCompare(op syntax.Token, cmp int) (bool, error) { diff --git a/lib/atom/helper_test.go b/lib/atom/helper_test.go deleted file mode 100644 index 5919170b..00000000 --- a/lib/atom/helper_test.go +++ /dev/null @@ -1,124 +0,0 @@ -package atom - -import ( - "math" - "strings" - "testing" -) - -func Test_hashInt64(t *testing.T) { - tests := []struct { - name string - value int64 - want uint32 - }{ - { - name: "zero", - value: 0, - want: 2615243109, - }, - { - name: "+1", - value: 1, - want: 1048580676, - }, - { - name: "-1", - value: -1, - want: 1823345245, - }, - { - name: "max", - value: math.MaxInt64, - want: 3970880477, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := hashInt64(tt.value); got != tt.want { - t.Errorf("hashInt64(%d) = %v, want %v", tt.value, got, tt.want) - } - }) - } -} - -func Test_hashFloat64(t *testing.T) { - tests := []struct { - name string - value float64 - want uint32 - }{ - { - name: "zero", - value: 0, - want: 2615243109, - }, - { - name: "+1", - value: 1, - want: 2355796088, - }, - { - name: "-1", - value: -1, - want: 208260856, - }, - { - name: "max", - value: math.MaxFloat64, - want: 3968320621, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := hashFloat64(tt.value); got != tt.want { - t.Errorf("hashFloat64(%f) = %v, want %v", tt.value, got, tt.want) - } - }) - } -} - -func Test_hashString(t *testing.T) { - tests := []struct { - name string - input string - want uint32 - }{ - { - name: "empty", - input: "", - want: 2166136261, - }, - { - name: "single", - input: "a", - want: 3826002220, - }, - { - name: "next", - input: "b", - want: 3876335077, - }, - { - name: "add", - input: "ab", - want: 1294271946, - }, - { - name: "hello", - input: "hello", - want: 1335831723, - }, - { - name: "long", - input: strings.Repeat("this is a long string", 100), - want: 229378413, - }, - } - for _, tt := range tests { - got := hashString(tt.input) - if got != tt.want { - t.Errorf("hashString(%q) = %v, want %v", tt.input, got, tt.want) - } - } -} diff --git a/lib/atom/internal_test.go b/lib/atom/internal_test.go new file mode 100644 index 00000000..bae5ab33 --- /dev/null +++ b/lib/atom/internal_test.go @@ -0,0 +1,40 @@ +package atom + +import ( + "sync" + "testing" + + "go.starlark.net/starlark" + "go.uber.org/atomic" +) + +// TestConcurrentFreezeAndSet guards the concurrency-safety the module +// advertises: the frozen flag is read by every mutating method and written by +// Freeze, so it must be an atomic — a plain bool would be a data race under +// `go test -race` (Freeze from one goroutine, a mutator from another). This +// test only makes sense with the race detector, which the module's bar runs. +func TestConcurrentFreezeAndSet(t *testing.T) { + a := &AtomicInt{val: atomic.NewInt64(0)} + set := intMethods["set"].BindReceiver(a) + args := starlark.Tuple{starlark.MakeInt(1)} + + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + <-start + for i := 0; i < 100000; i++ { + a.Freeze() + } + }() + go func() { + defer wg.Done() + <-start + for i := 0; i < 100000; i++ { + _, _ = intSet(nil, set, args, nil) + } + }() + close(start) + wg.Wait() +} diff --git a/lib/atom/method.go b/lib/atom/method.go index dded3553..74d32a50 100644 --- a/lib/atom/method.go +++ b/lib/atom/method.go @@ -1,10 +1,22 @@ package atom import ( + "fmt" + tps "github.com/1set/starlet/dataconv/types" "go.starlark.net/starlark" ) +// checkFrozen guards every mutating method: Freeze() must actually pin the +// cell, so a frozen cell rejects set/cas/add/... with an error instead of +// silently updating (get and comparisons stay allowed). +func checkFrozen(b *starlark.Builtin, frozen bool, typeName string) error { + if frozen { + return fmt.Errorf("cannot %s frozen %s", b.Name(), typeName) + } + return nil +} + // for integer var ( @@ -33,6 +45,9 @@ func intSet(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k return nil, err } recv := b.Receiver().(*AtomicInt) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } recv.val.Store(value) return starlark.None, nil } @@ -43,6 +58,9 @@ func intCAS(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k return nil, err } recv := b.Receiver().(*AtomicInt) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.Bool(recv.val.CAS(oldVal, newVal)), nil } @@ -52,6 +70,9 @@ func intAdd(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k return nil, err } recv := b.Receiver().(*AtomicInt) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.MakeInt64(recv.val.Add(delta)), nil } @@ -61,6 +82,9 @@ func intSub(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k return nil, err } recv := b.Receiver().(*AtomicInt) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.MakeInt64(recv.val.Sub(delta)), nil } @@ -69,6 +93,9 @@ func intInc(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k return nil, err } recv := b.Receiver().(*AtomicInt) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.MakeInt64(recv.val.Inc()), nil } @@ -77,6 +104,9 @@ func intDec(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, k return nil, err } recv := b.Receiver().(*AtomicInt) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.MakeInt64(recv.val.Dec()), nil } @@ -106,6 +136,9 @@ func floatSet(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, return nil, err } recv := b.Receiver().(*AtomicFloat) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } recv.val.Store(value.GoFloat()) return starlark.None, nil } @@ -116,6 +149,9 @@ func floatCAS(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, return nil, err } recv := b.Receiver().(*AtomicFloat) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.Bool(recv.val.CAS(oldVal.GoFloat(), newVal.GoFloat())), nil } @@ -125,6 +161,9 @@ func floatAdd(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, return nil, err } recv := b.Receiver().(*AtomicFloat) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.Float(recv.val.Add(delta.GoFloat())), nil } @@ -134,6 +173,9 @@ func floatSub(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, return nil, err } recv := b.Receiver().(*AtomicFloat) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.Float(recv.val.Sub(delta.GoFloat())), nil } @@ -161,6 +203,9 @@ func stringSet(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple return nil, err } recv := b.Receiver().(*AtomicString) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } recv.val.Store(value) return starlark.None, nil } @@ -171,5 +216,8 @@ func stringCAS(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple return nil, err } recv := b.Receiver().(*AtomicString) + if err := checkFrozen(b, recv.frozen.Load(), recv.Type()); err != nil { + return nil, err + } return starlark.Bool(recv.val.CompareAndSwap(oldVal, newVal)), nil }