Refine implicit priority change warnings

Fixes #21036
Fixes #20572

Author: odersky

compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -1302,9 +1302,8 @@ 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)]()
 
       /** Compare `alt1` with `alt2` to determine which one should be chosen.
        *
@@ -1319,11 +1318,16 @@ trait Implicits:
        *                  return new result with preferGeneral = true
        *  3.6 and higher: compare with preferGeneral = true
        *
+       *  @param only2ndCritical   If true only the second alternative is critical in case
+       *                           of a priority change.
        */
-      def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel): Int =
+      def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel, only2ndCritical: 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))
+          val critical =
+            if only2ndCritical then alt2.ref :: Nil
+            else alt1.ref :: alt2.ref :: Nil
+          priorityChangeWarnings += ((critical, msg))
         if alt1.ref eq alt2.ref then 0
         else if alt1.level != alt2.level then alt1.level - alt2.level
         else
@@ -1443,8 +1447,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)
@@ -1454,7 +1458,15 @@ trait Implicits:
                     val newPending =
                       if (retained eq found) || remaining.isEmpty then remaining
                       else remaining.filterConserve(cand =>
-                        compareAlternatives(retained, cand) <= 0)
+                        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, bit not snd appears in final result
+                          //  - Hence, snd was eliminated either here, or otherwise by a direct
+                          //    comparison later.
+                          //  - Hence, no change in resolution.
                     rank(newPending, retained, rfailures)
                   case fail: SearchFailure =>
                     // The ambiguity happened in the current search: to recover we
@@ -1601,8 +1613,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

tests/neg/given-triangle.check

@@ -0,0 +1,4 @@
+-- [E172] Type Error: tests/neg/given-triangle.scala:14:18 -------------------------------------------------------------
+14 |@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

tests/warn/given-triangle.scala renamed to tests/neg/given-triangle.scala

@@ -1,5 +1,3 @@
-//> using options -source 3.6-migration
-
 class A
 class B extends A
 class C extends A
@@ -13,4 +11,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

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

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

tests/pos/i20572.scala

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