From 5342719fe6745ab2c745a56b2fa61b967af5196b Mon Sep 17 00:00:00 2001 From: odersky Date: Sun, 17 Dec 2023 13:45:43 +0100 Subject: [PATCH 01/11] Avoid generating given definitions that loop --- .../dotty/tools/dotc/typer/Implicits.scala | 73 ++++++++++++++++--- .../changed-features/implicit-resolution.md | 21 +++++- tests/neg/i15474.check | 6 ++ tests/neg/i15474.scala | 8 +- tests/neg/i6716.check | 6 ++ tests/neg/i6716.scala | 18 +++++ tests/pos/i15474.scala | 20 +++++ tests/pos/i6716.scala | 15 ---- tests/run/i17115.check | 2 + tests/{pos => run}/i17115.scala | 0 tests/run/i6716.check | 2 + tests/run/i6716.scala | 20 +++++ 12 files changed, 161 insertions(+), 30 deletions(-) create mode 100644 tests/neg/i15474.check create mode 100644 tests/neg/i6716.check create mode 100644 tests/neg/i6716.scala create mode 100644 tests/pos/i15474.scala delete mode 100644 tests/pos/i6716.scala create mode 100644 tests/run/i17115.check rename tests/{pos => run}/i17115.scala (100%) create mode 100644 tests/run/i6716.check create mode 100644 tests/run/i6716.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index ff23e8180f1c..bb35306f696c 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/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.* @@ -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,11 +1550,15 @@ 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) { @@ -1562,6 +1566,43 @@ trait Implicits: } else ctx.implicits.eligible(wildProto) else implicitScope(wildProto).eligible + + /** 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 enclosing the search point and 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 = + if sym.owner == candSym.owner then + if sym.is(ModuleClass) then candSucceedsGiven(sym.sourceModule) + else sym.is(Given) && sym.span.exists && sym.span.start <= candSym.span.start + else if sym.is(Package) then false + else candSucceedsGiven(sym.owner) + + ctx.isTyper + && !candSym.isOneOf(TermParamOrAccessor | Synthetic) + && candSym.span.exists + && candSucceedsGiven(ctx.owner) + end comesTooLate + + val eligible = if contextual then preEligible.filterNot(comesTooLate) else preEligible + + def checkResolutionChange(result: SearchResult) = result match + case result: SearchSuccess + if (eligible ne preEligible) && !sourceVersion.isAtLeast(SourceVersion.`future`) => + searchImplicit(preEligible.diff(eligible), contextual) match + case prevResult: SearchSuccess => + report.error( + em"""Warning: result of implicit search for $pt will change. + |current result: ${prevResult.ref.symbol.showLocated} + |result with -source future: ${result.ref.symbol.showLocated}""", + srcPos + ) + case _ => + case _ => + searchImplicit(eligible, contextual) match case result: SearchSuccess => result @@ -1570,14 +1611,24 @@ trait Implicits: case _: AmbiguousImplicits => failure case reason => if contextual then - searchImplicit(contextual = false).recoverWith { + // 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" + val result = searchImplicit(newCtxImplicits).recoverWith: failure2 => failure2.reason match case _: AmbiguousImplicits => failure2 case _ => reason match case (_: DivergingImplicit) => failure case _ => List(failure, failure2).maxBy(_.tree.treeSize) - } + checkResolutionChange(result) + result else failure end searchImplicit @@ -1595,7 +1646,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) diff --git a/docs/_docs/reference/changed-features/implicit-resolution.md b/docs/_docs/reference/changed-features/implicit-resolution.md index 6a898690b565..ab8293724a4e 100644 --- a/docs/_docs/reference/changed-features/implicit-resolution.md +++ b/docs/_docs/reference/changed-features/implicit-resolution.md @@ -163,8 +163,27 @@ 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.** Implicit resolution now tries to avoid 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`, 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 under `-source future`. In earlier versions, a +warning is issued where that behavior will change. + +Old-style implicit definitions are unaffected by this change. -[//]: # todo: expand with precise rules diff --git a/tests/neg/i15474.check b/tests/neg/i15474.check new file mode 100644 index 000000000000..267a02a80786 --- /dev/null +++ b/tests/neg/i15474.check @@ -0,0 +1,6 @@ +-- Error: tests/neg/i15474.scala:16:56 --------------------------------------------------------------------------------- +16 | given Ordering[Price] = summon[Ordering[BigDecimal]] // error + | ^ + | Warning: result of implicit search for Ordering[BigDecimal] will change. + | current result: given instance given_Ordering_Price in object Price + | result with -source future: object BigDecimal in object Ordering diff --git a/tests/neg/i15474.scala b/tests/neg/i15474.scala index 8edf97a1e55a..c5cf934bdd7a 100644 --- a/tests/neg/i15474.scala +++ b/tests/neg/i15474.scala @@ -4,10 +4,10 @@ import scala.language.implicitConversions object Test1: given c: Conversion[ String, Int ] with - def apply(from: String): Int = from.toInt // error + def apply(from: String): Int = from.toInt // was error, now avoided 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 // now avoided, was loop not detected, could be used as a fallback to avoid the warning. object Prices { opaque type Price = BigDecimal @@ -15,4 +15,6 @@ object Prices { object Price{ given Ordering[Price] = summon[Ordering[BigDecimal]] // error } -} \ No newline at end of file +} + + diff --git a/tests/neg/i6716.check b/tests/neg/i6716.check new file mode 100644 index 000000000000..1e1359442bec --- /dev/null +++ b/tests/neg/i6716.check @@ -0,0 +1,6 @@ +-- Error: tests/neg/i6716.scala:12:39 ---------------------------------------------------------------------------------- +12 | given Monad[Bar] = summon[Monad[Foo]] // error + | ^ + | Warning: result of implicit search for Monad[Foo] will change. + | current result: given instance given_Monad_Bar in object Bar + | result with -source future: object given_Monad_Foo in object Foo diff --git a/tests/neg/i6716.scala b/tests/neg/i6716.scala new file mode 100644 index 000000000000..bbbd9d6d6cd0 --- /dev/null +++ b/tests/neg/i6716.scala @@ -0,0 +1,18 @@ +//> using options -Xfatal-warnings + +trait Monad[T]: + def id: String +class Foo +object Foo { + given Monad[Foo] with { def id = "Foo" } +} + +opaque type Bar = Foo +object Bar { + given Monad[Bar] = summon[Monad[Foo]] // error +} + +object Test extends App { + println(summon[Monad[Foo]].id) + println(summon[Monad[Bar]].id) +} \ No newline at end of file diff --git a/tests/pos/i15474.scala b/tests/pos/i15474.scala new file mode 100644 index 000000000000..e40e11d84581 --- /dev/null +++ b/tests/pos/i15474.scala @@ -0,0 +1,20 @@ +//> using options -Xfatal-warnings +import scala.language.implicitConversions +import language.future + +object Test1: + given c: Conversion[ String, Int ] with + def apply(from: String): Int = from.toInt // was error, now avoided + +object Test2: + given c: Conversion[ String, Int ] = _.toInt // now avoided, was loop not detected, could be used as a fallback to avoid the warning. + +object Prices { + opaque type Price = BigDecimal + + object Price{ + given Ordering[Price] = summon[Ordering[BigDecimal]] // was error, now avoided + } +} + + diff --git a/tests/pos/i6716.scala b/tests/pos/i6716.scala deleted file mode 100644 index 446cd49c9214..000000000000 --- a/tests/pos/i6716.scala +++ /dev/null @@ -1,15 +0,0 @@ -trait Monad[T] -class Foo -object Foo { - given Monad[Foo] with {} -} - -opaque type Bar = Foo -object Bar { - given Monad[Bar] = summon[Monad[Foo]] -} - -object Test { - val mf = summon[Monad[Foo]] - val mb = summon[Monad[Bar]] -} \ No newline at end of file diff --git a/tests/run/i17115.check b/tests/run/i17115.check new file mode 100644 index 000000000000..61c83cba41ce --- /dev/null +++ b/tests/run/i17115.check @@ -0,0 +1,2 @@ +4 +5 diff --git a/tests/pos/i17115.scala b/tests/run/i17115.scala similarity index 100% rename from tests/pos/i17115.scala rename to tests/run/i17115.scala diff --git a/tests/run/i6716.check b/tests/run/i6716.check new file mode 100644 index 000000000000..bb85bd267288 --- /dev/null +++ b/tests/run/i6716.check @@ -0,0 +1,2 @@ +Foo +Foo diff --git a/tests/run/i6716.scala b/tests/run/i6716.scala new file mode 100644 index 000000000000..7c4e7fe394d8 --- /dev/null +++ b/tests/run/i6716.scala @@ -0,0 +1,20 @@ +//> using options -Xfatal-warnings + +import language.future + +trait Monad[T]: + def id: String +class Foo +object Foo { + given Monad[Foo] with { def id = "Foo" } +} + +opaque type Bar = Foo +object Bar { + given Monad[Bar] = summon[Monad[Foo]] // error +} + +object Test extends App { + println(summon[Monad[Foo]].id) + println(summon[Monad[Bar]].id) +} \ No newline at end of file From 4547e1b82c6ae04fce01e173983ef32bdc3018ab Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 18 Dec 2023 14:10:56 +0100 Subject: [PATCH 02/11] Use experimental language import to enable the new behavior --- .../src/dotty/tools/dotc/config/Feature.scala | 1 + .../dotty/tools/dotc/semanticdb/Scala3.scala | 3 ++ .../dotty/tools/dotc/typer/Implicits.scala | 47 ++++++++++++++----- .../quoted/runtime/impl/QuotesImpl.scala | 4 ++ .../runtime/stdLibPatches/language.scala | 9 ++++ tests/neg/i15474.check | 12 +++-- tests/neg/i6716.check | 10 +++- tests/neg/i7294-a.check | 23 +++++++++ tests/neg/i7294-a.scala | 2 +- tests/neg/i7294-b.scala | 2 +- tests/pos/i15474.scala | 2 +- tests/run/i6716.scala | 4 +- 12 files changed, 96 insertions(+), 23 deletions(-) create mode 100644 tests/neg/i7294-a.check diff --git a/compiler/src/dotty/tools/dotc/config/Feature.scala b/compiler/src/dotty/tools/dotc/config/Feature.scala index fa262a5880ff..e8ca30ecb243 100644 --- a/compiler/src/dotty/tools/dotc/config/Feature.scala +++ b/compiler/src/dotty/tools/dotc/config/Feature.scala @@ -33,6 +33,7 @@ object Feature: val pureFunctions = experimental("pureFunctions") val captureChecking = experimental("captureChecking") val into = experimental("into") + val avoidLoopingGivens = experimental("avoidLoopingGivens") val globalOnlyImports: Set[TermName] = Set(pureFunctions, captureChecking) diff --git a/compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala b/compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala index f49b00089712..fdb9251951e5 100644 --- a/compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala +++ b/compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala @@ -77,6 +77,9 @@ object Scala3: type SemanticSymbol = Symbol | FakeSymbol given SemanticSymbolOps : AnyRef with + import SymbolOps.* + import StringOps.* + extension (sym: SemanticSymbol) def name(using Context): Name = sym match case s: Symbol => s.name diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index bb35306f696c..7cdc3d4a2508 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1589,19 +1589,40 @@ trait Implicits: val eligible = if contextual then preEligible.filterNot(comesTooLate) else preEligible - def checkResolutionChange(result: SearchResult) = result match - case result: SearchSuccess - if (eligible ne preEligible) && !sourceVersion.isAtLeast(SourceVersion.`future`) => - searchImplicit(preEligible.diff(eligible), contextual) match - case prevResult: SearchSuccess => - report.error( - em"""Warning: result of implicit search for $pt will change. - |current result: ${prevResult.ref.symbol.showLocated} - |result with -source future: ${result.ref.symbol.showLocated}""", - srcPos - ) - case _ => - case _ => + def checkResolutionChange(result: SearchResult) = + if (eligible ne preEligible) + && !Feature.enabled(Feature.avoidLoopingGivens) + then searchImplicit(preEligible.diff(eligible), 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 _ => + report.error( + em"""Warning: 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 `avoidLoopingGivens` language import, + | + |To fix the problem you could try one of the following: + | - rearrange definitions, + | - use an explicit $remedy.""", + srcPos) + case _ => + end checkResolutionChange searchImplicit(eligible, contextual) match case result: SearchSuccess => diff --git a/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala b/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala index 51f133c972b4..1203e309c484 100644 --- a/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala +++ b/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala @@ -1781,6 +1781,8 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler end TypeRepr given TypeReprMethods: TypeReprMethods with + import SymbolMethods.* + extension (self: TypeRepr) def show(using printer: Printer[TypeRepr]): String = printer.show(self) @@ -2608,6 +2610,8 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler end Symbol given SymbolMethods: SymbolMethods with + import FlagsMethods.* + extension (self: Symbol) def owner: Symbol = self.denot.owner def maybeOwner: Symbol = self.denot.maybeOwner diff --git a/library/src/scala/runtime/stdLibPatches/language.scala b/library/src/scala/runtime/stdLibPatches/language.scala index c2a12cec2ecc..9fa8bff120af 100644 --- a/library/src/scala/runtime/stdLibPatches/language.scala +++ b/library/src/scala/runtime/stdLibPatches/language.scala @@ -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/avoid-looping-givens]] + */ + @compileTimeOnly("`avoidLoopingGivens` can only be used at compile time in import statements") + object avoidLoopingGivens + /** 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 index 267a02a80786..4bf344dc5a71 100644 --- a/tests/neg/i15474.check +++ b/tests/neg/i15474.check @@ -1,6 +1,12 @@ -- Error: tests/neg/i15474.scala:16:56 --------------------------------------------------------------------------------- 16 | given Ordering[Price] = summon[Ordering[BigDecimal]] // error | ^ - | Warning: result of implicit search for Ordering[BigDecimal] will change. - | current result: given instance given_Ordering_Price in object Price - | result with -source future: object BigDecimal in object Ordering + | Warning: 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 `avoidLoopingGivens` language import, + | + | To fix the problem you could try one of the following: + | - rearrange definitions, + | - use an explicit argument. diff --git a/tests/neg/i6716.check b/tests/neg/i6716.check index 1e1359442bec..3746eaafad50 100644 --- a/tests/neg/i6716.check +++ b/tests/neg/i6716.check @@ -2,5 +2,11 @@ 12 | given Monad[Bar] = summon[Monad[Foo]] // error | ^ | Warning: result of implicit search for Monad[Foo] will change. - | current result: given instance given_Monad_Bar in object Bar - | result with -source future: object given_Monad_Foo in object Foo + | 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 `avoidLoopingGivens` language import, + | + | To fix the problem you could try one of the following: + | - rearrange definitions, + | - use an explicit argument. diff --git a/tests/neg/i7294-a.check b/tests/neg/i7294-a.check new file mode 100644 index 000000000000..9541f7979a7a --- /dev/null +++ b/tests/neg/i7294-a.check @@ -0,0 +1,23 @@ +-- Error: tests/neg/i7294-a.scala:6:10 --------------------------------------------------------------------------------- +6 | case x: T => x.g(10) // error // error + | ^ + | Warning: result of implicit search for scala.reflect.TypeTest[Nothing, T] will change. + | Current result foo.i7294-a$package.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 `avoidLoopingGivens` language import, + | + | To fix the problem you could try one of the following: + | - rearrange definitions, + | - use an explicit argument. + | + | where: T is a type in given instance f with bounds <: foo.Foo +-- [E007] Type Mismatch Error: tests/neg/i7294-a.scala:6:15 ------------------------------------------------------------ +6 | case x: T => x.g(10) // error // error + | ^ + | Found: (x : Nothing) + | Required: ?{ g: ? } + | Note that implicit conversions were not tried because the result of an implicit conversion + | must be more specific than ?{ g: [applied to (10) returning T] } + | + | longer explanation available when compiling with `-explain` diff --git a/tests/neg/i7294-a.scala b/tests/neg/i7294-a.scala index 13981fa4d375..538dc3159fb8 100644 --- a/tests/neg/i7294-a.scala +++ b/tests/neg/i7294-a.scala @@ -3,7 +3,7 @@ package foo trait Foo { def g(x: Int): Any } inline given f[T <: Foo]: T = ??? match { - case x: T => x.g(10) // error + case x: T => x.g(10) // error // error } @main def Test = f diff --git a/tests/neg/i7294-b.scala b/tests/neg/i7294-b.scala index 423d5037db96..b06d814444e8 100644 --- a/tests/neg/i7294-b.scala +++ b/tests/neg/i7294-b.scala @@ -3,7 +3,7 @@ package foo trait Foo { def g(x: Any): Any } inline given f[T <: Foo]: T = ??? match { - case x: T => x.g(10) // error + case x: T => x.g(10) // error // error } @main def Test = f diff --git a/tests/pos/i15474.scala b/tests/pos/i15474.scala index e40e11d84581..8adc5ad7233d 100644 --- a/tests/pos/i15474.scala +++ b/tests/pos/i15474.scala @@ -1,6 +1,6 @@ //> using options -Xfatal-warnings import scala.language.implicitConversions -import language.future +import scala.language.experimental.avoidLoopingGivens object Test1: given c: Conversion[ String, Int ] with diff --git a/tests/run/i6716.scala b/tests/run/i6716.scala index 7c4e7fe394d8..6208a52190fe 100644 --- a/tests/run/i6716.scala +++ b/tests/run/i6716.scala @@ -1,6 +1,6 @@ //> using options -Xfatal-warnings -import language.future +import scala.language.experimental.avoidLoopingGivens trait Monad[T]: def id: String @@ -11,7 +11,7 @@ object Foo { opaque type Bar = Foo object Bar { - given Monad[Bar] = summon[Monad[Foo]] // error + given Monad[Bar] = summon[Monad[Foo]] // was error fixed by avoidLoopingGivens } object Test extends App { From 4bb0aaaf39d3598ce0f443188a4c1f6b6c42fc10 Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 18 Dec 2023 17:52:30 +0100 Subject: [PATCH 03/11] Make the new behavior dependent on source >= 3.4 --- .../dotty/tools/dotc/typer/Implicits.scala | 91 ++++++++++--------- tests/neg/i15474.check | 42 +++++++-- tests/neg/i15474.scala | 4 +- tests/neg/i6716.check | 16 ++-- tests/neg/i7294-a.check | 18 ++-- tests/neg/looping-givens.scala | 9 ++ tests/pos/looping-givens.scala | 11 +++ 7 files changed, 122 insertions(+), 69 deletions(-) create mode 100644 tests/neg/looping-givens.scala create mode 100644 tests/pos/looping-givens.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 7cdc3d4a2508..77328cdbb33d 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1587,46 +1587,54 @@ trait Implicits: && candSucceedsGiven(ctx.owner) end comesTooLate - val eligible = if contextual then preEligible.filterNot(comesTooLate) else preEligible + 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.avoidLoopingGivens) - then searchImplicit(preEligible.diff(eligible), 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 _ => - report.error( - em"""Warning: 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 `avoidLoopingGivens` language import, - | - |To fix the problem you could try one of the following: - | - rearrange definitions, - | - use an explicit $remedy.""", - srcPos) - case _ => + 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 _ => + report.error( + em"""Warning: 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.avoidLoopingGivens` language import. + | + |To fix the problem without the language import, you could try one of the following: + | - rearrange definitions so that ${showResult(prevResult)} comes earlier, + | - use an explicit $remedy.""", + srcPos) + case _ => + prevResult + else result end checkResolutionChange - searchImplicit(eligible, contextual) match + val result = searchImplicit(eligible, contextual) + result match case result: SearchSuccess => - result + checkResolutionChange(result) case failure: SearchFailure => failure.reason match case _: AmbiguousImplicits => failure @@ -1641,15 +1649,14 @@ trait Implicits: 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" - val result = searchImplicit(newCtxImplicits).recoverWith: - failure2 => failure2.reason match - case _: AmbiguousImplicits => failure2 - case _ => - reason match - case (_: DivergingImplicit) => failure - case _ => List(failure, failure2).maxBy(_.tree.treeSize) - checkResolutionChange(result) - result + 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 diff --git a/tests/neg/i15474.check b/tests/neg/i15474.check index 4bf344dc5a71..0b23628d3051 100644 --- a/tests/neg/i15474.check +++ b/tests/neg/i15474.check @@ -1,12 +1,38 @@ +-- Error: tests/neg/i15474.scala:7:35 ---------------------------------------------------------------------------------- +7 | def apply(from: String): Int = from.toInt // error + | ^^^^ + | Warning: result of implicit search for ?{ toInt: ? } will change. + | Current result Test1.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.avoidLoopingGivens` language import. + | + | To fix the problem without the language import, you could try one of the following: + | - rearrange definitions so that Test1.c comes earlier, + | - use an explicit conversion, + | - use an import to get extension method into scope. +-- Error: tests/neg/i15474.scala:10:39 --------------------------------------------------------------------------------- +10 | given c: Conversion[ String, Int ] = _.toInt // error + | ^ + | Warning: 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.avoidLoopingGivens` language import. + | + | To fix the problem without the language import, you could try one of the following: + | - rearrange definitions so that Test2.c comes earlier, + | - use an explicit conversion, + | - use an import to get extension method into scope. -- Error: tests/neg/i15474.scala:16:56 --------------------------------------------------------------------------------- 16 | given Ordering[Price] = summon[Ordering[BigDecimal]] // error | ^ - | Warning: 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 `avoidLoopingGivens` language import, + | Warning: 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.avoidLoopingGivens` language import. | - | To fix the problem you could try one of the following: - | - rearrange definitions, - | - use an explicit argument. + | To fix the problem without the language import, you could try one of the following: + | - rearrange definitions so that Prices.Price.given_Ordering_Price comes earlier, + | - use an explicit argument. diff --git a/tests/neg/i15474.scala b/tests/neg/i15474.scala index c5cf934bdd7a..5a66ea016630 100644 --- a/tests/neg/i15474.scala +++ b/tests/neg/i15474.scala @@ -4,10 +4,10 @@ import scala.language.implicitConversions object Test1: given c: Conversion[ String, Int ] with - def apply(from: String): Int = from.toInt // was error, now avoided + def apply(from: String): Int = from.toInt // error object Test2: - given c: Conversion[ String, Int ] = _.toInt // now avoided, was 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 diff --git a/tests/neg/i6716.check b/tests/neg/i6716.check index 3746eaafad50..6771d736b6af 100644 --- a/tests/neg/i6716.check +++ b/tests/neg/i6716.check @@ -1,12 +1,12 @@ -- Error: tests/neg/i6716.scala:12:39 ---------------------------------------------------------------------------------- 12 | given Monad[Bar] = summon[Monad[Foo]] // error | ^ - | Warning: 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 `avoidLoopingGivens` language import, + | Warning: 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.avoidLoopingGivens` language import. | - | To fix the problem you could try one of the following: - | - rearrange definitions, - | - use an explicit argument. + | To fix the problem without the language import, you could try one of the following: + | - rearrange definitions so that Bar.given_Monad_Bar comes earlier, + | - use an explicit argument. diff --git a/tests/neg/i7294-a.check b/tests/neg/i7294-a.check index 9541f7979a7a..887635d89a35 100644 --- a/tests/neg/i7294-a.check +++ b/tests/neg/i7294-a.check @@ -5,19 +5,19 @@ | Current result foo.i7294-a$package.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 `avoidLoopingGivens` language import, + | To opt into the new rules, use the `experimental.avoidLoopingGivens` language import. | - | To fix the problem you could try one of the following: - | - rearrange definitions, + | To fix the problem without the language import, you could try one of the following: + | - rearrange definitions so that foo.i7294-a$package.f comes earlier, | - use an explicit argument. | | where: T is a type in given instance f with bounds <: foo.Foo --- [E007] Type Mismatch Error: tests/neg/i7294-a.scala:6:15 ------------------------------------------------------------ +-- [E007] Type Mismatch Error: tests/neg/i7294-a.scala:6:18 ------------------------------------------------------------ 6 | case x: T => x.g(10) // error // error - | ^ - | Found: (x : Nothing) - | Required: ?{ g: ? } - | Note that implicit conversions were not tried because the result of an implicit conversion - | must be more specific than ?{ g: [applied to (10) returning T] } + | ^^^^^^^ + | Found: Any + | Required: T + | + | where: T is a type in given instance f with bounds <: foo.Foo | | longer explanation available when compiling with `-explain` diff --git a/tests/neg/looping-givens.scala b/tests/neg/looping-givens.scala new file mode 100644 index 000000000000..572f1707861f --- /dev/null +++ b/tests/neg/looping-givens.scala @@ -0,0 +1,9 @@ +class A +class B + +given joint(using a: A, b: B): (A & B) = ??? + +def foo(using a: A, b: B) = + given aa: A = summon // error + given bb: B = summon // error + given ab: (A & B) = summon // error diff --git a/tests/pos/looping-givens.scala b/tests/pos/looping-givens.scala new file mode 100644 index 000000000000..ed11981c1bf6 --- /dev/null +++ b/tests/pos/looping-givens.scala @@ -0,0 +1,11 @@ +import language.experimental.avoidLoopingGivens + +class A +class B + +given joint(using a: A, b: B): (A & B) = ??? + +def foo(using a: A, b: B) = + given aa: A = summon // error + given bb: B = summon // error + given ab: (A & B) = summon // error From a48a759f00ffe4864b20b2f8b9b70288b8e3fb0a Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 18 Dec 2023 21:51:19 +0100 Subject: [PATCH 04/11] Restrict the new rules to givens that don't define a new class `given ... with` or `given ... = new { ... }` kinds of definitions now follow the old rules. This allows recursive `given...with` definitions as they are found in protoQuill. We still have the old check in a later phase against directly recursive methods. Of the three loops in the original i15474 we now detect #2 and #3 with new new restrictions. #1 slips through since it is a loop involving a `given...with` instance of `Conversion`, but is caught later with the recursive method check. Previously tests #1 and #3 were detected with the recursive methods check and #2 slipped through altogether. The new rules are enough for defining simple givens with `=` without fear of looping. --- .../dotty/tools/dotc/semanticdb/Scala3.scala | 3 --- .../dotty/tools/dotc/typer/Implicits.scala | 8 +++--- .../quoted/runtime/impl/QuotesImpl.scala | 4 --- tests/neg/i15474.check | 27 +++++-------------- tests/neg/i15474.scala | 4 --- tests/neg/i15474b.check | 5 ++++ tests/neg/i15474b.scala | 8 ++++++ tests/pos/i15474.scala | 4 --- 8 files changed, 25 insertions(+), 38 deletions(-) create mode 100644 tests/neg/i15474b.check create mode 100644 tests/neg/i15474b.scala diff --git a/compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala b/compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala index fdb9251951e5..f49b00089712 100644 --- a/compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala +++ b/compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala @@ -77,9 +77,6 @@ object Scala3: type SemanticSymbol = Symbol | FakeSymbol given SemanticSymbolOps : AnyRef with - import SymbolOps.* - import StringOps.* - extension (sym: SemanticSymbol) def name(using Context): Name = sym match case s: Symbol => s.name diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 77328cdbb33d..3d3320eb7589 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1569,8 +1569,10 @@ trait Implicits: /** 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 enclosing the search point and comes - * later in the source or coincides with that given definition. + * scope as a given definition enclosing the search point (with no + * class methods between the given definition and 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 @@ -1578,7 +1580,7 @@ trait Implicits: if sym.owner == candSym.owner then if sym.is(ModuleClass) then candSucceedsGiven(sym.sourceModule) else sym.is(Given) && sym.span.exists && sym.span.start <= candSym.span.start - else if sym.is(Package) then false + else if sym.owner.isClass then false else candSucceedsGiven(sym.owner) ctx.isTyper diff --git a/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala b/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala index 1203e309c484..51f133c972b4 100644 --- a/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala +++ b/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala @@ -1781,8 +1781,6 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler end TypeRepr given TypeReprMethods: TypeReprMethods with - import SymbolMethods.* - extension (self: TypeRepr) def show(using printer: Printer[TypeRepr]): String = printer.show(self) @@ -2610,8 +2608,6 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler end Symbol given SymbolMethods: SymbolMethods with - import FlagsMethods.* - extension (self: Symbol) def owner: Symbol = self.denot.owner def maybeOwner: Symbol = self.denot.maybeOwner diff --git a/tests/neg/i15474.check b/tests/neg/i15474.check index 0b23628d3051..f99c6778d1ae 100644 --- a/tests/neg/i15474.check +++ b/tests/neg/i15474.check @@ -1,31 +1,18 @@ --- Error: tests/neg/i15474.scala:7:35 ---------------------------------------------------------------------------------- -7 | def apply(from: String): Int = from.toInt // error - | ^^^^ +-- Error: tests/neg/i15474.scala:6:39 ---------------------------------------------------------------------------------- +6 | given c: Conversion[ String, Int ] = _.toInt // error + | ^ | Warning: result of implicit search for ?{ toInt: ? } will change. - | Current result Test1.c will be no longer eligible + | 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.avoidLoopingGivens` language import. | | To fix the problem without the language import, you could try one of the following: - | - rearrange definitions so that Test1.c comes earlier, + | - rearrange definitions so that Test2.c comes earlier, | - use an explicit conversion, | - use an import to get extension method into scope. --- Error: tests/neg/i15474.scala:10:39 --------------------------------------------------------------------------------- -10 | given c: Conversion[ String, Int ] = _.toInt // error - | ^ - | Warning: 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.avoidLoopingGivens` language import. - | - | To fix the problem without the language import, you could try one of the following: - | - rearrange definitions so that Test2.c comes earlier, - | - use an explicit conversion, - | - use an import to get extension method into scope. --- Error: tests/neg/i15474.scala:16:56 --------------------------------------------------------------------------------- -16 | given Ordering[Price] = summon[Ordering[BigDecimal]] // error +-- Error: tests/neg/i15474.scala:12:56 --------------------------------------------------------------------------------- +12 | given Ordering[Price] = summon[Ordering[BigDecimal]] // error | ^ | Warning: result of implicit search for Ordering[BigDecimal] will change. | Current result Prices.Price.given_Ordering_Price will be no longer eligible diff --git a/tests/neg/i15474.scala b/tests/neg/i15474.scala index 5a66ea016630..b196d1b400ef 100644 --- a/tests/neg/i15474.scala +++ b/tests/neg/i15474.scala @@ -2,10 +2,6 @@ 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 // error diff --git a/tests/neg/i15474b.check b/tests/neg/i15474b.check new file mode 100644 index 000000000000..73ef720af7e3 --- /dev/null +++ b/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 diff --git a/tests/neg/i15474b.scala b/tests/neg/i15474b.scala new file mode 100644 index 000000000000..9d496c37ef00 --- /dev/null +++ b/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 + diff --git a/tests/pos/i15474.scala b/tests/pos/i15474.scala index 8adc5ad7233d..6b9e55806ae3 100644 --- a/tests/pos/i15474.scala +++ b/tests/pos/i15474.scala @@ -2,10 +2,6 @@ import scala.language.implicitConversions import scala.language.experimental.avoidLoopingGivens -object Test1: - given c: Conversion[ String, Int ] with - def apply(from: String): Int = from.toInt // was error, now avoided - object Test2: given c: Conversion[ String, Int ] = _.toInt // now avoided, was loop not detected, could be used as a fallback to avoid the warning. From b0fecc5be60b2b090c51622bc06cdeb727cd2a5f Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 18 Dec 2023 22:58:34 +0100 Subject: [PATCH 05/11] Add tweaks and docs --- .../src/dotty/tools/dotc/config/Feature.scala | 2 +- .../tools/dotc/config/SourceVersion.scala | 1 + .../dotty/tools/dotc/typer/Implicits.scala | 32 ++++++++-------- .../changed-features/implicit-resolution.md | 23 ----------- .../experimental/given-loop-prevention.md | 31 +++++++++++++++ docs/sidebar.yml | 1 + .../runtime/stdLibPatches/language.scala | 6 +-- tests/neg/i15474.check | 38 ++++++++++--------- tests/neg/i6716.check | 18 +++++---- tests/neg/i7294-a.check | 28 +++++++------- tests/neg/i7294-a.scala | 2 + tests/neg/i7294-b.scala | 2 + tests/neg/looping-givens.scala | 2 + tests/pos/i15474.scala | 2 +- tests/pos/looping-givens.scala | 2 +- tests/run/i6716.scala | 4 +- 16 files changed, 110 insertions(+), 84 deletions(-) create mode 100644 docs/_docs/reference/experimental/given-loop-prevention.md diff --git a/compiler/src/dotty/tools/dotc/config/Feature.scala b/compiler/src/dotty/tools/dotc/config/Feature.scala index e8ca30ecb243..cdd83b15f4fc 100644 --- a/compiler/src/dotty/tools/dotc/config/Feature.scala +++ b/compiler/src/dotty/tools/dotc/config/Feature.scala @@ -33,7 +33,7 @@ object Feature: val pureFunctions = experimental("pureFunctions") val captureChecking = experimental("captureChecking") val into = experimental("into") - val avoidLoopingGivens = experimental("avoidLoopingGivens") + val givenLoopPrevention = experimental("givenLoopPrevention") val globalOnlyImports: Set[TermName] = Set(pureFunctions, captureChecking) diff --git a/compiler/src/dotty/tools/dotc/config/SourceVersion.scala b/compiler/src/dotty/tools/dotc/config/SourceVersion.scala index f6db0bac0452..33b946ed173f 100644 --- a/compiler/src/dotty/tools/dotc/config/SourceVersion.scala +++ b/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` diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 3d3320eb7589..1672c94fd969 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1569,19 +1569,18 @@ trait Implicits: /** 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 enclosing the search point (with no - * class methods between the given definition and the search point) - * and `cand` comes later in the source or coincides with that given - * definition. + * 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 = - if sym.owner == candSym.owner then - if sym.is(ModuleClass) then candSucceedsGiven(sym.sourceModule) - else sym.is(Given) && sym.span.exists && sym.span.start <= candSym.span.start - else if sym.owner.isClass then false - else candSucceedsGiven(sym.owner) + 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) @@ -1596,7 +1595,7 @@ trait Implicits: def checkResolutionChange(result: SearchResult) = if (eligible ne preEligible) - && !Feature.enabled(Feature.avoidLoopingGivens) + && !Feature.enabled(Feature.givenLoopPrevention) then val prevResult = searchImplicit(preEligible, contextual) prevResult match @@ -1617,17 +1616,20 @@ trait Implicits: case result: SearchSuccess if prevResult.ref frozen_=:= result.ref => // OK case _ => - report.error( - em"""Warning: result of implicit search for $pt will change. + 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.avoidLoopingGivens` language import. + |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.""", - srcPos) + | - 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 diff --git a/docs/_docs/reference/changed-features/implicit-resolution.md b/docs/_docs/reference/changed-features/implicit-resolution.md index ab8293724a4e..861a63bd4a05 100644 --- a/docs/_docs/reference/changed-features/implicit-resolution.md +++ b/docs/_docs/reference/changed-features/implicit-resolution.md @@ -164,26 +164,3 @@ 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.** Implicit resolution now tries to avoid 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`, 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 under `-source future`. In earlier versions, a -warning is issued where that behavior will change. - -Old-style implicit definitions are unaffected by this change. - diff --git a/docs/_docs/reference/experimental/given-loop-prevention.md b/docs/_docs/reference/experimental/given-loop-prevention.md new file mode 100644 index 000000000000..e306ba977d45 --- /dev/null +++ b/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. + diff --git a/docs/sidebar.yml b/docs/sidebar.yml index 65d7ac2f9ee4..d9f86d5141c3 100644 --- a/docs/sidebar.yml +++ b/docs/sidebar.yml @@ -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 diff --git a/library/src/scala/runtime/stdLibPatches/language.scala b/library/src/scala/runtime/stdLibPatches/language.scala index 9fa8bff120af..9a2a034c6b7d 100644 --- a/library/src/scala/runtime/stdLibPatches/language.scala +++ b/library/src/scala/runtime/stdLibPatches/language.scala @@ -95,10 +95,10 @@ object language: * 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/avoid-looping-givens]] + * @see [[https://dotty.epfl.ch/docs/reference/experimental/given-loop-prevention]] */ - @compileTimeOnly("`avoidLoopingGivens` can only be used at compile time in import statements") - object avoidLoopingGivens + @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. diff --git a/tests/neg/i15474.check b/tests/neg/i15474.check index f99c6778d1ae..6a60aed304aa 100644 --- a/tests/neg/i15474.check +++ b/tests/neg/i15474.check @@ -1,25 +1,29 @@ -- Error: tests/neg/i15474.scala:6:39 ---------------------------------------------------------------------------------- 6 | given c: Conversion[ String, Int ] = _.toInt // error | ^ - | Warning: 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.avoidLoopingGivens` 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, use the `experimental.givenLoopPrevention` language import. | - | To fix the problem without the language import, you could try one of the following: - | - rearrange definitions so that Test2.c comes earlier, - | - use an explicit conversion, - | - use an import to get extension method into scope. + | 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 | ^ - | Warning: 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.avoidLoopingGivens` 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, use the `experimental.givenLoopPrevention` language import. | - | To fix the problem without the language import, you could try one of the following: - | - rearrange definitions so that Prices.Price.given_Ordering_Price comes earlier, - | - use an explicit argument. + | 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 index 6771d736b6af..e70ac4b15f9c 100644 --- a/tests/neg/i6716.check +++ b/tests/neg/i6716.check @@ -1,12 +1,14 @@ -- Error: tests/neg/i6716.scala:12:39 ---------------------------------------------------------------------------------- 12 | given Monad[Bar] = summon[Monad[Foo]] // error | ^ - | Warning: 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.avoidLoopingGivens` 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, use the `experimental.givenLoopPrevention` language import. | - | To fix the problem without the language import, you could try one of the following: - | - rearrange definitions so that Bar.given_Monad_Bar comes earlier, - | - use an explicit argument. + | 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 index 887635d89a35..319ed8e1c9d0 100644 --- a/tests/neg/i7294-a.check +++ b/tests/neg/i7294-a.check @@ -1,23 +1,25 @@ --- Error: tests/neg/i7294-a.scala:6:10 --------------------------------------------------------------------------------- -6 | case x: T => x.g(10) // error // error +-- [E007] Type Mismatch Error: tests/neg/i7294-a.scala:8:18 ------------------------------------------------------------ +8 | case x: T => x.g(10) // error // error + | ^^^^^^^ + | Found: Any + | Required: T + | + | where: T is a type in given instance f with bounds <: foo.Foo + | + | longer explanation available when compiling with `-explain` +-- Error: tests/neg/i7294-a.scala:8:10 --------------------------------------------------------------------------------- +8 | case x: T => x.g(10) // error // error | ^ - | Warning: result of implicit search for scala.reflect.TypeTest[Nothing, T] will change. + | Result of implicit search for scala.reflect.TypeTest[Nothing, T] will change. | Current result foo.i7294-a$package.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.avoidLoopingGivens` language import. + | 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 foo.i7294-a$package.f comes earlier, | - use an explicit argument. + | This will be an error in Scala 3.5 and later. | | where: T is a type in given instance f with bounds <: foo.Foo --- [E007] Type Mismatch Error: tests/neg/i7294-a.scala:6:18 ------------------------------------------------------------ -6 | case x: T => x.g(10) // error // error - | ^^^^^^^ - | Found: Any - | Required: T - | - | where: T is a type in given instance f with bounds <: foo.Foo - | - | longer explanation available when compiling with `-explain` diff --git a/tests/neg/i7294-a.scala b/tests/neg/i7294-a.scala index 538dc3159fb8..b0710413eefd 100644 --- a/tests/neg/i7294-a.scala +++ b/tests/neg/i7294-a.scala @@ -1,3 +1,5 @@ +//> using options -Xfatal-warnings + package foo trait Foo { def g(x: Int): Any } diff --git a/tests/neg/i7294-b.scala b/tests/neg/i7294-b.scala index b06d814444e8..8c6f9328cc20 100644 --- a/tests/neg/i7294-b.scala +++ b/tests/neg/i7294-b.scala @@ -1,3 +1,5 @@ +//> using options -Xfatal-warnings + package foo trait Foo { def g(x: Any): Any } diff --git a/tests/neg/looping-givens.scala b/tests/neg/looping-givens.scala index 572f1707861f..357a417f0ed9 100644 --- a/tests/neg/looping-givens.scala +++ b/tests/neg/looping-givens.scala @@ -1,3 +1,5 @@ +//> using options -Xfatal-warnings + class A class B diff --git a/tests/pos/i15474.scala b/tests/pos/i15474.scala index 6b9e55806ae3..b006f8b61cf4 100644 --- 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.avoidLoopingGivens +import scala.language.experimental.givenLoopPrevention 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/looping-givens.scala b/tests/pos/looping-givens.scala index ed11981c1bf6..1b620b5c113e 100644 --- a/tests/pos/looping-givens.scala +++ b/tests/pos/looping-givens.scala @@ -1,4 +1,4 @@ -import language.experimental.avoidLoopingGivens +import language.experimental.givenLoopPrevention class A class B diff --git a/tests/run/i6716.scala b/tests/run/i6716.scala index 6208a52190fe..4cca37f96a6f 100644 --- a/tests/run/i6716.scala +++ b/tests/run/i6716.scala @@ -1,6 +1,6 @@ //> using options -Xfatal-warnings -import scala.language.experimental.avoidLoopingGivens +import scala.language.experimental.givenLoopPrevention trait Monad[T]: def id: String @@ -11,7 +11,7 @@ object Foo { opaque type Bar = Foo object Bar { - given Monad[Bar] = summon[Monad[Foo]] // was error fixed by avoidLoopingGivens + given Monad[Bar] = summon[Monad[Foo]] // was error, fixed by givenLoopPrevention } object Test extends App { From 6b621a3bb4770801dab06a140636f72df2bf2faa Mon Sep 17 00:00:00 2001 From: odersky Date: Tue, 19 Dec 2023 13:38:33 +0100 Subject: [PATCH 06/11] Swap two givens in Specs2 to satisfy new restriuction --- community-build/community-projects/specs2 | 2 +- tests/neg/i7294-a.check | 50 +++++++++++------------ tests/neg/i7294-a.scala | 10 +++-- 3 files changed, 32 insertions(+), 30 deletions(-) diff --git a/community-build/community-projects/specs2 b/community-build/community-projects/specs2 index e42f7987b4ce..ba01cca013d9 160000 --- a/community-build/community-projects/specs2 +++ b/community-build/community-projects/specs2 @@ -1 +1 @@ -Subproject commit e42f7987b4ce30d95fca3f30b9d508021f2fdac7 +Subproject commit ba01cca013d9d99e390d17619664bdedd716e0d7 diff --git a/tests/neg/i7294-a.check b/tests/neg/i7294-a.check index 319ed8e1c9d0..6fac76a9faa5 100644 --- a/tests/neg/i7294-a.check +++ b/tests/neg/i7294-a.check @@ -1,25 +1,25 @@ --- [E007] Type Mismatch Error: tests/neg/i7294-a.scala:8:18 ------------------------------------------------------------ -8 | case x: T => x.g(10) // error // error - | ^^^^^^^ - | Found: Any - | Required: T - | - | where: T is a type in given instance f with bounds <: foo.Foo - | - | longer explanation available when compiling with `-explain` --- Error: tests/neg/i7294-a.scala:8:10 --------------------------------------------------------------------------------- -8 | case x: T => x.g(10) // error // error - | ^ - | Result of implicit search for scala.reflect.TypeTest[Nothing, T] will change. - | Current result foo.i7294-a$package.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 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 foo.i7294-a$package.f comes earlier, - | - use an explicit argument. - | This will be an error in Scala 3.5 and later. - | - | where: T is a type in given instance f with bounds <: foo.Foo +-- [E007] Type Mismatch Error: tests/neg/i7294-a.scala:10:20 ----------------------------------------------------------- +10 | case x: T => x.g(10) // error // error + | ^^^^^^^ + | Found: Any + | Required: T + | + | where: T is a type in given instance f with bounds <: foo.Foo + | + | longer explanation available when compiling with `-explain` +-- Error: tests/neg/i7294-a.scala:10:12 -------------------------------------------------------------------------------- +10 | case x: T => x.g(10) // error // error + | ^ + | Result of implicit search for scala.reflect.TypeTest[Nothing, T] will change. + | 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 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 foo.Test.f comes earlier, + | - use an explicit argument. + | This will be an error in Scala 3.5 and later. + | + | where: T is a type in given instance f with bounds <: foo.Foo diff --git a/tests/neg/i7294-a.scala b/tests/neg/i7294-a.scala index b0710413eefd..3453e88cf741 100644 --- a/tests/neg/i7294-a.scala +++ b/tests/neg/i7294-a.scala @@ -4,8 +4,10 @@ package foo trait Foo { def g(x: Int): Any } -inline given f[T <: Foo]: T = ??? match { - case x: T => x.g(10) // error // error -} +object Test: -@main def Test = f + inline given f[T <: Foo]: T = ??? match { + case x: T => x.g(10) // error // error + } + + @main def Test = f From 6f4770227cdce8f1cf7cc1619e143cd76afaeea3 Mon Sep 17 00:00:00 2001 From: odersky Date: Sun, 7 Jan 2024 12:39:09 +0100 Subject: [PATCH 07/11] Turn given loop prevention on for -source future 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. --- .../src/dotty/tools/dotc/config/Feature.scala | 1 - .../dotty/tools/dotc/typer/Implicits.scala | 5 ++- .../runtime/stdLibPatches/language.scala | 9 ---- tests/neg/i15474.check | 44 ++++++++++--------- tests/neg/i6716.check | 21 ++++----- tests/neg/i7294-a.check | 3 +- tests/pos/i15474.scala | 2 +- tests/pos/looping-givens.scala | 2 +- tests/run/i6716.scala | 6 +-- 9 files changed, 43 insertions(+), 50 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/config/Feature.scala b/compiler/src/dotty/tools/dotc/config/Feature.scala index cdd83b15f4fc..fa262a5880ff 100644 --- 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 index 1672c94fd969..37086cff0b4c 100644 --- 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/library/src/scala/runtime/stdLibPatches/language.scala b/library/src/scala/runtime/stdLibPatches/language.scala index 9a2a034c6b7d..c2a12cec2ecc 100644 --- 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 index 6a60aed304aa..3205f703cd50 100644 --- 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 index e70ac4b15f9c..cdf655710452 100644 --- 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 index 6fac76a9faa5..2fe260fcf99a 100644 --- 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/i15474.scala b/tests/pos/i15474.scala index b006f8b61cf4..f2c85120e4b2 100644 --- 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/looping-givens.scala b/tests/pos/looping-givens.scala index 1b620b5c113e..0e615c8251df 100644 --- 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 index 4cca37f96a6f..3bef45ac7465 100644 --- 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 { From 65d9a0daca737c4c7d223df173bd84c0fa859113 Mon Sep 17 00:00:00 2001 From: odersky Date: Sun, 7 Jan 2024 18:46:43 +0100 Subject: [PATCH 08/11] Adapt docs --- .../changed-features/implicit-resolution.md | 32 +++++++++++++++++++ docs/sidebar.yml | 1 - 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/docs/_docs/reference/changed-features/implicit-resolution.md b/docs/_docs/reference/changed-features/implicit-resolution.md index 861a63bd4a05..1396ed04b6d3 100644 --- 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 index d9f86d5141c3..65d7ac2f9ee4 100644 --- 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 From 6dc38d9ae928178f6758cfbbbe32f3075dbf8c22 Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 8 Jan 2024 09:38:11 +0100 Subject: [PATCH 09/11] Add test showing use of @nowarn --- tests/pos/given-loop-prevention.scala | 14 ++++++++++++++ tests/pos/i6716.scala | 14 ++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 tests/pos/given-loop-prevention.scala create mode 100644 tests/pos/i6716.scala diff --git a/tests/pos/given-loop-prevention.scala b/tests/pos/given-loop-prevention.scala new file mode 100644 index 000000000000..0bae0bb24fed --- /dev/null +++ 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/i6716.scala b/tests/pos/i6716.scala new file mode 100644 index 000000000000..f02559af1e82 --- /dev/null +++ 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 {} +} From d449f0f3955b40950b8540de015e477de0f0fa58 Mon Sep 17 00:00:00 2001 From: odersky Date: Wed, 10 Jan 2024 17:29:35 +0100 Subject: [PATCH 10/11] Fix algorithm to prevent recursive givens Fixes #19404 Fixes #19407 --- .../dotty/tools/dotc/typer/Implicits.scala | 77 ++++++++++--------- tests/pos/i19404.scala | 13 ++++ tests/pos/i19407.scala | 11 +++ 3 files changed, 65 insertions(+), 36 deletions(-) create mode 100644 tests/pos/i19404.scala create mode 100644 tests/pos/i19407.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 37086cff0b4c..389669beff01 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -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 */ @@ -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 @@ -1553,18 +1560,18 @@ trait Implicits: /** Search implicit in context `ctxImplicits` or else in implicit scope * of expected type if `ctxImplicits == null`. */ - private def searchImplicit(ctxImplicits: ContextualImplicits | Null): SearchResult = + private def searchImplicit(ctxImplicits: ContextualImplicits | Null, mode: SearchMode): SearchResult = if isUnderspecified(wildProto) then SearchFailure(TooUnspecific(pt), span) else val contextual = ctxImplicits != null val preEligible = // the eligible candidates, ignoring positions - if contextual then + 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 /** Does candidate `cand` come too late for it to be considered as an @@ -1589,16 +1596,13 @@ trait Implicits: end comesTooLate val eligible = // the eligible candidates that come before the search point - if contextual && sourceVersion.isAtLeast(SourceVersion.`3.4`) + if contextual && mode != SearchMode.Old then preEligible.filterNot(comesTooLate) else preEligible def checkResolutionChange(result: SearchResult) = - if (eligible ne preEligible) - && !sourceVersion.isAtLeast(SourceVersion.future) - then - val prevResult = searchImplicit(preEligible, contextual) - prevResult match + if (eligible ne preEligible) && mode != SearchMode.New then + searchImplicit(preEligible, contextual) match case prevResult: SearchSuccess => def remedy = pt match case _: SelectionProto => @@ -1628,41 +1632,38 @@ trait Implicits: | - 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`) + 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) - case _ => - prevResult + prevResult + case prevResult: SearchFailure => result else result end checkResolutionChange - val result = searchImplicit(eligible, contextual) - result match - case result: SearchSuccess => - checkResolutionChange(result) - 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" - checkResolutionChange: - searchImplicit(newCtxImplicits).recoverWith: + 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 + else failure end searchImplicit /** Find a unique best implicit reference */ @@ -1679,7 +1680,11 @@ trait Implicits: case ref: TermRef => SearchSuccess(tpd.ref(ref).withSpan(span.startPos), ref, 0)(ctx.typerState, ctx.gadt) case _ => - searchImplicit(ctx.implicits) + 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) diff --git a/tests/pos/i19404.scala b/tests/pos/i19404.scala new file mode 100644 index 000000000000..8d6d4406ebb2 --- /dev/null +++ b/tests/pos/i19404.scala @@ -0,0 +1,13 @@ +given ipEncoder[IP <: IpAddress]: Encoder[IP] = Encoder[String].contramap(_.toString) + +class Encoder[A] { + final def contramap[B](f: B => A): Encoder[B] = new Encoder[B] +} + +object Encoder { + final def apply[A](implicit instance: Encoder[A]): Encoder[A] = instance + implicit final val encodeString: Encoder[String] = new Encoder[String] +} + +trait Json +trait IpAddress \ No newline at end of file diff --git a/tests/pos/i19407.scala b/tests/pos/i19407.scala new file mode 100644 index 000000000000..b7440a53540d --- /dev/null +++ b/tests/pos/i19407.scala @@ -0,0 +1,11 @@ +trait GeneratedEnum +trait Decoder[A] + +object Decoder: + given Decoder[Int] = ??? + +object GeneratedEnumDecoder: + + given [A <: GeneratedEnum]: Decoder[A] = + summon[Decoder[Int]] + ??? \ No newline at end of file From b9857ef417b9a93432c73059818ba811eb5be6bb Mon Sep 17 00:00:00 2001 From: odersky Date: Thu, 11 Jan 2024 19:58:05 +0100 Subject: [PATCH 11/11] Regression test for 19417 --- tests/pos/i19417/defs_1.scala | 5 +++++ tests/pos/i19417/usage_2.scala | 2 ++ 2 files changed, 7 insertions(+) create mode 100644 tests/pos/i19417/defs_1.scala create mode 100644 tests/pos/i19417/usage_2.scala diff --git a/tests/pos/i19417/defs_1.scala b/tests/pos/i19417/defs_1.scala new file mode 100644 index 000000000000..92dc10990d90 --- /dev/null +++ b/tests/pos/i19417/defs_1.scala @@ -0,0 +1,5 @@ +trait QueryParamDecoder[E]: + def emap[T](fn: E => Either[Throwable, T]): QueryParamDecoder[T] +object QueryParamDecoder: + def apply[T](implicit ev: QueryParamDecoder[T]): QueryParamDecoder[T] = ev + implicit lazy val stringQueryParamDecoder: QueryParamDecoder[String] = ??? \ No newline at end of file diff --git a/tests/pos/i19417/usage_2.scala b/tests/pos/i19417/usage_2.scala new file mode 100644 index 000000000000..c686f46280d7 --- /dev/null +++ b/tests/pos/i19417/usage_2.scala @@ -0,0 +1,2 @@ +given[E](using e: EnumOf[E]): QueryParamDecoder[E] = QueryParamDecoder[String].emap(_ => Right(???)) +trait EnumOf[E] \ No newline at end of file