From 84560b1e2d694b6253da188d17fa37619496339a Mon Sep 17 00:00:00 2001 From: Mike Curry Date: Sun, 17 Jul 2016 21:34:21 +0100 Subject: [PATCH 1/3] Make NonEmptyVector a value class. Rename variant of concat taking a Vector to concatVector. Updates test. Adds additional consistency test. --- .../main/scala/cats/data/NonEmptyVector.scala | 20 +++++++++---------- .../cats/tests/NonEmptyVectorTests.scala | 12 ++++++++--- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/core/src/main/scala/cats/data/NonEmptyVector.scala b/core/src/main/scala/cats/data/NonEmptyVector.scala index a9fe0077dc..05662f7dfa 100644 --- a/core/src/main/scala/cats/data/NonEmptyVector.scala +++ b/core/src/main/scala/cats/data/NonEmptyVector.scala @@ -6,9 +6,9 @@ import scala.collection.immutable.VectorBuilder import cats.instances.vector._ /** - * A data type which represents a non empty Vector. + * A data type which represents a `Vector` guaranteed to contain at least one element. */ -final case class NonEmptyVector[A] private (toVector: Vector[A]) { +final case class NonEmptyVector[A](val toVector: Vector[A]) extends AnyVal { /** Gets the element at the index, if it exists */ def get(i: Int): Option[A] = @@ -38,27 +38,27 @@ final case class NonEmptyVector[A] private (toVector: Vector[A]) { def filter(f: A => Boolean): Vector[A] = toVector.filter(f) /** - * Append another NonEmptyVector to this + * Append another `NonEmptyVector` to this, producing a new `NonEmptyVector`. */ def concat(other: NonEmptyVector[A]): NonEmptyVector[A] = NonEmptyVector(toVector ++ other.toVector) /** - * Alias for concat + * Alias for [[concat]] */ def ++(other: NonEmptyVector[A]): NonEmptyVector[A] = concat(other) /** - * Append another Vector to this + * Append another `Vector` to this, producing a new `NonEmptyVector`. */ - def concat(other: Vector[A]): NonEmptyVector[A] = NonEmptyVector(toVector ++ other) + def concatVector(other: Vector[A]): NonEmptyVector[A] = NonEmptyVector(toVector ++ other) /** - * Alias for concat + * Alias for [[concatVector]] */ - def ++(other: Vector[A]): NonEmptyVector[A] = concat(other) + def +++(other: Vector[A]): NonEmptyVector[A] = concatVector(other) /** - * find the first element matching the predicate, if one exists + * Find the first element matching the predicate, if one exists */ def find(f: A => Boolean): Option[A] = toVector.find(f) @@ -190,7 +190,7 @@ private[data] sealed trait NonEmptyVectorInstances { case Some(t) => go(t) case None => () } - case Xor.Left(a) => go(f(a).concat(v.tail)) + case Xor.Left(a) => go(f(a).concatVector(v.tail)) } go(f(a)) NonEmptyVector.fromVectorUnsafe(buf.result()) diff --git a/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala b/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala index 7710ac5bf8..dbb148d75f 100644 --- a/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala +++ b/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala @@ -55,7 +55,7 @@ class NonEmptyVectorTests extends CatsSuite { nonEmptyVector.size should === (nonEmptyVector.toList.size.toLong) } } - + test("Show is not empty and is formatted as expected") { forAll { (nonEmptyVector: NonEmptyVector[Int]) => @@ -175,9 +175,15 @@ class NonEmptyVectorTests extends CatsSuite { } } - test("++ Vector is consistent with concat") { + test("+++ Vector is consistent with concatVector") { forAll { (nonEmptyVector: NonEmptyVector[Int], vector: Vector[Int]) => - nonEmptyVector ++ vector should === (nonEmptyVector.concat(vector)) + nonEmptyVector +++ vector should === (nonEmptyVector.concatVector(vector)) + } + } + + test("+++ Vector is consistent with ++ NonEmptyVector") { + forAll { (nonEmptyVector: NonEmptyVector[Int], other: NonEmptyVector[Int]) => + nonEmptyVector +++ other.toVector should === (nonEmptyVector ++ other) } } From 046216f0ac44f7b777144a6dba405592798f572a Mon Sep 17 00:00:00 2001 From: Mike Curry Date: Sat, 23 Jul 2016 18:09:00 +0100 Subject: [PATCH 2/3] Make NonEmptyVector a value class, add tests, add note on scala 2.10 limitation --- build.sbt | 4 +- .../main/scala/cats/data/NonEmptyVector.scala | 39 ++++++++------- .../cats/tests/NonEmptyVectorTests.scala | 50 +++++++++++++++---- 3 files changed, 62 insertions(+), 31 deletions(-) diff --git a/build.sbt b/build.sbt index 2f8ceef46d..dfc33646b6 100644 --- a/build.sbt +++ b/build.sbt @@ -112,7 +112,7 @@ lazy val disciplineDependencies = Seq( lazy val testingDependencies = Seq( libraryDependencies += "org.typelevel" %%% "catalysts-platform" % "0.0.2", libraryDependencies += "org.typelevel" %%% "catalysts-macros" % "0.0.2" % "test", - libraryDependencies += "org.scalatest" %%% "scalatest" % "3.0.0-M7" % "test") + libraryDependencies += "org.scalatest" %%% "scalatest" % "3.0.0-M8" % "test") /** @@ -208,7 +208,7 @@ lazy val kernel = crossProject.crossType(CrossType.Pure) .jsSettings(commonJsSettings:_*) .jvmSettings((commonJvmSettings ++ (mimaPreviousArtifacts := Set("org.typelevel" %% "cats-kernel" % "0.6.0"))):_*) -lazy val kernelJVM = kernel.jvm +lazy val kernelJVM = kernel.jvm lazy val kernelJS = kernel.js lazy val kernelLaws = crossProject.crossType(CrossType.Pure) diff --git a/core/src/main/scala/cats/data/NonEmptyVector.scala b/core/src/main/scala/cats/data/NonEmptyVector.scala index 05662f7dfa..dcc7f4fb98 100644 --- a/core/src/main/scala/cats/data/NonEmptyVector.scala +++ b/core/src/main/scala/cats/data/NonEmptyVector.scala @@ -7,8 +7,12 @@ import cats.instances.vector._ /** * A data type which represents a `Vector` guaranteed to contain at least one element. + *
+ * Note that the constructor is `private` to prevent accidental construction of an empty + * `NonEmptyVector`. However, due to https://issues.scala-lang.org/browse/SI-6601, on + * Scala 2.10, this may be bypassed due to a compiler bug. */ -final case class NonEmptyVector[A](val toVector: Vector[A]) extends AnyVal { +final class NonEmptyVector[A] private (val toVector: Vector[A]) extends AnyVal { /** Gets the element at the index, if it exists */ def get(i: Int): Option[A] = @@ -19,14 +23,14 @@ final case class NonEmptyVector[A](val toVector: Vector[A]) extends AnyVal { /** Updates the element at the index, if it exists */ def updated(i: Int, a: A): Option[NonEmptyVector[A]] = - if (toVector.isDefinedAt(i)) Some(NonEmptyVector(toVector.updated(i, a))) else None + if (toVector.isDefinedAt(i)) Some(new NonEmptyVector(toVector.updated(i, a))) else None /** * Updates the element at the index, or throws an `IndexOutOfBoundsException` * if none exists (if `i` does not satisfy `0 <= i < length`). */ def updatedUnsafe(i: Int, a: A): - NonEmptyVector[A] = NonEmptyVector(toVector.updated(i, a)) + NonEmptyVector[A] = new NonEmptyVector(toVector.updated(i, a)) def head: A = toVector.head @@ -37,25 +41,20 @@ final case class NonEmptyVector[A](val toVector: Vector[A]) extends AnyVal { */ def filter(f: A => Boolean): Vector[A] = toVector.filter(f) - /** - * Append another `NonEmptyVector` to this, producing a new `NonEmptyVector`. - */ - def concat(other: NonEmptyVector[A]): NonEmptyVector[A] = NonEmptyVector(toVector ++ other.toVector) - /** * Alias for [[concat]] */ - def ++(other: NonEmptyVector[A]): NonEmptyVector[A] = concat(other) + def ++(other: Vector[A]): NonEmptyVector[A] = concat(other) /** * Append another `Vector` to this, producing a new `NonEmptyVector`. */ - def concatVector(other: Vector[A]): NonEmptyVector[A] = NonEmptyVector(toVector ++ other) + def concat(other: Vector[A]): NonEmptyVector[A] = new NonEmptyVector(toVector ++ other) /** - * Alias for [[concatVector]] + * Append another `NonEmptyVector` to this, producing a new `NonEmptyVector`. */ - def +++(other: Vector[A]): NonEmptyVector[A] = concatVector(other) + def concatNEV(other: NonEmptyVector[A]): NonEmptyVector[A] = new NonEmptyVector(toVector ++ other.toVector) /** * Find the first element matching the predicate, if one exists @@ -88,13 +87,13 @@ final case class NonEmptyVector[A](val toVector: Vector[A]) extends AnyVal { * Applies f to all the elements */ def map[B](f: A => B): NonEmptyVector[B] = - NonEmptyVector(toVector.map(f)) + new NonEmptyVector(toVector.map(f)) /** * Applies f to all elements and combines the result */ def flatMap[B](f: A => NonEmptyVector[B]): NonEmptyVector[B] = - NonEmptyVector(toVector.flatMap(a => f(a).toVector)) + new NonEmptyVector(toVector.flatMap(a => f(a).toVector)) /** * Left-associative reduce using f. @@ -129,6 +128,8 @@ final case class NonEmptyVector[A](val toVector: Vector[A]) extends AnyVal { s"NonEmpty${Show[Vector[A]].show(toVector)}" def length: Int = toVector.length + + override def toString: String = s"NonEmpty${toVector.toString}" } private[data] sealed trait NonEmptyVectorInstances { @@ -139,7 +140,7 @@ private[data] sealed trait NonEmptyVectorInstances { with Comonad[NonEmptyVector] with Traverse[NonEmptyVector] with MonadRec[NonEmptyVector] { def combineK[A](a: NonEmptyVector[A], b: NonEmptyVector[A]): NonEmptyVector[A] = - a concat b + a concatNEV b override def split[A](fa: NonEmptyVector[A]): (A, Vector[A]) = (fa.head, fa.tail) @@ -190,7 +191,7 @@ private[data] sealed trait NonEmptyVectorInstances { case Some(t) => go(t) case None => () } - case Xor.Left(a) => go(f(a).concatVector(v.tail)) + case Xor.Left(a) => go(f(a).concat(v.tail)) } go(f(a)) NonEmptyVector.fromVectorUnsafe(buf.result()) @@ -213,19 +214,19 @@ private[data] sealed trait NonEmptyVectorInstances { object NonEmptyVector extends NonEmptyVectorInstances { def apply[A](head: A, tail: Vector[A]): NonEmptyVector[A] = - NonEmptyVector(head +: tail) + new NonEmptyVector(head +: tail) def apply[A](head: A, tail: A*): NonEmptyVector[A] = { val buf = Vector.newBuilder[A] buf += head tail.foreach(buf += _) - NonEmptyVector(buf.result) + new NonEmptyVector(buf.result) } def fromVector[A](vector: Vector[A]): Option[NonEmptyVector[A]] = if (vector.isEmpty) None else Some(new NonEmptyVector(vector)) def fromVectorUnsafe[A](vector: Vector[A]): NonEmptyVector[A] = - if (vector.nonEmpty) NonEmptyVector(vector) + if (vector.nonEmpty) new NonEmptyVector(vector) else throw new IllegalArgumentException("Cannot create NonEmptyVector from empty vector") } diff --git a/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala b/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala index dbb148d75f..c74fec1669 100644 --- a/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala +++ b/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala @@ -7,6 +7,8 @@ import cats.data.NonEmptyVector import cats.laws.discipline.{ComonadTests, SemigroupKTests, FoldableTests, SerializableTests, TraverseTests, ReducibleTests, MonadRecTests} import cats.laws.discipline.arbitrary._ +import scala.util.Properties + class NonEmptyVectorTests extends CatsSuite { // Lots of collections here.. telling ScalaCheck to calm down a bit implicit override val generatorDrivenConfig: PropertyCheckConfiguration = @@ -169,21 +171,15 @@ class NonEmptyVectorTests extends CatsSuite { } } - test("++ NonEmptyVector is consistent with concat") { + test("++ Vector is consistent with concatNEV") { forAll { (nonEmptyVector: NonEmptyVector[Int], other: NonEmptyVector[Int]) => - nonEmptyVector ++ other should === (nonEmptyVector.concat(other)) + nonEmptyVector ++ other.toVector should === (nonEmptyVector.concatNEV(other)) } } - test("+++ Vector is consistent with concatVector") { + test("++ Vector is consistent with concat") { forAll { (nonEmptyVector: NonEmptyVector[Int], vector: Vector[Int]) => - nonEmptyVector +++ vector should === (nonEmptyVector.concatVector(vector)) - } - } - - test("+++ Vector is consistent with ++ NonEmptyVector") { - forAll { (nonEmptyVector: NonEmptyVector[Int], other: NonEmptyVector[Int]) => - nonEmptyVector +++ other.toVector should === (nonEmptyVector ++ other) + nonEmptyVector ++ vector should === (nonEmptyVector.concat(vector)) } } @@ -224,4 +220,38 @@ class NonEmptyVectorTests extends CatsSuite { } } } + + test("NonEmptyVector#hashCode consistent with Vector#hashCode") { + forAll { (nonEmptyVector: NonEmptyVector[Int]) => + nonEmptyVector.hashCode should === (nonEmptyVector.toVector.hashCode) + } + } + + test("NonEmptyVector#equals consistent with Vector#equals") { + forAll { (lhs: NonEmptyVector[Int], rhs: NonEmptyVector[Int]) => + lhs.equals(rhs) should === (lhs.toVector.equals(rhs.toVector)) + } + } + + test("NonEmptyVector#toString produces correct output") { + forAll { (nonEmptyVector: NonEmptyVector[Int]) => + nonEmptyVector.toString should === (s"NonEmpty${nonEmptyVector.toVector.toString}") + } + NonEmptyVector(1, Vector.empty).toString should === ("NonEmptyVector(1)") + NonEmptyVector(1, Vector.empty).toVector.toString should === ("Vector(1)") + } + + test("Cannot create a new NonEmptyVector from constructor") { + if (!Properties.versionNumberString.startsWith("2.10")) { + // A bug in scala 2.10 allows private constructors to be accessed. + // We should still ensure that on scala 2.11 and up we cannot construct the + // object directly. see: https://issues.scala-lang.org/browse/SI-6601 + "val bad: NonEmptyVector[Int] = new NonEmptyVector(Vector(1))" shouldNot compile + } + } + + test("Cannot create a new NonEmptyVector from apply with an empty vector") { + "val bad: NonEmptyVector[Int] = NonEmptyVector(Vector(1))" shouldNot compile + } + } From a5dbc18bbe04fcab8d2738fb17907c5c5cc0b00b Mon Sep 17 00:00:00 2001 From: Mike Curry Date: Sat, 23 Jul 2016 19:51:36 +0100 Subject: [PATCH 3/3] Altered test to detect whether the platform is jvm prior to checking the scala version --- .../scala/cats/tests/NonEmptyVectorTests.scala | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala b/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala index c74fec1669..482852fa69 100644 --- a/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala +++ b/tests/src/test/scala/cats/tests/NonEmptyVectorTests.scala @@ -1,6 +1,8 @@ package cats package tests +import catalysts.Platform + import cats.kernel.laws.{GroupLaws, OrderLaws} import cats.data.NonEmptyVector @@ -242,11 +244,13 @@ class NonEmptyVectorTests extends CatsSuite { } test("Cannot create a new NonEmptyVector from constructor") { - if (!Properties.versionNumberString.startsWith("2.10")) { - // A bug in scala 2.10 allows private constructors to be accessed. - // We should still ensure that on scala 2.11 and up we cannot construct the - // object directly. see: https://issues.scala-lang.org/browse/SI-6601 - "val bad: NonEmptyVector[Int] = new NonEmptyVector(Vector(1))" shouldNot compile + if(Platform.isJvm) { + if (!Properties.versionNumberString.startsWith("2.10")) { + // A bug in scala 2.10 allows private constructors to be accessed. + // We should still ensure that on scala 2.11 and up we cannot construct the + // object directly. see: https://issues.scala-lang.org/browse/SI-6601 + "val bad: NonEmptyVector[Int] = new NonEmptyVector(Vector(1))" shouldNot compile + } } }