Avoid generating given definitions that loop (#19282)

Fixes a long-standing footgun of implicit resolution: givens that loop
back to themselves.

Author: odersky

community-build/community-projects/specs2

@@ -33,6 +33,7 @@ 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)
 

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

@@ -26,8 +26,8 @@ import Scopes.newScope
 import Typer.BindingPrec, BindingPrec.*
 import Hashable.*
 import util.{EqHashMap, Stats}
-import config.{Config, Feature}
-import Feature.migrateTo3
+import config.{Config, Feature, SourceVersion}
+import Feature.{migrateTo3, sourceVersion}
 import config.Printers.{implicits, implicitsDetailed}
 import collection.mutable
 import reporting.*
@@ -324,7 +324,7 @@ object Implicits:
     /** Is this the outermost implicits? This is the case if it either the implicits
      *  of NoContext, or the last one before it.
      */
-    private def isOuterMost = {
+    private def isOutermost = {
       val finalImplicits = NoContext.implicits
       (this eq finalImplicits) || (outerImplicits eqn finalImplicits)
     }
@@ -356,7 +356,7 @@ object Implicits:
       Stats.record("uncached eligible")
       if monitored then record(s"check uncached eligible refs in irefCtx", refs.length)
       val ownEligible = filterMatching(tp)
-      if isOuterMost then ownEligible
+      if isOutermost then ownEligible
       else combineEligibles(ownEligible, outerImplicits.nn.uncachedEligible(tp))
 
     /** The implicit references that are eligible for type `tp`. */
@@ -383,7 +383,7 @@ object Implicits:
     private def computeEligible(tp: Type): List[Candidate] = /*>|>*/ trace(i"computeEligible $tp in $refs%, %", implicitsDetailed) /*<|<*/ {
       if (monitored) record(s"check eligible refs in irefCtx", refs.length)
       val ownEligible = filterMatching(tp)
-      if isOuterMost then ownEligible
+      if isOutermost then ownEligible
       else combineEligibles(ownEligible, outerImplicits.nn.eligible(tp))
     }
 
@@ -392,7 +392,7 @@ object Implicits:
 
     override def toString: String = {
       val own = i"(implicits: $refs%, %)"
-      if (isOuterMost) own else own + "\n " + outerImplicits
+      if (isOutermost) own else own + "\n " + outerImplicits
     }
 
     /** This context, or a copy, ensuring root import from symbol `root`
@@ -1550,34 +1550,117 @@ trait Implicits:
       case _ =>
         tp.isAny || tp.isAnyRef
 
-    private def searchImplicit(contextual: Boolean): SearchResult =
+    /** Search implicit in context `ctxImplicits` or else in implicit scope
+     *  of expected type if `ctxImplicits == null`.
+     */
+    private def searchImplicit(ctxImplicits: ContextualImplicits | Null): SearchResult =
       if isUnderspecified(wildProto) then
         SearchFailure(TooUnspecific(pt), span)
       else
-        val eligible =
+        val contextual = ctxImplicits != null
+        val preEligible = // the eligible candidates, ignoring positions
           if contextual then
             if ctx.gadt.isNarrowing then
               withoutMode(Mode.ImplicitsEnabled) {
                 ctx.implicits.uncachedEligible(wildProto)
               }
             else ctx.implicits.eligible(wildProto)
           else implicitScope(wildProto).eligible
-        searchImplicit(eligible, contextual) match
+
+        /** Does candidate `cand` come too late for it to be considered as an
+         *  eligible candidate? This is the case if `cand` appears in the same
+         *  scope as a given definition of the form `given ... = ...` that
+         *  encloses the search point and `cand` comes later in the source or
+         *  coincides with that given definition.
+         */
+        def comesTooLate(cand: Candidate): Boolean =
+          val candSym = cand.ref.symbol
+          def candSucceedsGiven(sym: Symbol): Boolean =
+            val owner = sym.owner
+            if owner == candSym.owner then
+              sym.is(GivenVal) && sym.span.exists && sym.span.start <= candSym.span.start
+            else if owner.isClass then false
+            else candSucceedsGiven(owner)
+
+          ctx.isTyper
+          && !candSym.isOneOf(TermParamOrAccessor | Synthetic)
+          && candSym.span.exists
+          && candSucceedsGiven(ctx.owner)
+        end comesTooLate
+
+        val eligible = // the eligible candidates that come before the search point
+          if contextual && sourceVersion.isAtLeast(SourceVersion.`3.4`)
+          then preEligible.filterNot(comesTooLate)
+          else preEligible
+
+        def checkResolutionChange(result: SearchResult) =
+          if (eligible ne preEligible)
+              && !Feature.enabled(Feature.givenLoopPrevention)
+          then
+            val prevResult = searchImplicit(preEligible, contextual)
+            prevResult match
+              case prevResult: SearchSuccess =>
+                def remedy = pt match
+                  case _: SelectionProto =>
+                    "conversion,\n  - use an import to get extension method into scope"
+                  case _: ViewProto =>
+                    "conversion"
+                  case _ =>
+                    "argument"
+
+                def showResult(r: SearchResult) = r match
+                  case r: SearchSuccess => ctx.printer.toTextRef(r.ref).show
+                  case r => r.show
+
+                result match
+                  case result: SearchSuccess if prevResult.ref frozen_=:= result.ref =>
+                    // OK
+                  case _ =>
+                    val msg =
+                      em"""Result of implicit search for $pt will change.
+                          |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 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 ${showResult(prevResult)} comes earlier,
+                          |  - use an explicit $remedy."""
+                    if sourceVersion.isAtLeast(SourceVersion.`3.5`)
+                    then report.error(msg, srcPos)
+                    else report.warning(msg.append("\nThis will be an error in Scala 3.5 and later."), srcPos)
+              case _ =>
+            prevResult
+          else result
+        end checkResolutionChange
+
+        val result = searchImplicit(eligible, contextual)
+        result match
           case result: SearchSuccess =>
-            result
+            checkResolutionChange(result)
           case failure: SearchFailure =>
             failure.reason match
               case _: AmbiguousImplicits => failure
               case reason =>
                 if contextual then
-                  searchImplicit(contextual = false).recoverWith {
-                    failure2 => failure2.reason match
-                      case _: AmbiguousImplicits => failure2
-                      case _ =>
-                        reason match
-                          case (_: DivergingImplicit) => failure
-                          case _ => List(failure, failure2).maxBy(_.tree.treeSize)
-                  }
+                  // If we filtered out some candidates for being too late, we should
+                  // do another contextual search further out, since the dropped candidates
+                  // might have shadowed an eligible candidate in an outer level.
+                  // Otherwise, proceed with a search of the implicit scope.
+                  val newCtxImplicits =
+                    if eligible eq preEligible then null
+                    else ctxImplicits.nn.outerImplicits: ContextualImplicits | Null
+                      // !!! Dotty problem: without the ContextualImplicits | Null type ascription
+                      // we get a Ycheck failure after arrayConstructors due to "Types differ"
+                  checkResolutionChange:
+                    searchImplicit(newCtxImplicits).recoverWith:
+                      failure2 => failure2.reason match
+                        case _: AmbiguousImplicits => failure2
+                        case _ =>
+                          reason match
+                            case (_: DivergingImplicit) => failure
+                            case _ => List(failure, failure2).maxBy(_.tree.treeSize)
                 else failure
     end searchImplicit
 
@@ -1595,7 +1678,7 @@ trait Implicits:
         case ref: TermRef =>
           SearchSuccess(tpd.ref(ref).withSpan(span.startPos), ref, 0)(ctx.typerState, ctx.gadt)
         case _ =>
-          searchImplicit(contextual = true)
+          searchImplicit(ctx.implicits)
     end bestImplicit
 
     def implicitScope(tp: Type): OfTypeImplicits = ctx.run.nn.implicitScope(tp)

compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -163,8 +163,4 @@ 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

docs/_docs/reference/changed-features/implicit-resolution.md

@@ -0,0 +1,31 @@
+---
+layout: doc-page
+title: Given Loop Prevention
+redirectFrom: /docs/reference/other-new-features/into-modifier.html
+nightlyOf: https://docs.scala-lang.org/scala3/reference/experimental/into-modifier.html
+---
+
+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 enabled with the `experimental.givenLoopPrevention` language import. If no such import or setting is given, a warning is issued where the behavior would change under that import (for source version 3.4 and later).
+
+Old-style implicit definitions are unaffected by this change.
+

docs/_docs/reference/experimental/given-loop-prevention.md

@@ -153,6 +153,7 @@ 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

docs/sidebar.yml

@@ -91,6 +91,15 @@ 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]]

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

@@ -0,0 +1,29 @@
+-- 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.
+  |
+  |                            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.
+   |
+   |                          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.

tests/neg/i15474.check

@@ -2,17 +2,15 @@
 
 import scala.language.implicitConversions
 
-object Test1:
-  given c: Conversion[ String, Int ] with
-    def apply(from: String): Int = from.toInt   // error
-
 object Test2:
-  given c: Conversion[ String, Int ] = _.toInt   // loop not detected, could be used as a fallback to avoid the warning.
+  given c: Conversion[ String, Int ] = _.toInt   // error
 
 object Prices {
   opaque type Price = BigDecimal
 
   object Price{
     given Ordering[Price] = summon[Ordering[BigDecimal]] // error
   }
-}
+}
+
+

tests/neg/i15474.scala

@@ -0,0 +1,5 @@
+-- Error: tests/neg/i15474b.scala:7:40 ---------------------------------------------------------------------------------
+7 |    def apply(from: String): Int = from.toInt   // error: infinite loop in function body
+  |                                   ^^^^^^^^^^
+  |                                   Infinite loop in function body
+  |                                   Test1.c.apply(from).toInt