From acffad65f474deab08cf32b90e99fd87e3d2c18c Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 8 Jul 2024 19:36:27 +0200 Subject: [PATCH] Fix priority change logic for ranking As worked out in collaboration with @EugeneFlesselle --- .../dotty/tools/dotc/typer/Implicits.scala | 37 ++++++++----------- tests/warn/i21036a.check | 6 +++ tests/warn/i21036a.scala | 7 ++++ tests/warn/i21036b.check | 6 +++ tests/warn/i21036b.scala | 7 ++++ 5 files changed, 41 insertions(+), 22 deletions(-) create mode 100644 tests/warn/i21036a.check create mode 100644 tests/warn/i21036a.scala create mode 100644 tests/warn/i21036b.check create mode 100644 tests/warn/i21036b.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 86be195fae43..45c8731c553e 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -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` @@ -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" @@ -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 @@ -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 @@ -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 diff --git a/tests/warn/i21036a.check b/tests/warn/i21036a.check new file mode 100644 index 000000000000..673c01374ef3 --- /dev/null +++ b/tests/warn/i21036a.check @@ -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 diff --git a/tests/warn/i21036a.scala b/tests/warn/i21036a.scala new file mode 100644 index 000000000000..ab97429852d6 --- /dev/null +++ b/tests/warn/i21036a.scala @@ -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 \ No newline at end of file diff --git a/tests/warn/i21036b.check b/tests/warn/i21036b.check new file mode 100644 index 000000000000..ff7fdfd7a87c --- /dev/null +++ b/tests/warn/i21036b.check @@ -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 diff --git a/tests/warn/i21036b.scala b/tests/warn/i21036b.scala new file mode 100644 index 000000000000..16dd72266613 --- /dev/null +++ b/tests/warn/i21036b.scala @@ -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 \ No newline at end of file