Provide instance of Alternative[Set] in alleycats

[rtyley]

This allows the use of methods from Alternative on a Set - for instance, I want to be able to use separate on a Set:

import cats.implicits._
import alleycats.std.set._

val stringsAndInts: Set[Either[String, Int]] = Set(Right(6),Left("Foo"))
val (strings: Set[String], ints: Set[Int]) = stringsAndInts.separate

The Alternative typeclass just requires MonoidK (already provided for Set in cats-core) & Applicative (already provided for Set by Monad in alleycats), so adding it to alleycats is a small change.

Here's a diagram I drew mainly for myself (!), as a novice contributing to Cats, to understand the typeclass relationships involved:

This is a kind reminder to run sbt +prePR and commit the changed files, if any, before submitting.

I did try running this - but it crashed on unrelated files like this:

[info] Forcing Scala version to 3.0.0-M3 on all projects.
[info] Reapplying settings...
[info] set current project to cats (in build file:/home/roberto/development/cats/)
[info] Formatting 2 Scala sources...
[info] Formatting 2 Scala sources...
[info] Formatting 2 Scala sources...
[info] Formatting 1 Scala sources...
[info] Formatting 1 Scala sources...
[info] Formatting 1 Scala sources...
[error] /home/roberto/development/cats/free/src/main/scala-3.x/cats/free/FreeFoldStep.scala:19: error: identifier expected but [ found
[error]     onFlatMapped: [X] => (S[X], X => Free[S, A]) => B

[rtyley]

These are duplicated from catsStdInstancesForSet in cats-core:

cats/core/src/main/scala/cats/instances/set.scala

Lines 23 to 25 in 3d900de

	def empty[A]: Set[A] = Set.empty[A]
	def combineK[A](x: Set[A], y: Set[A]): Set[A] = x | y

...I wasn't sure if delegating to the cats-core typeclass instance was the thing to do, or if it was clearer just to duplicate!

[rossabaker]

I'm okay with this. The duplication is slight and the laws ought to catch problems.

[rtyley]

I think this PR is finally ready for review, but I'm not sure who to ask for review! - maybe @larsrh or @kailuowang ?

[rossabaker]

Thanks for this!

What happens if we try to summon a MonoidK[Set] with this in scope? There's only one Alternative[Set], so your test passes, but there are now two MonoidK[Set] instances, and I don't see a prioritization. I am not too familiar with alleycats, so perhaps there is precedent for a workaround. Or maybe it works, but I'd throw in a test for that to be sure. Also maybe try it from a REPL with the standard imports, just to be super sure.

[rossabaker]

I'm okay with this. The duplication is slight and the laws ought to catch problems.

[rtyley]

There's only one Alternative[Set], so your test passes, but there are now two MonoidK[Set] instances, and I don't see a prioritization. I am not too familiar with alleycats, so perhaps there is precedent for a workaround. Or maybe it works, but I'd throw in a test for that to be sure.

Thanks - that all makes sense, I will take a look at this a bit later 👍

[rtyley]

I tried summoning a MonoidK[Set], and as you anticipated, ambiguous implicit values are a problem:

scala> import cats._
     | import cats.data._
     | import cats.implicits._
     | import alleycats.std.all._
import cats._
import cats.data._
import cats.implicits._
import alleycats.std.all._

scala> implicitly[MonoidK[Set]]
                 ^
       error: ambiguous implicit values:
        both value catsStdInstancesForSet in trait SetInstances of type cats.UnorderedTraverse[Set] with cats.MonoidK[Set]
        and value alleyCatsStdSetMonad in trait SetInstances of type cats.Monad[Set] with cats.Alternative[Set]
        match expected type cats.MonoidK[Set]

I don't know how to get round this - if both imports (cats.implicits._ & alleycats.std.all._) make an implicit instance of MonoidK[Set] available, I don't know how to resolve that ambiguity for the compiler. Although I've read https://typelevel.org/cats/guidelines.html#-implicit-instance-priority I can't see how to apply it to this situation, where the concrete instance in cats-core can't refer to anything in alleycats. Is any kind of disambiguation possible?!

[larsrh]

I don't think we can get around that. Possibly the instance could be made non-implicit?

[rossabaker]

Clients could get around it by excluding the cats one in the import, or using a la carte imports. But, yuck.

Is this the first time we've needed to duplicate a cats instance as part of the hierarchy in an alleycats instance? I'm surprised this hasn't come up before.

[rossabaker]

Alleycats' Traverse[Map[K, *]] vs. Cats' UnorderedTraverse[Map[K, *]]? Both are implicit. Seems analogous to what is being done here.

[larsrh]

I think the problem here is that neither implicit is more specific, whereas Traverse is more specific than UnorderedTraverse.

[rossabaker]

But if someone asked for a UnorderedTraverse[Map[K, *]], they'd have the same problem as a MonoidK[Set], no? And Traverse[Map[K, *]] is fine, like Alternative[Set] is fine.

[larsrh]

The more specific implicit beats the less specific one every time:

@ implicitly[Traverse[({ type l[a] = Map[String, a] })#l]] 
res3: Traverse[Map[String, a]] = alleycats.std.MapInstances$$anon$1@124dac75

@ implicitly[UnorderedTraverse[({ type l[a] = Map[String, a] })#l]] 
res4: UnorderedTraverse[Map[String, a]] = alleycats.std.MapInstances$$anon$1@750f64fe

[rossabaker]

I must have weekend brain, because I don't see why that should be different. What is "more specific"? Alternative is a subtype of MonoidK like Traverse is a subtype of UnorderedTraverse.

[larsrh]

You might be right ... maybe the actual problem here is that the with in the implicit definition in this PR breaks the specificity rule somehow?

[rossabaker]

Is it your REPL that's allowing the Map instances? Nested objects as an implementation of the REPL?

[joroKr21]

You might be right ... maybe the actual problem here is that the with in the implicit definition in this PR breaks the specificity rule somehow?

Yes, that is correct - neither is subtype of the other: cats.UnorderedTraverse[Set] with cats.MonoidK[Set] and cats.Monad[Set] with cats.Alternative[Set]. In this case bundling instances together coming to bite us. If we wish to avoid ambiguity we should separate them at the cost of duplication (or factor out common implementation into private traits).

[rossabaker]

cats.implicits._ isn't necessary for standard instances anymore, and I think that's the difference:

@ import $ivy.`org.typelevel::alleycats-core:2.5.0` 
import $ivy.$                                    

@ import cats._, cats.data._, alleycats.std.all._ 
import cats._, cats.data._, alleycats.std.all._

@ implicitly[Traverse[({ type l[a] = Map[String, a] })#l]]  
res2: Traverse[Map[String, a]] = alleycats.std.MapInstances$$anon$1@5f395ce1

@ implicitly[UnorderedTraverse[({ type l[a] = Map[String, a] })#l]]  
res3: UnorderedTraverse[Map[String, a]] = alleycats.std.MapInstances$$anon$1@16f7f59f

@ import cats._, cats.data._, cats.implicits._, alleycats.std.all._ 
import cats._, cats.data._, cats.implicits._, alleycats.std.all._

@ implicitly[Traverse[({ type l[a] = Map[String, a] })#l]]  
res5: Traverse[Map[String, a]] = alleycats.std.MapInstances$$anon$1@486dd616

@ implicitly[UnorderedTraverse[({ type l[a] = Map[String, a] })#l]]  
cmd6.sc:1: ambiguous implicit values:
 both method alleycatsStdInstancesForMap in trait MapInstances of type [K]cats.Traverse[[β$0$]scala.collection.immutable.Map[K,β$0$]]
 and method catsStdInstancesForMap in trait MapInstances of type [K]cats.UnorderedTraverse[[β$0$]scala.collection.immutable.Map[K,β$0$]] with cats.FlatMap[[β$1$]scala.collection.immutable.Map[K,β$1$]] with cats.Align[[β$2$]scala.collection.immutable.Map[K,β$2$]]
 match expected type cats.UnorderedTraverse[[a]scala.collection.immutable.Map[String,a]]
val res6 = implicitly[UnorderedTraverse[({ type l[a] = Map[String, a] })#l]] 
                     ^
Compilation Failed

I bet this works without that import, too.

[larsrh]

@rossabaker is correct, so this PR is good to go.