Refine implicit priority change warnings (#21045)

Fixes #21036
Fixes #20572

Author: odersky

Diff for: compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -1302,9 +1302,11 @@ trait Implicits:
 
       // A map that associates a priority change warning (between -source 3.4 and 3.6)
       // with the candidate refs mentioned in the warning. We report the associated
-      // message if both candidates qualify in tryImplicit and at least one of the candidates
-      // is part of the result of the implicit search.
-      val priorityChangeWarnings = mutable.ListBuffer[(TermRef, TermRef, Message)]()
+      // 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.
        *
@@ -1319,19 +1321,24 @@ trait Implicits:
        *                  return new result with preferGeneral = true
        *  3.6 and higher: compare with preferGeneral = true
        *
+       *  @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.
        */
-      def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel): Int =
+      def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel, disambiguate: Boolean = false): Int =
         def comp(using Context) = explore(compare(alt1.ref, alt2.ref, preferGeneral = true))
-        def warn(msg: Message) =
-          priorityChangeWarnings += ((alt1.ref, alt2.ref, msg))
         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) =
+                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"
@@ -1348,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
 
@@ -1358,8 +1367,9 @@ trait Implicits:
        */
       def disambiguate(alt1: SearchResult, alt2: SearchSuccess) = alt1 match
         case alt1: SearchSuccess =>
-          var diff = compareAlternatives(alt1, alt2)
-          assert(diff <= 0)   // diff > 0 candidates should already have been eliminated in `rank`
+          var diff = compareAlternatives(alt1, alt2, disambiguate = true)
+          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
@@ -1443,8 +1453,8 @@ trait Implicits:
                       compareAlternatives(newCand, cand) > 0)
                 else
                   // keep only warnings that don't involve the failed candidate reference
-                  priorityChangeWarnings.filterInPlace: (ref1, ref2, _) =>
-                    ref1 != cand.ref && ref2 != cand.ref
+                  priorityChangeWarnings.filterInPlace: (critical, _) =>
+                    !critical.contains(cand.ref)
                   rank(remaining, found, fail :: rfailures)
               case best: SearchSuccess =>
                 if (ctx.mode.is(Mode.ImplicitExploration) || isCoherent)
@@ -1453,8 +1463,7 @@ trait Implicits:
                   case retained: SearchSuccess =>
                     val newPending =
                       if (retained eq found) || remaining.isEmpty then remaining
-                      else remaining.filterConserve(cand =>
-                        compareAlternatives(retained, cand) <= 0)
+                      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
@@ -1601,8 +1610,8 @@ trait Implicits:
             throw ex
 
       val result = rank(sort(eligible), NoMatchingImplicitsFailure, Nil)
-      for (ref1, ref2, msg) <- priorityChangeWarnings do
-        if result.found.exists(ref => ref == ref1 || ref == ref2) then
+      for (critical, msg) <- priorityChangeWarnings do
+        if result.found.exists(critical.contains(_)) then
           report.warning(msg, srcPos)
       result
     end searchImplicit

Diff for: tests/neg/given-triangle.check

@@ -0,0 +1,4 @@
+-- [E172] Type Error: tests/neg/given-triangle.scala:15:18 -------------------------------------------------------------
+15 |@main def Test = f  // error
+   |                  ^
+   |Ambiguous given instances: both given instance given_B and given instance given_C match type A of parameter a of method f

Diff for: tests/warn/given-triangle.scala renamed to tests/neg/given-triangle.scala

@@ -1,5 +1,4 @@
-//> using options -source 3.6-migration
-
+//> using options -source 3.5
 class A
 class B extends A
 class C extends A
@@ -13,4 +12,4 @@ def f(using a: A, b: B, c: C) =
   println(b.getClass)
   println(c.getClass)
 
-@main def Test = f  // warn
+@main def Test = f  // error

Diff for: tests/warn/bson/Test.scala renamed to tests/pos/bson/Test.scala

@@ -0,0 +1,7 @@
+//> using options -Werror
+trait Writes[T]
+trait Format[T] extends Writes[T]
+given [T: List]: Writes[T] = null
+given [T]: Format[T] = null
+
+val _ = summon[Writes[Int]]

Diff for: tests/warn/bson/bson.scala renamed to tests/pos/bson/bson.scala

@@ -0,0 +1,16 @@
+//> using options -source 3.5 -Werror
+trait SameRuntime[A, B]
+trait BSONWriter[T]
+trait BSONHandler[T] extends BSONWriter[T]
+
+opaque type Id = String
+object Id:
+  given SameRuntime[Id, String] = ???
+
+given BSONHandler[String]                    = ???
+given [T: BSONHandler]: BSONHandler[List[T]] = ???
+
+given opaqueWriter[T, A](using rs: SameRuntime[T, A], writer: BSONWriter[A]): BSONWriter[T] = ???
+
+val x = summon[BSONHandler[List[Id]]] // this doesn't emit warning
+val y = summon[BSONWriter[List[Id]]] // this did emit warning

Diff for: tests/pos/i20572.scala

@@ -1,4 +1,4 @@
-import language.future
+import language.`3.6`
 
 class A
 class B extends A

Diff for: tests/pos/i21036.scala

@@ -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 for: tests/run/given-triangle.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

Diff for: tests/warn/bson.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 for: tests/warn/given-triangle.check

@@ -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