Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix implicit search failure reporting #20261

Merged
merged 4 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 55 additions & 48 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -3872,49 +3871,39 @@ 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

wtp.paramNames.lazyZip(wtp.paramInfos).lazyZip(args).foreach { (paramName, formal, arg) =>
arg.tpe match {
/** Reports errors for arguments of `appTree` that have a
* `SearchFailureType`, recursively traversing arguments that are
* themselves applications. `mt` must be the type of `appTree.fun`.
*/
def reportErrors(appTree: Apply, mt: MethodType): Unit =
val Apply(fun, args) = appTree
for (paramName, formal, arg) <- mt.paramNames.lazyZip(mt.paramInfos).lazyZip(args) do
arg.tpe match
case failure: SearchFailureType =>
report.error(
missingArgMsg(arg, formal, implicitParamString(paramName, methodStr, tree), paramSymWithMethodTree(paramName)),
tree.srcPos.endPos
)
case _ =>
}
}
untpd.Apply(tree, args).withType(propFail)
}
arg match
case childAppTree: Apply =>
childAppTree.fun.tpe.widen match
case childMt: MethodType => reportErrors(childAppTree, childMt)
case _ => ()
case _ => ()

val methodStr = err.refStr(methPart(fun).tpe)
val paramStr = implicitParamString(paramName, methodStr, fun)
val paramSymWithMethodCallTree =
fun.symbol.paramSymss.flatten
.find(_.name == paramName)
.map((_, appTree))
val message = missingArgMsg(arg, formal, paramStr, paramSymWithMethodCallTree)
// Note: if the same error type appears on several trees, we
// might report it several times, but this is not a problem
// because only the first one will be displayed. We traverse in
// post-order, so that the most detailed message gets displayed.
mbovel marked this conversation as resolved.
Show resolved Hide resolved
report.error(message, fun.srcPos.endPos)
case _ => ()

