Change for SIP-62: betterFors

content/better-fors.md

@@ -10,10 +10,11 @@ title: SIP-62 - For comprehension improvements
 
 ## History
 
-| Date          | Version            |
-|---------------|--------------------|
-| June 6th 2023 | Initial Draft      |
-| Feb 15th 2024 | Reviewed Version   |
+| Date          | Version                |
+|---------------|------------------------|
+| June 6th 2023 | Initial Draft          |
+| Feb 15th 2024 | Reviewed Version       |
+| Nov 21th 2024 | Addendum for change 3. |
 
 ## Summary
 
@@ -333,6 +334,60 @@ doSth(arg).map { a =>
 
 This change is strictly an optimization. This allows for the compiler to get rid of the final `map` call, if the yielded value is the same as the last bound pattern. The pattern can be either a single variable binding or a tuple.
 
+This optimization should be done after type checking (e.g. around first transform). See the reasons to why it cannot be done in desugaring in [here](#previous-design-in-desugaring).
+
+We propose an approach where an attachment (`TrailingForMap`) is attached to the last `map` `Apply` node. After that, a later phase will look for `Apply` nodes with this attachment and possibly remove the `map` call.
+
+The condition for allowing to remove the last map call (for a binding `pat <- gen yield pat1`) are as follows:
+- `pat` is (syntactically) equivalent to `pat1` ($pat =_{s} pat1$)
+
+  where
+
+  $x =_{s} x, \text{if x is a variable reference}$
+
+  $x =_{s} (), \text{if x is a variable reference of type Unit}$
+
+  $(x_1, ..., x_n) =_{s} (y_1, ..., y_n) \iff \forall i \in n.\; x_i =_{s} y_i$
+
+  This means that the two patterns are equivalent if they are the same variable, if they are tuples of the same variables, or if one is a variable reference of type `Unit` and the other is a `Unit` literal.
+- `pat` and `pat1` have the same types (`pat.tpe` =:= `pat1.tpe`)
+
+##### Changes discussion
+
+This adresses the problem of changing the resulting type after removing trailing `map` calls.
+
+There are two main changes compared to the previous design:
+1. Moving the implementation to the later phase, to be able to use the type information and explicitly checking that the types are the same.
+2. Allowing to remove the last `map` call if the yielded value is a `Unit` literal (and obviously the type doesn't change).
+
+The motivation for the second change is to avoid potential memory leaks in effecting loops. e.g.
+
+```scala
+//> using scala 3.3.3
+//> using lib "dev.zio::zio:2.1.5"
+
+import zio.*
+
+def loop: Task[Unit] =
+  for
+    _ <- Console.print("loop")
+    _ <- loop
+  yield ()
+
+@main
+def run =
+  val runtime = Runtime.default
+  Unsafe.unsafe { implicit unsafe =>
+    runtime.unsafe.run(loop).getOrThrowFiberFailure()
+  }
+```
+
+This kind of effect loop is pretty commonly used in Scala FP programs and often ends in `yield ()`.
+
+The problem with the desugaring of this for-comprehensions is that it leaks memory because the result of `loop` has to be mapped over with `_ => ()`, which often does nothing.
+
+##### Previous design (in desugaring)
+
 One desugaring rule has to be modified for this purpose.
 
 ```scala
@@ -356,6 +411,46 @@ will just be desugared to
 List(1, 2, 3)
 ```
 
+**Cause of change**
+
+This design ended up breaking quite a few existing projects in the open community build run.
+
+For example, consider the following code:
+
+```scala
+//>  using scala 3.nightly
+
+import scala.language.experimental.betterFors
+
+case class Container[A](val value: A) {
+  def map[B](f: A => B): Container[B] = Container(f(value))
+}
+
+sealed trait Animal
+case class Dog() extends Animal
+
+def opOnDog(dog: Container[Dog]): Container[Animal] =
+  for
+    v <- dog
+  yield v
+```
+
+With the new desugaring, the code gave an error about type mismatch.
+
+```scala
+-- [E007] Type Mismatch Error: /home/kpi/bugs/better-fors-bug.scala:13:2 -------
+13 |  for
+   |  ^
+   |  Found:    (dog : Container[Dog])
+   |  Required: Container[Animal]
+14 |    v <- dog
+15 |  yield v
+   |
+   | longer explanation available when compiling with `-explain`
+```
+
+This is because the container is invariant. And even though the last `map` was an identity function, it was used to upcast `Dog` to `Animal`.
+
 ### Compatibility
 
 This change may change the semantics of some programs. It may remove some `map` calls in the desugared code, which may change the program semantics (if the `map` implementation was side-effecting).
@@ -372,10 +467,15 @@ yield a + b
 
 As far as I know, there are no widely used Scala 3 libraries that depend on the desugaring specification of `for`-comprehensions.
 
+The only Open community build library that failed because of the change to the desugaring specification is [`avocADO`](https://github.com/VirtusLab/avocado).
+
 ## Links
 
 1. Scala contributors discussion thread (pre-SIP): https://contributors.scala-lang.org/t/pre-sip-improve-for-comprehensions-functionality/3509/51
 2. Github issue discussion about for desugaring: https://github.com/lampepfl/dotty/issues/2573
 3. Scala 2 implementation of some of the improvements: https://github.com/oleg-py/better-monadic-for
 4. Implementation of one of the simplifications: https://github.com/lampepfl/dotty/pull/16703
 5. Draft implementation branch: https://github.com/dotty-staging/dotty/tree/improved-fors
+6. Minimized issue reproducing the problem with the current desugaring: https://github.com/scala/scala3/issues/21804
+7. (empty :sad:) Contributors thread about better effect loops with for-comprehensions: https://contributors.scala-lang.org/t/pre-sip-sip-62-addition-proposal-better-effect-loops-with-for-comprehensions/6759
+8. Draft implementation of dropping the last map call after type checking (only for `Unit` literals): https://github.com/KacperFKorban/dotty/commit/31cbd4744b9375443a0770a8b8a9d16de694c6bb#diff-ed248bb93940ea4f38e6da698051f882e81df6f33fea91a046d1d4f6af506296R2066