Treat Stream and LazyList as different types #2983

travisbrown · 2019-08-09T12:16:13Z

~~A quick implementation of the second proposal in #2982.~~

This became a much deeper fix for #2982 after conversation and a vote in #2991. In the initial version it only removed the aliasing in instances, but now it removes the aliasing altogether.

…List on 2.13

travisbrown · 2019-08-09T12:17:33Z

core/src/main/scala-2.13+/cats/instances/lazyList.scala

-            case Left(a) => Some((None, fn(a).iterator ++ it))
-            case Right(b) => Some((Some(b), it))
-          }
+          else


This is Scalafmt, which doesn't seem to be running for 2.13-specific code?

codecov-io · 2019-08-09T12:46:21Z

Codecov Report

Merging #2983 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   93.83%   93.75%   -0.08%     
==========================================
  Files         369      366       -3     
  Lines        7074     6922     -152     
  Branches      189      179      -10     
==========================================
- Hits         6638     6490     -148     
+ Misses        436      432       -4

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/package.scala	`88.88% <ø> (-2.03%)`	⬇️
core/src/main/scala/cats/data/OneAnd.scala	`95.45% <ø> (-0.5%)`	⬇️
core/src/main/scala/cats/instances/parallel.scala	`100% <ø> (ø)`	⬆️
...rc/main/scala/cats/laws/discipline/arbitrary.scala	`99.14% <ø> (-0.01%)`	⬇️
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/instances/sortedSet.scala	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48c8b34...6a1c334. Read the comment docs.

kailuowang · 2019-08-09T13:40:30Z

core/src/main/scala-2.13+/cats/instances/package.scala

