Tweaks to given priority

 - Don't treat givens as higher priority than implicits. The previous logic was faulty anyway.
 - Drop special handling of NotGiven in prioritization. The previous logic pretended to do so,
   but was ineffective.
 - Don't use old priority in general for implicit/implicit pairs. This would make upgrading to givens a constant
   struggle. But do use it as a tie breaker in the case of ambiguities.
 - Also use owner score as a tie breaker if after all other tests we still have an ambiguity.

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

@@ -2,8 +2,6 @@ package dotty.tools
 package dotc
 package printing
 
-import scala.language.unsafeNulls
-
 import scala.collection.mutable
 
 import core.*
@@ -77,12 +75,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))
@@ -148,8 +144,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/typer/Applications.scala

@@ -1830,15 +1830,13 @@ trait Applications extends Compatibility {
             isAsGood(alt1, tp1.instantiate(tparams.map(_.typeRef)), alt2, tp2)
           }
         case _ => // (3)
-          def compareValues(tp1: Type, tp2: Type)(using Context) =
-            isAsGoodValueType(tp1, tp2, alt1.symbol.is(Implicit), alt2.symbol.is(Implicit))
           tp2 match
             case tp2: MethodType => true // (3a)
             case tp2: PolyType if tp2.resultType.isInstanceOf[MethodType] => true // (3a)
             case tp2: PolyType => // (3b)
-              explore(compareValues(tp1, instantiateWithTypeVars(tp2)))
+              explore(isAsGoodValueType(tp1, instantiateWithTypeVars(tp2)))
             case _ => // 3b)
-              compareValues(tp1, tp2)
+              isAsGoodValueType(tp1, tp2)
     }
 
     /** Test whether value type `tp1` is as good as value type `tp2`.
@@ -1868,15 +1866,14 @@ trait Applications extends Compatibility {
      *     for overloading resolution (when `preferGeneral is false), and the opposite relation
      *     `U <: T` or `U convertible to `T` for implicit disambiguation between givens
      *     (when `preferGeneral` is true). For old-style implicit values, the 3.4 behavior is kept.
-     *     If one of the alternatives is an implicit and the other is a given (or an extension), the implicit loses.
      *
      *   - In Scala 3.5 and Scala 3.6-migration, we issue a warning if the result under
      *     Scala 3.6 differ wrt to the old behavior up to 3.5.
      *
      *  Also and only for given resolution: If a compared type refers to a given or its module class, use
      *  the intersection of its parent classes instead.
      */
-    def isAsGoodValueType(tp1: Type, tp2: Type, alt1IsImplicit: Boolean, alt2IsImplicit: Boolean)(using Context): Boolean =
+    def isAsGoodValueType(tp1: Type, tp2: Type)(using Context): Boolean =
       val oldResolution = ctx.mode.is(Mode.OldImplicitResolution)
       if !preferGeneral || Feature.migrateTo3 && oldResolution then
         // Normal specificity test for overloading resolution (where `preferGeneral` is false)
