Backport "Avoid generating given definitions that loop" (#19477)

Backports #19282 with later improvements (#19392, #19411) to 3.4.0

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

@@ -10,6 +10,7 @@ enum SourceVersion:
   case `3.2-migration`, `3.2`
   case `3.3-migration`, `3.3`
   case `3.4-migration`, `3.4`
+  case `3.5-migration`, `3.5`
   // !!! Keep in sync with scala.runtime.stdlibPatches.language !!!
   case `future-migration`, `future`
 

compiler/src/dotty/tools/dotc/typer/Implicits.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.*
@@ -93,7 +93,7 @@ object Implicits:
       if (initctx eq NoContext) initctx else initctx.retractMode(Mode.ImplicitsEnabled)
     protected given Context = irefCtx
 
-    /** The nesting level of this context. Non-zero only in ContextialImplicits */
+    /** The nesting level of this context. Non-zero only in ContextualImplicits */
     def level: Int = 0
 
     /** The implicit references */
@@ -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`
@@ -408,6 +408,13 @@ object Implicits:
       }
   }
 
+  /** Search mode to use for possibly avoiding looping givens */
+  enum SearchMode:
+    case Old,          // up to 3.3, old mode w/o protection
+    	 CompareWarn,  // from 3.4, old mode, warn if new mode would change result
+    	 CompareErr,   // from 3.5, old mode, error if new mode would change result
+    	 New           // from future, new mode where looping givens are avoided
+
   /** The result of an implicit search */
   sealed abstract class SearchResult extends Showable {
     def tree: Tree
@@ -1550,35 +1557,113 @@ 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, mode: SearchMode): SearchResult =
       if isUnderspecified(wildProto) then
         SearchFailure(TooUnspecific(pt), span)
       else
-        val eligible =
-          if contextual then
+        val contextual = ctxImplicits != null
+        val preEligible = // the eligible candidates, ignoring positions
+          if ctxImplicits != null then
             if ctx.gadt.isNarrowing then
               withoutMode(Mode.ImplicitsEnabled) {
-                ctx.implicits.uncachedEligible(wildProto)
+                ctxImplicits.uncachedEligible(wildProto)
               }
-            else ctx.implicits.eligible(wildProto)
+            else ctxImplicits.eligible(wildProto)
           else implicitScope(wildProto).eligible
-        searchImplicit(eligible, contextual) match
-          case result: SearchSuccess =>
-            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)
-                  }
-                else failure
+
+        /** 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 && mode != SearchMode.Old
+          then preEligible.filterNot(comesTooLate)
+          else preEligible
+
+        def checkResolutionChange(result: SearchResult) =
+          if (eligible ne preEligible) && mode != SearchMode.New then
+            searchImplicit(preEligible, contextual) 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, 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 ${showResult(prevResult)} comes earlier,
+                          |  - use an explicit $remedy."""
+                    if mode == SearchMode.CompareErr
+                    then report.error(msg, srcPos)
+                    else report.warning(msg.append("\nThis will be an error in Scala 3.5 and later."), srcPos)
+                prevResult
+              case prevResult: SearchFailure => result
+          else result
+        end checkResolutionChange
+
+        checkResolutionChange:
+          searchImplicit(eligible, contextual).recoverWith:
+            case failure: SearchFailure =>
+              failure.reason match
+                case _: AmbiguousImplicits => failure
+                case reason =>
+                  if contextual then
+                    // 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"
+                    searchImplicit(newCtxImplicits, SearchMode.New).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
 
     /** Find a unique best implicit reference */
@@ -1595,7 +1680,11 @@ 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,
+            if sourceVersion.isAtLeast(SourceVersion.future) then SearchMode.New
+            else if sourceVersion.isAtLeast(SourceVersion.`3.5`) then SearchMode.CompareErr
+            else if sourceVersion.isAtLeast(SourceVersion.`3.4`) then SearchMode.CompareWarn
+            else SearchMode.Old)
     end bestImplicit
 
     def implicitScope(tp: Type): OfTypeImplicits = ctx.run.nn.implicitScope(tp)

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

@@ -163,8 +163,36 @@ 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.
 
-[//]: # todo: expand with precise rules

docs/_docs/reference/experimental/given-loop-prevention.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.
+

tests/neg/i15474.check

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

tests/neg/i15474.scala

@@ -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/i15474b.check

@@ -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

tests/neg/i15474b.scala

@@ -0,0 +1,8 @@
+//> using options -Xfatal-warnings
+
+import scala.language.implicitConversions
+
+object Test1:
+  given c: Conversion[ String, Int ] with
+    def apply(from: String): Int = from.toInt   // error: infinite loop in function body
+

tests/neg/i6716.check

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