@@ -0,0 +1,62 @@
+package cats
+
+package object instances {


Can we use an abstract class for objects from both scala specific versions to inherit from and remove the duplication?

Not without dropping in some lazy vals. Here's a minimization:

package foo package object bar extends BarPackage package bar { abstract class BarPackage { object baz extends Baz } trait Baz { val x = foo.bar.baz } }

Referring to baz will stack overflow.

kailuowang · 2019-08-09T13:44:47Z

core/src/main/scala/cats/data/OneAnd.scala

@@ -209,7 +209,7 @@ sealed abstract private[data] class OneAndLowPriority4 {
        fa.head

      def map[A, B](fa: OneAnd[LazyList, A])(f: A => B): OneAnd[LazyList, B] =
-        fa.map(f)
+        fa.map(f)(crossVersionInstancesForLazyList)


so for a user who needs to cross compile, they either have to create this crossVersionInstancesForLazyList themselves or have to write version specific code for every import cats.instances.stream._ they have in the past?

I have a suggestion in my comment below.

kailuowang · 2019-08-09T13:53:39Z

I am bit concerned with the ergonomics for cross compiling users who migrated away from Stream on 2.13.
Now that we have version specific cats.instances and cats.kernel.instances why not provide a
lazyListXStream (or a better name) that simply points to StreamInstances on 2.12- and LazyInstances on 2.13? That way the users mentioned above can simply replace import cats.intances.stream with import cats.instance.lazyListXStream._ in their codebase?

travisbrown · 2019-08-09T14:09:31Z

@kailuowang Personally I think the alias is a bad idea in general. Stream and LazyList are different types with different semantics, and pretending they're the same is likely to cause subtle and annoying problems. I'm not going to use the alias in any code I maintain, and I think it's a mistake to use it here.

I've only included this crossVersionInstancesForLazyList stuff because it doesn't seem like there's any will to make a more complete separation of the types in the implementation of Cats itself. That's fine, I guess—I don't really care that much. I don't think something like this should be included in the public API, but if other people really want a cats.instance.lazyListXStream I can add it.

kailuowang · 2019-08-09T15:06:23Z

If you can make all Stream dependent code version specific, then we don't need the type alias. The approach before this PR was that alias was used as much as possible to support cross compiling except where it can't, such as LazyList instances in cats.core or NonEmptyStream, in which case version specific code was used.
This PR made more Stream dependent code (namely stream instance in kernel) version specific, but still not all of them, hence we still need the alias and the inconsistency.

I'd argue that we either leave as is, or we just go ahead and really make ALL Stream dependent code version specific and remove the need for type alias all together.

Either way, I think the crossVersionInstancesForLazyList and crossVersionEqForLazyList is awkward even as an internal API in Cats.
cats.instance.lazyListXStream would be better. I don't strongly believe it should be public either, but at the very least we should document to let users how to create their own cats.instance.lazyListXStream.

kailuowang · 2019-08-09T15:56:49Z

One more thing, if we make all Stream dependent code version specific we then add back the stream instances on 2.13 and have them deprecated.

travisbrown · 2019-08-15T09:54:00Z

@kailuowang

This PR made more Stream dependent code (namely stream instance in kernel) version specific, but still not all of them, hence we still need the alias and the inconsistency.

My goal was to remove any public conflation of the two types. My position is that I think conflating the two is a mistake, but as long as Cats doesn't force that decision on me I don't really care about the implementation.

I'd argue that we either leave as is, or we just go ahead and really make ALL Stream dependent code version specific and remove the need for type alias all together.

How should we decide on this?

kailuowang · 2019-08-15T11:31:16Z

But there is still public conflation (e.g. ZipStream and parallel instance for stream. ), There doesn't seem to be a lot left with the changes from this PR, completely removing the conflation with version specific code seems a reasonable effort.
Adding stream depending API deprecated back to 2.13 also becomes possible.
The trade off here is code duplication, hard to discover code duplication.

We can hold a vote among the maintainers, because if we chose duplication it will be maintainers burden.

travisbrown · 2019-08-15T11:42:13Z

Okay! How do we call for a vote? I'm happy to make the remaining changes if we decide to go that route.

kailuowang · 2019-08-15T14:05:47Z

Issue created for vote
#2991

travisbrown · 2019-08-20T11:31:25Z

build.sbt

@@ -523,6 +523,7 @@ lazy val kernelLaws = crossProject(JSPlatform, JVMPlatform)
  .settings(scoverageSettings)
  .settings(disciplineDependencies)
  .settings(testingDependencies)
+  .settings(scalacOptions in Test := (scalacOptions in Test).value.filter(_ != "-Xfatal-warnings"))


This is necessary because we now have deprecation warnings for Stream on 2.13.

travisbrown · 2019-08-20T11:46:52Z

core/src/main/scala-2.13+/cats/data/ScalaVersionSpecificPackage.scala

+  type NonEmptyStream[A] = OneAnd[Stream, A]
+
+  @deprecated("2.0.0-RC2", "Use NonEmptyLazyList")
+  def NonEmptyStream[A](head: A, tail: Stream[A]): NonEmptyStream[A] =


Note that we can't have the default argument here that we have on 2.11 and 2.12, since it results in a deprecation warning (which is clearly a compiler bug).

travisbrown · 2019-08-20T11:54:24Z

This PR is becoming a nightmare, but I think it's ready for review.

Some notes:

All conflation of Stream and LazyList is removed (no more type alias, toLazyList helpers, etc.).
All Stream instances are still available on 2.13, but are deprecated.
The cats.instances.stream object itself is also deprecated, but cats.kernel.instances.stream is not, because it's a package object, and package objects can't be deprecated (this is a particularly fun interaction of our pervasive inconsistency and general Scala brokenness).
All Stream-related tests are still around on 2.13.
Everything that exists for Stream (instances, tests, NonEmptyStream, etc.) has a LazyList equivalent on 2.13.
In some cases 2.11 bincompat requires a horrifying amount of code duplication (e.g. if we want to deprecate catsStdParallelForZipStream I believe we have to duplicate most of parallel.scala).
Comparison and equality for SortedSet was previously implemented via conversions to Stream / LazyList. I've reimplemented them in terms of StaticMethods.iteratorCompare / iteratorEq, which is equivalent and in my view a little clearer, anyway (and definitely more efficient).

There is one difference between Stream-related APIs on 2.13 and earlier versions: the NonEmptyStream constructor no longer includes a default argument for tail:

@deprecated("2.0.0-RC2", "Use NonEmptyLazyList")
   def NonEmptyStream[A](head: A, tail: Stream[A]): NonEmptyStream[A] = ...

While on 2.11-12 we have:

   def NonEmptyStream[A](head: A, tail: Stream[A] = Stream.empty): NonEmptyStream[A] = ...

I removed the default argument on 2.13 because it results in a deprecation warning, which is a compiler bug—it clearly shouldn't warn. We could alternatively leave the default argument and use the silencer scalac plugin to ignore this one (spurious) warning, but that seemed like overkill for a deprecated method where we have no bincompat / source compat obligations.

kailuowang · 2019-08-20T14:20:26Z

core/src/main/scala-2.12-/cats/instances/stream.scala

-trait LazyListInstances extends StreamInstances with StreamInstancesBinCompat0 {
-  val catsStdInstancesForLazyList = catsStdInstancesForStream
-}
+private[instances] trait LazyListInstances


Is this still needed?

If we don't want two copies of cats/instances/all.scala we do need it.

(I guess you could argue that we have two copies of everything else at this point so we might as well do it here.)

didn't you already add version specific copies for cats/instances/package.scala?

Yes, but the definition of AllInstances in all.scala also needs to bring in the LazyList instances on 2.13.

Ah i see. I didn't see you were able to deprecate all the methods in StreamInstance to avoid deprecating the trait.
Since it confused me a bit, I think we might just as well create two copies for it so that hopefully we can get rid off all Stream/LazyList related ScalaVersionSpecific stuff in non test code?

Sure, I can go ahead and do this.

kailuowang · 2019-08-20T14:25:49Z

core/src/main/scala-2.13+/cats/instances/ScalaVersionSpecificParallelInstances.scala

+import cats.kernel.Semigroup
+import cats.syntax.either._
+import cats.{~>, Applicative, Apply, FlatMap, Functor, Monad, NonEmptyParallel, Parallel}
+import kernel.compat.scalaVersionSpecific._


In all version specific code (i.e. not being cross compiled), you shouldn't need to import a compat.scalaVersionSpecific._ right? which also means that you shouldn't need the @suppressUnusedImportWarningForScalaVersionSpecific below as well.

I think you're right. Let me try removing all of these in the version-specific source trees.

In some cases we might still want to use some of this stuff to minimize the diff between version-specific files? Like IterableOnce? I'm not sure—let me try it.

Thanks for catching that, @kailuowang—there were only a couple of place we were using IterableOnce or lazyZip in version-specific code. Just pushed a commit removing scalaVersionSpecific in all version-specific code (except where it's defined).

This reverts commit bf10981.

travisbrown · 2019-08-21T12:48:24Z

Okay, after a bunch of slow experiments today, here's what I've figured out about the CI failure:

Changing the Travis CI JDK to OpenJDK fixes the issue (see Separate Stream and LazyList #2998).
Restarting, clearing the cache, rebasing commits, etc. doesn't fix the issue.
I can't reproduce the issue locally (on any JDK or on either Linux or OS X).
I can't make any sense of how the changes since 67dc634 (which was green) could cause this.

kailuowang · 2019-08-21T14:38:53Z

Thanks for the trouble shooting and update.
I vote we just change the jdk in this PR and call it a day.

This reverts commit 9b28588.

travisbrown · 2019-08-21T15:18:19Z

@kailuowang Sounds reasonable. I've just pushed the change. I've also dropped the explicit dist config—I'm assuming it was only there because oraclejdk8 isn't available on the default and more recent options.

LukaJCB · 2019-08-21T15:26:32Z

Thanks for this monster of an effort 👍

kailuowang · 2019-08-22T00:22:54Z

tests/src/test/scala/cats/tests/ParallelSuite.scala

@@ -303,22 +303,6 @@ class ParallelSuite extends CatsSuite with ApplicativeErrorForEitherTest {
    }
  }

-  test("ParMap over Stream should be consistent with zip") {


I couldn't find the Stream version of this test any more?

Oh, ouch, that's my fault. Fixed now.

Well, not quite, one second…

kailuowang · 2019-08-22T00:23:24Z

tests/src/test/scala/cats/tests/ParallelSuite.scala

@@ -343,12 +327,6 @@ class ParallelSuite extends CatsSuite with ApplicativeErrorForEitherTest {
    }
  }

-  test("ParTupled of Stream should be consistent with ParMap of Tuple.apply") {


I couldn't find the Stream version of this test any more?

kailuowang · 2019-08-22T00:23:42Z

tests/src/test/scala/cats/tests/ParallelSuite.scala

@@ -361,12 +339,6 @@ class ParallelSuite extends CatsSuite with ApplicativeErrorForEitherTest {
    }
  }

-  test("ParTupled of Stream should be consistent with zip") {


I couldn't find the Stream version of this test any more?

kailuowang · 2019-08-22T00:24:37Z

tests/src/test/scala/cats/tests/ParallelSuite.scala

@@ -445,8 +417,6 @@ class ParallelSuite extends CatsSuite with ApplicativeErrorForEitherTest {
  checkAll("NonEmptyParallel[Vector, ZipVector]",
           NonEmptyParallelTests[Vector, ZipVector].nonEmptyParallel[Int, String])
  checkAll("NonEmptyParallel[List, ZipList]", NonEmptyParallelTests[List, ZipList].nonEmptyParallel[Int, String])
-  // Can't test Parallel here, as Applicative[ZipStream].pure doesn't terminate
-  checkAll("Parallel[Stream, ZipStream]", NonEmptyParallelTests[LazyList, ZipStream].nonEmptyParallel[Int, String])


I couldn't find the Stream version of this test any more?

kailuowang · 2019-08-22T00:27:13Z

tests/src/test/scala/cats/tests/RegressionSuite.scala

@@ -115,7 +115,7 @@ class RegressionSuite extends CatsSuite {
    // shouldn't have ever evaluted validate(8)
    checkAndResetCount(3)

-    LazyList(1, 2, 6, 8).traverse(validate) should ===(Either.left("6 is greater than 5"))
+    Stream(1, 2, 6, 8).traverse(validate) should ===(Either.left("6 is greater than 5"))


I don't know much of these regression tests, but are we sure that LazyList doesn't need them (couldn't find the LazyList version)?

I omitted these intentionally, but looking more closely I'm surprised the property being tested isn't covered by the Traverse laws, and we probably do want it for LazyList. Will add now.

kailuowang

LGTM thanks again!

travisbrown · 2019-08-22T15:49:56Z

Thanks for the thorough review!

kubukoz · 2019-08-22T15:50:21Z

Remove cats.instances.stream and introduce cats.kernel.instances.lazy…

4edcd9e

…List on 2.13

travisbrown commented Aug 9, 2019

View reviewed changes

LukaJCB self-requested a review August 9, 2019 12:52

kailuowang reviewed Aug 9, 2019

View reviewed changes

Merge branch 'master' into topic/stream-instances

73c389f

kailuowang mentioned this pull request Aug 15, 2019

[Vote] How to handle Stream -> LazyList migration #2991

Closed

travisbrown added 4 commits August 19, 2019 02:58

Merge branch 'master' into topic/stream-instances

dd79248

Reinstate stream instances on 2.13

d843f04

Don't alias LazyList to Stream pre-2.13

6f8658b

Fix duplicate test names

67dc634

travisbrown commented Aug 20, 2019

View reviewed changes

kubukoz previously approved these changes Aug 20, 2019

View reviewed changes

travisbrown commented Aug 20, 2019

View reviewed changes

LukaJCB previously approved these changes Aug 20, 2019

View reviewed changes

travisbrown changed the title ~~Remove cats.instances.stream, introduce cats.kernel.instances.lazyList on 2.13~~ Treat Stream and LazyList as different types Aug 20, 2019

kailuowang reviewed Aug 20, 2019

View reviewed changes

Don't use scalaVersionSpecific helpers in version-specific code

40333b2

travisbrown dismissed LukaJCB’s stale review via 40333b2 August 20, 2019 14:39

travisbrown mentioned this pull request Aug 21, 2019

Separate Stream and LazyList #2997

Closed

travisbrown added 3 commits August 21, 2019 04:42

Desperate attempt to fix Travis CI weirdness

bf10981

Revert "Desperate attempt to fix Travis CI weirdness"

ae7b6e5

This reverts commit bf10981.

Another desperate attempt

9b28588

travisbrown mentioned this pull request Aug 21, 2019

Separate Stream and LazyList #2998

Closed

travisbrown added 2 commits August 21, 2019 17:07

Revert "Another desperate attempt"

f80d9ab

This reverts commit 9b28588.

Switch to OpenJDK 8

fd972c5

LukaJCB previously approved these changes Aug 21, 2019

View reviewed changes

kailuowang reviewed Aug 22, 2019

View reviewed changes

kailuowang mentioned this pull request Aug 22, 2019

optimize chain's get. #2999

Merged

Reinstate Stream tests

7e52796

travisbrown dismissed LukaJCB’s stale review via 7e52796 August 22, 2019 07:02

travisbrown added 2 commits August 22, 2019 09:09

...with the right types this time

a2cdc6b

Add regression tests for typelevel#513 for LazyList on 2.13

6a1c334

kailuowang approved these changes Aug 22, 2019

View reviewed changes

LukaJCB approved these changes Aug 22, 2019

View reviewed changes

travisbrown merged commit 8c031e5 into typelevel:master Aug 22, 2019

travisbrown mentioned this pull request Aug 23, 2019

We need to talk about Stream #2982

Closed

travisbrown added this to the 2.0-RC2 milestone Aug 26, 2019

travisbrown added the Source Breaking label Aug 26, 2019

Treat Stream and LazyList as different types #2983

Treat Stream and LazyList as different types #2983

Conversation

travisbrown commented Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Aug 9, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

kailuowang commented Aug 9, 2019

travisbrown commented Aug 9, 2019

kailuowang commented Aug 9, 2019 • edited Loading

kailuowang commented Aug 9, 2019

travisbrown commented Aug 15, 2019

kailuowang commented Aug 15, 2019 • edited Loading

travisbrown commented Aug 15, 2019

kailuowang commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbrown commented Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbrown commented Aug 21, 2019

kailuowang commented Aug 21, 2019

travisbrown commented Aug 21, 2019

LukaJCB commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang left a comment

Choose a reason for hiding this comment

travisbrown commented Aug 22, 2019

kubukoz commented Aug 22, 2019

travisbrown commented Aug 9, 2019 •

edited

Loading

codecov-io commented Aug 9, 2019 •

edited

Loading

kailuowang Aug 9, 2019 •

edited

Loading

kailuowang commented Aug 9, 2019 •

edited

Loading

kailuowang commented Aug 15, 2019 •

edited

Loading

travisbrown commented Aug 20, 2019 •

edited

Loading

kailuowang Aug 20, 2019 •

edited

Loading

kailuowang Aug 22, 2019 •

edited

Loading