Skip to content

Commit

Permalink
Fix healAmbiguous to compareAlternatives with disambiguate = true
Browse files Browse the repository at this point in the history
On the final result, compared with all the ambiguous candidates we are trying
to recover from. We should still use `disambiguate = false` when filtering the
`pending` candidates for the purpose of warnings, as in the other cases.

Before the changes, it was possible for an ambiguous SearchFailure
to be healed by a candidate which was considered better (possibly only) under
a prioritization scheme different from the current one.

As an optimization, we can avoid redoing compareAlternatives in versions which
could have only used the new prioritization scheme to begin with.

Also restores behaviour avoiding false positive warnings.
Specifically, in cases where we could report a change in prioritization,
despite having not yet done `tryImplicit` on the alternative,
i.e. it was only compared as part of an early filtering

See scala#21045 for related changes
  • Loading branch information
EugeneFlesselle authored and WojciechMazur committed Aug 7, 2024
1 parent 33d7da8 commit d439b58
Showing 1 changed file with 27 additions and 22 deletions.
49 changes: 27 additions & 22 deletions compiler/src/dotty/tools/dotc/typer/Implicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1310,6 +1310,10 @@ 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)]()

val sv = Feature.sourceVersion
val isLastOldVersion = sv.stable == SourceVersion.`3.6`
val isWarnPriorityChangeVersion = isLastOldVersion || sv == SourceVersion.`3.7-migration`

/** Compare `alt1` with `alt2` to determine which one should be chosen.
*
* @return a number > 0 if `alt1` is preferred over `alt2`
Expand All @@ -1333,10 +1337,7 @@ trait Implicits:
else if alt1.level != alt2.level then alt1.level - alt2.level
else
val cmp = comp(using searchContext())
val sv = Feature.sourceVersion
val isLastOldVersion = sv.stable == SourceVersion.`3.6`
val isMigratingVersion = sv == SourceVersion.`3.7-migration`
if isLastOldVersion || isMigratingVersion then
if isWarnPriorityChangeVersion then
val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution))
if disambiguate && cmp != prev then
implicits.println(i"PRIORITY CHANGE ${alt1.ref}, ${alt2.ref}")
Expand Down Expand Up @@ -1419,15 +1420,7 @@ trait Implicits:
if diff < 0 then alt2
else if diff > 0 then alt1
else SearchFailure(new AmbiguousImplicits(alt1, alt2, pt, argument), span)
case fail: SearchFailure =>
fail.reason match
case ambi: AmbiguousImplicits =>
if compareAlternatives(ambi.alt1, alt2, disambiguate = true) < 0
&& compareAlternatives(ambi.alt2, alt2, disambiguate = true) < 0
then alt2
else alt1
case _ =>
alt2
case _: SearchFailure => alt2

/** Try to find a best matching implicit term among all the candidates in `pending`.
* @param pending The list of candidates that remain to be tested
Expand All @@ -1451,12 +1444,27 @@ trait Implicits:
pending match {
case cand :: remaining =>
/** To recover from an ambiguous implicit failure, we need to find a pending
* candidate that is strictly better than the failed candidate(s).
* candidate that is strictly better than the failed `ambiguous` candidate(s).
* If no such candidate is found, we propagate the ambiguity.
*/
def healAmbiguous(fail: SearchFailure, betterThanFailed: Candidate => Boolean) =
val newPending = remaining.filter(betterThanFailed)
rank(newPending, fail, Nil).recoverWith(_ => fail)
def healAmbiguous(fail: SearchFailure, ambiguous: List[RefAndLevel]) =
def betterThanAmbiguous(newCand: RefAndLevel, disambiguate: Boolean): Boolean =
ambiguous.forall(compareAlternatives(newCand, _, disambiguate) > 0)

inline def betterByCurrentScheme(newCand: RefAndLevel): Boolean =
if isWarnPriorityChangeVersion then
// newCand may have only been kept in pending because it was better in the other priotization scheme.
// If that candidate produces a SearchSuccess, disambiguate will return it as the found SearchResult.
// We must now recheck it was really better than the ambigous candidates we are recovering from,
// under the rules of the current scheme, which are applied when disambiguate = true.
betterThanAmbiguous(newCand, disambiguate = true)
else true

val newPending = remaining.filter(betterThanAmbiguous(_, disambiguate = false))
rank(newPending, fail, Nil) match
case found: SearchSuccess if betterByCurrentScheme(found) => found
case _ => fail
end healAmbiguous

negateIfNot(tryImplicit(cand, contextual)) match {
case fail: SearchFailure =>
Expand All @@ -1471,8 +1479,7 @@ trait Implicits:
else
// The ambiguity happened in a nested search: to recover we
// need a candidate better than `cand`
healAmbiguous(fail, newCand =>
compareAlternatives(newCand, cand) > 0)
healAmbiguous(fail, cand :: Nil)
else
// keep only warnings that don't involve the failed candidate reference
priorityChangeWarnings.filterInPlace: (critical, _) =>
Expand All @@ -1491,9 +1498,7 @@ trait Implicits:
// The ambiguity happened in the current search: to recover we
// need a candidate better than the two ambiguous alternatives.
val ambi = fail.reason.asInstanceOf[AmbiguousImplicits]
healAmbiguous(fail, newCand =>
compareAlternatives(newCand, ambi.alt1) > 0 &&
compareAlternatives(newCand, ambi.alt2) > 0)
healAmbiguous(fail, ambi.alt1 :: ambi.alt2 :: Nil)
}
}
case nil =>
Expand Down

0 comments on commit d439b58

Please sign in to comment.