From 97c4132287e03c111e414667e448932b46916ca1 Mon Sep 17 00:00:00 2001 From: Kacper Korban Date: Thu, 21 Nov 2024 15:34:20 +0100 Subject: [PATCH] Addendum for SIP-62 to fix open community build fails --- content/better-fors.md | 108 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 4 deletions(-) diff --git a/content/better-fors.md b/content/better-fors.md index ed614f7..51014dc 100644 --- a/content/better-fors.md +++ b/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,6 +467,8 @@ 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 @@ -379,3 +476,6 @@ As far as I know, there are no widely used Scala 3 libraries that depend on the 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