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

Add SortedMap and SortedSet instances/Move Set and Map instances to Alleycats #1972

Merged
merged 20 commits into from
Oct 30, 2017
Merged
Show file tree
Hide file tree
Changes from 14 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
2 changes: 1 addition & 1 deletion alleycats-core/src/main/scala/alleycats/std/all.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ import export._
SetInstances,
TryInstances,
IterableInstances
) object all extends LegacySetInstances with LegacyTryInstances with LegacyIterableInstances
) object all extends LegacySetInstances with LegacyTryInstances with LegacyIterableInstances with MapInstances
46 changes: 46 additions & 0 deletions alleycats-core/src/main/scala/alleycats/std/map.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package alleycats
package std

import cats._

trait MapInstances {

// toList is inconsistent. See https://github.com/typelevel/cats/issues/1831
implicit def alleycatsStdInstancesForMap[K]: Traverse[Map[K, ?]] =
new Traverse[Map[K, ?]] {

def traverse[G[_], A, B](fa: Map[K, A])(f: A => G[B])(implicit G: Applicative[G]): G[Map[K, B]] = {
val gba: Eval[G[Map[K, B]]] = Always(G.pure(Map.empty))
val gbb = Foldable.iterateRight(fa, gba){ (kv, lbuf) =>
G.map2Eval(f(kv._2), lbuf)({ (b, buf) => buf + (kv._1 -> b)})
}.value
G.map(gbb)(_.toMap)
}

override def map[A, B](fa: Map[K, A])(f: A => B): Map[K, B] =
fa.map { case (k, a) => (k, f(a)) }

def foldLeft[A, B](fa: Map[K, A], b: B)(f: (B, A) => B): B =
fa.foldLeft(b) { case (x, (k, a)) => f(x, a)}

def foldRight[A, B](fa: Map[K, A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
Foldable.iterateRight(fa.values, lb)(f)

override def size[A](fa: Map[K, A]): Long = fa.size.toLong

override def get[A](fa: Map[K, A])(idx: Long): Option[A] =
if (idx < 0L || Int.MaxValue < idx) None
else {
val n = idx.toInt
if (n >= fa.size) None
else Some(fa.valuesIterator.drop(n).next)
}

override def isEmpty[A](fa: Map[K, A]): Boolean = fa.isEmpty

override def fold[A](fa: Map[K, A])(implicit A: Monoid[A]): A =
A.combineAll(fa.values)

override def toList[A](fa: Map[K, A]): List[A] = fa.values.toList
}
}
35 changes: 34 additions & 1 deletion alleycats-core/src/main/scala/alleycats/std/set.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package alleycats.std

import cats.{Applicative, Eval, Foldable, Monad, Traverse}
import cats.{Applicative, Eval, Foldable, Monad, Monoid, Traverse}
import export._

import scala.annotation.tailrec
Expand Down Expand Up @@ -67,12 +67,45 @@ object SetInstances {
fa.foldLeft(b)(f)
def foldRight[A, B](fa: Set[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
Foldable.iterateRight(fa, lb)(f)

def traverse[G[_]: Applicative, A, B](sa: Set[A])(f: A => G[B]): G[Set[B]] = {
val G = Applicative[G]
sa.foldLeft(G.pure(Set.empty[B])) { (buf, a) =>
G.map2(buf, f(a))(_ + _)
}
}

override def get[A](fa: Set[A])(idx: Long): Option[A] = {
@tailrec
def go(idx: Int, it: Iterator[A]): Option[A] = {
if (it.hasNext) {
if (idx == 0) Some(it.next) else {
it.next
go(idx - 1, it)
}
} else None
}
if (idx < Int.MaxValue && idx >= 0L) go(idx.toInt, fa.toIterator) else None
}

override def size[A](fa: Set[A]): Long = fa.size.toLong

override def exists[A](fa: Set[A])(p: A => Boolean): Boolean =
fa.exists(p)

override def forall[A](fa: Set[A])(p: A => Boolean): Boolean =
fa.forall(p)

override def isEmpty[A](fa: Set[A]): Boolean = fa.isEmpty

override def fold[A](fa: Set[A])(implicit A: Monoid[A]): A = A.combineAll(fa)

override def toList[A](fa: Set[A]): List[A] = fa.toList

override def reduceLeftOption[A](fa: Set[A])(f: (A, A) => A): Option[A] =
fa.reduceLeftOption(f)

override def find[A](fa: Set[A])(f: A => Boolean): Option[A] = fa.find(f)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,16 @@ package alleycats
package tests


import alleycats.std.MapInstances
import catalysts.Platform

import cats._
import cats.instances.AllInstances
import cats.syntax.{AllSyntax, EqOps}
import cats.tests.StrictCatsEquality
import org.scalactic.anyvals.{PosZDouble, PosInt, PosZInt}
import org.scalactic.anyvals.{PosInt, PosZDouble, PosZInt}
import org.scalatest.{FunSuite, Matchers}
import org.scalatest.prop.{Configuration, GeneratorDrivenPropertyChecks}
import org.typelevel.discipline.scalatest.Discipline

import org.scalacheck.{Arbitrary, Gen}
import org.scalacheck.Arbitrary.arbitrary

Expand All @@ -37,7 +36,7 @@ trait TestSettings extends Configuration with Matchers {
* An opinionated stack of traits to improve consistency and reduce
* boilerplate in Alleycats tests. Derived from Cats.
*/
trait AlleycatsSuite extends FunSuite with Matchers with GeneratorDrivenPropertyChecks with Discipline with TestSettings with AllInstances with AllSyntax with TestInstances with StrictCatsEquality {
trait AlleycatsSuite extends FunSuite with Matchers with GeneratorDrivenPropertyChecks with Discipline with TestSettings with AllInstances with AllSyntax with TestInstances with StrictCatsEquality with MapInstances {
implicit override val generatorDrivenConfig: PropertyCheckConfiguration =
checkConfiguration

Expand Down
9 changes: 9 additions & 0 deletions alleycats-tests/src/test/scala/alleycats/tests/MapSuite.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package alleycats.tests

import cats.laws.discipline.{SerializableTests, TraverseTests}
import cats.Traverse

class MapSuite extends AlleycatsSuite {
checkAll("Map[Int, Int] with Option", TraverseTests[Map[Int, ?]].traverse[Int, Int, Int, Int, Option, Option])
checkAll("Traverse[Map[Int, ?]]", SerializableTests.serializable(Traverse[Map[Int, ?]]))
}
21 changes: 21 additions & 0 deletions alleycats-tests/src/test/scala/alleycats/tests/SetSuite.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package alleycats.tests

import alleycats.laws.discipline._
import cats.Foldable
import cats.kernel.laws.discipline.SerializableTests
import cats.laws.discipline.FoldableTests

import alleycats.std.all._

class SetSuite extends AlleycatsSuite {

checkAll("FlatMapRec[Set]", FlatMapRecTests[Set].tailRecM[Int])

checkAll("Set[Int]", FoldableTests[Set].foldable[Int, Int])
checkAll("Foldable[Set]", SerializableTests.serializable(Foldable[Set]))


}



14 changes: 0 additions & 14 deletions alleycats-tests/src/test/scala/alleycats/tests/SetTests.scala

This file was deleted.

2 changes: 2 additions & 0 deletions core/src/main/scala/cats/instances/all.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ trait AllInstances
with PartialOrderingInstances
with QueueInstances
with SetInstances
with SortedMapInstances
with SortedSetInstances
with StreamInstances
with StringInstances
with SymbolInstances
Expand Down
35 changes: 2 additions & 33 deletions core/src/main/scala/cats/instances/map.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,8 @@ trait MapInstances extends cats.kernel.instances.MapInstances {
}

// scalastyle:off method.length
implicit def catsStdInstancesForMap[K]: Traverse[Map[K, ?]] with FlatMap[Map[K, ?]] =
new Traverse[Map[K, ?]] with FlatMap[Map[K, ?]] {

def traverse[G[_], A, B](fa: Map[K, A])(f: A => G[B])(implicit G: Applicative[G]): G[Map[K, B]] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some law that this violates? I hate to remove it if we don’t actually have a law that it violates. I think we can write some (the original ticket had an example). I fear we could have other lawless instances without a law.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

val gba: Eval[G[Map[K, B]]] = Always(G.pure(Map.empty))
val gbb = Foldable.iterateRight(fa, gba){ (kv, lbuf) =>
G.map2Eval(f(kv._2), lbuf)({ (b, buf) => buf + (kv._1 -> b)})
}.value
G.map(gbb)(_.toMap)
}
implicit def catsStdInstancesForMap[K]: FlatMap[Map[K, ?]] =
new FlatMap[Map[K, ?]] {

override def map[A, B](fa: Map[K, A])(f: A => B): Map[K, B] =
fa.map { case (k, a) => (k, f(a)) }
Expand All @@ -47,12 +39,6 @@ trait MapInstances extends cats.kernel.instances.MapInstances {
def flatMap[A, B](fa: Map[K, A])(f: (A) => Map[K, B]): Map[K, B] =
fa.flatMap { case (k, a) => f(a).get(k).map((k, _)) }

def foldLeft[A, B](fa: Map[K, A], b: B)(f: (B, A) => B): B =
fa.foldLeft(b) { case (x, (k, a)) => f(x, a)}

def foldRight[A, B](fa: Map[K, A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
Foldable.iterateRight(fa.values, lb)(f)

def tailRecM[A, B](a: A)(f: A => Map[K, Either[A, B]]): Map[K, B] = {
val bldr = Map.newBuilder[K, B]

Expand All @@ -71,23 +57,6 @@ trait MapInstances extends cats.kernel.instances.MapInstances {
f(a).foreach { case (k, a) => descend(k, a) }
bldr.result
}

override def size[A](fa: Map[K, A]): Long = fa.size.toLong

override def get[A](fa: Map[K, A])(idx: Long): Option[A] =
if (idx < 0L || Int.MaxValue < idx) None
else {
val n = idx.toInt
if (n >= fa.size) None
else Some(fa.valuesIterator.drop(n).next)
}

override def isEmpty[A](fa: Map[K, A]): Boolean = fa.isEmpty

override def fold[A](fa: Map[K, A])(implicit A: Monoid[A]): A =
A.combineAll(fa.values)

override def toList[A](fa: Map[K, A]): List[A] = fa.values.toList
}
// scalastyle:on method.length
}
2 changes: 2 additions & 0 deletions core/src/main/scala/cats/instances/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ package object instances {
object queue extends QueueInstances
object set extends SetInstances
object short extends ShortInstances
object sortedMap extends SortedMapInstances
object sortedSet extends SortedSetInstances
object stream extends StreamInstances
object string extends StringInstances
object try_ extends TryInstances
Expand Down
43 changes: 2 additions & 41 deletions core/src/main/scala/cats/instances/set.scala
Original file line number Diff line number Diff line change
@@ -1,56 +1,17 @@
package cats
package instances

import scala.annotation.tailrec

import cats.syntax.show._

trait SetInstances extends cats.kernel.instances.SetInstances {

implicit val catsStdInstancesForSet: Foldable[Set] with MonoidK[Set] =
new Foldable[Set] with MonoidK[Set] {
implicit val catsStdInstancesForSet: MonoidK[Set] =
new MonoidK[Set] {

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

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

def foldLeft[A, B](fa: Set[A], b: B)(f: (B, A) => B): B =
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concern here about adding a law that this violates. Maybe a == b implies toList(a) == toList(b)

Copy link
Member Author

Choose a reason for hiding this comment

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

We're going to need to spend some more time to find something. That one won't work, because that way e.g
Either.Left(123) =!= Either.Left(-30) would imply List() =!= List()

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if it's the best solution but I worked around it, by only considering the positive case. I.e a == b implies toList(a) == toList(b) but not a != b implies toList(a) != toList(b)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I meant (which is why I didn’t say (a == b) == (a.toList == b.toList))

fa.foldLeft(b)(f)

def foldRight[A, B](fa: Set[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
Foldable.iterateRight(fa, lb)(f)

override def get[A](fa: Set[A])(idx: Long): Option[A] = {
@tailrec
def go(idx: Int, it: Iterator[A]): Option[A] = {
if (it.hasNext) {
if (idx == 0) Some(it.next) else {
it.next
go(idx - 1, it)
}
} else None
}
if (idx < Int.MaxValue && idx >= 0L) go(idx.toInt, fa.toIterator) else None
}

override def size[A](fa: Set[A]): Long = fa.size.toLong

override def exists[A](fa: Set[A])(p: A => Boolean): Boolean =
fa.exists(p)

override def forall[A](fa: Set[A])(p: A => Boolean): Boolean =
fa.forall(p)

override def isEmpty[A](fa: Set[A]): Boolean = fa.isEmpty

override def fold[A](fa: Set[A])(implicit A: Monoid[A]): A = A.combineAll(fa)

override def toList[A](fa: Set[A]): List[A] = fa.toList

override def reduceLeftOption[A](fa: Set[A])(f: (A, A) => A): Option[A] =
fa.reduceLeftOption(f)

override def find[A](fa: Set[A])(f: A => Boolean): Option[A] = fa.find(f)
}

implicit def catsStdShowForSet[A:Show]: Show[Set[A]] = new Show[Set[A]] {
Expand Down
Loading