Fix unintended unchunking by adding override map #3189

yyy1000 · 2023-03-26T15:22:51Z

It wants to fix #3187
It's my first time to PR typelevel! If there's something I need to improve or change, I'm willing to do that!

mpilquist · 2023-03-26T16:40:50Z

Thanks! Can you run set prePR and commit the result?

bwiercinski · 2023-03-26T19:18:50Z

core/shared/src/test/scala/fs2/StreamSuite.scala

@@ -1024,6 +1024,11 @@ class StreamSuite extends Fs2Suite {
    assert(compileErrors("Stream.eval(IO(1)).through(p)").nonEmpty)
  }

+  test("Unintended unchunking when using Monad instance of Stream") {


you have only tested StreamLowPriority instance. I think that duplicating this test and adding .covary[IO] before .through (and .compile before .toList) will be sufficient :)

Thanks! I will do that.

yyy1000 · 2023-03-27T02:25:06Z

Hi, I run prePR and add the new test. :)

core/shared/src/test/scala/fs2/StreamSuite.scala

armanbilge · 2023-03-30T14:04:13Z

Re-triggering CI

diesalbla · 2023-03-30T15:17:26Z

core/shared/src/test/scala/fs2/StreamSuite.scala

+  test("Unintended unchunking when using StreamLowPriority#monadInstance") {
+    def generateTuple(pipe: Pipe[fs2.Pure, Int, (Int, Int)]) =
+      (Stream(1, 2, 3) ++ Stream(4, 5, 6)).through(pipe).chunks.toList
+    assertEquals(generateTuple(_.map(e => e -> (e + 1))), generateTuple(_.fproduct(_ + 1)))
+  }
+
+  test("Unintended unchunking when using Stream#monadErrorInstance") {
+    def generateTuple(pipe: Pipe[IO, Int, (Int, Int)]) =
+      (Stream(1, 2, 3) ++ Stream(4, 5, 6)).covary[IO].through(pipe).chunks.compile.toList
+    val lhs = generateTuple(_.map(e => e -> (e + 1)))
+    val rhs = generateTuple(_.fproduct(_ + 1))
+    (lhs, rhs).mapN(assertEquals(_, _))
+  }
+


These tests seem to be needlessly complicated, with the use of different literal values, the tuple, the increment, etc.

In a "test-driven-development" fashion, the process is to 1. write the smallest possible tests that fails 2. change code, 3. check that test passes. It seems to me that would be achieved, for the problem, with a test core that did the following:

def countChunks(str: Stream[Nothing, _]): Int = str.chunks.toList.length val source = Stream(0) ++ Stream(0, 0) val mapped = source.map(_ => 1) assertEquals(countChunks(mapped), 2)

Thanks for your review!
It's helpful, and I will make a change for this:)

Hi, sorry if I made some mistakes.
I run a local test and found that only calling the 'map' method like source.map(_ => 1) can't call the method I changed. :)
I changed my code to below to make it simpler, is it OK?

test("Unintended unchunking when using StreamLowPriority#monadInstance") { def countChunks(): Int = (Stream(0) ++ Stream(0, 0)).fproduct(_ + 1).chunks.toList.length assertEquals(countChunks(), 2) } test("Unintended unchunking when using Stream#monadErrorInstance") { def countChunks() = (Stream(0) ++ Stream(0, 0)).covary[IO].fproduct(_ + 1).chunks.compile.toList.map(_.length) countChunks().assertEquals(2) }

That's a great point, if we add methods directly on stream in the future it may break these tests.

Instead I recommend doing like this:

Monad[Stream[F, *]].map(source)(_ => 1) MonadError[Stream[F, *]].map(source)(_ => 1)

That way we can be sure we are testing exactly the right thing :)

Actually, we can do even better.

Stream.monadInstance.map(source)(_ => 1) // etc.

