FreeC: make the F parameter covariant #1730

diesalbla · 2020-01-05T13:39:41Z

Follows on the early work on #1615.

In the internal implementation of streams, we change the definition of the FreeC[F[_], +O, +R] generalised abstract data type (GADT), to turn the F into a covariant parameter of higuer-kind, that is, FreeC[ + F[_], +O, +R ].

Pure subclasses: those subclasses of FreeC that do not involve F, such as the Result and the Output, can be specialised with F = Pure (an alias for Nothing of the F kind). We can then integrate Result into the FreeC GADT, and drop asFreeC.
The classes Pull and Stream no longer need to resort to the Nothing parameter and use type-casts. Instead, the type-parameters of Stream and FreeC are no aligned. As a consequence, we will no longer need to use the covary methods of Stream or Pull.

Broken edge for Scala 2.12.

When we made the first changes, we noticed that pattern-matching on the algebra classes Step and OpenScope was causing a compiler crash. This problem has been fixed in Scala 2.13. However, in order to keep support for 2.12, we need to use a small hack for these two case classes:

In Step, we have to replace an appearance of F by Any, on the definition, and use some type-casts (asInstanceOf) when using that field.
In OpenScope, we use type parameter G[_] for the Concurrent instance, different to the F from the FreeC. Of course, we know from the rest of the program that the F should be the G.

Changes for a subsequent Pull Request

To keep this Pull Request small, there are some pending changes to be done later on:

Remove any use of the internal covary, get, and fromFreeC methods in Pull and Sstream
Consider turning Pull into a type alias (not just an AnyVal wrapper) of FreeC, closer to the vision of the fs3 branch.

mpilquist

Looks awesome. Some questions about source and binary compatibility. I'm okay with some limited source breakages in 2.2.0 as long as they are relatively infrequently hit by users. Binary breakages have to wait a while.

mpilquist · 2020-01-05T15:41:36Z

build.sbt

