Add Order and PartialOrder For SortedMap

[isomarcte]

No description provided.

[johnynek]

I'm a bit nervous here.

1. SortedMap[K, V] has .ordering which returns Ordering[K] but we are taking the Order[K] in any case.
2. if we can't trust the Ordering[K] then actually we will be iterating in that order, but not correctly for the Order[K].

So, a few solutions to my objection:

1. assume we can always use the left or right side ordering and not accept an implicit order in the caller
2. we could require that every K comparison compares the same using the Ordering from the right or left.

In any case, I don't see how the PartialOrder should take only a PartialOrder[K] since we have to have an Order to make the map.

[isomarcte]

@johnynek

I'm a bit nervous here.

SortedMap[K, V] has .ordering which returns Ordering[K] but we are taking the Order[K] in any case.
if we can't trust the Ordering[K] then actually we will be iterating in that order, but not correctly for the Order[K].

I see what you're saying. The interaction between Order and Ordering with SortedMap and SortedSet is a bit strange.

They way I wrote it is consistent with what we already do for SortedSet (

cats/kernel/src/main/scala/cats/kernel/instances/SortedSetInstances.scala

Line 16 in e0784fc

implicit def catsKernelStdOrderForSortedSet[A: Order]: Order[SortedSet[A]] =

).

Would you prefer I change it to use the Ordering on the instance or stay consistent with what we do for SortedSet? The other option would be to rebuild the SortedMap values using our Order to ensure consistency, at a performance cost.

I'm torn because in the Scala stdlib Ordering has this note.

This trait and scala.math.Ordered both provide this same functionality, but in different ways. A type T can be given a single way to order itself by extending Ordered. Using Ordering, this same type may be sorted in many other ways. Ordered and Ordering both provide implicits allowing them to be used interchangeably.

Which is the opposite of how we canonically think of cats.Order in terms of coherence. In other words, we can't necessarily rely on the .ordering to be the canonical instance of it for K.

All that said, I'm fine with which ever strategy is preferred.

In any case, I don't see how the PartialOrder should take only a PartialOrder[K] since we have to have an Order to make the map.

I was attempting to be consistent with this:

cats/kernel/src/main/scala/cats/kernel/instances/SortedMapInstances.scala

Line 10 in e0784fc

@deprecated("Use catsKernelStdHashForSortedMap override without Order", "2.2.0-M3")

We used to have a Hash instance for SortedMap which required an Order for the K, but technically you didn't need that Order to write the Hash instance. You do need an Ordering from somewhere obviously to even created a SortedMap, so again it is this strange interaction between Ordering and Order and their idiomatic usage.

[isomarcte]

@johnynek I noticed a few issues around SortedMap/SortedSet when looking into this deeper. I've opened this issue to discuss and we'll probably be in a holding pattern here until that is resolved.

#4103

[isomarcte]

@johnynek I've removed the PartialOrder and Order constraints on the instances as you requested. Per the discussion on #4105, this PR assumes that the ordering between two instances of a SortedMap[A, B] is consistent.

Let me know if you want to proceed here or on #4105.

[johnynek]

minor comment.

I think this is the way to go. Yes it is somewhat unsafe to have multiple typeclasses in play at the same time, but I think that risk already exists in many places and it is not something we can easily fix in a small PR.

It would require a real significant design change at least to structures that are built using typeclasses (Sets, Maps, Heaps come to mind).

[isomarcte]

@johnynek I've removed the unneeded constraints. Thanks for catching that.

[johnynek]

Thanks for all the effort on this.

[isomarcte]

@satorg would you be able to take another look here?

[satorg]

Looks good to me, thanks!