Skip to content

Commit

Permalink
Restrict the new rules to givens that don't define a new class
Browse files Browse the repository at this point in the history
`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.
  • Loading branch information
odersky authored and Kordyjan committed Jan 18, 2024
1 parent 4bb0aaa commit a48a759
Show file tree
Hide file tree
Showing 8 changed files with 25 additions and 38 deletions.
3 changes: 0 additions & 3 deletions compiler/src/dotty/tools/dotc/semanticdb/Scala3.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions compiler/src/dotty/tools/dotc/typer/Implicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1569,16 +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 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
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 if sym.owner.isClass then false
else candSucceedsGiven(sym.owner)

ctx.isTyper
Expand Down
4 changes: 0 additions & 4 deletions compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
27 changes: 7 additions & 20 deletions tests/neg/i15474.check
Original file line number Diff line number Diff line change
@@ -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
Expand Down
4 changes: 0 additions & 4 deletions tests/neg/i15474.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 5 additions & 0 deletions tests/neg/i15474b.check
Original file line number Diff line number Diff line change
@@ -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
8 changes: 8 additions & 0 deletions tests/neg/i15474b.scala
Original file line number Diff line number Diff line change
@@ -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

4 changes: 0 additions & 4 deletions tests/pos/i15474.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down

0 comments on commit a48a759

Please sign in to comment.