@@ -205,6 +205,16 @@ lazy val mimaSettings = Seq(
    }.toSet
  },
  mimaBinaryIssueFilters ++= Seq(
+    ProblemFilters.exclude[DirectMissingMethodProblem]("fs2.Stream.PurePipeOps"),


To preserve binary compatibility, we should deprecate this stuff until 3.0 release -- I think that will coincide with cats-effect 3, probably in the 6+ month timeframe.

mpilquist · 2020-01-05T15:43:15Z

core/jvm/src/test/scala/fs2/MemorySanityChecks.scala

 }

 object RepeatEvalSanityTest extends App {
-  def id[A]: Pipe[Pure, A, A] = {
-    def go(s: Stream[Pure, A]): Pull[Pure, A, Unit] =
+  def id[F[_], A]: Pipe[F, A, A] = {


Did these have to change? If so, could you update the PR with a summary of public facing source compatibility API changes?

So, at some point in writing these changes, I thought I may need to change it. It

Now that the F is covariant and not invariant, I am not sure if there is still much use in keeping the idea of a Pipe with a same parameter for the input and output stream. Ideally, you may want the pipe to be applied to any sub-F-type of the input, and be compatible with any super-F-type of the output (just like any function).

F in Stream and Pull have always been covariant though, so changing internals shouldn't cause changes to the API/usage.

mpilquist · 2020-01-05T15:44:27Z

core/shared/src/main/scala/fs2/Pull.scala

@@ -161,8 +160,8 @@ object Pull extends PullLowPriority {
    * Returns a pull that evaluates the supplied by-name each time the pull is used,
    * allowing use of a mutable value in pull computations.
    */
-  def suspend[F[x] >: Pure[x], O, R](p: => Pull[F, O, R]): Pull[F, O, R] =
-    fromFreeC(FreeC.suspend(p.get))
+  def suspend[F[_], O, R](p: => Pull[F, O, R]): Pull[F, O, R] =


The F[x] >: Pure[x] bound is to prevent inference of Nothing. Any reason this changes here?

Do you have an example code snippet where lack of this >: Pure[x] was causing a problem?

In general, I would think that covariance of both Pull and FreeC would make locally inferring F[A] = Nothing not a problem, since it can up-cast it naturally when used in another context.

Right, but Stream and Pull were already covariant in F. I can try to come up with a failing example later, but take a look at this blog post which documents why we had to create Pure in the first place: https://mpilquist.github.io/blog/2018/07/04/fs2/. Basically, there are cases where inferring F = Nothing causes scalac to do the wrong thing.

mpilquist · 2020-01-05T15:45:19Z

core/shared/src/main/scala/fs2/Stream.scala


  /** Creates a pure stream that emits the supplied values. To convert to an effectful stream, use `covary`. */
-  def apply[F[x] >: Pure[x], O](os: O*): Stream[F, O] = emits(os)
+  def apply[O](os: O*): Stream[Pure, O] = emits(os)


I think this one has the potential to impact lots of folks, though maybe not -- perhaps folks typically use Stream(...).covary[F]

Note that, with a covariant F, we can deprecate the covary methods in favour of type-ascriptions, such as

Stream(1): Stream[IO, Int]

I'm 👎 on deprecating covary, it's much more handy than type ascriptions (especially because you only need to type in the effect type and the element type is inferred).

I rarely use apply with an explicit type though, so I'm okay with this particular change :)

Yeah I much prefer covary[F] over type ascription as I don't have to repeat the output type -- and it's worse with pull where you have to specify both output and result types.

I have undone the change to the apply method. In general, I am undoing any change to the public API that is not strictily necessary to introduce the covariance in this Pull Request.

We change the definition of `FreeC`, and its several subclasses, to be covariant on the kind-1 type `F[_]` parameter. Since the F is covariant in FreeC, we can define the Result classes as "Pure" cases of FreeC, that is to say those where the `F` parameter is the `fs2.Pure` (an alias for Nothing). We can also remove the `asFreeC` type-casting method, as well as a few other "asInstanceOf" operations. Now that FreeC is Covariant, in Stream and Pull we can unify the outer type parameter `F` with the type parameter of the inner `FreeC`, so that a Pull (or a Stream) are now just simple wrappers. This allows to remove some internal uses of the `get` function. Whenever we need to add some type information (over Pure or INothing), we can just do that with plain Scala type annotation. - Remove some uses of asInstanceOf - Also, we can save some type annotations in the benchmarks. Remove some usages of the covary methods. - Remove commented lines inside tut. Apparently Scala 2.13 console does not deal with them. - TextSpec: issues involving pipes and variance - Add Mima problems

When compiling on Scala 2.12, we were hitting on a compiler bug, that manifests as a compiler crash, wherever we pattern-match on the Step and OpenScope sub-classes of FreeC. This bug is due to our FreeC now being a Generalised ADT, with a type parameter that is covariant and of higuer-kind. The specific characteristic why this fails for these two sub-classes and not others is that, in these, the type-parameter F appears in the fields of the case class. While this bug has been fixed in Scala 2.13, in fs2 we still need to adapt to work on Scala 2.12. To that end, we have to relax a bit the type-safety of these classes, and introduce some `asInstanceOf` to bridge the gaps left by that.

core/shared/src/main/scala/fs2/Stream.scala

- In order to preserve source-compatibility, we avoid removing the extra F[_] type parameters from Stream/Pull factory methods. - Restore the "PurePipeOps" that had been deleted at first. - We can drop the "Mima" filters, so our changes no longer modify the build.

- Undo changes to MemorySanityChecks.scala that may not be needed. - Remove more uses of the `fromFreeC` and `get` methods in Stream/Pull.

mpilquist · 2020-01-07T12:28:49Z

I reverted a few other unnecessary changes in StreamSpec, CompilationTest, and the README. Merging on successful Travis build. Thanks for all the work on this one, it looks great!

diesalbla force-pushed the covariant_freec branch from a41fb15 to 968a50b Compare January 5, 2020 13:40

diesalbla changed the title ~~Core-FreeC: make the F[_] parameter into a covariant +F[_]~~ FreeC: make the F parameter covariant Jan 5, 2020

diesalbla force-pushed the covariant_freec branch from 968a50b to 633b06e Compare January 5, 2020 14:12

mpilquist requested changes Jan 5, 2020

View reviewed changes

diesalbla added 2 commits January 6, 2020 20:46

diesalbla force-pushed the covariant_freec branch 2 times, most recently from 0a7ce71 to 2feb2d1 Compare January 6, 2020 20:27

mpilquist reviewed Jan 6, 2020

View reviewed changes

core/shared/src/main/scala/fs2/Stream.scala Outdated Show resolved Hide resolved

mpilquist reviewed Jan 6, 2020

View reviewed changes

core/shared/src/main/scala/fs2/Stream.scala Outdated Show resolved Hide resolved

diesalbla force-pushed the covariant_freec branch 2 times, most recently from 2455fdf to 18a34d1 Compare January 6, 2020 22:48

diesalbla force-pushed the covariant_freec branch from 18a34d1 to 0dc2946 Compare January 6, 2020 22:48

diesalbla added 2 commits January 7, 2020 00:33

More changes with the

9e42515

- Undo changes to MemorySanityChecks.scala that may not be needed. - Remove more uses of the `fromFreeC` and `get` methods in Stream/Pull.

Undo changes to the fs2.text module

6931c5c

diesalbla force-pushed the covariant_freec branch from 6aa5a32 to 4308637 Compare January 7, 2020 00:21

Undo some unneeded changes.

9c7bef2

diesalbla force-pushed the covariant_freec branch from 4308637 to 9c7bef2 Compare January 7, 2020 01:15

Reverted a few other unneeded changes

95e8751

mpilquist approved these changes Jan 7, 2020

View reviewed changes

mpilquist merged commit b47eb6a into typelevel:master Jan 7, 2020

diesalbla deleted the covariant_freec branch January 8, 2020 02:52

mpilquist added this to the 2.2.0 milestone Jan 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FreeC: make the F parameter covariant #1730

FreeC: make the F parameter covariant #1730

diesalbla commented Jan 5, 2020 •

edited

mpilquist left a comment

mpilquist Jan 5, 2020

mpilquist Jan 5, 2020

diesalbla Jan 6, 2020

mpilquist Jan 6, 2020

mpilquist Jan 5, 2020

diesalbla Jan 5, 2020

mpilquist Jan 5, 2020

mpilquist Jan 5, 2020

diesalbla Jan 5, 2020 •

edited

kubukoz Jan 5, 2020

mpilquist Jan 5, 2020

diesalbla Jan 7, 2020

mpilquist commented Jan 7, 2020

FreeC: make the F parameter covariant #1730

FreeC: make the F parameter covariant #1730

Conversation

diesalbla commented Jan 5, 2020 • edited

Broken edge for Scala 2.12.

Changes for a subsequent Pull Request

mpilquist left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diesalbla Jan 5, 2020 • edited

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

mpilquist commented Jan 7, 2020

diesalbla commented Jan 5, 2020 •

edited

diesalbla Jan 5, 2020 •

edited