Make SIP-62 - betterFors a standard feature (#22652)

- Deprecate experimental language import
- Make betterFors conditional on `-source >= 3.7` instead
- Drop the `experimental.betterFors` import from tests

Author: KacperFKorban

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -11,7 +11,7 @@ import NameKinds.{UniqueName, ContextBoundParamName, ContextFunctionParamName, D
 import typer.{Namer, Checking}
 import util.{Property, SourceFile, SourcePosition, SrcPos, Chars}
 import config.{Feature, Config}
-import config.Feature.{sourceVersion, migrateTo3, enabled, betterForsEnabled}
+import config.Feature.{sourceVersion, migrateTo3, enabled}
 import config.SourceVersion.*
 import collection.mutable
 import reporting.*
@@ -1953,9 +1953,9 @@ object desugar {
     /** Create tree for for-comprehension `<for (enums) do body>` or
      *   `<for (enums) yield body>` where mapName and flatMapName are chosen
      *  corresponding to whether this is a for-do or a for-yield.
-     *  If betterFors are enabled, the creation performs the following rewrite rules:
+     *  If sourceVersion >= 3.7 are enabled, the creation performs the following rewrite rules:
      *
-     *  1. if betterFors is enabled:
+     *  1. if sourceVersion >= 3.7:
      *
      *    for () do E  ==>  E
      *      or
@@ -1986,13 +1986,13 @@ object desugar {
      *      ==>
      *    for (P <- G.withFilter (P => E); ...) ...
      *
-     *  6. For any N, if betterFors is enabled:
+     *  6. For any N, if sourceVersion >= 3.7:
      *
      *    for (P <- G; P_1 = E_1; ... P_N = E_N; P1 <- G1; ...) ...
      *      ==>
      *    G.flatMap (P => for (P_1 = E_1; ... P_N = E_N; ...))
      *
-     *  7. For any N, if betterFors is enabled:
+     *  7. For any N, if sourceVersion >= 3.7:
      *
      *    for (P <- G; P_1 = E_1; ... P_N = E_N) ...
      *      ==>
@@ -2013,7 +2013,7 @@ object desugar {
      *    If any of the P_i are variable patterns, the corresponding `x_i @ P_i` is not generated
      *    and the variable constituting P_i is used instead of x_i
      *
-     *  9. For any N, if betterFors is enabled:
+     *  9. For any N, if sourceVersion >= 3.7:
      *
      *    for (P_1 = E_1; ... P_N = E_N; ...)
      *      ==>
@@ -2044,6 +2044,16 @@ object desugar {
           makeCaseLambda(CaseDef(gen.pat, EmptyTree, body) :: Nil, matchCheckMode)
       }
 
+      def hasGivenBind(pat: Tree): Boolean = pat.existsSubTree {
+        case pat @ Bind(_, pat1) => pat.mods.is(Given)
+        case _ => false
+      }
+
+      /** Does this pattern define any given bindings */
+      def isNestedGivenPattern(pat: Tree): Boolean = pat match
+        case pat @ Bind(_, pat1) => hasGivenBind(pat1)
+        case _ => hasGivenBind(pat)
+
       /** If `pat` is not an Identifier, a Typed(Ident, _), or a Bind, wrap
        *  it in a Bind with a fresh name. Return the transformed pattern, and the identifier
        *  that refers to the bound variable for the pattern. Wildcard Binds are
@@ -2147,15 +2157,15 @@ object desugar {
           case _ => false
 
       def markTrailingMap(aply: Apply, gen: GenFrom, selectName: TermName): Unit =
-        if betterForsEnabled
+        if sourceVersion.isAtLeast(`3.7`)
           && selectName == mapName
           && gen.checkMode != GenCheckMode.Filtered // results of withFilter have the wrong type
           && (deepEquals(gen.pat, body) || deepEquals(body, Tuple(Nil)))
         then
           aply.putAttachment(TrailingForMap, ())
 
       enums match {
-        case Nil if betterForsEnabled => body
+        case Nil if sourceVersion.isAtLeast(`3.7`) => body
         case (gen: GenFrom) :: Nil =>
           val aply = Apply(rhsSelect(gen, mapName), makeLambda(gen, body))
           markTrailingMap(aply, gen, mapName)
@@ -2164,8 +2174,9 @@ object desugar {
           val cont = makeFor(mapName, flatMapName, rest, body)
           Apply(rhsSelect(gen, flatMapName), makeLambda(gen, cont))
         case (gen: GenFrom) :: rest
-        if betterForsEnabled
-           && rest.dropWhile(_.isInstanceOf[GenAlias]).headOption.forall(e => e.isInstanceOf[GenFrom]) => // possible aliases followed by a generator or end of for
+        if sourceVersion.isAtLeast(`3.7`)
+          && rest.dropWhile(_.isInstanceOf[GenAlias]).headOption.forall(e => e.isInstanceOf[GenFrom]) // possible aliases followed by a generator or end of for
+          && !rest.takeWhile(_.isInstanceOf[GenAlias]).exists(a => isNestedGivenPattern(a.asInstanceOf[GenAlias].pat)) =>
           val cont = makeFor(mapName, flatMapName, rest, body)
           val selectName =
             if rest.exists(_.isInstanceOf[GenFrom]) then flatMapName
@@ -2191,9 +2202,9 @@ object desugar {
           makeFor(mapName, flatMapName, vfrom1 :: rest1, body)
         case (gen: GenFrom) :: test :: rest =>
           val filtered = Apply(rhsSelect(gen, nme.withFilter), makeLambda(gen, test))
-          val genFrom = GenFrom(gen.pat, filtered, if betterForsEnabled then GenCheckMode.Filtered else GenCheckMode.Ignore)
+          val genFrom = GenFrom(gen.pat, filtered, if sourceVersion.isAtLeast(`3.7`) then GenCheckMode.Filtered else GenCheckMode.Ignore)
           makeFor(mapName, flatMapName, genFrom :: rest, body)
-        case GenAlias(_, _) :: _ if betterForsEnabled =>
+        case GenAlias(_, _) :: _ if sourceVersion.isAtLeast(`3.7`) =>
           val (valeqs, rest) = enums.span(_.isInstanceOf[GenAlias])
           val pats = valeqs.map { case GenAlias(pat, _) => pat }
           val rhss = valeqs.map { case GenAlias(_, rhs) => rhs }

compiler/src/dotty/tools/dotc/config/Feature.scala

@@ -35,7 +35,6 @@ object Feature:
   val into = experimental("into")
   val modularity = experimental("modularity")
   val quotedPatternsWithPolymorphicFunctions = experimental("quotedPatternsWithPolymorphicFunctions")
-  val betterFors = experimental("betterFors")
   val packageObjectValues = experimental("packageObjectValues")
 
   def experimentalAutoEnableFeatures(using Context): List[TermName] =
@@ -62,8 +61,7 @@ object Feature:
     (pureFunctions, "Enable pure functions for capture checking"),
     (captureChecking, "Enable experimental capture checking"),
     (into, "Allow into modifier on parameter types"),
-    (modularity, "Enable experimental modularity features"),
-    (betterFors, "Enable improvements in `for` comprehensions")
+    (modularity, "Enable experimental modularity features")
   )
 
   // legacy language features from Scala 2 that are no longer supported.
@@ -118,8 +116,6 @@ object Feature:
 
   def namedTypeArgsEnabled(using Context) = enabled(namedTypeArguments)
 
-  def betterForsEnabled(using Context) = enabled(betterFors)
-
   def genericNumberLiteralsEnabled(using Context) = enabled(genericNumberLiterals)
 
   def scala2ExperimentalMacroEnabled(using Context) = enabled(scala2macros)

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -2956,7 +2956,7 @@ object Parsers {
     /** Enumerators ::= Generator {semi Enumerator | Guard}
      */
     def enumerators(): List[Tree] =
-      if in.featureEnabled(Feature.betterFors) then
+      if sourceVersion.isAtLeast(`3.7`) then
         aliasesUntilGenerator() ++ enumeratorsRest()
       else
         generator() :: enumeratorsRest()

docs/_docs/reference/experimental/better-fors.md renamed to docs/_docs/reference/changed-features/better-fors.md

@@ -1,12 +1,10 @@
 ---
 layout: doc-page
 title: "Better fors"
-nightlyOf: https://docs.scala-lang.org/scala3/reference/experimental/better-fors.html
+nightlyOf: https://docs.scala-lang.org/scala3/reference/changed-features/better-fors.html
 ---
 
-The `betterFors` language extension improves the usability of `for`-comprehensions.
-
-The extension is enabled by the language import `import scala.language.experimental.betterFors` or by setting the command line option `-language:experimental.betterFors`.
+Starting in Scala `3.7`, the usability of `for`-comprehensions is improved.
 
 The biggest user facing change is the new ability to start `for`-comprehensions with aliases. This means that the following previously invalid code is now valid:
 
@@ -30,11 +28,11 @@ for
 yield a + b
 ```
 
-Additionally this extension changes the way `for`-comprehensions are desugared. The desugaring is now done in a more intuitive way and the desugared code can be more efficient, because it avoids some unnecessary method calls. There are two main changes in the desugaring:
+Additionally, this extension changes the way `for`-comprehensions are desugared. The desugaring is now done in a more intuitive way and the desugared code can be more efficient, because it avoids some unnecessary method calls. There are two main changes in the desugaring:
 
 1. **Simpler Desugaring for Pure Aliases**:
     When an alias is not followed by a guard, the desugaring is simplified. The last generator and the aliases don't have to be wrapped in a tuple, and instead the aliases are simply introduced as local variables in a block with the next generator.
-    **Current Desugaring**:
+    **Previous Desugaring**:
       ```scala
       for {
         a <- doSth(arg)

docs/sidebar.yml

@@ -116,6 +116,7 @@ subsection:
           - page: reference/changed-features/lazy-vals-init.md
           - page: reference/changed-features/main-functions.md
           - page: reference/changed-features/interpolation-escapes.md
+          - page: reference/changed-features/better-fors.md
       - title: Dropped Features
         index: reference/dropped-features/dropped-features.md
         subsection:
@@ -162,7 +163,6 @@ subsection:
           - page: reference/experimental/modularity.md
           - page: reference/experimental/typeclasses.md
           - page: reference/experimental/runtimeChecked.md
-          - page: reference/experimental/better-fors.md
           - page: reference/experimental/unrolled-defs.md
           - page: reference/experimental/package-object-values.md
       - page: reference/syntax.md

library/src/scala/runtime/stdLibPatches/language.scala

@@ -140,6 +140,7 @@ object language:
      * @see [[https://github.com/scala/improvement-proposals/pull/79]]
      */
     @compileTimeOnly("`betterFors` can only be used at compile time in import statements")
+    @deprecated("The `experimental.betterFors` language import is no longer needed since the feature is now standard", since = "3.7")
     object betterFors
 
     /** Experimental support for package object values

presentation-compiler/test/dotty/tools/pc/tests/tokens/SemanticTokensSuite.scala

@@ -350,9 +350,9 @@ class SemanticTokensSuite extends BaseSemanticTokensSuite:
          |
          |object <<B>>/*class*/ {
          |  val <<a>>/*variable,definition,readonly*/ = for {
-         |    <<foo>>/*variable,definition,readonly*/ <- <<List>>/*class*/("a", "b", "c")
+         |    <<foo>>/*parameter,declaration,readonly*/ <- <<List>>/*class*/("a", "b", "c")
          |    <<_>>/*class,abstract*/ = <<println>>/*method*/("print!")
-         |  } yield <<foo>>/*variable,readonly*/
+         |  } yield <<foo>>/*parameter,readonly*/
          |}
          |""".stripMargin
     )

tests/debug/eval-in-for-comprehension.check

@@ -8,16 +8,9 @@ eval list(0)
 result 1
 eval x
 result 1
-break Test$ 6 // in main$$anonfun$1$$anonfun$adapted$1
 break Test$ 7 // in main$$anonfun$1$$anonfun$1
 eval x + y
 result 2
-// TODO this line position does not make any sense
-break Test$ 6 // in main$$anonfun$1$$anonfun$1
-break Test$ 7 // in main$$anonfun$1$$anonfun$1
-break Test$ 6 // in main$$anonfun$1$$anonfun$2
-break Test$ 6 // in main$$anonfun$1$$anonfun$2
-break Test$ 7 // in main$$anonfun$1$$anonfun$2
 
 break Test$ 11 // in main$$anonfun$2
 eval x

tests/pos/better-fors-given.scala

@@ -0,0 +1,68 @@
+@main def Test: Unit =
+  for
+    x <- Option(23 -> "abc")
+    (a @ given Int, b @ given String) = x
+    _ <- Option(1)
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    x <- Option((1.3, 23 -> "abc"))
+    (_, (a @ given Int, b @ given String)) = x
+    _ <- Option(1)
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    x <- Option(Some(23 -> "abc"))
+    Some(a @ given Int, b @ given String) = x
+    _ <- Option(1)
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    x <- Option(Some(23))
+    Some(a @ given Int) = x
+    _ <- Option(1)
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    x <- Option(23)
+    a @ given Int = x
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    x <- Option(23)
+    _ @ given Int = x
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    x <- Option(23)
+    given Int = x
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    x <- Option(23)
+    given Int = x
+    _ <- Option(1)
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    a @ given Int <- Option(23)
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    _ @ given Int <- Option(23)
+  yield
+    assert(summon[Int] == 23)
+
+  for
+    given Int <- Option(23)
+  yield
+    assert(summon[Int] == 23)

tests/run/better-fors.scala

@@ -1,5 +1,3 @@
-import scala.language.experimental.betterFors
-
 def for1 =
   for {
     a = 1

tests/run/fors.scala

@@ -113,8 +113,6 @@ object Test extends App {
 
   /////////////////// elimination of map ///////////////////
 
-  import scala.language.experimental.betterFors
-
   @tailrec
   def pair[B](xs: List[Int], ys: List[B], n: Int): List[(Int, B)] =
     if n == 0 then xs.zip(ys)

tests/semanticdb/expect/ForComprehension.expect.scala

@@ -3,43 +3,43 @@ package example
 class ForComprehension/*<-example::ForComprehension#*/ {
   for {
     a/*<-local0*/ <- List/*->scala::package.List.*/(1)
-    b/*<-local1*//*->local1*/ <- List/*->scala::package.List.*/(1)
+    b/*<-local1*/ <- List/*->scala::package.List.*/(1)
     if b/*->local1*/ >/*->scala::Int#`>`(+3).*/ 1
-    c/*<-local2*//*->local2*/ = a/*->local0*/ +/*->scala::Int#`+`(+4).*/ b/*->local1*/
+    c/*<-local2*/ = a/*->local0*/ +/*->scala::Int#`+`(+4).*/ b/*->local1*/
   } yield (a/*->local0*/, b/*->local1*/, c/*->local2*/)
   for {
-    a/*<-local4*/ <- List/*->scala::package.List.*/(1)
-    b/*<-local5*/ <- List/*->scala::package.List.*/(a/*->local4*/)
+    a/*<-local3*/ <- List/*->scala::package.List.*/(1)
+    b/*<-local4*/ <- List/*->scala::package.List.*/(a/*->local3*/)
     if (
-      a/*->local4*/,
-      b/*->local5*/
+      a/*->local3*/,
+      b/*->local4*/
     ) ==/*->scala::Any#`==`().*/ (1, 2)
     (
-      c/*<-local7*/,
-      d/*<-local8*/
-    ) <- List/*->scala::package.List.*/((a/*->local4*/, b/*->local5*/))
+      c/*<-local6*/,
+      d/*<-local7*/
+    ) <- List/*->scala::package.List.*/((a/*->local3*/, b/*->local4*/))
     if (
-      a/*->local4*/,
-      b/*->local5*/,
-      c/*->local7*/,
-      d/*->local8*/
+      a/*->local3*/,
+      b/*->local4*/,
+      c/*->local6*/,
+      d/*->local7*/
     ) ==/*->scala::Any#`==`().*/ (1, 2, 3, 4)
-    e/*<-local9*//*->local9*/ = (
-      a/*->local4*/,
-      b/*->local5*/,
-      c/*->local7*/,
-      d/*->local8*/
+    e/*<-local8*//*->local8*/ = (
+      a/*->local3*/,
+      b/*->local4*/,
+      c/*->local6*/,
+      d/*->local7*/
     )
-    if e/*->local9*/ ==/*->scala::Any#`==`().*/ (1, 2, 3, 4)
-    f/*<-local10*/ <- List/*->scala::package.List.*/(e/*->local9*/)
+    if e/*->local8*/ ==/*->scala::Any#`==`().*/ (1, 2, 3, 4)
+    f/*<-local9*/ <- List/*->scala::package.List.*/(e/*->local8*/)
   } yield {
     (
-      a/*->local4*/,
-      b/*->local5*/,
-      c/*->local7*/,
-      d/*->local8*/,
-      e/*->local9*/,
-      f/*->local10*/
+      a/*->local3*/,
+      b/*->local4*/,
+      c/*->local6*/,
+      d/*->local7*/,
+      e/*->local8*/,
+      f/*->local9*/
     )
   }
 }