search

deckarep / golang-set

A simple, battle-tested and generic set type for the Go language. Trusted by Docker, 1Password, Ethereum and Hashicorp.
Other
4.15k stars 274 forks source link

Panics with trivial test cases #99

Closed natefinch closed 1 year ago

natefinch commented 2 years ago

The internals of this library use a downcast of the Set interface to *threadSafeSet and *threadUnsafeSet everywhere. I'm unclear why an interface is used in this package when the logic panics if you use literally any implementation of the interface other than one that matches the type of set you requested.

For example: https://github.com/deckarep/golang-set/blob/main/threadsafe.go#L88

Right now, the API is suggesting that any implementation of the Set interface is acceptable (that's what an interface means), but that's not true. In fact, once you have two Sets returned from this package, you can't even tell if they are compatible or if they'll panic when you try to use methods of one on the other.

s1 := mapset.NewSet[int]()
s1.Add(1)
s2 := mapset.NewThreadUnsafeSet[int]()
s2.Add(2)
s1.Union(s2) // compiles, but panics at runtime

My suggestion is instead to simply return structs for both the threadsafe and unsafe versions. The Set interface is not giving the package any benefit, and indeed, there is actually a negative to its existence, since the two types are incompatible (as are any third-party implementations). If they each just were structs and the arguments you pass to Union etc were structs, there would be compile-time safety that ensures you don't pass in a struct that the code doesn't work with.

deckarep commented 2 years ago

First off Hello! 👋,

Thanks for your interest and critique of this Go set based implementation.

This implementation has actually gone through several iterations and refactors as I’m sure you can imagine and if I remember correctly someone proposed this implementation to prevent the mixing of threadsafe vs non-threadsafe instances which at the time I agreed felt was a good call and compromise.

I suppose in hindsight you can argue that this approach isn’t quite ideal but so far I haven’t been too concerned to change it.

Of course anyone who might feel strongly about it is welcome to propose changes and submit a PR since it is open source after all.

I might chalk it up to an implementation detail that doesn’t seem to affect many people at all.