Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Follow up to #751 #757

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion core/src/main/scala/cats/Alternative.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,20 @@ package cats

import simulacrum.typeclass

@typeclass trait Alternative[F[_]] extends Applicative[F] with MonoidK[F]
@typeclass trait Alternative[F[_]] extends Applicative[F] with MonoidK[F] { self =>
/**
* Compose two Alternative intsances.
*/
def compose[G[_]](implicit GG: Alternative[G]): Alternative[λ[α => F[G[α]]]] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the GG constraint can be relaxed to Applicative while still returning an Alternative (also on CompositeAlternative below).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed ... updated accordingly ...

new CompositeAlternative[F, G] {
implicit def F: Alternative[F] = self
implicit def G: Alternative[G] = GG
}
}

trait CompositeAlternative[F[_], G[_]]
extends Alternative[λ[α => F[G[α]]]] with CompositeApplicative[F, G] with CompositeMonoidK[F, G] {

implicit def F: Alternative[F]
implicit def G: Alternative[G]
}
19 changes: 19 additions & 0 deletions core/src/main/scala/cats/MonoidK.scala
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ import simulacrum.typeclass
*/
def empty[A]: F[A]

/**
* Compose two MonoidK intsances.
*/
def compose[G[_]](implicit GG: MonoidK[G]): MonoidK[λ[α => F[G[α]]]] =
new CompositeMonoidK[F, G] {
implicit def F: MonoidK[F] = self
implicit def G: MonoidK[G] = GG
}

/**
* Given a type A, create a concrete Monoid[F[A]].
*/
Expand All @@ -38,3 +47,13 @@ import simulacrum.typeclass
def combine(x: F[A], y: F[A]): F[A] = self.combine(x, y)
}
}

trait CompositeMonoidK[F[_],G[_]]
extends MonoidK[λ[α => F[G[α]]]] {

implicit def F: MonoidK[F]
implicit def G: MonoidK[G]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually use this G at all. The same is true for the current SemigroupK - if you drop the SemigroupK[G] implicit evidence, everything still compiles.

We can freely take a SemigroupK[F] and turn it into a SemigroupK[λ[α => F[G[α]]] with no requirements on G (it can even be a polykinded type like Nothing or Any!). I think we should probably drop the requirements on G. I'm also curious if another name would make more sense, since we aren't actually composing two MonoidK instances together. However, we are creating a MonoidK instance for the composite type, so it still may be a reasonable name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point ... I'll remove the constraint.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is usage of the from MonoidK[F] compose MonoidK[G] (resp. SemigroupK) which depends on the constraint generating an implicit argument which which be supplied explicitly. I'll try providing two variants, one with the constraint and one without.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this comment. I'm saying that I think we should be able to change MonoidK[F] compose MonoidK[G] to simply MonoidK[F].compose[G] since the MonoidK[G] is ignored as far as I can tell. I might be missing something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is that there is usage of the form f compose g for this entire family. I think we should preserve that, and also add an operator of the form f.composedWith[G] for the cases where the constraint isn't necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I guess that seems to be a bit misleading to me. You aren't really composing the instances at all. You are just grabbing a type parameter from the second one. My inclination would be to have def compose[G[_]]: Semigroup[...] on SemigroupK, def compose[G[_]]: MonoidK[...] on MonoidK, def compose[G[_]:Applicative: Applicative[...] on Applicative, and def compose[G[_]:Applicative]: Alternative[...] on Alternative. To me this seems to be the clearest about what's going on and which constraints are needed.

@non I think you originally added the SemigroupK[G] requirement to compose. Was that intentional or an oversight? What are your thoughts?

Edit: for the cases where the evidence isn't required, I'm completely happy with calling these something other than compose to avoid ambiguity.


def empty[A]: F[G[A]] = F.empty
def combine[A](x: F[G[A]], y: F[G[A]]): F[G[A]] = F.combine(x, y)
}
6 changes: 6 additions & 0 deletions core/src/main/scala/cats/data/OneAnd.scala
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,12 @@ trait OneAndLowPriority1 extends OneAndLowPriority0 {
def map[A, B](fa: OneAnd[F, A])(f: A => B): OneAnd[F, B] =
OneAnd(f(fa.head), F.map(fa.tail)(f))
}

implicit def oneAndReducible[F[_]](implicit foldable: Foldable[F]): Reducible[OneAnd[F, ?]] =
new NonEmptyReducible[OneAnd[F, ?], F] {
def split[A](fa: OneAnd[F, A]): (A, F[A]) = (fa.head, fa.tail)
}

}

object OneAnd extends OneAndInstances
31 changes: 20 additions & 11 deletions tests/src/test/scala/cats/tests/ComposeTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ package cats
package tests

import cats.data.{ NonEmptyList, NonEmptyVector, OneAnd }
import cats.laws.discipline.{ ApplicativeTests, FoldableTests, MonoidalTests, SemigroupKTests, arbitrary, eq }, arbitrary._, eq._
import cats.laws.discipline.{ AlternativeTests, ApplicativeTests, FoldableTests, MonoidKTests, MonoidalTests, SemigroupKTests }
import cats.laws.discipline.{ arbitrary, eq }, arbitrary._, eq._
import org.scalacheck.Arbitrary

class ComposeTests extends CatsSuite {
Expand All @@ -12,6 +13,15 @@ class ComposeTests extends CatsSuite {
implicit override val generatorDrivenConfig: PropertyCheckConfiguration =
PropertyCheckConfig(maxSize = 5, minSuccessful = 20)

{
// Alternative composition

implicit val alternativeListVector: Alternative[Lambda[A => List[Vector[A]]]] = Alternative[List] compose Alternative[Vector]
implicit val iso = MonoidalTests.Isomorphisms.invariant[Lambda[A => List[Vector[A]]]]

checkAll("Alternative[Lambda[A => List[Vector[A]]]]", AlternativeTests[Lambda[A => List[Vector[A]]]].alternative[Int, Int, Int])
}

{
// Applicative composition

Expand All @@ -30,19 +40,18 @@ class ComposeTests extends CatsSuite {
}

{
// Reducible composition
// MonoidK composition

implicit val monoidKListVector: MonoidK[Lambda[A => List[Vector[A]]]] = MonoidK[List] compose MonoidK[Vector]

val nelReducible =
new NonEmptyReducible[NonEmptyList, List] {
def split[A](fa: NonEmptyList[A]): (A, List[A]) = (fa.head, fa.tail)
}
checkAll("MonoidK[Lambda[A => List[Vector[A]]]]", MonoidKTests[Lambda[A => List[Vector[A]]]].monoidK[Int])
}

val nevReducible =
new NonEmptyReducible[NonEmptyVector, Vector] {
def split[A](fa: NonEmptyVector[A]): (A, Vector[A]) = (fa.head, fa.tail)
}
{
// Reducible composition

implicit val reducibleListVector: Reducible[Lambda[A => NonEmptyList[NonEmptyVector[A]]]] = nelReducible compose nevReducible
implicit val reducibleListVector: Reducible[Lambda[A => NonEmptyList[NonEmptyVector[A]]]] =
Reducible[NonEmptyList] compose Reducible[NonEmptyVector]

// No Reducible-specific laws, so check the Foldable laws are satisfied
checkAll("Reducible[Lambda[A => List[Vector[A]]]]", FoldableTests[Lambda[A => NonEmptyList[NonEmptyVector[A]]]].foldable[Int, Int])
Expand Down