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.

Sign up for GitHub

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

Optimize Alternative (part 3): add prependK/appendK specializations for Cats NE wrappers #4055

Merged
merged 1 commit into from
Sep 15, 2022
Merged

Optimize Alternative (part 3): add prependK/appendK specializations for Cats NE wrappers #4055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion core/src/main/scala-2.13+/cats/data/NonEmptyLazyList.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ sealed abstract private[data] class NonEmptyLazyListInstances extends NonEmptyLa

implicit val catsDataInstancesForNonEmptyLazyList: Bimonad[NonEmptyLazyList]
with NonEmptyTraverse[NonEmptyLazyList]
with SemigroupK[NonEmptyLazyList]
with NonEmptyAlternative[NonEmptyLazyList]
with Align[NonEmptyLazyList] =
new AbstractNonEmptyInstances[LazyList, NonEmptyLazyList] with Align[NonEmptyLazyList] {

Expand Down
14 changes: 10 additions & 4 deletions core/src/main/scala/cats/data/AbstractNonEmptyInstances.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,23 @@ abstract private[data] class AbstractNonEmptyInstances[F[_], NonEmptyF[_]](impli
MF: Monad[F],
CF: CoflatMap[F],
TF: Traverse[F],
SF: SemigroupK[F]
SF: Alternative[F]
) extends Bimonad[NonEmptyF]
with NonEmptyTraverse[NonEmptyF]
with SemigroupK[NonEmptyF] {
with NonEmptyAlternative[NonEmptyF] {
val monadInstance = MF.asInstanceOf[Monad[NonEmptyF]]
val coflatMapInstance = CF.asInstanceOf[CoflatMap[NonEmptyF]]
val traverseInstance = Traverse[F].asInstanceOf[Traverse[NonEmptyF]]
val semiGroupKInstance = SemigroupK[F].asInstanceOf[SemigroupK[NonEmptyF]]
val alternativeInstance = Alternative[F].asInstanceOf[Alternative[NonEmptyF]]

def combineK[A](a: NonEmptyF[A], b: NonEmptyF[A]): NonEmptyF[A] =
semiGroupKInstance.combineK(a, b)
alternativeInstance.combineK(a, b)

override def prependK[A](a: A, fa: NonEmptyF[A]): NonEmptyF[A] =
alternativeInstance.prependK(a, fa)

override def appendK[A](fa: NonEmptyF[A], a: A): NonEmptyF[A] =
alternativeInstance.appendK(fa, a)

def pure[A](x: A): NonEmptyF[A] = monadInstance.pure(x)

Expand Down
12 changes: 11 additions & 1 deletion core/src/main/scala/cats/data/NonEmptyChain.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,10 +617,20 @@ class NonEmptyChainOps[A](private val value: NonEmptyChain[A])

sealed abstract private[data] class NonEmptyChainInstances extends NonEmptyChainInstances1 {

implicit val catsDataInstancesForNonEmptyChain: SemigroupK[NonEmptyChain]
@deprecated(
"maintained for the sake of binary compatibility only, use catsDataInstancesForNonEmptyChainBinCompat1 instead",
"2.9.0"
)
def catsDataInstancesForNonEmptyChain: SemigroupK[NonEmptyChain]
Copy link
Member

@armanbilge armanbilge Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should we either deprecate these or make them private[data]?

Copy link
Contributor Author

@satorg satorg Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I was pretty sure that I tried to make it package private in the original PR and it didn't work – it broke binary compatibility for some Scala version (or at least Mima though it does). But I've just given it another try and don't see any errors from Mima neither for Scala 2.12 nor for Scala 2.13. I got confused a little bit :)

On the second thought, I would assume that deprecating it might be slightly better because making it package private potentially (although unlikely) breaks source compatibility – in case if someone references this field explicitly in their code. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, @djspiewak just mentioned in another issue:

We generally don't consider implicits to be part of source compatibility, so IMO this is something we can (and should) do in 2.7.1

Good to know, thanks. I guess I can safely make it package private then (since it seemingly doesn't break BC).

Copy link
Contributor Author

@satorg satorg Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Turns out, private[data] works for catsDataInstancesForNonEmptyChain only. For three other similar obsolete former implicits in this PR it breaks the binary compatibility. So I annotated them as deprecated without changes in their visibility.

Copy link
Member

Choose a reason for hiding this comment

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

Were the mima problems only for static methods? I don't know what the policy about static methods is for cats, since those are only used by Java.

with NonEmptyTraverse[NonEmptyChain]
with Bimonad[NonEmptyChain]
with Align[NonEmptyChain] =
catsDataInstancesForNonEmptyChainBinCompat1

implicit val catsDataInstancesForNonEmptyChainBinCompat1: Align[NonEmptyChain]
with Bimonad[NonEmptyChain]
with NonEmptyAlternative[NonEmptyChain]
with NonEmptyTraverse[NonEmptyChain] =
new AbstractNonEmptyInstances[Chain, NonEmptyChain] with Align[NonEmptyChain] {
def extract[A](fa: NonEmptyChain[A]): A = fa.head

Expand Down
24 changes: 20 additions & 4 deletions core/src/main/scala/cats/data/NonEmptyList.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,23 +788,39 @@ object NonEmptyList extends NonEmptyListInstances {

sealed abstract private[data] class NonEmptyListInstances extends NonEmptyListInstances0 {

@deprecated(
"maintained for the sake of binary compatibility only - use catsDataInstancesForNonEmptyListBinCompat1 instead",
"2.9.0"
)
def catsDataInstancesForNonEmptyList
: SemigroupK[NonEmptyList] with Bimonad[NonEmptyList] with NonEmptyTraverse[NonEmptyList] with Align[NonEmptyList] =
catsDataInstancesForNonEmptyListBinCompat1

/**
* This is not a bug. The declared type of `catsDataInstancesForNonEmptyList` intentionally ignores
* `NonEmptyReducible` trait for it not being a typeclass.
*
* Also see the discussion: PR #3541 and issue #3069.
*/
implicit val catsDataInstancesForNonEmptyList
: SemigroupK[NonEmptyList] with Bimonad[NonEmptyList] with NonEmptyTraverse[NonEmptyList] with Align[NonEmptyList] =
implicit val catsDataInstancesForNonEmptyListBinCompat1: NonEmptyAlternative[NonEmptyList]
with Bimonad[NonEmptyList]
with NonEmptyTraverse[NonEmptyList]
with Align[NonEmptyList] =
new NonEmptyReducible[NonEmptyList, List]
with SemigroupK[NonEmptyList]
with NonEmptyAlternative[NonEmptyList]
with Bimonad[NonEmptyList]
with NonEmptyTraverse[NonEmptyList]
with Align[NonEmptyList] {

def combineK[A](a: NonEmptyList[A], b: NonEmptyList[A]): NonEmptyList[A] =
a.concatNel(b)

override def prependK[A](a: A, fa: NonEmptyList[A]): NonEmptyList[A] =
fa.prepend(a)

override def appendK[A](fa: NonEmptyList[A], a: A): NonEmptyList[A] =
fa.append(a)

override def split[A](fa: NonEmptyList[A]): (A, List[A]) = (fa.head, fa.tail)

override def reduceLeft[A](fa: NonEmptyList[A])(f: (A, A) => A): A =
Expand Down Expand Up @@ -955,7 +971,7 @@ sealed abstract private[data] class NonEmptyListInstances extends NonEmptyListIn
new NonEmptyParallel[NonEmptyList] {
type F[x] = ZipNonEmptyList[x]

def flatMap: FlatMap[NonEmptyList] = NonEmptyList.catsDataInstancesForNonEmptyList
def flatMap: FlatMap[NonEmptyList] = NonEmptyList.catsDataInstancesForNonEmptyListBinCompat1

def apply: Apply[ZipNonEmptyList] = ZipNonEmptyList.catsDataCommutativeApplyForZipNonEmptyList

Expand Down
26 changes: 21 additions & 5 deletions core/src/main/scala/cats/data/NonEmptySeq.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,23 +381,39 @@ final class NonEmptySeq[+A] private (val toSeq: Seq[A]) extends AnyVal with NonE
@suppressUnusedImportWarningForScalaVersionSpecific
sealed abstract private[data] class NonEmptySeqInstances {

@deprecated(
"maintained for the sake of binary compatibility only - use catsDataInstancesForNonEmptySeqBinCompat1 instead",
"2.9.0"
)
def catsDataInstancesForNonEmptySeq
: SemigroupK[NonEmptySeq] with Bimonad[NonEmptySeq] with NonEmptyTraverse[NonEmptySeq] with Align[NonEmptySeq] =
catsDataInstancesForNonEmptySeqBinCompat1

/**
* This is not a bug. The declared type of `catsDataInstancesForNonEmptySeq` intentionally ignores
* `NonEmptyReducible` trait for it not being a typeclass.
*
* Also see the discussion: PR #3541 and issue #3069.
*/
implicit val catsDataInstancesForNonEmptySeq
: SemigroupK[NonEmptySeq] with Bimonad[NonEmptySeq] with NonEmptyTraverse[NonEmptySeq] with Align[NonEmptySeq] =
implicit val catsDataInstancesForNonEmptySeqBinCompat1: NonEmptyAlternative[NonEmptySeq]
with Bimonad[NonEmptySeq]
with NonEmptyTraverse[NonEmptySeq]
with Align[NonEmptySeq] =
new NonEmptyReducible[NonEmptySeq, Seq]
with SemigroupK[NonEmptySeq]
with NonEmptyAlternative[NonEmptySeq]
with Bimonad[NonEmptySeq]
with NonEmptyTraverse[NonEmptySeq]
with Align[NonEmptySeq] {

def combineK[A](a: NonEmptySeq[A], b: NonEmptySeq[A]): NonEmptySeq[A] =
a.concatNeSeq(b)

override def prependK[A](a: A, fa: NonEmptySeq[A]): NonEmptySeq[A] =
fa.prepend(a)

override def appendK[A](fa: NonEmptySeq[A], a: A): NonEmptySeq[A] =
fa.append(a)

override def split[A](fa: NonEmptySeq[A]): (A, Seq[A]) = (fa.head, fa.tail)

override def size[A](fa: NonEmptySeq[A]): Long = fa.length.toLong
Expand Down Expand Up @@ -532,14 +548,14 @@ sealed abstract private[data] class NonEmptySeqInstances {
implicit def catsDataShowForNonEmptySeq[A: Show]: Show[NonEmptySeq[A]] = _.show

implicit def catsDataSemigroupForNonEmptySeq[A]: Semigroup[NonEmptySeq[A]] =
catsDataInstancesForNonEmptySeq.algebra
catsDataInstancesForNonEmptySeqBinCompat1.algebra

implicit def catsDataParallelForNonEmptySeq: NonEmptyParallel.Aux[NonEmptySeq, ZipNonEmptySeq] =
new NonEmptyParallel[NonEmptySeq] {
type F[x] = ZipNonEmptySeq[x]

def apply: Apply[ZipNonEmptySeq] = ZipNonEmptySeq.catsDataCommutativeApplyForZipNonEmptySeq
def flatMap: FlatMap[NonEmptySeq] = NonEmptySeq.catsDataInstancesForNonEmptySeq
def flatMap: FlatMap[NonEmptySeq] = NonEmptySeq.catsDataInstancesForNonEmptySeqBinCompat1

def sequential: ZipNonEmptySeq ~> NonEmptySeq =
new (ZipNonEmptySeq ~> NonEmptySeq) { def apply[A](a: ZipNonEmptySeq[A]): NonEmptySeq[A] = a.value }
Expand Down
24 changes: 20 additions & 4 deletions core/src/main/scala/cats/data/NonEmptyVector.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,25 +386,41 @@ final class NonEmptyVector[+A] private (val toVector: Vector[A])
@suppressUnusedImportWarningForScalaVersionSpecific
sealed abstract private[data] class NonEmptyVectorInstances {

@deprecated(
"maintained for the sake of binary compatibility only - use catsDataInstancesForNonEmptyChainBinCompat1 instead",
"2.9.0"
)
def catsDataInstancesForNonEmptyVector: SemigroupK[NonEmptyVector]
with Bimonad[NonEmptyVector]
with NonEmptyTraverse[NonEmptyVector]
with Align[NonEmptyVector] =
catsDataInstancesForNonEmptyVectorBinCompat1

/**
* This is not a bug. The declared type of `catsDataInstancesForNonEmptyVector` intentionally ignores
* `NonEmptyReducible` trait for it not being a typeclass.
*
* Also see the discussion: PR #3541 and issue #3069.
*/
implicit val catsDataInstancesForNonEmptyVector: SemigroupK[NonEmptyVector]
implicit val catsDataInstancesForNonEmptyVectorBinCompat1: NonEmptyAlternative[NonEmptyVector]
with Bimonad[NonEmptyVector]
with NonEmptyTraverse[NonEmptyVector]
with Align[NonEmptyVector] =
new NonEmptyReducible[NonEmptyVector, Vector]
with SemigroupK[NonEmptyVector]
with NonEmptyAlternative[NonEmptyVector]
with Bimonad[NonEmptyVector]
with NonEmptyTraverse[NonEmptyVector]
with Align[NonEmptyVector] {

def combineK[A](a: NonEmptyVector[A], b: NonEmptyVector[A]): NonEmptyVector[A] =
a.concatNev(b)

override def prependK[A](a: A, fa: NonEmptyVector[A]): NonEmptyVector[A] =
fa.prepend(a)

override def appendK[A](fa: NonEmptyVector[A], a: A): NonEmptyVector[A] =
fa.append(a)

override def split[A](fa: NonEmptyVector[A]): (A, Vector[A]) = (fa.head, fa.tail)

override def size[A](fa: NonEmptyVector[A]): Long = fa.length.toLong
Expand Down Expand Up @@ -550,14 +566,14 @@ sealed abstract private[data] class NonEmptyVectorInstances {
implicit def catsDataShowForNonEmptyVector[A: Show]: Show[NonEmptyVector[A]] = _.show

implicit def catsDataSemigroupForNonEmptyVector[A]: Semigroup[NonEmptyVector[A]] =
catsDataInstancesForNonEmptyVector.algebra
catsDataInstancesForNonEmptyVectorBinCompat1.algebra

implicit def catsDataParallelForNonEmptyVector: NonEmptyParallel.Aux[NonEmptyVector, ZipNonEmptyVector] =
new NonEmptyParallel[NonEmptyVector] {
type F[x] = ZipNonEmptyVector[x]

def apply: Apply[ZipNonEmptyVector] = ZipNonEmptyVector.catsDataCommutativeApplyForZipNonEmptyVector
def flatMap: FlatMap[NonEmptyVector] = NonEmptyVector.catsDataInstancesForNonEmptyVector
def flatMap: FlatMap[NonEmptyVector] = NonEmptyVector.catsDataInstancesForNonEmptyVectorBinCompat1

def sequential: ZipNonEmptyVector ~> NonEmptyVector =
new (ZipNonEmptyVector ~> NonEmptyVector) { def apply[A](a: ZipNonEmptyVector[A]): NonEmptyVector[A] = a.value }
Expand Down
6 changes: 3 additions & 3 deletions core/src/main/scala/cats/data/OneAnd.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ final case class OneAnd[F[_], A](head: A, tail: F[A]) {
* Combine the head and tail into a single `F[A]` value.
*/
def unwrap(implicit F: Alternative[F]): F[A] =
F.combineK(F.pure(head), tail)
F.prependK(head, tail)

/**
* remove elements not matching the predicate
Expand All @@ -54,7 +54,7 @@ final case class OneAnd[F[_], A](head: A, tail: F[A]) {
* Append another OneAnd to this
*/
def combine(other: OneAnd[F, A])(implicit F: Alternative[F]): OneAnd[F, A] =
OneAnd(head, F.combineK(tail, F.combineK(F.pure(other.head), other.tail)))
OneAnd(head, F.combineK(tail, other.unwrap))

/**
* find the first element matching the predicate, if one exists
Expand Down Expand Up @@ -239,7 +239,7 @@ sealed abstract private[data] class OneAndLowPriority2 extends OneAndLowPriority
override def ap[A, B](ff: OneAnd[F, A => B])(fa: OneAnd[F, A]): OneAnd[F, B] = {
val (f, tf) = (ff.head, ff.tail)
val (a, ta) = (fa.head, fa.tail)
val fb = F.ap(tf)(F.combineK(F.pure(a), ta))
val fb = F.ap(tf)(F.prependK(a, ta))
OneAnd(f(a), F.combineK(F.map(ta)(f), fb))
}
}
Expand Down
33 changes: 16 additions & 17 deletions tests/shared/src/test/scala-2.13+/cats/tests/NonEmptyLazyListSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,21 @@

package cats.tests

import cats.{Align, Bimonad, SemigroupK, Show, Traverse}
import cats.data.{NonEmptyLazyList, NonEmptyLazyListOps, NonEmptyVector}
import cats.kernel.{Eq, Hash, Order, PartialOrder, Semigroup}
import cats.kernel.laws.discipline.{EqTests, HashTests, OrderTests, PartialOrderTests, SemigroupTests}
import cats.laws.discipline.{
AlignTests,
BimonadTests,
NonEmptyTraverseTests,
SemigroupKTests,
SerializableTests,
ShortCircuitingTests
}
import cats._
import cats.data.NonEmptyLazyList
import cats.data.NonEmptyLazyListOps
import cats.data.NonEmptyVector
import cats.kernel.laws.discipline.EqTests
import cats.kernel.laws.discipline.HashTests
import cats.kernel.laws.discipline.OrderTests
import cats.kernel.laws.discipline.PartialOrderTests
import cats.kernel.laws.discipline.SemigroupTests
import cats.kernel.laws.discipline.SerializableTests
import cats.laws.discipline._
import cats.laws.discipline.arbitrary._
import cats.syntax.either._
import cats.syntax.foldable._
import cats.syntax.eq._
import cats.Reducible
import cats.Eval
import cats.syntax.foldable._
import org.scalacheck.Prop._

class NonEmptyLazyListSuite extends NonEmptyCollectionSuite[LazyList, NonEmptyLazyList, NonEmptyLazyListOps] {
Expand All @@ -52,8 +49,10 @@ class NonEmptyLazyListSuite extends NonEmptyCollectionSuite[LazyList, NonEmptyLa
checkAll(s"NonEmptyLazyList[Int]", HashTests[NonEmptyLazyList[Int]].hash)
checkAll(s"Hash[NonEmptyLazyList[Int]]", SerializableTests.serializable(Hash[NonEmptyLazyList[Int]]))

checkAll("NonEmptyLazyList[Int]", SemigroupKTests[NonEmptyLazyList].semigroupK[Int])
checkAll("SemigroupK[NonEmptyLazyList]", SerializableTests.serializable(SemigroupK[NonEmptyLazyList]))
checkAll("NonEmptyLazyList[Int]", NonEmptyAlternativeTests[NonEmptyLazyList].nonEmptyAlternative[Int, Int, Int])
checkAll("NonEmptyAlternative[NonEmptyLazyList]",
SerializableTests.serializable(NonEmptyAlternative[NonEmptyLazyList])
)

checkAll("NonEmptyLazyList[Int] with Option",
NonEmptyTraverseTests[NonEmptyLazyList].nonEmptyTraverse[Option, Int, Int, Int, Int, Option, Option]
Expand Down
27 changes: 12 additions & 15 deletions tests/shared/src/test/scala/cats/tests/NonEmptyChainSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,31 +21,28 @@

package cats.tests

import cats.{Align, Bimonad, SemigroupK, Show, Traverse}
import cats.data.{Chain, NonEmptyChain, NonEmptyChainOps}
import cats.kernel.{Eq, Order, PartialOrder, Semigroup}
import cats.kernel.laws.discipline.{EqTests, OrderTests, PartialOrderTests, SemigroupTests}
import cats.laws.discipline.{
AlignTests,
BimonadTests,
NonEmptyTraverseTests,
SemigroupKTests,
SerializableTests,
ShortCircuitingTests
}
import cats._
import cats.data.Chain
import cats.data.NonEmptyChain
import cats.data.NonEmptyChainOps
import cats.kernel.laws.discipline.EqTests
import cats.kernel.laws.discipline.OrderTests
import cats.kernel.laws.discipline.PartialOrderTests
import cats.kernel.laws.discipline.SemigroupTests
import cats.laws.discipline._
import cats.laws.discipline.arbitrary._
import cats.syntax.either._
import cats.syntax.foldable._
import cats.syntax.eq._
import cats.syntax.foldable._
import org.scalacheck.Prop._

class NonEmptyChainSuite extends NonEmptyCollectionSuite[Chain, NonEmptyChain, NonEmptyChainOps] {
protected def toList[A](value: NonEmptyChain[A]): List[A] = value.toChain.toList
protected def underlyingToList[A](underlying: Chain[A]): List[A] = underlying.toList
protected def toNonEmptyCollection[A](nea: NonEmptyChain[A]): NonEmptyChainOps[A] = nea

checkAll("NonEmptyChain[Int]", SemigroupKTests[NonEmptyChain].semigroupK[Int])
checkAll("SemigroupK[NonEmptyChain]", SerializableTests.serializable(SemigroupK[NonEmptyChain]))
checkAll("NonEmptyChain[Int]", NonEmptyAlternativeTests[NonEmptyChain].nonEmptyAlternative[Int, Int, Int])
checkAll("NonEmptyAlternative[NonEmptyChain]", SerializableTests.serializable(NonEmptyAlternative[NonEmptyChain]))

checkAll("NonEmptyChain[Int] with Option",
NonEmptyTraverseTests[NonEmptyChain].nonEmptyTraverse[Option, Int, Int, Int, Int, Option, Option]
Expand Down
Loading