-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add the Empty[Map[A, B]]
instance in alleycats
#4735
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
Conversation
Do you want to add SortedMap at the same time? That will require the ordering on the key and is a bit less trivial. |
@johnynek private[this] val emptyMapSingleton: Empty[Map[Nothing, Nothing]] = Empty(Map.empty)
implicit def alleycatsEmptyForMap[A, B]: Empty[Map[A, B]] = emptyMapSingleton.asInstanceOf[Empty[Map[A, B]]] then the methods of Given that, we can use the same singleton instance for
UPD. Disregard please: as @danicheg pointed out, |
1187e32
to
78d9d55
Compare
private[this] object OrderingAny extends Ordering[Any] { | ||
override def compare(x: Any, y: Any): Int = | ||
x.hashCode().compareTo(y.hashCode()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s a bit controversial, but:
- it’s consistent with object equality;
- using
Ordering[Nothing]
isn’t a choice, asSortedMap
API exposesOrdering
instance to end-users; - this lets us enable the zero-allocation trick when creating an
Empty[SortedMap[A, B]]
. Otherwise, we’d need to require anOrdering[A]
inalleycatsSortedEmptyForMap
and pass it along to theSortedMap.empty
constructor — which would mean extra allocations; - FWIW, we also can’t cast a
Map[Nothing, Nothing]
toSortedMap[Nothing, Nothing]
. cc @satorg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right – since the empty map is exposed, its ordering
is exposed too. Maybe that zero-allocation trick is not exactly suitable for SortedMap
then. Because someone may decide to get Empty.empty
map and start building a non-empty map from it.
|
||
private[this] val emptySortedMapSingleton: Empty[SortedMap[Any, Any]] = | ||
Empty(SortedMap.empty[Any, Any](OrderingAny)) | ||
implicit def alleycatsSortedEmptyForMap[A, B]: Empty[SortedMap[A, B]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are being too clever here and it will lead to big problems.
What do you expect Empty[SortedMap[A, B]].empty.ordering
to return? It will return an ordering on A
but which one? I certainly wouldn't expect it to order on hashcode.
I think we need to simply do:
implicit def alleycatsSortedEmptyForMap[A: Ordering, B]: Empty[SortedMap[A, B]] =
Empty(SortedMap.empty[A, B])
That said... I'd love to see an example of using the Empty typeclass generically. Since it is lawless, I just don't see how you can write code with it that makes sense. It seems like what you really want is Alternative.empty
, Monoid.empty
or Applicative.unit
or something depending on what you are trying to do.
In any case, IMO we definitely shouldn't add the casting/hashcode version in the PR currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will lead to big problems
I can't judge the magnitude, but it could definitely introduce behavioural discrepancies in certain cases. So I agree — the safer approach would be what I proposed above (which is the same as what you suggested).
That said... I'd love to see an example of using the Empty typeclass generically. ... It seems like what you really want is
Alternative.empty
,Monoid.empty
orApplicative.unit
I just need a handy way to create empty entities (which are often just collections) without requiring them to form a Monoid
.
4f2a283
to
d5a7e2f
Compare
Adding yet another instance of
Empty
that might be commonly used (at the very least, I need it). I thought of adding this:but as @johnynek pointed out in #4733, it will also add support for
mutable.Map
, which isn't a desirable outcome.The existing derivation capabilities fail to derive
Empty[Map[A, B]]
when the key/value types are case classes or more sophisticated than primitives. See https://scastie.scala-lang.org/ggnoSWYGTFiGGqD6wA0VuQ