Provide instance of `Alternative[Set]` in alleycats #3837

rtyley · 2021-03-28T22:37:28Z

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

alleycats-core/src/main/scala/alleycats/std/set.scala

rtyley · 2021-03-31T15:19:48Z

alleycats-core/src/main/scala/alleycats/std/set.scala

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


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!

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

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. https://github.com/typelevel/cats/tree/main/alleycats-core#set_-instances

rtyley · 2021-03-31T18:00:42Z

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 · 2021-04-01T02:39:55Z

alleycats-core/src/main/scala/alleycats/std/set.scala

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


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

rtyley · 2021-04-01T10:01:31Z

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 · 2021-04-02T09:28:14Z

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 · 2021-04-03T14:28:51Z

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

rossabaker · 2021-04-03T14:41:02Z

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 · 2021-04-03T14:45:11Z

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

larsrh · 2021-04-03T14:50:12Z

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

rossabaker · 2021-04-03T15:35:26Z

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 · 2021-04-03T15:50:22Z

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 · 2021-04-03T15:54:12Z

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 · 2021-04-03T16:00:10Z

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 · 2021-04-03T16:59:48Z

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

joroKr21 · 2021-04-04T07:30:31Z

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 · 2021-04-04T15:42:22Z

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 · 2021-04-05T14:07:01Z

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

rtyley force-pushed the provide-instance-of-alternative-for-Set branch 7 times, most recently from 73962e2 to 1d5d3ec Compare March 31, 2021 15:07

rtyley commented Mar 31, 2021

View reviewed changes

alleycats-core/src/main/scala/alleycats/std/set.scala Outdated Show resolved Hide resolved

rtyley commented Mar 31, 2021

View reviewed changes

rtyley changed the title ~~Provide instance of Alternative for Set in alleycats~~ Provide instance of Alternative for Set in alleycats Mar 31, 2021

rtyley changed the title ~~Provide instance of Alternative for Set in alleycats~~ Provide instance of Alternative[Set] in alleycats Mar 31, 2021

rtyley force-pushed the provide-instance-of-alternative-for-Set branch from 1d5d3ec to a900607 Compare March 31, 2021 15:33

rossabaker reviewed Apr 1, 2021

View reviewed changes

larsrh approved these changes Apr 5, 2021

View reviewed changes

larsrh merged commit 7cd2911 into typelevel:main Apr 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide instance of `Alternative[Set]` in alleycats #3837

Provide instance of `Alternative[Set]` in alleycats #3837

rtyley commented Mar 28, 2021 •

edited

Loading

rtyley Mar 31, 2021 •

edited

Loading

rossabaker Apr 1, 2021

rtyley commented Mar 31, 2021

rossabaker left a comment

rossabaker Apr 1, 2021

rtyley commented Apr 1, 2021

rtyley commented Apr 2, 2021

larsrh commented Apr 3, 2021

rossabaker commented Apr 3, 2021

rossabaker commented Apr 3, 2021

larsrh commented Apr 3, 2021 •

edited

Loading

rossabaker commented Apr 3, 2021

larsrh commented Apr 3, 2021

rossabaker commented Apr 3, 2021

larsrh commented Apr 3, 2021

rossabaker commented Apr 3, 2021

joroKr21 commented Apr 4, 2021

rossabaker commented Apr 4, 2021

larsrh commented Apr 5, 2021

		override def empty[A]: Set[A] = Set.empty

		override def combineK[A](x: Set[A], y: Set[A]): Set[A] = x \| y

	def empty[A]: Set[A] = Set.empty[A]

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

Provide instance of Alternative[Set] in alleycats #3837

Provide instance of Alternative[Set] in alleycats #3837

Conversation

rtyley commented Mar 28, 2021 • edited Loading

rtyley Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

rossabaker Apr 1, 2021

Choose a reason for hiding this comment

rtyley commented Mar 31, 2021

rossabaker left a comment

Choose a reason for hiding this comment

rossabaker Apr 1, 2021

Choose a reason for hiding this comment

rtyley commented Apr 1, 2021

rtyley commented Apr 2, 2021

larsrh commented Apr 3, 2021

rossabaker commented Apr 3, 2021

rossabaker commented Apr 3, 2021

larsrh commented Apr 3, 2021 • edited Loading

rossabaker commented Apr 3, 2021

larsrh commented Apr 3, 2021

rossabaker commented Apr 3, 2021

larsrh commented Apr 3, 2021

rossabaker commented Apr 3, 2021

joroKr21 commented Apr 4, 2021

rossabaker commented Apr 4, 2021

larsrh commented Apr 5, 2021

Provide instance of `Alternative[Set]` in alleycats #3837

Provide instance of `Alternative[Set]` in alleycats #3837

rtyley commented Mar 28, 2021 •

edited

Loading

rtyley Mar 31, 2021 •

edited

Loading

larsrh commented Apr 3, 2021 •

edited

Loading