Semigroup for NonEmptyMap is kind of weird

[travisbrown]

In any recent Cats version (back to before 2.0) |+| is ++ for non-empty maps:

scala> import cats.data.NonEmptyMap, cats.implicits._
import cats.data.NonEmptyMap
import cats.implicits._

scala> NonEmptyMap.of("a" -> List(1)) |+| NonEmptyMap.of("a" -> List(2))
res0: cats.data.NonEmptyMap[String,List[Int]] = Map(a -> List(2))

I don't really use NonEmptyMap, so maybe I'm missing something, but this seems like the wrong thing to do, since |+| for other maps combines at the element level:

scala> Map("a" -> List(1)) |+| Map("a" -> List(2))
res1: scala.collection.immutable.Map[String,List[Int]] = Map(a -> List(1, 2))

We can fix this but it'll take some fussing with, since we'll have to make the current Band instance non-implicit, add some new prioritized instances, etc.

[travisbrown]

@djspiewak This issue is actually unrelated to #3380, although I ran into it when I was looking into the other issue (which had to do with Map, not NonEmptyMap, which the problem here is specific to).

[joroKr21]

I guess this would mean deprecating Band for NonEmptyMap because otherwise it would take priority. Which should be fine because SortedMap doesn't have a Band instance.

On the other hand seems to me that resolving #3223 first would be good.

[brunoatArgos]

Hi @travisbrown, I had to face this issue and I resolved locally adding the following implicit:

  implicit def replaceCatsDataBandForNonEmptyMap[K: Order, V: Semigroup]: Band[NonEmptyMap[K, V]] =
    (x: NonEmptyMap[K, V], y: NonEmptyMap[K, V]) =>
      NonEmptyMap.fromMapUnsafe(x.toSortedMap combine y.toSortedMap)

to be used instead of the current implementation that uses concat for combine

cats/core/src/main/scala/cats/data/NonEmptyMapImpl.scala

Lines 315 to 317 in f5a6172

	implicit def catsDataBandForNonEmptyMap[K, A]: Band[NonEmptyMap[K, A]] = new Band[NonEmptyMap[K, A]] {
	def combine(x: NonEmptyMap[K, A], y: NonEmptyMap[K, A]): NonEmptyMap[K, A] = x ++ y
	}

[nigredo-tori]

I've just hit this as well... I'm used to the "deep combine" behavior for SortedMap, and the destructive behavior for NonEmptyMap came as a shock. Thankfully, I happened to catch this before it led to any issues - but It's easy to miss a Semigroup instance where there shouldn't be one. I feel like this has a potential to introduce a lot of subtle errors.