Deprecate universal Equiv
#7414
Conversation
src/library/scala/math/Equiv.scala
Outdated
| @@ -58,4 +71,438 @@ object Equiv extends LowPriorityEquiv { | |||
| ((x, y) => implicitly[Equiv[S]].equiv(f(x), f(y))) | |||
|
|
|||
| @inline def apply[T: Equiv]: Equiv[T] = implicitly[Equiv[T]] | |||
|
|
|||
| implicit def equivFromPartialOrdering[T](implicit ev: PartialOrdering[T]): Equiv[T] = ev | |||
There was a problem hiding this comment.
This feels a bit contradictory to me... Ordering[A] <: PartialOrdering[A] <: Equiv[A]. These implicit conversions already mean that Equiv and PartialOrdering need to have knowledge about their subtypes. Why not just go down the rabbit hole while we're already stuck halfway, and add all implicit instances of Equiv and its subtypes to the Equiv companion object?
Also, can someone quickly remind me why Equiv needs its own instances for all the primitives and tuples etc?
There was a problem hiding this comment.
For primitives, it technically doesn't - it can summon those from Ordering (that answers the first part of your question I think, kinda). For parameterised types, we can't rely on summoning ones from Ordering, because they rely on the existence of implicit Orderings for each of the types, when only implicit Equivs should be needed.
We could theoretically not have the instances for primitives, but they are by far not the maintenance burden, and it seems a bit odd to me to have methods for Ordering, tuples, Seqs, etc. but not the primitives. Also, in order to not have confusing deprecation messages, we still need Equiv instances for Float and Double.
Would it be better if we moved the implicit conversions to the subtypes? I'm not sure if that would still put them on the search path. (i.e. Ordering contains Ordering[A] => PartialOrdering[A], and PartialOrdering contains PartialOrdering[A] => Equiv[A])
There was a problem hiding this comment.
Apparently you don't actually need the implicit methods anywhere for Ordering and PartialOrdering to be on the implicit search path, so I'm going to remove them.
Props to @ceedubs for making really good tests for these implicit methods, making it super easy for me to test if that worked.
This is a replacement for scala#7187 (see discussion there). This does the following things: 1. Deprecate universal Equiv instance. 2. Create an Ordering -> PartialOrdering conversion. 3. Create a PartialOrdering -> Equiv conversion at a higher priority than the universal instance. 4. Add some unit tests that check that implicit resolution is working as expected. @NthPortal suggested the conversions as a simple way to have `Equiv` instances that could be resolved without duplicating a lot of the work that has been done for `Order` instances for basic types.
NthPortal
force-pushed
the
topic/deprecate-universal-equiv/PR
branch
from
1cc260d
to
65c0f64
Compare
|
no community build breakage |
|
Thanks for checking, Seth! Sorry I never got around to doing more than a cursory analysis of the results. |
|
Btw, thanks so much Nth for the PR. It's partially a shame that this simple sounding PR is so elaborate, but more so thank you for contributing the effort! |
|
no problem! my goal is to steal Rex's position as the resident expert on |
|
Is there anything else that needs to be done before merging this? |
|
not sure whether to add "release-notes" label, kinda leaning towards not since hopefully people weren't using this and if they were they'll see the deprecation message thanks @NthPortal and @ceedubs for your perseverance on this |
Followup to #7358