Fix implicit search failure reporting

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

@@ -2938,11 +2938,20 @@ class MissingImplicitArgument(
 
     def location(preposition: String) = if (where.isEmpty) "" else s" $preposition $where"
 
+    /** Default error message for non-nested ambiguous implicits. */
     def defaultAmbiguousImplicitMsg(ambi: AmbiguousImplicits) =
       s"Ambiguous given instances: ${ambi.explanation}${location("of")}"
 
+    /** Default error messages for non-ambiguous implicits, or nested ambiguous
+     *  implicits.
+     *
+     *  The default message is shown for ambiguous implicits only if they have
+     *  the `nested` flag set. In this case, we output "no best given instance"
+     *  instead of "no given instance".
+     */
     def defaultImplicitNotFoundMessage =
-      i"No given instance of type $pt was found${location("for")}"
+      val bestStr = if arg.tpe.isInstanceOf[AmbiguousImplicits] then " best" else ""
+      i"No$bestStr given instance of type $pt was found${location("for")}"
 
     /** Construct a custom error message given an ambiguous implicit
      *  candidate `alt` and a user defined message `raw`.
@@ -2980,7 +2989,7 @@ class MissingImplicitArgument(
      *  def foo(implicit foo: Foo): Any = ???
      */
     arg.tpe match
-      case ambi: AmbiguousImplicits =>
+      case ambi: AmbiguousImplicits if !ambi.nested =>
         (ambi.alt1, ambi.alt2) match
           case (alt @ AmbiguousImplicitMsg(msg), _) =>
             userDefinedAmbiguousImplicitMsg(alt, msg)

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

@@ -537,7 +537,7 @@ object Implicits:
   end TooUnspecific
 
   /** An ambiguous implicits failure */
-  class AmbiguousImplicits(val alt1: SearchSuccess, val alt2: SearchSuccess, val expectedType: Type, val argument: Tree) extends SearchFailureType:
+  class AmbiguousImplicits(val alt1: SearchSuccess, val alt2: SearchSuccess, val expectedType: Type, val argument: Tree, val nested: Boolean = false) extends SearchFailureType:
 
     def msg(using Context): Message =
       var str1 = err.refStr(alt1.ref)

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

@@ -3707,7 +3707,6 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
 
   private def adapt1(tree: Tree, pt: Type, locked: TypeVars)(using Context): Tree = {
     assert(pt.exists && !pt.isInstanceOf[ExprType] || ctx.reporter.errorsReported, i"tree: $tree, pt: $pt")
-    def methodStr = err.refStr(methPart(tree).tpe)
 
     def readapt(tree: Tree)(using Context) = adapt(tree, pt, locked)
     def readaptSimplified(tree: Tree)(using Context) = readapt(simplify(tree, pt, locked))
@@ -3872,49 +3871,37 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
                 arg :: inferArgsAfter(arg)
         end implicitArgs
 
-        val args = implicitArgs(wtp.paramInfos, 0, pt)
-
-        def propagatedFailure(args: List[Tree]): Type = args match {
-          case arg :: args1 =>
-            arg.tpe match {
-              case ambi: AmbiguousImplicits =>
-                propagatedFailure(args1) match {
-                  case NoType | (_: AmbiguousImplicits) => ambi
-                  case failed => failed
-                }
-              case failed: SearchFailureType => failed
-              case _ => propagatedFailure(args1)
-            }
-          case Nil => NoType
-        }
-
-        val propFail = propagatedFailure(args)
-
-        def issueErrors(): Tree = {
-          def paramSymWithMethodTree(paramName: TermName) =
-            if tree.symbol.exists then
-              tree.symbol.paramSymss.flatten
-                .map(sym => sym.name -> sym)
-                .toMap
-                .get(paramName)
-                .map((_, tree))
-            else
-              None
+        /** Reports errors for arguments of `appTree` that have a
+          * `SearchFailureType`.
+          */
+        def issueErrors(fun: Tree, args: List[Tree]): Tree =
+          def firstFailure = args.tpes.find(_.isInstanceOf[SearchFailureType]).getOrElse(NoType)
+          val errorType =
+            firstFailure match
+              case tp: AmbiguousImplicits =>
+                AmbiguousImplicits(tp.alt1, tp.alt2, tp.expectedType, tp.argument, nested = true)
+              case tp =>
+                tp
+          val res = untpd.Apply(fun, args).withType(errorType)
 
           wtp.paramNames.lazyZip(wtp.paramInfos).lazyZip(args).foreach { (paramName, formal, arg) =>
-            arg.tpe match {
+            arg.tpe match
               case failure: SearchFailureType =>
+                val methodStr = err.refStr(methPart(fun).tpe)
+                val paramStr = implicitParamString(paramName, methodStr, fun)
+                val paramSym = fun.symbol.paramSymss.flatten.find(_.name == paramName)
+                val paramSymWithMethodCallTree = paramSym.map((_, res))
                 report.error(
-                  missingArgMsg(arg, formal, implicitParamString(paramName, methodStr, tree), paramSymWithMethodTree(paramName)),
-                  tree.srcPos.endPos
-                )
+                   missingArgMsg(arg, formal, paramStr, paramSymWithMethodCallTree),
+                   tree.srcPos.endPos
+                 )
               case _ =>
-            }
           }
-          untpd.Apply(tree, args).withType(propFail)
-        }
 
-        if (propFail.exists) {
+          res
+
+        val args = implicitArgs(wtp.paramInfos, 0, pt)
+        if (args.tpes.exists(_.isInstanceOf[SearchFailureType])) {
           // If there are several arguments, some arguments might already
           // have influenced the context, binding variables, but later ones
           // might fail. In that case the constraint and instantiated variables
@@ -3923,18 +3910,31 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
 
           // If method has default params, fall back to regular application
           // where all inferred implicits are passed as named args.
-          if hasDefaultParams && !propFail.isInstanceOf[AmbiguousImplicits] then
-            val namedArgs = wtp.paramNames.lazyZip(args).flatMap { (pname, arg) =>
-              if (arg.tpe.isError) Nil else untpd.NamedArg(pname, untpd.TypedSplice(arg)) :: Nil
-            }
+          if hasDefaultParams then
+            // Only keep the arguments that don't have an error type, or that
+            // have an `AmbiguousImplicits` error type. The later ensures that a
+            // default argument can't override an ambiguous implicit. See tests
+            // `given-ambiguous-default*` and `19414*`.
+            val namedArgs =
+              wtp.paramNames.lazyZip(args)
+                .filter((_, arg) => !arg.tpe.isError || arg.tpe.isInstanceOf[AmbiguousImplicits])
+                .map((pname, arg) => untpd.NamedArg(pname, untpd.TypedSplice(arg)))
+
             val app = cpy.Apply(tree)(untpd.TypedSplice(tree), namedArgs)
             val needsUsing = wtp.isContextualMethod || wtp.match
               case MethodType(ContextBoundParamName(_) :: _) => sourceVersion.isAtLeast(`3.4`)
               case _ => false
             if needsUsing then app.setApplyKind(ApplyKind.Using)
             typr.println(i"try with default implicit args $app")
-            typed(app, pt, locked)
-          else issueErrors()
+            val retyped = typed(app, pt, locked)
+
+            // If the retyped tree still has an error type and is an `Apply`
+            // node, we can report the errors for each argument nicely.
+            // Otherwise, we don't report anything here.
+            retyped match
+              case Apply(tree, args) if retyped.tpe.isError => issueErrors(tree, args)
+              case _ => retyped
+          else issueErrors(tree, args)
         }
         else tree match {
           case tree: Block =>

tests/neg/19414-desugared.check

@@ -0,0 +1,14 @@
+-- [E172] Type Error: tests/neg/19414-desugared.scala:22:34 ------------------------------------------------------------
+22 |  summon[BodySerializer[JsObject]] // error: Ambiguous given instances
+   |                                  ^
+   |No best given instance of type BodySerializer[JsObject] was found for parameter x of method summon in object Predef.
+   |I found:
+   |
+   |    given_BodySerializer_B[B](
+   |      writer =
+   |        /* ambiguous: both given instance given_Writer_JsValue and given instance given_Writer_JsObject match type Writer[B] */
+   |          summon[Writer[B]]
+   |      ,
+   |    this.given_BodySerializer_B$default$2[B])
+   |
+   |But both given instance given_Writer_JsValue and given instance given_Writer_JsObject match type Writer[B].

tests/neg/19414-desugared.scala

@@ -0,0 +1,22 @@
+trait JsValue
+trait JsObject extends JsValue
+
+trait Writer[T]
+trait BodySerializer[-B]
+
+class Printer
+
+given Writer[JsValue] = ???
+given Writer[JsObject] = ???
+
+// This is not an exact desugaring of the original code: currently the compiler
+// actually changes the modifier of the parameter list from `using` to
+// `implicit` when desugaring the context-bound `B: Writer` to `implicit writer:
+// Writer[B]`, but we can't write it in user code as this is not valid syntax.
+given [B](using
+    writer: Writer[B],
+    printer: Printer = new Printer
+): BodySerializer[B] = ???
+
+def f: Unit =
+  summon[BodySerializer[JsObject]] // error: Ambiguous given instances

tests/neg/19414.check

@@ -0,0 +1,14 @@
+-- [E172] Type Error: tests/neg/19414.scala:15:34 ----------------------------------------------------------------------
+15 |  summon[BodySerializer[JsObject]] // error: Ambiguous given instances
+   |                                  ^
+   |No best given instance of type BodySerializer[JsObject] was found for parameter x of method summon in object Predef.
+   |I found:
+   |
+   |    given_BodySerializer_B[B](
+   |      evidence$1 =
+   |        /* ambiguous: both given instance given_Writer_JsValue and given instance given_Writer_JsObject match type Writer[B] */
+   |          summon[Writer[B]]
+   |      ,
+   |    this.given_BodySerializer_B$default$2[B])
+   |
+   |But both given instance given_Writer_JsValue and given instance given_Writer_JsObject match type Writer[B].

tests/neg/19414.scala

@@ -0,0 +1,15 @@
+trait JsValue
+trait JsObject extends JsValue
+
+trait Writer[T]
+trait BodySerializer[-B]
+
+class Printer
+
+given Writer[JsValue] = ???
+given Writer[JsObject] = ???
+
+given [B: Writer](using printer: Printer = new Printer): BodySerializer[B] = ???
+
+def f: Unit =
+  summon[BodySerializer[JsObject]] // error: Ambiguous given instances

tests/neg/given-ambiguous-1.check

@@ -0,0 +1,9 @@
+-- [E172] Type Error: tests/neg/given-ambiguous-1.scala:12:23 ----------------------------------------------------------
+12 |def f: Unit = summon[B] // error: Ambiguous given instances
+   |                       ^
+   |                No best given instance of type B was found for parameter x of method summon in object Predef.
+   |                I found:
+   |
+   |                    given_B(/* ambiguous: both given instance a1 and given instance a2 match type A */summon[A])
+   |
+   |                But both given instance a1 and given instance a2 match type A.

tests/neg/given-ambiguous-1.scala

@@ -0,0 +1,12 @@
+class A
+class B
+given a1: A = ???
+given a2: A = ???
+given (using a: A): B = ???
+
+// In this case, the ambiguous given instance is not directly the argument of
+// `summon`; it is the argument of `given_B` which is needed for the argument of
+// `summon`. This is a nested ambiguous implicit, thus we report an error in
+// the style "I found ... but". See `given-ambiguous-2` for a direct ambiguous
+// implicit error.
+def f: Unit = summon[B] // error: Ambiguous given instances

tests/neg/given-ambiguous-2.check

@@ -0,0 +1,4 @@
+-- [E172] Type Error: tests/neg/given-ambiguous-2.scala:10:15 ----------------------------------------------------------
+10 |def f: Unit = g // error: Ambiguous given instances
+   |               ^
+   | Ambiguous given instances: both given instance a1 and given instance a2 match type A of parameter a of method g

tests/neg/given-ambiguous-2.scala

@@ -0,0 +1,10 @@
+class A
+class B
+given a1: A = ???
+given a2: A = ???
+def g(using a: A): B = ???
+
+// In this case, the ambiguous given instance is directly the argument of
+// `summon`. This is a direct ambiguous implicit, thus we report the error
+// directly. See `given-ambiguous-1` for a nested ambiguous implicit error.
+def f: Unit = g // error: Ambiguous given instances

tests/neg/given-ambiguous-default-1.check

@@ -0,0 +1,9 @@
+-- [E172] Type Error: tests/neg/given-ambiguous-default-1.scala:18:23 --------------------------------------------------
+18 |def f: Unit = summon[B] // error: Ambiguous given instances
+   |                       ^
+   |            No best given instance of type B was found for parameter x of method summon in object Predef.
+   |            I found:
+   |
+   |                given_B(a = /* ambiguous: both given instance a1 and given instance a2 match type A */summon[A])
+   |
+   |            But both given instance a1 and given instance a2 match type A.

tests/neg/given-ambiguous-default-1.scala

@@ -0,0 +1,18 @@
+/** This test checks that provided ambiguous given instances take precedence
+  * over default given arguments. In the following code, the compiler must
+  * report an "Ambiguous implicits" error for the parameter `a`, and must not
+  * use its default value.
+  *
+  * See also:
+  *   - tests/neg/19414.scala
+  *   - tests/neg/19414-desugared.scala
+  *   - tests/neg/given-ambiguous-default-2.scala
+  */
+
+class A
+class B
+given a1: A = ???
+given a2: A = ???
+given (using a: A = A()): B = ???
+
+def f: Unit = summon[B] // error: Ambiguous given instances

tests/neg/given-ambiguous-default-2.check

@@ -0,0 +1,9 @@
+-- [E172] Type Error: tests/neg/given-ambiguous-default-2.scala:18:23 --------------------------------------------------
+18 |def f: Unit = summon[C] // error: Ambiguous given instances
+   |                       ^
+   |No best given instance of type C was found for parameter x of method summon in object Predef.
+   |I found:
+   |
+   |    given_C(a = /* ambiguous: both given instance a1 and given instance a2 match type A */summon[A], this.given_C$default$2)
+   |
+   |But both given instance a1 and given instance a2 match type A.

tests/neg/given-ambiguous-default-2.scala

@@ -0,0 +1,18 @@
+/** This test checks that provided given instances take precedence over default
+  * given arguments, even when there are multiple default arguments. Before the
+  * fix for issue #19414, this code would compile without errors.
+  *
+  * See also:
+  *   - tests/neg/given-ambiguous-default-1.scala
+  *   - tests/neg/19414.scala
+  *   - tests/neg/19414-desugared.scala
+  */
+
+class A
+class B
+class C
+given a1: A = ???
+given a2: A = ???
+given (using a: A = A(), b: B = B()): C = ???
+
+def f: Unit = summon[C] // error: Ambiguous given instances