Another given priority scheme to ensure transitivity

It applies the following tweaks:

 - Do treat givens as higher priority than implicits. The previous logic was faulty, but is corrected now.
 - Drop special handling of NotGiven in prioritization. The previous logic pretended to do so,
   but was ineffective.
 - Do use the old priority scheme for implicit/implicit pairs.
 - Do owner score for givens as a tie breaker if after all other tests we still have an ambiguity.

Author: odersky

community-build/community-projects/PPrint

@@ -2,8 +2,6 @@ package dotty.tools
 package dotc
 package printing
 
-import scala.language.unsafeNulls
-
 import scala.collection.mutable
 
 import core.*
@@ -52,7 +50,11 @@ object Formatting {
     object ShowAny extends Show[Any]:
       def show(x: Any): Shown = x
 
-    class ShowImplicits3:
+    class ShowImplicits4:
+      given [X: Show]: Show[X | Null] with
+        def show(x: X | Null) = if x == null then "null" else CtxShow(toStr(x.nn))
+
+    class ShowImplicits3 extends ShowImplicits4:
       given Show[Product] = ShowAny
 
     class ShowImplicits2 extends ShowImplicits3:
@@ -77,15 +79,10 @@ object Formatting {
       given [K: Show, V: Show]: Show[Map[K, V]] with
         def show(x: Map[K, V]) =
           CtxShow(x.map((k, v) => s"${toStr(k)} => ${toStr(v)}"))
-      end given
 
       given [H: Show, T <: Tuple: Show]: Show[H *: T] with
         def show(x: H *: T) =
           CtxShow(toStr(x.head) *: toShown(x.tail).asInstanceOf[Tuple])
-      end given
-
-      given [X: Show]: Show[X | Null] with
-        def show(x: X | Null) = if x == null then "null" else CtxShow(toStr(x.nn))
 
       given Show[FlagSet] with
         def show(x: FlagSet) = x.flagsString
@@ -148,8 +145,8 @@ object Formatting {
     private def treatArg(arg: Shown, suffix: String)(using Context): (String, String) = arg.runCtxShow match {
       case arg: Seq[?] if suffix.indexOf('%') == 0 && suffix.indexOf('%', 1) != -1 =>
         val end = suffix.indexOf('%', 1)
-        val sep = StringContext.processEscapes(suffix.substring(1, end))
-        (arg.mkString(sep), suffix.substring(end + 1))
+        val sep = StringContext.processEscapes(suffix.substring(1, end).nn)
+        (arg.mkString(sep), suffix.substring(end + 1).nn)
       case arg: Seq[?] =>
         (arg.map(showArg).mkString("[", ", ", "]"), suffix)
       case arg =>

compiler/src/dotty/tools/dotc/printing/Formatting.scala

@@ -2988,7 +2988,7 @@ class MissingImplicitArgument(
 
     /** Default error message for non-nested ambiguous implicits. */
     def defaultAmbiguousImplicitMsg(ambi: AmbiguousImplicits) =
-      s"Ambiguous given instances: ${ambi.explanation}${location("of")}"
+      s"Ambiguous given instances: ${ambi.explanation}${location("of")}${ambi.priorityChangeWarningNote}"
 
     /** Default error messages for non-ambiguous implicits, or nested ambiguous
      *  implicits.

compiler/src/dotty/tools/dotc/reporting/messages.scala

@@ -1762,6 +1762,17 @@ trait Applications extends Compatibility {
       else if sym2.is(Module) then compareOwner(sym1, cls2)
       else 0
 
+  enum CompareScheme:
+    case Old          // Normal specificity test for overloading resolution (where `preferGeneral` is false)
+                      // and in mode Scala3-migration when we compare with the old Scala 2 rules.
+
+    case Intermediate // Intermediate rules: better means specialize, but map all type arguments downwards
+                      // These are enabled for 3.0-3.4, or if OldImplicitResolution
+                      // is specified, and also for all comparisons between old-style implicits,
+
+    case New          // New rules: better means generalize, givens (and extensions) always beat implicits
+  end CompareScheme
+
   /** Compare two alternatives of an overloaded call or an implicit search.
    *
    *  @param  alt1, alt2      Non-overloaded references indicating the two choices
@@ -1788,6 +1799,15 @@ trait Applications extends Compatibility {
    */
   def compare(alt1: TermRef, alt2: TermRef, preferGeneral: Boolean = false)(using Context): Int = trace(i"compare($alt1, $alt2)", overload) {
     record("resolveOverloaded.compare")
+    val scheme =
+      val oldResolution = ctx.mode.is(Mode.OldImplicitResolution)
+      if !preferGeneral || Feature.migrateTo3 && oldResolution then
+        CompareScheme.Old
+      else if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`)
+        || oldResolution
+        || alt1.symbol.is(Implicit) && alt2.symbol.is(Implicit)
+      then CompareScheme.Intermediate
+      else CompareScheme.New
 
     /** Is alternative `alt1` with type `tp1` as good as alternative
      *  `alt2` with type `tp2` ?
@@ -1877,8 +1897,7 @@ trait Applications extends Compatibility {
      *  the intersection of its parent classes instead.
      */
     def isAsGoodValueType(tp1: Type, tp2: Type, alt1IsImplicit: Boolean, alt2IsImplicit: Boolean)(using Context): Boolean =
-      val oldResolution = ctx.mode.is(Mode.OldImplicitResolution)
-      if !preferGeneral || Feature.migrateTo3 && oldResolution then
+      if scheme == CompareScheme.Old then
         // Normal specificity test for overloading resolution (where `preferGeneral` is false)
         // and in mode Scala3-migration when we compare with the old Scala 2 rules.
         isCompatible(tp1, tp2)
@@ -1892,13 +1911,7 @@ trait Applications extends Compatibility {
         val tp1p = prepare(tp1)
         val tp2p = prepare(tp2)
 
-        if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`)
-            || oldResolution
-            || alt1IsImplicit && alt2IsImplicit
-        then
-          // Intermediate rules: better means specialize, but map all type arguments downwards
-          // These are enabled for 3.0-3.5, and for all comparisons between old-style implicits,
-          // and in 3.5 and 3.6-migration when we compare with previous rules.
+        if scheme == CompareScheme.Intermediate then
           val flip = new TypeMap:
             def apply(t: Type) = t match
               case t @ AppliedType(tycon, args) =>
@@ -1909,7 +1922,6 @@ trait Applications extends Compatibility {
               case _ => mapOver(t)
           (flip(tp1p) relaxed_<:< flip(tp2p)) || viewExists(tp1, tp2)
         else
-          // New rules: better means generalize, givens (and extensions) always beat implicits
           if alt1IsImplicit != alt2IsImplicit then alt2IsImplicit
           else (tp2p relaxed_<:< tp1p) || viewExists(tp2, tp1)
     end isAsGoodValueType
@@ -1982,13 +1994,19 @@ trait Applications extends Compatibility {
         // alternatives are the same after following ExprTypes, pick one of them
         // (prefer the one that is not a method, but that's arbitrary).
         if alt1.widenExpr =:= alt2 then -1 else 1
-      else ownerScore match
-        case  1 => if winsType1 || !winsType2 then  1 else 0
-        case -1 => if winsType2 || !winsType1 then -1 else 0
-        case  0 =>
-          if winsType1 != winsType2 then if winsType1 then 1 else -1
-          else if alt1.symbol == alt2.symbol then comparePrefixes
-          else 0
+      else
+        // For new implicit resolution, take ownerscore as more significant than type resolution
+        // Reason: People use owner hierarchies to explicitly prioritize, we should not
+        // break that by changing implicit priority of types.
+        def drawOrOwner =
+          if scheme == CompareScheme.New then ownerScore else 0
+        ownerScore match
+          case  1 => if winsType1 || !winsType2 then  1 else drawOrOwner
+          case -1 => if winsType2 || !winsType1 then -1 else drawOrOwner
+          case  0 =>
+            if winsType1 != winsType2 then if winsType1 then 1 else -1
+            else if alt1.symbol == alt2.symbol then comparePrefixes
+            else 0
     end compareWithTypes
 
     if alt1.symbol.is(ConstructorProxy) && !alt2.symbol.is(ConstructorProxy) then -1

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

@@ -549,6 +549,11 @@ object Implicits:
   /** An ambiguous implicits failure */
   class AmbiguousImplicits(val alt1: SearchSuccess, val alt2: SearchSuccess, val expectedType: Type, val argument: Tree, val nested: Boolean = false) extends SearchFailureType:
 
+    private[Implicits] var priorityChangeWarnings: List[Message] = Nil
+
+    def priorityChangeWarningNote(using Context): String =
+      priorityChangeWarnings.map(msg => s"\n\nNote: $msg").mkString
+
     def msg(using Context): Message =
       var str1 = err.refStr(alt1.ref)
       var str2 = err.refStr(alt2.ref)
@@ -1330,7 +1335,7 @@ trait Implicits:
         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 cmp = comp(using searchContext())
           val sv = Feature.sourceVersion
           if isWarnPriorityChangeVersion(sv) then
             val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution))
@@ -1345,13 +1350,21 @@ trait Implicits:
                 case _  => "none - it's ambiguous"
               if sv.stable == SourceVersion.`3.5` then
                 warn(
-                  em"""Given search preference for $pt between alternatives ${alt1.ref} and ${alt2.ref} will change
+                  em"""Given search preference for $pt between alternatives
+                      |  ${alt1.ref}
+                      |and
+                      |  ${alt2.ref}
+                      |will change.
                       |Current choice           : ${choice(prev)}
                       |New choice from Scala 3.6: ${choice(cmp)}""")
                 prev
               else
                 warn(
-                  em"""Change in given search preference for $pt between alternatives ${alt1.ref} and ${alt2.ref}
+                  em"""Given search preference for $pt between alternatives
+                      |  ${alt1.ref}
+                      |and
+                      |  ${alt2.ref}
+                      |has changed.
                       |Previous choice          : ${choice(prev)}
                       |New choice from Scala 3.6: ${choice(cmp)}""")
                 cmp
@@ -1610,9 +1623,23 @@ trait Implicits:
             throw ex
 
       val result = rank(sort(eligible), NoMatchingImplicitsFailure, Nil)
-      for (critical, msg) <- priorityChangeWarnings do
-        if result.found.exists(critical.contains(_)) then
-          report.warning(msg, srcPos)
+
+      // Issue all priority change warnings that can affect the result
+      val shownWarnings = priorityChangeWarnings.toList.collect:
+        case (critical, msg) if result.found.exists(critical.contains(_)) =>
+          msg
+      result match
+        case result: SearchFailure =>
+          result.reason match
+            case ambi: AmbiguousImplicits =>
+              // Make warnings part of error message because otherwise they are suppressed when
+              // the error is emitted.
+              ambi.priorityChangeWarnings = shownWarnings
+            case _ =>
+        case _ =>
+      for msg <- shownWarnings do
+        report.warning(msg, srcPos)
+
       result
     end searchImplicit
 

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

@@ -23,6 +23,7 @@ class StringFormatterTest extends AbstractStringFormatterTest:
   @Test def flagsTup   = check("(<static>,final)", i"${(JavaStatic, Final)}")
   @Test def seqOfTup2  = check("(final,given), (private,lazy)", i"${Seq((Final, Given), (Private, Lazy))}%, %")
   @Test def seqOfTup3  = check("(Foo,given, (right is approximated))", i"${Seq((Foo, Given, TypeComparer.ApproxState.None.addHigh))}%, %")
+  @Test def tupleNull   = check("(1,null)", i"${(1, null: String | Null)}")
 
   class StorePrinter extends Printer:
     var string: String = "<never set>"

compiler/test/dotty/tools/dotc/StringFormatterTest.scala

@@ -2,3 +2,11 @@
 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
+   |
+   |Note: Given search preference for A between alternatives
+   |  (given_A : A)
+   |and
+   |  (given_B : B)
+   |will change.
+   |Current choice           : the second alternative
+   |New choice from Scala 3.6: the first alternative

tests/neg/given-triangle.check

@@ -0,0 +1 @@
+public enum JavaEnum { ABC, DEF, GHI }

tests/neg/i21303/JavaEnum.java

@@ -0,0 +1,33 @@
+//> using options -source 3.6-migration
+import scala.deriving.Mirror
+import scala.compiletime.*
+import scala.reflect.ClassTag
+import scala.annotation.implicitNotFound
+
+
+trait TSType[T]
+object TSType extends DefaultTSTypes with TSTypeMacros
+
+trait TSNamedType[T] extends TSType[T]
+
+trait DefaultTSTypes extends JavaTSTypes
+trait JavaTSTypes {
+  given javaEnumTSType[E <: java.lang.Enum[E]: ClassTag]: TSNamedType[E] = ???
+}
+object DefaultTSTypes extends DefaultTSTypes
+trait TSTypeMacros {
+  inline given [T: Mirror.Of]: TSType[T] = derived[T]
+  inline def derived[T](using m: Mirror.Of[T]): TSType[T] = {
+    val elemInstances = summonAll[m.MirroredElemTypes]
+    ???
+  }
+
+  private inline def summonAll[T <: Tuple]: List[TSType[_]] = {
+    inline erasedValue[T] match {
+      case _: EmptyTuple => Nil
+      case _: (t *: ts)  => summonInline[TSType[t]] :: summonAll[ts]
+    }
+  }
+}
+
+@main def Test = summon[TSType[JavaEnum]] // error

tests/neg/i21303/Test.scala

@@ -0,0 +1,16 @@
+
+trait Foo[-T]
+trait Bar[-T] extends Foo[T]
+
+object Test {
+
+  locally:
+    implicit val fa: Foo[Int] = ???
+    implicit val ba: Bar[Int] = ???
+    summon[Foo[Int]] // ok
+
+  locally:
+    implicit val fa: Foo[Int] = ???
+    implicit val ba: Bar[Any] = ???
+    summon[Foo[Int]] // error: ambiguous
+}

tests/neg/i2974.scala

@@ -0,0 +1,14 @@
+-- [E172] Type Error: tests/neg/scala-uri.scala:30:59 ------------------------------------------------------------------
+30 |@main def Test = summon[QueryKeyValue[(String, None.type)]]  // error
+   |                                                           ^
+   |No best given instance of type QueryKeyValue[(String, None.type)] was found for parameter x of method summon in object Predef.
+   |I found:
+   |
+   |    QueryKeyValue.tuple2QueryKeyValue[String, None.type](QueryKey.stringQueryKey,
+   |      QueryValue.optionQueryValue[A](
+   |        /* ambiguous: both given instance stringQueryValue in trait QueryValueInstances1 and given instance noneQueryValue in trait QueryValueInstances1 match type QueryValue[A] */
+   |          summon[QueryValue[A]]
+   |      )
+   |    )
+   |
+   |But both given instance stringQueryValue in trait QueryValueInstances1 and given instance noneQueryValue in trait QueryValueInstances1 match type QueryValue[A].

tests/neg/scala-uri.check

@@ -0,0 +1,30 @@
+import scala.language.implicitConversions
+
+trait QueryKey[A]
+object QueryKey extends QueryKeyInstances
+sealed trait QueryKeyInstances:
+  given stringQueryKey: QueryKey[String] = ???
+
+trait QueryValue[-A]
+object QueryValue extends QueryValueInstances
+sealed trait QueryValueInstances1:
+  given stringQueryValue: QueryValue[String] = ???
+  given noneQueryValue: QueryValue[None.type] = ???
+    // The noneQueryValue makes no sense at this priority. Since QueryValue
+    // is contravariant, QueryValue[None.type] is always better than QueryValue[Option[A]]
+    // no matter whether it's old or new resolution. So taking both owner and type
+    // score into account, it's always a draw. With the new disambiguation, we prefer
+    // the optionQueryValue[A], which gives an ambiguity down the road, because we don't
+    // know what the  wrapped type A is. Previously, we preferred QueryValue[None.type]
+    // because it is unconditional. The solution is to put QueryValue[None.type] in the
+    // same trait as QueryValue[Option[A]], as is shown in pos/scala-uri.scala.
+
+sealed trait QueryValueInstances extends QueryValueInstances1:
+  given optionQueryValue[A: QueryValue]: QueryValue[Option[A]] = ???
+
+trait QueryKeyValue[A]
+object QueryKeyValue:
+  given tuple2QueryKeyValue[K: QueryKey, V: QueryValue]: QueryKeyValue[(K, V)] = ???
+
+
+@main def Test = summon[QueryKeyValue[(String, None.type)]]  // error

tests/neg/scala-uri.scala

@@ -0,0 +1,24 @@
+/* These tests show various mechanisms available for implicit prioritization.
+ */
+import language.`3.6`
+
+class A  // The type for which we infer terms below
+class B extends A
+
+/* First, two schemes that require a pre-planned architecture for how and
+ * where given instances are defined.
+ *
+ * Traditional scheme: prioritize with location in class hierarchy
+ */
+class LowPriorityImplicits:
+  given g1: A()
+
+object NormalImplicits extends LowPriorityImplicits:
+  given g2: B()
+
+def test1 =
+  import NormalImplicits.given
+  val x = summon[A]
+  val _: B = x
+  val y = summon[B]
+  val _: B = y

tests/pos/given-priority.scala

@@ -0,0 +1 @@
+public enum JavaEnum { ABC, DEF, GHI }