typelevel / cats

Lightweight, modular, and extensible library for functional programming.
https://typelevel.org/cats/
Other
5.2k stars 1.19k forks source link

Unexpected behavior of NonEmptyMap #3117

Open vilu opened 4 years ago

vilu commented 4 years ago

I noticed that NEM seems to prefer .find on Foldable rather on the concrete type when cats.implicits._ or more concretely import cats.instances.int._ is in scope. This confused me since I had everything working, I pulled in cats.implicits._ for some other reason and this broke the compile. What is the expected order of preference here?

object TreeWithCatsImplicit extends App {
  import cats.data.NonEmptyMap
  import cats.implicits._ // import cats.instances.int._
  val nem: NonEmptyMap[Int, String] = NonEmptyMap.of(
    1 -> "one",
    2 -> "two"
  )

  {
    import scala.reflect.runtime.universe.{reify, show}
    println(show(reify { nem.find(_ == "one") }.tree))
  }
  // implicits.toFoldableOps(Test.this.nem)(NonEmptyMapImpl.catsDataInstancesForNonEmptyMap(implicits.catsKernelStdOrderForInt)).find(((x$1) => x$1.$eq$eq("one")))
}

object WithCatsImplicit extends App {
  import cats.data.NonEmptyMap
  import cats.implicits._
  val nem: NonEmptyMap[Int, String] = NonEmptyMap.of(
    1 -> "one",
    2 -> "two"
  )

  println(nem.find(_ == "one"))
  // Some(one)
}

object WithoutCatsImplicit extends App {
  import cats.data.NonEmptyMap
  import cats.kernel.Order

  implicit val order: Order[Int] = Order.fromOrdering

  val nem: NonEmptyMap[Int, String] = NonEmptyMap.of(
    1 -> "one",
    2 -> "two"
  )

  println(nem.find(_ == "one"))
  //Some((1,one))
}

scala version 2.13 cats version 2.0.0

GodPlaysChess commented 4 years ago

should not foldable of NonEmptyMap fold over tuple (K, V) (like Seq[(K, V)])? It would fix this issue. Though I have no idea if it is even possible to write it in such way. May be through getting the evidence that A <:< (K, V) or smth like that..

Yep, Foldable[NonEmptyMap] does not take into account key :

override def foldLeft[A, B](fa: NonEmptyMap[K, A], b: B)(f: (B, A) => B): B 

btw. standard scala map does it correctly

travisbrown commented 4 years ago

This seems like a good argument against this newtype encoding (which to be honest I've always hated, so this feels kind of vindicating).

I think the issue is Foldable syntax, though, not the Order instance for Int:

scala> import cats.data.NonEmptyMap
import cats.data.NonEmptyMap

scala> import cats.instances.int._
import cats.instances.int._

scala> val nem: NonEmptyMap[Int, String] = NonEmptyMap.of(1 -> "one", 2 -> "two")
nem: cats.data.NonEmptyMap[Int,String] = Map(1 -> one, 2 -> two)

scala> nem.find(_ == "one")
res0: Option[(Int, String)] = Some((1,one))

scala> import cats.syntax.foldable._
import cats.syntax.foldable._

scala> nem.find(_ == "one")
res1: Option[String] = Some(one)

scala> import NonEmptyMap._
import NonEmptyMap._

scala> nem.find(_ == "one")
res2: Option[(Int, String)] = Some((1,one))

I think we're stuck with this (at least until Cats 3), since the imported Foldable syntax will always outrank the NonEmptyMapOps syntax in implicit scope. The best workaround is probably to import NonEmptyMap._ as in my code above.