Make catsKernelOrderingForOrder a proper implicit conversion

[edmundnoble]

The existing implicit conversion from Order to Ordering does not function as an implicit conversion, because the argument is marked implicit. I.e., as @paulp stated:

scala> import cats.Order.catsKernelOrderingForOrder
import cats.Order.catsKernelOrderingForOrder

scala> implicitly[cats.Order[Int] => scala.math.Ordering[Int]]
<console>:13: error: No implicit view available from cats.Order[Int] => scala.math.Ordering[Int].
       implicitly[cats.Order[Int] => scala.math.Ordering[Int]]

[adelbertc]

👍

[codecov-io]

Codecov Report

Merging #1646 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1646   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files         240      240           
  Lines        3941     3941           
  Branches      138      138           
=======================================
  Hits         3680     3680           
  Misses        261      261

Impacted Files	Coverage Δ
kernel/src/main/scala/cats/kernel/Order.scala	88.88% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a90b61...877f421. Read the comment docs.

[kailuowang]

nitpick: shall we rename the param name as well?

[dwijnand]

Can anyone explain (to me) why it mustn't be implicit? Should an implicit def never take an implicit parameter?

[yilinwei]

@dwijnand It's simply because this is an implicit conversion rather than trying to define an implicit value.

i.e. the difference between def foo(order: cats.Order[A]) or def foo(implicit order: cats.Order[A]) given an implicit Ordering in scope.

[kailuowang]

@dwijnand an implicit def that takes an implicit parameter is often used in implicit induction in type level programming. For example, if you provide an implicit construction of an instance of type A, and you have an implicit def that takes an implicit A and returns an instance of B, then it means the compiler can also implicitly construct a B.
Let's take the cats.Order and math.Ordering example.
In this line

  def fromOrdering[A](implicit ev: Ordering[A]): Order[A] =

if we make the def implicit, then it means that if we have an implicit Ordering[A] in scope then we automatically get an implicit Order[A] in scope. In fact, I actually don't know why it isn't that way. @edmundnoble @paulp what do you guys think?

[dwijnand]

Well in investigating this more I've found a bug in the compiler: you can't eta-expand an implicit conversion with an implicit parameter scala/bug#10299. Perhaps that's related here, perhaps not.

[paulp]

To possibly clarify previous comments, there are two situations:

• a nullary implicit definition which has an implicit parameter
• a unary implicit definition (an implicit conversion) which does not have implicit parameters

Right now catsKernelOrderingForOrder is the first, but if it's the first it's not the second. We could invent reasons why it can't be both, but it doesn't matter, it's just how it is and couldn't be changed without breaking code.

A more important aspect is I don't think this PR actually fixes anything, because there is a second problem: you can't put conversions like this in the companion object and expect them to be found when calling e.g. .sorted. At least I don't think you can. If I'm not sure that's probably sufficient to suggest a test is needed.

[paulp]

And in fact I think the parameter being implicit is fine as it is since one expects the Order instance to be implicit. It's the location of the definition which is a problem.

The case which won't work with the parameter being implicit is something like

xs.sorted( new SomeCatsOrder )

because only an implicit conversion could convert that non-implicit value to the expected Ordering.

[johnynek]

As @paulp said, I don't think this is a real problem. I think the problem is the comment calling it an implicit conversion.

I think it is a method to provide an Ordering given an Order when one imports that method.

I don't think we want an implicit conversion from Order to Ordering in any case.

[johnynek]

Maybe the real issue is that you should get this method when you do cats.implicits._ and maybe we don't now? (I'm on the a phone apologies for not digging myself).

[paulp]

Yes if it came in via cats.implicits._ that would be fine with me, one already expects to have to import the implicit machinery. (It doesn't right now, because it's in the companion.)

[dwijnand]

A more important aspect is I don't think this PR actually fixes anything, because there is a second problem: you can't put conversions like this in the companion object and expect them to be found when calling e.g. .sorted. At least I don't think you can. If I'm not sure that's probably sufficient to suggest a test is needed.

Yeah looks that way (using MyOrdering to avoid the original Ordering):

Welcome to Scala 2.12.2 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_131).
Type in expressions for evaluation. Or try :help.

scala> class MyOrdering[A]
defined class MyOrdering

scala> class Order[A]; object Order { implicit def toOrdering[A](x: Order[A]): MyOrdering[A] = new MyOrdering[A] }
defined class Order
defined object Order

scala> def sorted[A: MyOrdering] = 1
sorted: [A](implicit evidence$1: MyOrdering[A])Int

scala> implicit val o: Order[Int] = new Order[Int]
o: Order[Int] = Order@2aeefcc

scala> sorted[Int]
<console>:14: error: could not find implicit value for evidence parameter of type MyOrdering[Int]
       sorted[Int]
             ^
<console>:10: warning: Unused import
import o
                            ^

scala> sorted[Int](Order toOrdering o)
res1: Int = 1

When the compiler needs a Foo[A], (iirc) it'll look on Foo's and A's companion objects, not on Bar's companion object just because there's an implicit value of type Bar.

[edmundnoble]

@johnynek @paulp Yeah, I managed to continue the misdiagnosis by using a type ascription to actually test this. How about a new PR which 1) has tests and 2) adds an OrderConversion trait the Order companion and cats.implicits can mix in?

[ceedubs]

It sounds like there is a general consensus that this PR in its current form doesn't accomplish much and that this conversion should come in with import cats.implicits._. I'm going to go ahead and close this in favor of #1665.