if (propFail.exists) {
val args = implicitArgs(wtp.paramInfos, 0, pt)
val firstFailure = args.tpes.find(_.isInstanceOf[SearchFailureType])
if (firstFailure.isDefined) {
// 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
Expand All @@ -3923,18 +3912,36 @@ 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 retyped: Apply if retyped.tpe.isError => reportErrors(retyped, wtp)
case _ => ()

retyped
else
val res = untpd.Apply(tree, args).withType(firstFailure.get)
Copy link
Member Author

@mbovel mbovel Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we would use the first non-ambiguous error if there is one; here I simplified it to just use the first error.

@odersky commented on #19414 that:

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

But I think this is not relevant anymore after the changes in this PR; as we're anyway reporting all errors by recursively visiting the argument trees.

reportErrors(res, wtp)
res
}
else tree match {
case tree: Block =>
Expand Down
4 changes: 4 additions & 0 deletions tests/neg/19414-desugared.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- [E172] Type Error: tests/neg/19414-desugared.scala:22:34 ------------------------------------------------------------
22 | summon[BodySerializer[JsObject]] // error: Ambiguous given instances
| ^
|Ambiguous given instances: both given instance given_Writer_JsValue and given instance given_Writer_JsObject match type Writer[B] of parameter writer of given instance given_BodySerializer_B
22 changes: 22 additions & 0 deletions tests/neg/19414-desugared.scala
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions tests/neg/19414.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- [E172] Type Error: tests/neg/19414.scala:15:34 ----------------------------------------------------------------------
15 | summon[BodySerializer[JsObject]] // error: Ambiguous given instances
| ^
|Ambiguous given instances: both given instance given_Writer_JsValue and given instance given_Writer_JsObject match type Writer[B] of a context parameter of given instance given_BodySerializer_B
15 changes: 15 additions & 0 deletions tests/neg/19414.scala
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions tests/neg/given-ambiguous-default-1.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- [E172] Type Error: tests/neg/given-ambiguous-default-1.scala:18:23 --------------------------------------------------
18 |def f: Unit = summon[B] // error: Ambiguous given instances
| ^
|Ambiguous given instances: both given instance a1 and given instance a2 match type A of parameter a of given instance given_B
18 changes: 18 additions & 0 deletions tests/neg/given-ambiguous-default-1.scala
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions tests/neg/given-ambiguous-default-2.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- [E172] Type Error: tests/neg/given-ambiguous-default-2.scala:18:23 --------------------------------------------------
18 |def f: Unit = summon[C] // error: Ambiguous given instances
| ^
|Ambiguous given instances: both given instance a1 and given instance a2 match type A of parameter a of given instance given_C
18 changes: 18 additions & 0 deletions tests/neg/given-ambiguous-default-2.scala
Original file line number Diff line number Diff line change
@@ -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
7 changes: 1 addition & 6 deletions tests/neg/i8827a.check
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
-- [E172] Type Error: tests/neg/i8827a.scala:16:26 ---------------------------------------------------------------------
16 | summon[Order[List[Foo]]] // error
| ^
| No given instance of type pkg.Order[List[pkg.Foo]] was found for parameter x of method summon in object Predef.
| I found:
|
| pkg.Order.orderList[pkg.Foo](/* missing */summon[pkg.Order[pkg.Foo]])
|
| But no implicit values were found that match type pkg.Order[pkg.Foo].
| No given instance of type pkg.Order[pkg.Foo] was found for parameter orderA of method orderList in object Order
|
| The following import might fix the problem:
|
Expand Down
7 changes: 1 addition & 6 deletions tests/neg/i8827b.check
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
-- [E172] Type Error: tests/neg/i8827b.scala:16:28 ---------------------------------------------------------------------
16 | summon[Order[Option[Foo]]] // error
| ^
|No given instance of type pkg.Order[Option[pkg.Foo]] was found for parameter x of method summon in object Predef.
|I found:
|
| pkg.Order.given_Order_Option[pkg.Foo](/* missing */summon[pkg.Order[pkg.Foo]])
|
|But no implicit values were found that match type pkg.Order[pkg.Foo].
|No given instance of type pkg.Order[pkg.Foo] was found for parameter orderA of given instance given_Order_Option in object Order
|
|The following import might fix the problem:
|
Expand Down
7 changes: 2 additions & 5 deletions tests/neg/i9568.check
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,10 @@
| No given instance of type => Monad[F] was found for parameter ev of method blaMonad in object Test.
| I found:
|
| Test.blaMonad[F², S](Test.blaMonad[F³, S²])
| Test.blaMonad[F², S]
|
| But method blaMonad in object Test does not match type => Monad[F²]
| But method blaMonad in object Test does not match type => Monad[F]
|
| where: F is a type variable with constraint <: [_] =>> Any
| F² is a type variable with constraint <: [_] =>> Any
| F³ is a type variable with constraint <: [_] =>> Any
| S is a type variable
| S² is a type variable
| .
7 changes: 1 addition & 6 deletions tests/neg/implicitSearch.check
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
-- [E172] Type Error: tests/neg/implicitSearch.scala:13:12 -------------------------------------------------------------
13 | sort(xs) // error (with a partially constructed implicit argument shown)
| ^
| No given instance of type Test.Ord[List[List[T]]] was found for parameter o of method sort in object Test.
| I found:
|
| Test.listOrd[List[T]](Test.listOrd[T](/* missing */summon[Test.Ord[T]]))
|
| But no implicit values were found that match type Test.Ord[T].
| No given instance of type Test.Ord[T] was found for parameter o of method listOrd in object Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in parameter name is good, but I think we should keep the "I found" part of the error message. Otherwise it's not clear why a listOrd is required in the first place. The "I found" part tells you that.

-- [E172] Type Error: tests/neg/implicitSearch.scala:15:38 -------------------------------------------------------------
15 | listOrd(listOrd(implicitly[Ord[T]] /*not found*/)) // error
| ^
Expand Down
11 changes: 3 additions & 8 deletions tests/neg/missing-implicit3.check
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
-- [E172] Type Error: tests/neg/missing-implicit3.scala:13:36 ----------------------------------------------------------
13 |val sortedFoos = sort(List(new Foo)) // error
| ^
| No given instance of type ord.Ord[ord.Foo] was found for a context parameter of method sort in package ord.
| I found:
|No given instance of type ord.Foo => Comparable[? >: ord.Foo] was found for parameter x$1 of given instance ordered in object Ord
|
| ord.Ord.ordered[ord.Foo](/* missing */summon[ord.Foo => Comparable[? >: ord.Foo]])
|The following import might make progress towards fixing the problem:
|
| But no implicit values were found that match type ord.Foo => Comparable[? >: ord.Foo].
|
| The following import might make progress towards fixing the problem:
|
| import scala.math.Ordered.orderingToOrdered
| import scala.math.Ordered.orderingToOrdered
|
Loading