See also #7186 for the deprecation in 2.12.

• scala/test/files/pos/t5744/Macros_1.scala

  Line 6 in 57c2286

  def bar[U: Numeric : Equiv, Y <% String](x: U)(implicit s: String): Unit = macro bar_impl[U, Y]

• https://scala-ci.typesafe.com/job/scala-2.13.x-validate-main/4341/testReport/junit/test.files/pos/t5744/

Error Message
scala.tools.partest.sbt.TestFailedThrowable: compilation failed with 0 errors  % scalac t5744/Macros_1.scala % scalac t5744/Test_2.scala
Stacktrace
                sbt.ForkMain$ForkError: scala.tools.partest.sbt.TestFailedThrowable: compilation failed with 0 errors

% scalac t5744/Macros_1.scala
% scalac t5744/Test_2.scala
	at scala.tools.partest.sbt.SBTRunner.makeStatus(SBTRunner.scala:83)
	at scala.tools.partest.sbt.SBTRunner$$anon$1.<init>(SBTRunner.scala:71)
	at scala.tools.partest.sbt.SBTRunner.onFinishTest(SBTRunner.scala:67)
	at scala.tools.partest.nest.AbstractRunner.runTest(AbstractRunner.scala:319)
	at scala.tools.partest.nest.AbstractRunner.$anonfun$runTestsForFiles$2(AbstractRunner.scala:326)
	at scala.tools.partest.package$$anon$2.call(package.scala:131)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

Thanks @xuwei-k. Sorry, but I still don't quite understand how this is failing. Is it depending on universalEquiv? I'm struggling to figure out how.

@xuwei-k @ceedubs is it possible that we don't have implicit Equivs for primitives, and have actually been relying on universalEquiv for them all along?

Ah, I think that this gets a little complicated. Currently Equiv[Int] returns the universalEquiv instance. Once that instance is removed, you should pick up the Ordering[Int] instance, but due to implicit prioritization rules, when you request an Equiv[Int], the one available in the Equiv companion object is found first (at least this is my understanding). This means that people could be hit by this deprecation warning for code that would be fine in the absence of the universalEquiv method.

Should we add a low-priority implicit method that summons up an Equiv by summoning the implicit Ordering for that type? That seems like an easy fix, since Orderings are already defined for primitives, Strings, Symbols, and several other types.

@NthPortal sorry for going silent for a while. I can't think of a better solution than that. I guess that for consistency there should also be one for PartialOrdering?

I took a stab at adding prioritized implicits for Ordering and PartialOrdering. Please let me know what you think.

When I wrote #7187 (comment) I was thinking that this was on #7186 where there is a deprecation message as opposed to removal. I guess that I still don't understand why that test failed (unless it was really depending on universal equality, in which case this build is probably going to fail too).

@ceedubs I don't think you need one for PartialOrdering. The only reason to have one for Ordering IMO is to reuse the Ordering implementations and not have to duplicate code

@NthPortal my thought was that if someone had written their own PartialOrdering instance but not Ordering instance then they could still benefit from this. But admittedly as I mentioned in #7187 (comment) I'm confused about why a test was even failing before with this, so I'm happy to do whatever the maintainers suggest. Should I just keep the Ordering-based Equiv and drop the PartialOrdering one? Why does that actually help? I could understand if the universal instance were still there and deprecated, but it's removed.

To be clear, I don't think we should implicitly summon Equivs from Orderings in general - I just think it's much less work than implementing Equivs for all the primitives (plus extra complications for dealing with IEEE floating point numbers), as well as for several other core types (String, Option, Tuples, etc.)

@NthPortal adding the instances for Ordering but not PartialOrdering seems to be inconsistent to me. I've been thinking about this issue and #7186, and here are my thoughts:

Deprecating the universal instance without further action would be a really annoying change, because when people summon an Equiv[Int] it will find the deprecated universal instance instead of the Ordering[Int] instance that is desired. So they will get deprecation warnings for code that is doing reasonable things and would require pretty verbose and explicit workarounds to change.

Since we already have Ordering instances for many basic types, we'd like the users to pick up those instances as opposed to the universal instance. Also, users may have their own Ordering and/or PartialOrdering instances defined in their own code base. We could provide conversions from Ordering[A] to Equiv[A] and PartialOrdering[A] to Equiv[A] at a higher priority than the universal Equiv instance and they should pick up the relevant instances rather than the universal instance for all cases other than when they really do depend on the universal instance. While most of the types in the standard library have Ordering as opposed to just PartialOrdering instances, since the users may have only PartialOrdering instances defined for their types, I think that we'd be needlessly making their lives more difficult if we don't provide the PartialOrdering conversion.

As a Cats maintainer, one thing that struck me is that with the way that things are set up in the standard library, an Ordering[A] instances in the Ordering companion object won't be found when a user requests an Equiv[A] (unless we provide the prioritized conversions that I mentioned above). This is different from the setup in Cats in which type class instances are defined in the data structure companion object as opposed to the type class companion object. There are reasons that this approach may make sense for the standard library, and I don't mean much by pointing out this observation other than that it might be further support for the idea of the prioritized conversions for subtypes of the type class.

A solution that seems pretty clean to me (and that I've verified should work) is to provide implicit def orderingToPartialOrdering[A](implicit orderingA: Ordering[A]): PartialOrdering[A] = orderingA in the (current nonexistent) PartialOrdering companion object and provide a similar PartialOrdering => Equiv instance conversion in the Equiv companion object. When someone requests an Equiv[A] instance, it will follow the implicit chain.

So here's my proposal:

• In both Scala 2.12 and Scala 2.13, add prioritized conversions from Ordering[A] and PartialOrdering[A] to Equiv[A].
  • We could use the approach that I described just above, or we could provide them both directly in the Equiv companion object.
• In Scala 2.12 we deprecate the universal instance and ensure that the conversions mentioned above have a higher implicit resolution priority.
• In scala 2.13 we ideally remove the instance, but if it's too late to do that, we could retain binary compatibility by making it no longer implicit.

What do you think? cc @adriaanm

I must be missing something here. How does any of this matter as long as PartialOrdering actually extends Equiv? Why do we need the new implicits?

@szeiger The implicit instances for PartialOrdering/Ordering aren't in search scope though (I don't think)

In both Scala 2.12 and Scala 2.13

You can't do it in 2.12, for bincompat reasons.

We could use the approach that I described just above, or we could provide them both directly in the Equiv companion object.

I'm fine with either approach; the former seems maybe slightly better to me, as I feel like Equiv shouldn't have to know about its sub-types. I think making those changes in 2.13 (with a higher priority than the universal Equiv), as well as deprecating the universal Equiv, sounds like the right path.

@NthPortal sorry for the delay on this.

What would be the binary incompatibility in adding a concrete function to a trait in Scala 2.12? I thought that this would be binary compatible.

@szeiger when when scala looks for an implicit Equiv[A] instance, (to simplify a bit) it looks in the current scope, the Equiv companion object, and the A companion object, but not in the Order (a subclass) companion object. Since Equiv isn't sealed, it wouldn't be possible for it to have an exhaustive set of subtype companion objects to look into.

@ceedubs Scala minor releases are both forwards and backwards compatible. Adding a method on a newer minor release would mean that compiling against that release and running against an older minor release will yield a missing method.

@NthPortal oh I see. Thanks for the explanation. Okay, I'll try to get this PR in shape by the end of the weekend.

Replaced by #7358 since @NthPortal recommended going with deprecation as opposed to removal.