@@ -1892,10 +1889,7 @@ trait Applications extends Compatibility {
         val tp1p = prepare(tp1)
         val tp2p = prepare(tp2)
 
-        if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`)
-            || oldResolution
-            || alt1IsImplicit && alt2IsImplicit
-        then
+        if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`) || oldResolution 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.
@@ -1909,9 +1903,8 @@ 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)
+          // New rules: better means generalize
+          (tp2p relaxed_<:< tp1p) || viewExists(tp2, tp1)
     end isAsGoodValueType
 
     /** Widen the result type of synthetic given methods from the implementation class to the
@@ -1970,8 +1963,9 @@ trait Applications extends Compatibility {
       else if winsPrefix1 then 1
       else -1
 
+    val ownerScore = compareOwner(alt1.symbol.maybeOwner, alt2.symbol.maybeOwner)
+
     def compareWithTypes(tp1: Type, tp2: Type) =
-      val ownerScore = compareOwner(alt1.symbol.maybeOwner, alt2.symbol.maybeOwner)
       val winsType1 = isAsGood(alt1, tp1, alt2, tp2)
       val winsType2 = isAsGood(alt2, tp2, alt1, tp1)
 
@@ -1982,15 +1976,27 @@ 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
+        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
     end compareWithTypes
 
+    // For 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. On the other hand, we do
+    // want to exhaust all other possibilities before using owner score as a tie breaker.
+    // For instance, pos/scala-uri.scala depends on that.
+    def drawOrOwner =
+      if preferGeneral && !ctx.mode.is(Mode.OldImplicitResolution) then
+        //println(i"disambi compare($alt1, $alt2)? $ownerScore")
+        ownerScore
+      else 0
+
     if alt1.symbol.is(ConstructorProxy) && !alt2.symbol.is(ConstructorProxy) then -1
     else if alt2.symbol.is(ConstructorProxy) && !alt1.symbol.is(ConstructorProxy) then 1
     else
@@ -2000,11 +2006,12 @@ trait Applications extends Compatibility {
       val strippedType2 = stripImplicit(fullType2)
 
       val result = compareWithTypes(strippedType1, strippedType2)
-      if (result != 0) result
-      else if (strippedType1 eq fullType1)
-        if (strippedType2 eq fullType2) 0         // no implicits either side: its' a draw
+      if result != 0 then result
+      else if strippedType1 eq fullType1 then
+        if strippedType2 eq fullType2
+        then drawOrOwner                          // no implicits either side: its' a draw
         else 1                                    // prefer 1st alternative with no implicits
-      else if (strippedType2 eq fullType2) -1     // prefer 2nd alternative with no implicits
+      else if strippedType2 eq fullType2 then -1     // prefer 2nd alternative with no implicits
       else compareWithTypes(fullType1, fullType2) // continue by comparing implicits parameters
   }
   end compare

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

@@ -1330,10 +1330,13 @@ 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())
+          lazy val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution))
+          val cmp = comp(using searchContext()) match
+            // if we get an ambiguity with new rules for a pair of old-style implicits, fall back to old rules
+            case 0 if alt1.ref.symbol.is(Implicit) && alt2.ref.symbol.is(Implicit) => prev
+            case cmp => cmp
           val sv = Feature.sourceVersion
           if isWarnPriorityChangeVersion(sv) then
-            val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution))
             if disambiguate && cmp != prev then
               def warn(msg: Message) =
                 val critical = alt1.ref :: alt2.ref :: Nil

compiler/test/dotty/tools/dotc/StringFormatterTest.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>"

tests/neg/i2974.scala

@@ -0,0 +1,28 @@
+
+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]] // ok
+
+  locally:
+    given fa: Foo[Any] = ???
+    given ba: Bar[Int] = ???
+    summon[Foo[Int]] // error: now ambiguous,
+    // was resolving to `ba` when using intermediate rules:
+    // better means specialize, but map all type arguments downwards
+
+  locally:
+    implicit val fa: Foo[Any] = ???
+    implicit val ba: Bar[Int] = ???
+    summon[Foo[Int]] // is OK since we fall back to old rules for old-style implicits as a tie breaker
+}

tests/pos/given-priority.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/scala-uri.scala

@@ -0,0 +1,29 @@
+import scala.language.implicitConversions
+
+trait QueryKey[A]
+object QueryKey extends QueryKeyInstances
+sealed trait QueryKeyInstances:
+  implicit val stringQueryKey: QueryKey[String] = ???
+
+trait QueryValue[-A]
+object QueryValue extends QueryValueInstances
+sealed trait QueryValueInstances1:
+  implicit final val stringQueryValue: QueryValue[String] = ???
+  implicit final val noneQueryValue: QueryValue[None.type] = ???
+
+sealed trait QueryValueInstances extends QueryValueInstances1:
+  implicit final def optionQueryValue[A: QueryValue]: QueryValue[Option[A]] = ???
+
+trait QueryKeyValue[A]
+object QueryKeyValue:
+  implicit def tuple2QueryKeyValue[K: QueryKey, V: QueryValue]: QueryKeyValue[(K, V)] = ???
+
+
+@main def Test = summon[QueryKeyValue[(String, None.type)]]
+
+/*(using
+  QueryKeyValue.tuple2QueryKeyValue[String, None.type](
+    QueryKey.stringQueryKey,
+    QueryValue.optionQueryValue[A]))*/
+
+ // error

tests/run/enrich-gentraversable.check

@@ -4,5 +4,5 @@ List(2, 4)
 HW
 ArraySeq(72, 108, 108, 32, 114, 108, 100)
 Map(bar -> 2)
-TreeMap(bar -> 2)
+Map(bar -> 2)
 Map(bar -> 2)

tests/run/enrich-gentraversable.scala

@@ -47,7 +47,8 @@ object Test extends App {
 
     val tm = TreeMap(1 -> "foo", 2 -> "bar")
     val tmm = tm.filterMap { case (k, v) => if (k % 2 == 0) Some(v -> k) else None }
-    typed[TreeMap[String, Int]](tmm)
+    typed[Map[String, Int]](tmm) // was TreeMap, now Map,
+    // since `BuildFrom` is covariant in `That` and `TreeMap[String, Int] <:< Map[String, Int]`
     println(tmm)
 
     val mv = m.view

tests/run/implicits_poly.check

@@ -1 +1 @@
-barFoo
+fooFoo

tests/warn/i15503a.scala

@@ -149,8 +149,8 @@ object GivenImportOrderAtoB:
   object A { implicit val x: X = new X }
   object B { implicit val y: Y = new Y }
   class C {
-    import A._ // warn
-    import B._ // OK
+    import A._ // OK
+    import B._ // warn
     def t = implicitly[X]
   }
 
@@ -160,8 +160,8 @@ object GivenImportOrderBtoA:
   object A { implicit val x: X = new X }
   object B { implicit val y: Y = new Y }
   class C {
-    import B._ // OK
-    import A._ // warn
+    import B._ // warn
+    import A._ // OK
     def t = implicitly[X]
   }
 /* END : tests on given import order */