Wrong error message for ambiguous given instances

[KuceraMartin]

Compiler version

3.3.1, 3.4.0-RC1-bin-20240109-91db06a-NIGHTLY

Minimized code

trait JsValue
trait JsObject extends JsValue

trait JsonWriter[T]
trait BodySerializer[-B]

class JsonPrinter

given JsonWriter[JsValue] = ???
given JsonWriter[JsObject] = ???

given[B : JsonWriter](using printer: JsonPrinter = new JsonPrinter): BodySerializer[B] = ???

def f: Unit =
  summon[BodySerializer[JsObject]]

Output

[error] No given instance of type BodySerializer[JsObject] was found for parameter x of method summon in object Predef.
[error] I found:
[error]
[error]     given_BodySerializer_B[B]()
[error]
[error] But given instance given_BodySerializer_B does not match type BodySerializer[JsObject].
[error]   summon[BodySerializer[JsObject]]
[error]                                   ^

Expectation

[error] Ambiguous given instances: both given instance given_JsonWriter_JsValue and given instance given_JsonWriter_JsObject match type JsonWriter[B] of parameter x of method summon in object Predef
[error]   summon[BodySerializer[JsObject]]
[error]                                   ^

note: the expectation is what I got when I removed the implicit JsonPrinter parameter from the given BodySerializer:

given[B : JsonWriter]: BodySerializer[B] = ???

[joroKr21]

Perhaps a related issue - when the instance below I found is a macro, it might have a custom error message.
It would be nice to show the custom error message instead of a type mismatch.

[mbovel]

If I add given JsonPrinter = new JsonPrinter, I also get the expected error message.

[scala-center-bot]

This issue was picked for the Issue Spree No. 42 of February 6th, 2024. @mbovel, @KuceraMartin, @nmcb will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

[mbovel]

Note: it works if the default parameter is in a separate list:

given[B](using writer: JsonWriter[B])(using printer: JsonPrinter = new JsonPrinter): BodySerializer[B] = ???

[Jezisek-Ematiq]

The contextual bound desugars to an implicit rather than using which seems to cause a problem in Applications.scala:641 (it doesn't fall into the else if branch because it's not a contextual function)

[mbovel]

We tried:

diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
index 7727c125d1..9d35f80f66 100644
--- a/compiler/src/dotty/tools/dotc/typer/Typer.scala
+++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -3899,7 +3899,7 @@ 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
+          if hasDefaultParams && !args.exists(_.tpe.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

which yields the following error message:

-- [E172] Type Error: tests/init/pos/19414.scala:14:46 -------------------------
14 |def f: Unit = summon[BodySerializer[JsObject]]
   |                                              ^
   |No given instance of type BodySerializer[JsObject] was found for parameter x of method summon in object Predef.
   |I found:
   |
   |    given_BodySerializer_B[B](
   |      /* ambiguous: both given instance given_Writer_JsValue and given instance given_Writer_JsObject match type Writer[B] */
   |        summon[Writer[B]],
   |    /* missing */summon[Printer])
   |
   |But no implicit values were found that match type Printer.
1 error found

Note: it says summon[Printer] is missing, which is wrong.

---

I also tried:

diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
index 7727c125d1..32c9474891 100644
--- a/compiler/src/dotty/tools/dotc/typer/Typer.scala
+++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -3899,9 +3899,9 @@ 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
+          if hasDefaultParams then
             val namedArgs = wtp.paramNames.lazyZip(args).flatMap { (pname, arg) =>
-              if (arg.tpe.isError) Nil else untpd.NamedArg(pname, untpd.TypedSplice(arg)) :: Nil
+              if (arg.tpe.isError && !arg.tpe.isInstanceOf[AmbiguousImplicits]) Nil else untpd.NamedArg(pname, untpd.TypedSplice(arg)) 
:: Nil
             }

which yields the following error message:

-- [E172] Type Error: tests/init/pos/19414.scala:14:46 -------------------------
14 |def f: Unit = summon[BodySerializer[JsObject]]
   |                                              ^
   |No given instance of type BodySerializer[JsObject] was found for parameter x of method summon in object Predef.
   |I found:
   |
   |    given_BodySerializer_B[B](using
   |      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 given instance given_BodySerializer_B does not match type BodySerializer[JsObject].
   |
   |Note: given instance given_BodySerializer_B was not considered because it was not imported with `import given`.
1 error found

Note: the note is wrong.

[mbovel]

I don't understand why propagatedFailure has its current semantic:

https://github.com/lampepfl/dotty/blob/641ab7a4984cd76ff77fadaf822472a0a930e4a2/compiler/src/dotty/tools/dotc/typer/Typer.scala#L3853-L3867

It returns the first non-ambiguous error if there is one, or else the first error, or else NoType.

Written otherwise:

val firstNonAmbiguous = args.tpes.find(tp => tp.isError && !tp.isInstanceOf[AmbiguousImplicits])
def firstError = args.tpes.find(_.isError)
val propFail = firstNonAmbiguous.orElse(firstError).getOrElse(NoType)

Why isn't it the opposite; why doesn't it return the first ambiguous error if there is one?

val firstAmbiguous = args.tpes.find(_.isInstanceOf[AmbiguousImplicits])
def firstError = args.tpes.find(_.isError)
val propFail = firstAmbiguous.orElse(firstError).getOrElse(NoType)

Can adding default arguments avoid an ambiguous error? That would explain it. If not, we could probably report it directly.

[mbovel]

Something else: even when the expected message is shown Ambiguous given instances: ..., the parameters name is wrong: it should be evidence$1 of given_BodySerializer_B, not x of summon.

[odersky]

don't understand why propagatedFailure has its current semantic:

Ambiguous implicits often arise when something else is failing (for instance a variable was not instantiated enough). That's why we try to find first other errors. That was the thinking behind the current semantic. So I am not sure reversing this as in #19647 is always the thing to do. It might make error messages worse in other contexts.

[mbovel]

Looking again into this, the main difference between the case with the default arg and the case without is that in the former, issueErrors is not called, so the tree doesn't get a SearchFailure type and the failure does not "bubble up", while it does in the later case.

I made two tentatives to call it in the default args case:

1. 292865a: Parametrize issueErrors with the argument trees and call it with the retyped arg trees in the default args case. However, the retyped tree is not always an Apply node.
2. 7468f25: Call issueErrors when the type of the retyped tree in the default args case is an error, but use the arguments of the apply tree without default args and filter out default arguments.

Neither look very nice… Not sure if there is a simpler way.

By the way, I also found why the parameters name is wrong: as the SearchFailure bubble up, issueFailure can't know if the failure comes from a nested application or not. Currently, it just assumes it's not nested, which is wrong for the test case of this issue.