Skip to content

Commit

Permalink
Fix priority change logic for ranking
Browse files Browse the repository at this point in the history
As worked out in collaboration with @EugeneFlesselle
  • Loading branch information
odersky committed Jul 8, 2024
1 parent 4bc7c10 commit 1474e69
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 22 deletions.
37 changes: 15 additions & 22 deletions compiler/src/dotty/tools/dotc/typer/Implicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1305,6 +1305,9 @@ trait Implicits:
// message if one of the critical candidates is part of the result of the implicit search.
val priorityChangeWarnings = mutable.ListBuffer[(/*critical:*/ List[TermRef], Message)]()

def isWarnPriorityChangeVersion(sv: SourceVersion): Boolean =
sv.stable == SourceVersion.`3.5` || sv == SourceVersion.`3.6-migration`

/** Compare `alt1` with `alt2` to determine which one should be chosen.
*
* @return a number > 0 if `alt1` is preferred over `alt2`
Expand All @@ -1321,25 +1324,21 @@ trait Implicits:
* @param disambiguate The call is used to disambiguate two successes, not for ranking.
* When ranking, we are always filtering out either > 0 or <= 0 results.
* In each case a priority change from 0 to -1 or vice versa makes no difference.
* @param only2ndCritical If true only the second alternative is critical in case
* of a priority change.
*/
def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel, disambiguate: Boolean = false, only2ndCritical: Boolean = false): Int =
def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel, disambiguate: Boolean = false): Int =
def comp(using Context) = explore(compare(alt1.ref, alt2.ref, preferGeneral = true))
if alt1.ref eq alt2.ref then 0
else if alt1.level != alt2.level then alt1.level - alt2.level
else
var cmp = comp(using searchContext())
val sv = Feature.sourceVersion
if sv.stable == SourceVersion.`3.5` || sv == SourceVersion.`3.6-migration` then
if isWarnPriorityChangeVersion(sv) then
val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution))
if cmp != prev then
if disambiguate && cmp != prev then
def warn(msg: Message) =
if disambiguate || cmp > 0 || prev > 0 then
val critical =
if only2ndCritical then alt2.ref :: Nil
else alt1.ref :: alt2.ref :: Nil
priorityChangeWarnings += ((critical, msg))
val critical = alt1.ref :: alt2.ref :: Nil
priorityChangeWarnings += ((critical, msg))
implicits.println(i"PRIORITY CHANGE ${alt1.ref}, ${alt2.ref}, $disambiguate")
def choice(c: Int) = c match
case -1 => "the second alternative"
case 1 => "the first alternative"
Expand All @@ -1356,7 +1355,9 @@ trait Implicits:
|Previous choice : ${choice(prev)}
|New choice from Scala 3.6: ${choice(cmp)}""")
cmp
else cmp
else cmp max prev
// When ranking, we keep the better of cmp and prev, which ends up retaining a candidate
// if it is retained in either version.
else cmp
end compareAlternatives

Expand All @@ -1367,7 +1368,8 @@ trait Implicits:
def disambiguate(alt1: SearchResult, alt2: SearchSuccess) = alt1 match
case alt1: SearchSuccess =>
var diff = compareAlternatives(alt1, alt2, disambiguate = true)
assert(diff <= 0) // diff > 0 candidates should already have been eliminated in `rank`
assert(diff <= 0 || isWarnPriorityChangeVersion(Feature.sourceVersion))
// diff > 0 candidates should already have been eliminated in `rank`
if diff == 0 && alt1.ref =:= alt2.ref then
diff = 1 // See i12951 for a test where this happens
else if diff == 0 && alt2.isExtension then
Expand Down Expand Up @@ -1461,16 +1463,7 @@ trait Implicits:
case retained: SearchSuccess =>
val newPending =
if (retained eq found) || remaining.isEmpty then remaining
else remaining.filterConserve(cand =>
compareAlternatives(retained, cand, only2ndCritical = true) <= 0)
// Here we drop some pending alternatives but retain in each case
// `retained`. Therefore, it's a priorty change only if the
// second alternative appears in the final search result. Otherwise
// we have the following scenario:
// - 1st alternative, but not snd appears in final result
// - Hence, snd was eliminated either here, or otherwise by a direct
// comparison later.
// - Hence, no change in resolution.
else remaining.filterConserve(newCand => compareAlternatives(newCand, retained) >= 0)
rank(newPending, retained, rfailures)
case fail: SearchFailure =>
// The ambiguity happened in the current search: to recover we
Expand Down
6 changes: 6 additions & 0 deletions tests/warn/i21036a.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
-- Warning: tests/warn/i21036a.scala:7:17 ------------------------------------------------------------------------------
7 |val y = summon[A] // warn
| ^
| Given search preference for A between alternatives (b : B) and (a : A) will change
| Current choice : the first alternative
| New choice from Scala 3.6: the second alternative
7 changes: 7 additions & 0 deletions tests/warn/i21036a.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//> using options -source 3.5
trait A
trait B extends A
given b: B = ???
given a: A = ???

val y = summon[A] // warn
6 changes: 6 additions & 0 deletions tests/warn/i21036b.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
-- Warning: tests/warn/i21036b.scala:7:17 ------------------------------------------------------------------------------
7 |val y = summon[A] // warn
| ^
| Change in given search preference for A between alternatives (b : B) and (a : A)
| Previous choice : the first alternative
| New choice from Scala 3.6: the second alternative
7 changes: 7 additions & 0 deletions tests/warn/i21036b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//> using options -source 3.6-migration
trait A
trait B extends A
given b: B = ???
given a: A = ???

val y = summon[A] // warn

0 comments on commit 1474e69

Please sign in to comment.