-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix implicit search failure reporting #20261
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
Changes from 2 commits
fc06435
0890e7a
0e4d51a
ebf58bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,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. | ||
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 | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 => | ||
|
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| ^ | ||
|
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 | ||
| |
Uh oh!
There was an error while loading. Please reload this page.