Sorry, but I encountered a problem. :(
Stream.monadInstance.map(source) works well, but for Stream.monadErrorInstance.map(str), my IDE shows 'No implicits found for parameter ev: ApplicativeError[F_ , Throwable]', what's that? 😂

Thanks for trying that! If you make the change I suggested in #3189 (comment), then we only need one test.

OK, but can it cover the 'MonadErrorInstance'? 🤔

Yes, because my proposal is that Monad and MonadError implementations should share the same code. Then we don't need to write the same code twice, fix the same bugs twice, and add the same tests twice :)

That sounds good.
I will look at the repo and make them share the same code.

armanbilge · 2023-03-31T14:37:06Z

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

@@ -5222,6 +5222,7 @@ object Stream extends StreamLowPriority {
      def handleErrorWith[A](s: Stream[F, A])(h: Throwable => Stream[F, A]) =
        s.handleErrorWith(h)
      def raiseError[A](t: Throwable) = Stream.raiseError[F](t)
+      override def map[A, B](fa: Stream[F, A])(f: A => B): Stream[F, B] = fa.map(f)


It's unfortunate that this fix has to be duplicated. Perhaps we should extract a:

class StreamMonad[F] extends Monad[Stream[F, *]] { ... }

Then the code can be shared.

Now my understanding is:🤔
After extracting a class like

class StreamMonad[F] extends Monad[Stream[F, *]] { ... }

the original code

private[fs2] trait StreamLowPriority { implicit def monadInstance[F[_]]: Monad[Stream[F, *]] = new Monad[Stream[F, *]] { ... }

will become like below?

private[fs2] trait StreamLowPriority { implicit def monadInstance[F[_]]: StreamMonad[F] = new StreamMonad[F]] { ... } }

But if it is, how to do a similar thing to MonadError[Stream[F, *], Throwable]? I see it has the second param called 'Throwable'. :)

implicit def monadErrorInstance[F[_]]: MonadError[Stream[F, *], Throwable] = new StreamMonad[F] with MonadError[Stream[F, *], Throwable] { // inherit monad methods, implement monad error methods }

Thanks! It's very helpful.😊

@armanbilge IMHIO it is always good practice to give explicit names to long classes, just as it is good practice to name long lambdas. @yyy1000 Just remember to keep it private to the Stream companion object.

yyy1000 · 2023-04-01T15:50:26Z

I try to extract the class and please review. :)

diesalbla · 2023-04-01T15:57:14Z

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

@@ -3089,6 +3089,21 @@ final class Stream[+F[_], +O] private[fs2] (private[fs2] val underlying: Pull[F,
  override def toString: String = "Stream(..)"
 }

+class StreamMonad[F[_]] extends Monad[Stream[F, *]] {


Could we place this inside the Stream companion object and mark it as private?

OK, I will do it the next day :), (it's a little late in my place)!

diesalbla · 2023-04-01T15:58:23Z

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

-      override def pure[A](x: A): Stream[F, A] = Stream.emit(x)
-
+  implicit def monadInstance[F[_]]: StreamMonad[F] =
+    new StreamMonad[F] {


Since StreamMonad is not an abstract class, you can omit the body (the { / } block) here.

Thanks! :)
If I do this, I will move the unit method to the StreamMonad. And monadErrorInstance will also use that method.
But it don't use the override unit method before.

armanbilge · 2023-04-01T22:42:49Z

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

+  override def tailRecM[A, B](a: A)(f: A => Stream[F, Either[A, B]]): Stream[F, B] =
+    f(a).flatMap {
+      case Left(a)  => tailRecM(a)(f)
+      case Right(b) => Stream(b)
+    }


If you do extends StackSafeMonad[Stream[F, *]] instead of Monad then we don't need this method implementation :)

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

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

…erride-map

core/shared/src/test/scala/fs2/StreamSuite.scala

armanbilge

Thanks for all your work on this!

fix: add override map for Stream instance

265fd19

bwiercinski suggested changes Mar 26, 2023

View reviewed changes

prePR and add a new test

d45541d

armanbilge reviewed Mar 27, 2023

View reviewed changes

core/shared/src/test/scala/fs2/StreamSuite.scala Outdated Show resolved Hide resolved

yyy1000 added 2 commits March 27, 2023 13:18

fix using IO

9600a28

fix compare equal

9902136

armanbilge closed this Mar 30, 2023

armanbilge reopened this Mar 30, 2023

diesalbla reviewed Mar 30, 2023

View reviewed changes

armanbilge reviewed Mar 31, 2023

View reviewed changes

yyy1000 added 2 commits April 1, 2023 23:45

extract class and make test one

e1838c9

fix

c0a07cc

diesalbla reviewed Apr 1, 2023

View reviewed changes

diesalbla approved these changes Apr 1, 2023

View reviewed changes

armanbilge reviewed Apr 1, 2023

View reviewed changes

move the class to private and inside Stream object

853f923

armanbilge reviewed Apr 2, 2023

View reviewed changes

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

yyy1000 and others added 3 commits April 2, 2023 10:24

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

687bb71

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

scalafmt

38405bc

Merge branch 'override-map' of https://github.com/yyy1000/fs2 into ov…

b8304d5

…erride-map

armanbilge reviewed Apr 2, 2023

View reviewed changes

core/shared/src/test/scala/fs2/StreamSuite.scala Outdated Show resolved Hide resolved

Tweak test name

0292f29

armanbilge approved these changes Apr 2, 2023

View reviewed changes

diesalbla merged commit 8f937b5 into typelevel:main Apr 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unintended unchunking by adding override map #3189

Fix unintended unchunking by adding override map #3189

yyy1000 commented Mar 26, 2023 •

edited by armanbilge

Loading

mpilquist commented Mar 26, 2023

bwiercinski Mar 26, 2023

yyy1000 Mar 27, 2023

yyy1000 commented Mar 27, 2023

armanbilge commented Mar 30, 2023

diesalbla Mar 30, 2023

yyy1000 Mar 31, 2023

yyy1000 Mar 31, 2023

armanbilge Mar 31, 2023

armanbilge Mar 31, 2023 •

edited

Loading

yyy1000 Mar 31, 2023

armanbilge Mar 31, 2023

yyy1000 Mar 31, 2023

armanbilge Mar 31, 2023

yyy1000 Mar 31, 2023

armanbilge Mar 31, 2023

yyy1000 Apr 1, 2023 •

edited

Loading

armanbilge Apr 1, 2023 •

edited

Loading

yyy1000 Apr 1, 2023

diesalbla Apr 1, 2023

yyy1000 commented Apr 1, 2023

diesalbla Apr 1, 2023

yyy1000 Apr 1, 2023

diesalbla Apr 1, 2023

yyy1000 Apr 2, 2023

armanbilge Apr 1, 2023

yyy1000 Apr 2, 2023

armanbilge left a comment

Fix unintended unchunking by adding override map #3189

Fix unintended unchunking by adding override map #3189

Conversation

yyy1000 commented Mar 26, 2023 • edited by armanbilge Loading

mpilquist commented Mar 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyy1000 commented Mar 27, 2023

armanbilge commented Mar 30, 2023

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

armanbilge Mar 31, 2023 • 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

yyy1000 Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

armanbilge Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyy1000 commented Apr 1, 2023

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

armanbilge left a comment

Choose a reason for hiding this comment

yyy1000 commented Mar 26, 2023 •

edited by armanbilge

Loading

armanbilge Mar 31, 2023 •

edited

Loading

yyy1000 Apr 1, 2023 •

edited

Loading

armanbilge Apr 1, 2023 •

edited

Loading