From b519258c00e484ec24c49f1f53eafe4ad0b96771 Mon Sep 17 00:00:00 2001 From: odersky Date: Fri, 5 Jul 2024 14:13:45 +0200 Subject: [PATCH] Refine implicit priority change warnings Fixes #21036 Fixes #20572 --- .../dotty/tools/dotc/typer/Implicits.scala | 32 +++++++++++++------ tests/neg/given-triangle.check | 4 +++ tests/{warn => neg}/given-triangle.scala | 4 +-- tests/{warn => pos}/bson/Test.scala | 0 tests/{warn => pos}/bson/bson.scala | 0 tests/pos/i20572.scala | 7 ++++ tests/pos/i21036.scala | 16 ++++++++++ tests/run/given-triangle.scala | 2 +- tests/warn/bson.check | 10 ------ tests/warn/given-triangle.check | 6 ---- 10 files changed, 51 insertions(+), 30 deletions(-) create mode 100644 tests/neg/given-triangle.check rename tests/{warn => neg}/given-triangle.scala (73%) rename tests/{warn => pos}/bson/Test.scala (100%) rename tests/{warn => pos}/bson/bson.scala (100%) create mode 100644 tests/pos/i20572.scala create mode 100644 tests/pos/i21036.scala delete mode 100644 tests/warn/bson.check delete mode 100644 tests/warn/given-triangle.check diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 80f9b4f2fd31..36fed0b15d70 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1302,9 +1302,8 @@ trait Implicits: // A map that associates a priority change warning (between -source 3.4 and 3.6) // with the candidate refs mentioned in the warning. We report the associated - // message if both candidates qualify in tryImplicit and at least one of the candidates - // is part of the result of the implicit search. - val priorityChangeWarnings = mutable.ListBuffer[(TermRef, TermRef, Message)]() + // message if one of the critical candidates is part of the result of the implicit search. + val priorityChangeWarnings = mutable.ListBuffer[(/*critical:*/ List[TermRef], Message)]() /** Compare `alt1` with `alt2` to determine which one should be chosen. * @@ -1319,11 +1318,16 @@ trait Implicits: * return new result with preferGeneral = true * 3.6 and higher: compare with preferGeneral = true * + * @param only2ndCritical If true only the second alternative is critical in case + * of a priority change. */ - def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel): Int = + def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel, only2ndCritical: Boolean = false): Int = def comp(using Context) = explore(compare(alt1.ref, alt2.ref, preferGeneral = true)) def warn(msg: Message) = - priorityChangeWarnings += ((alt1.ref, alt2.ref, msg)) + val critical = + if only2ndCritical then alt2.ref :: Nil + else alt1.ref :: alt2.ref :: Nil + priorityChangeWarnings += ((critical, msg)) if alt1.ref eq alt2.ref then 0 else if alt1.level != alt2.level then alt1.level - alt2.level else @@ -1443,8 +1447,8 @@ trait Implicits: compareAlternatives(newCand, cand) > 0) else // keep only warnings that don't involve the failed candidate reference - priorityChangeWarnings.filterInPlace: (ref1, ref2, _) => - ref1 != cand.ref && ref2 != cand.ref + priorityChangeWarnings.filterInPlace: (critical, _) => + !critical.contains(cand.ref) rank(remaining, found, fail :: rfailures) case best: SearchSuccess => if (ctx.mode.is(Mode.ImplicitExploration) || isCoherent) @@ -1454,7 +1458,15 @@ trait Implicits: val newPending = if (retained eq found) || remaining.isEmpty then remaining else remaining.filterConserve(cand => - compareAlternatives(retained, cand) <= 0) + compareAlternatives(retained, cand, only2ndCritical = true) <= 0) + // Here we drop some pending alternatives but retain in each case + // `retained`. Therefore, it's a priorty change only if the + // second alternative appears in the final search result. Otherwise + // we have the following scenario: + // - 1st alternative, bit not snd appears in final result + // - Hence, snd was eliminated either here, or otherwise by a direct + // comparison later. + // - Hence, no change in resolution. rank(newPending, retained, rfailures) case fail: SearchFailure => // The ambiguity happened in the current search: to recover we @@ -1601,8 +1613,8 @@ trait Implicits: throw ex val result = rank(sort(eligible), NoMatchingImplicitsFailure, Nil) - for (ref1, ref2, msg) <- priorityChangeWarnings do - if result.found.exists(ref => ref == ref1 || ref == ref2) then + for (critical, msg) <- priorityChangeWarnings do + if result.found.exists(critical.contains(_)) then report.warning(msg, srcPos) result end searchImplicit diff --git a/tests/neg/given-triangle.check b/tests/neg/given-triangle.check new file mode 100644 index 000000000000..bf92efac17fd --- /dev/null +++ b/tests/neg/given-triangle.check @@ -0,0 +1,4 @@ +-- [E172] Type Error: tests/neg/given-triangle.scala:14:18 ------------------------------------------------------------- +14 |@main def Test = f // error + | ^ + |Ambiguous given instances: both given instance given_B and given instance given_C match type A of parameter a of method f diff --git a/tests/warn/given-triangle.scala b/tests/neg/given-triangle.scala similarity index 73% rename from tests/warn/given-triangle.scala rename to tests/neg/given-triangle.scala index ee4888ed1e06..9cc23104fcce 100644 --- a/tests/warn/given-triangle.scala +++ b/tests/neg/given-triangle.scala @@ -1,5 +1,3 @@ -//> using options -source 3.6-migration - class A class B extends A class C extends A @@ -13,4 +11,4 @@ def f(using a: A, b: B, c: C) = println(b.getClass) println(c.getClass) -@main def Test = f // warn +@main def Test = f // error diff --git a/tests/warn/bson/Test.scala b/tests/pos/bson/Test.scala similarity index 100% rename from tests/warn/bson/Test.scala rename to tests/pos/bson/Test.scala diff --git a/tests/warn/bson/bson.scala b/tests/pos/bson/bson.scala similarity index 100% rename from tests/warn/bson/bson.scala rename to tests/pos/bson/bson.scala diff --git a/tests/pos/i20572.scala b/tests/pos/i20572.scala new file mode 100644 index 000000000000..4ee4490c839c --- /dev/null +++ b/tests/pos/i20572.scala @@ -0,0 +1,7 @@ +//> using options -Werror +trait Writes[T] +trait Format[T] extends Writes[T] +given [T: List]: Writes[T] = null +given [T]: Format[T] = null + +val _ = summon[Writes[Int]] diff --git a/tests/pos/i21036.scala b/tests/pos/i21036.scala new file mode 100644 index 000000000000..1c98346e4ef3 --- /dev/null +++ b/tests/pos/i21036.scala @@ -0,0 +1,16 @@ +//> using options -source 3.5 -Werror +trait SameRuntime[A, B] +trait BSONWriter[T] +trait BSONHandler[T] extends BSONWriter[T] + +opaque type Id = String +object Id: + given SameRuntime[Id, String] = ??? + +given BSONHandler[String] = ??? +given [T: BSONHandler]: BSONHandler[List[T]] = ??? + +given opaqueWriter[T, A](using rs: SameRuntime[T, A], writer: BSONWriter[A]): BSONWriter[T] = ??? + +val x = summon[BSONHandler[List[Id]]] // this doesn't emit warning +val y = summon[BSONWriter[List[Id]]] // this did emit warning diff --git a/tests/run/given-triangle.scala b/tests/run/given-triangle.scala index 5ddba8df8b7b..0b483e87f28c 100644 --- a/tests/run/given-triangle.scala +++ b/tests/run/given-triangle.scala @@ -1,4 +1,4 @@ -import language.future +import language.`3.6` class A class B extends A diff --git a/tests/warn/bson.check b/tests/warn/bson.check deleted file mode 100644 index 258ac4b4ff2c..000000000000 --- a/tests/warn/bson.check +++ /dev/null @@ -1,10 +0,0 @@ --- Warning: tests/warn/bson/Test.scala:5:60 ---------------------------------------------------------------------------- -5 |def typedMapHandler[K, V: BSONHandler] = stringMapHandler[V] // warn - | ^ - |Given search preference for bson.BSONWriter[Map[String, V]] between alternatives (bson.BSONWriter.mapWriter : [V²](using x$1: bson.BSONWriter[V²]): bson.BSONDocumentWriter[Map[String, V²]]) and (bson.BSONWriter.collectionWriter : - | [T, Repr <: Iterable[T]](using x$1: bson.BSONWriter[T], x$2: Repr ¬ Option[T]): bson.BSONWriter[Repr]) will change - |Current choice : the first alternative - |New choice from Scala 3.6: none - it's ambiguous - | - |where: V is a type in method typedMapHandler - | V² is a type variable diff --git a/tests/warn/given-triangle.check b/tests/warn/given-triangle.check deleted file mode 100644 index e849f9d4d642..000000000000 --- a/tests/warn/given-triangle.check +++ /dev/null @@ -1,6 +0,0 @@ --- Warning: tests/warn/given-triangle.scala:16:18 ---------------------------------------------------------------------- -16 |@main def Test = f // warn - | ^ - | Change in given search preference for A between alternatives (given_A : A) and (given_B : B) - | Previous choice : the second alternative - | New choice from Scala 3.6: the first alternative