Closed laher closed 1 year ago
The Set
structures and SetMany
functions are broken.
Calling (*Map).clone()
before doing operations with mutable=true
is not safe, as clone
only performs a very shallow copy. The cloned map will still contain references to old nodes which are shared with previous versions of the map.
This in turn means that the operations using mutable=true
will mutate old versions of the map in addition to the clone.
I can supply tests if you need it.
This simple change to the tests shows the issue:
diff --git a/sets_test.go b/sets_test.go
index b3900fc..5a83eb9 100644
--- a/sets_test.go
+++ b/sets_test.go
@@ -7,6 +7,7 @@ import (
func TestSetsPut(t *testing.T) {
s := NewSet[string](nil)
s2 := s.Set("1").Set("1")
+ s2.Set("2")
if s.Len() != 0 {
t.Fatalf("Unexpected mutation of set")
}
$ go test . -run Sets -v
=== RUN TestSetsPut
sets_test.go:27: 1 true
sets_test.go:27: 2 true
sets_test.go:31: iterator wrong length
--- FAIL: TestSetsPut (0.00s)
=== RUN TestSetsDelete
--- PASS: TestSetsDelete (0.00s)
=== RUN TestSortedSetsPut
sets_test.go:73: 0 true
sets_test.go:73: 1 true
--- PASS: TestSortedSetsPut (0.00s)
=== RUN TestSortedSetsDelete
--- PASS: TestSortedSetsDelete (0.00s)
FAIL
FAIL github.com/benbjohnson/immutable 0.002s
FAIL
I think this is done, thanks. Closing. Ta
Would it be OK to add a Set & SortedSet types, wrapping the Map/SortedMap types?
It seems like a natural use case for
immutable
types, now that we have generics.Beyond the convenience & readability benefits (vs using
immutable.Map[T,struct{}]
), I would find it useful for adding Intersection, Union, Complement & Difference methods.These could potentially be in a subdirectory package, or at least a separate file. I've prototyped something in the same package, it seems nice.
I'd be happy to submit a PR for this, but wanted to check first.
Ta.