Turn given loop prevention on for -source future (#19392)

Drop the experimental language import. I believe everybody agrees that this is a desirable improvement, and type inference and implicit search have traditionally been in the realm of the compiler implementers. Experimental language imports have to live forever (albeit as deprecated once the feature is accepted as standard). So they are rather heavyweight and it is unergonomic to require them for smallish improvements to type inference. The new road map is as follows: - In 3.4: warning if behavior would change in the future. - In 3.5: error if behavior would change in the future - In 3.future (at the earliest 3.6): new behavior.
scala · Jan 8, 2024 · 65fca39 · 65fca39
2 parents dce597f + eb3ca86
commit 65fca39
Show file tree

Hide file tree

Showing 13 changed files with 103 additions and 51 deletions.
diff --git a/compiler/src/dotty/tools/dotc/config/Feature.scala b/compiler/src/dotty/tools/dotc/config/Feature.scala
@@ -33,7 +33,6 @@ object Feature:
   val pureFunctions = experimental("pureFunctions")
   val captureChecking = experimental("captureChecking")
   val into = experimental("into")
-  val givenLoopPrevention = experimental("givenLoopPrevention")
 
   val globalOnlyImports: Set[TermName] = Set(pureFunctions, captureChecking)
 

diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala
@@ -1595,7 +1595,7 @@ trait Implicits:
 
         def checkResolutionChange(result: SearchResult) =
           if (eligible ne preEligible)
-              && !Feature.enabled(Feature.givenLoopPrevention)
+              && !sourceVersion.isAtLeast(SourceVersion.future)
           then
             val prevResult = searchImplicit(preEligible, contextual)
             prevResult match
@@ -1621,7 +1621,8 @@ trait Implicits:
                           |Current result ${showResult(prevResult)} will be no longer eligible
                           |  because it is not defined before the search position.
                           |Result with new rules: ${showResult(result)}.
-                          |To opt into the new rules, use the `experimental.givenLoopPrevention` language import.
+                          |To opt into the new rules, compile with `-source future` or use
+                          |the `scala.language.future` language import.
                           |
                           |To fix the problem without the language import, you could try one of the following:
                           |  - use a `given ... with` clause as the enclosing given,

diff --git a/docs/_docs/reference/changed-features/implicit-resolution.md b/docs/_docs/reference/changed-features/implicit-resolution.md
@@ -164,3 +164,35 @@ The new rules are as follows: An implicit `a` defined in `A` is more specific th
 Condition (*) is new. It is necessary to ensure that the defined relation is transitive.
 
 [//]: # todo: expand with precise rules
+
+**9.** The following change is currently enabled in `-source future`:
+
+Implicit resolution now avoids generating recursive givens that can lead to an infinite loop at runtime. Here is an example:
+
+```scala
+object Prices {
+  opaque type Price = BigDecimal
+
+  object Price{
+    given Ordering[Price] = summon[Ordering[BigDecimal]] // was error, now avoided
+  }
+}
+```
+
+Previously, implicit resolution would resolve the `summon` to the given in `Price`, leading to an infinite loop (a warning was issued in that case). We now use the underlying given in `BigDecimal` instead. We achieve that by adding the following rule for implicit search:
+
+ - When doing an implicit search while checking the implementation of a `given` definition `G` of the form
+    ```
+    given ... = ....
+    ```
+    discard all search results that lead back to `G` or to a given with the same owner as `G` that comes later in the source than `G`.
+
+The new behavior is currently enabled in `source.future` and will be enabled at the earliest in Scala 3.6. For earlier source versions, the behavior is as
+follows:
+
+ - Scala 3.3: no change
+ - Scala 3.4: A warning is issued where the behavior will change in 3.future.
+ - Scala 3.5: An error is issued where the behavior will change in 3.future.
+
+Old-style implicit definitions are unaffected by this change.
+
diff --git a/docs/sidebar.yml b/docs/sidebar.yml
@@ -153,7 +153,6 @@ subsection:
           - page: reference/experimental/cc.md
           - page: reference/experimental/purefuns.md
           - page: reference/experimental/tupled-function.md
-          - page: reference/experimental/given-loop-prevention.md
       - page: reference/syntax.md
       - title: Language Versions
         index: reference/language-versions/language-versions.md

diff --git a/library/src/scala/runtime/stdLibPatches/language.scala b/library/src/scala/runtime/stdLibPatches/language.scala
@@ -91,15 +91,6 @@ object language:
     @compileTimeOnly("`into` can only be used at compile time in import statements")
     object into
 
-    /** Experimental support for new given resolution rules that avoid looping
-     *  givens. By the new rules, a given may not implicitly use itself or givens
-     *  defined after it.
-     *
-     *  @see [[https://dotty.epfl.ch/docs/reference/experimental/given-loop-prevention]]
-     */
-    @compileTimeOnly("`givenLoopPrevention` can only be used at compile time in import statements")
-    object givenLoopPrevention
-
     /** Was needed to add support for relaxed imports of extension methods.
       * The language import is no longer needed as this is now a standard feature since SIP was accepted.
       * @see [[http://dotty.epfl.ch/docs/reference/contextual/extension-methods]]

diff --git a/tests/neg/i15474.check b/tests/neg/i15474.check
@@ -1,29 +1,31 @@
 -- Error: tests/neg/i15474.scala:6:39 ----------------------------------------------------------------------------------
 6 |  given c: Conversion[ String, Int ] = _.toInt   // error
   |                                       ^
-  |                            Result of implicit search for ?{ toInt: ? } will change.
-  |                            Current result Test2.c will be no longer eligible
-  |                              because it is not defined before the search position.
-  |                            Result with new rules: augmentString.
-  |                            To opt into the new rules, use the `experimental.givenLoopPrevention` language import.
+  |                               Result of implicit search for ?{ toInt: ? } will change.
+  |                               Current result Test2.c will be no longer eligible
+  |                                 because it is not defined before the search position.
+  |                               Result with new rules: augmentString.
+  |                               To opt into the new rules, compile with `-source future` or use
+  |                               the `scala.language.future` language import.
   |
-  |                            To fix the problem without the language import, you could try one of the following:
-  |                              - use a `given ... with` clause as the enclosing given,
-  |                              - rearrange definitions so that Test2.c comes earlier,
-  |                              - use an explicit conversion,
-  |                              - use an import to get extension method into scope.
-  |                            This will be an error in Scala 3.5 and later.
+  |                               To fix the problem without the language import, you could try one of the following:
+  |                                 - use a `given ... with` clause as the enclosing given,
+  |                                 - rearrange definitions so that Test2.c comes earlier,
+  |                                 - use an explicit conversion,
+  |                                 - use an import to get extension method into scope.
+  |                               This will be an error in Scala 3.5 and later.
 -- Error: tests/neg/i15474.scala:12:56 ---------------------------------------------------------------------------------
 12 |    given Ordering[Price] = summon[Ordering[BigDecimal]] // error
    |                                                        ^
-   |                          Result of implicit search for Ordering[BigDecimal] will change.
-   |                          Current result Prices.Price.given_Ordering_Price will be no longer eligible
-   |                            because it is not defined before the search position.
-   |                          Result with new rules: scala.math.Ordering.BigDecimal.
-   |                          To opt into the new rules, use the `experimental.givenLoopPrevention` language import.
+   |                             Result of implicit search for Ordering[BigDecimal] will change.
+   |                             Current result Prices.Price.given_Ordering_Price will be no longer eligible
+   |                               because it is not defined before the search position.
+   |                             Result with new rules: scala.math.Ordering.BigDecimal.
+   |                             To opt into the new rules, compile with `-source future` or use
+   |                             the `scala.language.future` language import.
    |
-   |                          To fix the problem without the language import, you could try one of the following:
-   |                            - use a `given ... with` clause as the enclosing given,
-   |                            - rearrange definitions so that Prices.Price.given_Ordering_Price comes earlier,
-   |                            - use an explicit argument.
-   |                          This will be an error in Scala 3.5 and later.
+   |                             To fix the problem without the language import, you could try one of the following:
+   |                               - use a `given ... with` clause as the enclosing given,
+   |                               - rearrange definitions so that Prices.Price.given_Ordering_Price comes earlier,
+   |                               - use an explicit argument.
+   |                             This will be an error in Scala 3.5 and later.
diff --git a/tests/neg/i6716.check b/tests/neg/i6716.check
@@ -1,14 +1,15 @@
 -- Error: tests/neg/i6716.scala:12:39 ----------------------------------------------------------------------------------
 12 |  given Monad[Bar] = summon[Monad[Foo]] // error
    |                                       ^
-   |                          Result of implicit search for Monad[Foo] will change.
-   |                          Current result Bar.given_Monad_Bar will be no longer eligible
-   |                            because it is not defined before the search position.
-   |                          Result with new rules: Foo.given_Monad_Foo.
-   |                          To opt into the new rules, use the `experimental.givenLoopPrevention` language import.
+   |                             Result of implicit search for Monad[Foo] will change.
+   |                             Current result Bar.given_Monad_Bar will be no longer eligible
+   |                               because it is not defined before the search position.
+   |                             Result with new rules: Foo.given_Monad_Foo.
+   |                             To opt into the new rules, compile with `-source future` or use
+   |                             the `scala.language.future` language import.
    |
-   |                          To fix the problem without the language import, you could try one of the following:
-   |                            - use a `given ... with` clause as the enclosing given,
-   |                            - rearrange definitions so that Bar.given_Monad_Bar comes earlier,
-   |                            - use an explicit argument.
-   |                          This will be an error in Scala 3.5 and later.
+   |                             To fix the problem without the language import, you could try one of the following:
+   |                               - use a `given ... with` clause as the enclosing given,
+   |                               - rearrange definitions so that Bar.given_Monad_Bar comes earlier,
+   |                               - use an explicit argument.
+   |                             This will be an error in Scala 3.5 and later.
diff --git a/tests/neg/i7294-a.check b/tests/neg/i7294-a.check
@@ -14,7 +14,8 @@
    |            Current result foo.Test.f will be no longer eligible
    |              because it is not defined before the search position.
    |            Result with new rules: No Matching Implicit.
-   |            To opt into the new rules, use the `experimental.givenLoopPrevention` language import.
+   |            To opt into the new rules, compile with `-source future` or use
+   |            the `scala.language.future` language import.
    |
    |            To fix the problem without the language import, you could try one of the following:
    |              - use a `given ... with` clause as the enclosing given,

diff --git a/tests/pos/given-loop-prevention.scala b/tests/pos/given-loop-prevention.scala
@@ -0,0 +1,14 @@
+//> using options -Xfatal-warnings
+
+class Foo
+
+object Bar {
+  given Foo with {}
+  given List[Foo] = List(summon[Foo]) // ok
+}
+
+object Baz {
+  @annotation.nowarn
+  given List[Foo] = List(summon[Foo]) // gives a warning, which is suppressed
+  given Foo with {}
+}
diff --git a/tests/pos/i15474.scala b/tests/pos/i15474.scala
@@ -1,6 +1,6 @@
 //> using options -Xfatal-warnings
 import scala.language.implicitConversions
-import scala.language.experimental.givenLoopPrevention
+import scala.language.future
 
 object Test2:
   given c: Conversion[ String, Int ] = _.toInt   // now avoided, was loop not detected, could be used as a fallback to avoid the warning.

diff --git a/tests/pos/i6716.scala b/tests/pos/i6716.scala
@@ -0,0 +1,14 @@
+//> using options -Xfatal-warnings -source 3.4
+
+class Foo
+
+object Bar {
+  given Foo with {}
+  given List[Foo] = List(summon[Foo]) // ok
+}
+
+object Baz {
+  @annotation.nowarn
+  given List[Foo] = List(summon[Foo]) // gives a warning, which is suppressed
+  given Foo with {}
+}
diff --git a/tests/pos/looping-givens.scala b/tests/pos/looping-givens.scala
@@ -1,4 +1,4 @@
-import language.experimental.givenLoopPrevention
+import language.future
 
 class A
 class B

diff --git a/tests/run/i6716.scala b/tests/run/i6716.scala
@@ -1,6 +1,4 @@
-//> using options -Xfatal-warnings
-
-import scala.language.experimental.givenLoopPrevention
+//> using options -Xfatal-warnings -source future
 
 trait Monad[T]:
   def id: String
@@ -11,7 +9,7 @@ object Foo {
 
 opaque type Bar = Foo
 object Bar {
-  given Monad[Bar] = summon[Monad[Foo]] // was error, fixed by givenLoopPrevention
+  given Monad[Bar] = summon[Monad[Foo]] // was error, fixed by given loop prevention
 }
 
 object Test extends App {