Skip to content
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.

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

Key constraint might not be ideal #25

Open
anacrolix opened this issue Oct 26, 2022 · 8 comments
Open

Key constraint might not be ideal #25

anacrolix opened this issue Oct 26, 2022 · 8 comments

Comments

@anacrolix
Copy link

I have a wrapper of immutable that adds sets and some other generic interface stuff. I'm having difficulty mapping it onto v0.4.0 of immutable. I read the PR for the generics, and I think that constraints.Ordered isn't quite right for the key values. I haven't looked into it more, but I suspect it's too strict and that comparable should be possible instead, particularly because the user already has to provide comparer/hasher types in the constructors.

@laher

anacrolix added a commit to anacrolix/stm that referenced this issue Oct 26, 2022
List has to be dropped because type aliases are not allowed for generic types.

There's an outstanding issue that Set can't contain pointer values. benbjohnson/immutable#25

I would abandon this package altogether, but there's no Set type in immutable, and its comparer and hasher types are more boilerplate than I want.
@laher
Copy link
Contributor

laher commented Oct 26, 2022

Hi @anacrolix . The problem was that SortedMap requires constraints.Ordered, and there's so much code shared between Map and SortedMap that it was dramatically more straightforward to use constraints.Ordered for both.

Maybe it's not the best approach long-term, but Ben seemed happy with this for now..

Unless there's something I'm missing - maybe I am - I think you'd need to duplicate all the map-related types like Hasher, defaultHasher, MapIterator, mapEntry, mapNode, mapValueNode, mapLeafNode, mapArrayNode, MapBuilder, ... there's a lot of types in there. Hopefully there's some opportunities to avoid some of the duplication, but it's an undertaking - feel free to take it on.

At least you know why it is like it is now :)

@benbjohnson
Copy link
Owner

I don't have a strong opinion on any of this—mostly because I haven't done much with generics. If anybody has a good solution then I'm open to it but I haven't had time to commit to the immutable project lately.

@anacrolix
Copy link
Author

I definitely don't have the time to dedicate to this right now. It's used in the old and obsoleted STM concurrency implementation in anacrolix/dht that I think only gets dredged up because of golang/go#55955.

@laher
Copy link
Contributor

laher commented Nov 4, 2022

@anacrolix I had a quick experiment with widening the constraint for immutable.Map. Does it help?

@anacrolix
Copy link
Author

I will give it a go and report back, thank you @laher !

@anacrolix
Copy link
Author

I gave it a spin, it didn't work for the underlying user (although the stm code passed). I think the type can be widened for SortedMap (and any other variants too). Again, because comparers are manually given, the restriction on types belong to those implementations. I'm pretty sure it works for the builtin comparers too, as those use type sets to check for orderable types anyway. Short answer: I think K comparable can be used everywhere in immutable.

@laher
Copy link
Contributor

laher commented Nov 7, 2022

Short answer: I think K comparable can be used everywhere in immutable.

Good shout - I think it should be OK, just needing a bit of jiggery-pokery on the defaultComparer.

@anacrolix
Copy link
Author

Let me know if you do that change, and I'll report back. Thanks @laher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants