From 2549aeca80723561a93a86a97a11b4e891033e8e Mon Sep 17 00:00:00 2001 From: Eugene Flesselle Date: Thu, 25 Jul 2024 14:30:58 +0200 Subject: [PATCH 01/13] Update check-files and error annotations .. of tests explicitly checking implicits resolving to most specific --- tests/neg/i2974.scala | 24 ++++++++++++++++++++++++ tests/pos/i2974.scala | 12 ------------ tests/run/enrich-gentraversable.check | 2 +- tests/run/enrich-gentraversable.scala | 3 ++- tests/run/implicits_poly.check | 2 +- tests/warn/i15503a.scala | 8 ++++---- 6 files changed, 32 insertions(+), 19 deletions(-) create mode 100644 tests/neg/i2974.scala delete mode 100644 tests/pos/i2974.scala diff --git a/tests/neg/i2974.scala b/tests/neg/i2974.scala new file mode 100644 index 000000000000..df5dc5c461fa --- /dev/null +++ b/tests/neg/i2974.scala @@ -0,0 +1,24 @@ + +trait Foo[-T] +trait Bar[-T] extends Foo[T] + +object Test { + + locally: + implicit val fa: Foo[Int] = ??? + implicit val ba: Bar[Int] = ??? + summon[Foo[Int]] // ok + + locally: + implicit val fa: Foo[Int] = ??? + implicit val ba: Bar[Any] = ??? + summon[Foo[Int]] // ok + + locally: + implicit val fa: Foo[Any] = ??? + implicit val ba: Bar[Int] = ??? + summon[Foo[Int]] // error: now ambiguous, + // was resolving to `ba` when using intermediate rules: + // better means specialize, but map all type arguments downwards + +} diff --git a/tests/pos/i2974.scala b/tests/pos/i2974.scala deleted file mode 100644 index 75c6a24a41bb..000000000000 --- a/tests/pos/i2974.scala +++ /dev/null @@ -1,12 +0,0 @@ -trait Foo[-T] - -trait Bar[-T] extends Foo[T] - -object Test { - implicit val fa: Foo[Any] = ??? - implicit val ba: Bar[Int] = ??? - - def test: Unit = { - implicitly[Foo[Int]] - } -} diff --git a/tests/run/enrich-gentraversable.check b/tests/run/enrich-gentraversable.check index 7a5611a13a08..dd33eda94c7a 100644 --- a/tests/run/enrich-gentraversable.check +++ b/tests/run/enrich-gentraversable.check @@ -4,5 +4,5 @@ List(2, 4) HW ArraySeq(72, 108, 108, 32, 114, 108, 100) Map(bar -> 2) -TreeMap(bar -> 2) +Map(bar -> 2) Map(bar -> 2) diff --git a/tests/run/enrich-gentraversable.scala b/tests/run/enrich-gentraversable.scala index 887a6aea7983..170861fc861e 100644 --- a/tests/run/enrich-gentraversable.scala +++ b/tests/run/enrich-gentraversable.scala @@ -47,7 +47,8 @@ object Test extends App { val tm = TreeMap(1 -> "foo", 2 -> "bar") val tmm = tm.filterMap { case (k, v) => if (k % 2 == 0) Some(v -> k) else None } - typed[TreeMap[String, Int]](tmm) + typed[Map[String, Int]](tmm) // was TreeMap, now Map, + // since `BuildFrom` is covariant in `That` and `TreeMap[String, Int] <:< Map[String, Int]` println(tmm) val mv = m.view diff --git a/tests/run/implicits_poly.check b/tests/run/implicits_poly.check index 0b148ee47940..d7e3e4927e18 100644 --- a/tests/run/implicits_poly.check +++ b/tests/run/implicits_poly.check @@ -1 +1 @@ -barFoo +fooFoo diff --git a/tests/warn/i15503a.scala b/tests/warn/i15503a.scala index df8691c21a13..972ffe878d15 100644 --- a/tests/warn/i15503a.scala +++ b/tests/warn/i15503a.scala @@ -149,8 +149,8 @@ object GivenImportOrderAtoB: object A { implicit val x: X = new X } object B { implicit val y: Y = new Y } class C { - import A._ // warn - import B._ // OK + import A._ // OK + import B._ // warn def t = implicitly[X] } @@ -160,8 +160,8 @@ object GivenImportOrderBtoA: object A { implicit val x: X = new X } object B { implicit val y: Y = new Y } class C { - import B._ // OK - import A._ // warn + import B._ // warn + import A._ // OK def t = implicitly[X] } /* END : tests on given import order */ From 59102c56717306413ee7f5390cd02345e7a19392 Mon Sep 17 00:00:00 2001 From: Eugene Flesselle Date: Thu, 25 Jul 2024 15:10:04 +0200 Subject: [PATCH 02/13] Drop prioritization of `given`s over `implicit`s But keep the exception that NotGiven has lower priority --- .../dotty/tools/dotc/typer/Applications.scala | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 42765cd6c0bf..2942fac68805 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1831,7 +1831,8 @@ trait Applications extends Compatibility { } case _ => // (3) def compareValues(tp1: Type, tp2: Type)(using Context) = - isAsGoodValueType(tp1, tp2, alt1.symbol.is(Implicit), alt2.symbol.is(Implicit)) + extension (alt: TermRef) def isNotGivenClass: Boolean = alt.symbol == defn.NotGivenClass + isAsGoodValueType(tp1, tp2, alt1.isNotGivenClass, alt2.isNotGivenClass) tp2 match case tp2: MethodType => true // (3a) case tp2: PolyType if tp2.resultType.isInstanceOf[MethodType] => true // (3a) @@ -1868,7 +1869,7 @@ trait Applications extends Compatibility { * for overloading resolution (when `preferGeneral is false), and the opposite relation * `U <: T` or `U convertible to `T` for implicit disambiguation between givens * (when `preferGeneral` is true). For old-style implicit values, the 3.4 behavior is kept. - * If one of the alternatives is an implicit and the other is a given (or an extension), the implicit loses. + * If one of the alternatives is a NotGivenClass, and the other is not, then the NotGivenClass loses. * * - In Scala 3.5 and Scala 3.6-migration, we issue a warning if the result under * Scala 3.6 differ wrt to the old behavior up to 3.5. @@ -1876,7 +1877,7 @@ trait Applications extends Compatibility { * Also and only for given resolution: If a compared type refers to a given or its module class, use * the intersection of its parent classes instead. */ - def isAsGoodValueType(tp1: Type, tp2: Type, alt1IsImplicit: Boolean, alt2IsImplicit: Boolean)(using Context): Boolean = + def isAsGoodValueType(tp1: Type, tp2: Type, alt1IsNotGivenClass: Boolean, alt2IsNotGivenClass: Boolean)(using Context): Boolean = val oldResolution = ctx.mode.is(Mode.OldImplicitResolution) if !preferGeneral || Feature.migrateTo3 && oldResolution then // Normal specificity test for overloading resolution (where `preferGeneral` is false) @@ -1892,10 +1893,7 @@ trait Applications extends Compatibility { val tp1p = prepare(tp1) val tp2p = prepare(tp2) - if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`) - || oldResolution - || alt1IsImplicit && alt2IsImplicit - then + if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`) || oldResolution then // Intermediate rules: better means specialize, but map all type arguments downwards // These are enabled for 3.0-3.5, and for all comparisons between old-style implicits, // and in 3.5 and 3.6-migration when we compare with previous rules. @@ -1909,8 +1907,8 @@ trait Applications extends Compatibility { case _ => mapOver(t) (flip(tp1p) relaxed_<:< flip(tp2p)) || viewExists(tp1, tp2) else - // New rules: better means generalize, givens (and extensions) always beat implicits - if alt1IsImplicit != alt2IsImplicit then alt2IsImplicit + // New rules: better means generalize, except `NotGivenClass` which is given lower priority + if alt1IsNotGivenClass != alt2IsNotGivenClass then alt2IsNotGivenClass else (tp2p relaxed_<:< tp1p) || viewExists(tp2, tp1) end isAsGoodValueType From dd57f6defa4215f19488c6240da2e70b29e59ee5 Mon Sep 17 00:00:00 2001 From: odersky Date: Fri, 26 Jul 2024 13:32:42 +0200 Subject: [PATCH 03/13] Weigh ownerScore more than type score for implicit search --- .../dotty/tools/dotc/typer/Applications.scala | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 2942fac68805..cb379a6a3b62 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1970,23 +1970,29 @@ trait Applications extends Compatibility { def compareWithTypes(tp1: Type, tp2: Type) = val ownerScore = compareOwner(alt1.symbol.maybeOwner, alt2.symbol.maybeOwner) - val winsType1 = isAsGood(alt1, tp1, alt2, tp2) - val winsType2 = isAsGood(alt2, tp2, alt1, tp1) - - overload.println(i"compare($alt1, $alt2)? $tp1 $tp2 $ownerScore $winsType1 $winsType2") - if winsType1 && winsType2 - && alt1.widenExpr.isStable && (alt1.widenExpr frozen_=:= alt2.widenExpr) - then - // alternatives are the same after following ExprTypes, pick one of them - // (prefer the one that is not a method, but that's arbitrary). - if alt1.widenExpr =:= alt2 then -1 else 1 - else ownerScore match - case 1 => if winsType1 || !winsType2 then 1 else 0 - case -1 => if winsType2 || !winsType1 then -1 else 0 - case 0 => - if winsType1 != winsType2 then if winsType1 then 1 else -1 - else if alt1.symbol == alt2.symbol then comparePrefixes - else 0 + if preferGeneral && ownerScore != 0 then + // For implicit resolution, take ownerscre as more significat than type resoltion + // Reason: People use owner hierarchies to explicitly prioritize, we should not + // break that by changing implicit priority of types. + ownerScore + else + val winsType1 = isAsGood(alt1, tp1, alt2, tp2) + val winsType2 = isAsGood(alt2, tp2, alt1, tp1) + + overload.println(i"compare($alt1, $alt2)? $tp1 $tp2 $ownerScore $winsType1 $winsType2") + if winsType1 && winsType2 + && alt1.widenExpr.isStable && (alt1.widenExpr frozen_=:= alt2.widenExpr) + then + // alternatives are the same after following ExprTypes, pick one of them + // (prefer the one that is not a method, but that's arbitrary). + if alt1.widenExpr =:= alt2 then -1 else 1 + else ownerScore match + case 1 => if winsType1 || !winsType2 then 1 else 0 + case -1 => if winsType2 || !winsType1 then -1 else 0 + case 0 => + if winsType1 != winsType2 then if winsType1 then 1 else -1 + else if alt1.symbol == alt2.symbol then comparePrefixes + else 0 end compareWithTypes if alt1.symbol.is(ConstructorProxy) && !alt2.symbol.is(ConstructorProxy) then -1 From cd686a0ba7bc9d0f694c9a619677d09187473603 Mon Sep 17 00:00:00 2001 From: odersky Date: Fri, 26 Jul 2024 13:58:45 +0200 Subject: [PATCH 04/13] Use old prioritization for old-style implicits if new is ambiguous --- compiler/src/dotty/tools/dotc/typer/Implicits.scala | 3 +++ tests/neg/i2974.scala | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index dac0c0e78448..37ebeec08b73 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1331,6 +1331,9 @@ trait Implicits: else if alt1.level != alt2.level then alt1.level - alt2.level else var cmp = comp(using searchContext()) + if cmp == 0 && alt1.ref.symbol.is(Implicit) && alt2.ref.symbol.is(Implicit) then + // if we get an ambiguity with new rules for a pair of old-style implicits, fall back to old rules + cmp = comp(using searchContext().addMode(Mode.OldImplicitResolution)) val sv = Feature.sourceVersion if isWarnPriorityChangeVersion(sv) then val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution)) diff --git a/tests/neg/i2974.scala b/tests/neg/i2974.scala index df5dc5c461fa..a77b4a61da45 100644 --- a/tests/neg/i2974.scala +++ b/tests/neg/i2974.scala @@ -15,10 +15,14 @@ object Test { summon[Foo[Int]] // ok locally: - implicit val fa: Foo[Any] = ??? - implicit val ba: Bar[Int] = ??? + given fa: Foo[Any] = ??? + given ba: Bar[Int] = ??? summon[Foo[Int]] // error: now ambiguous, // was resolving to `ba` when using intermediate rules: // better means specialize, but map all type arguments downwards + locally: + implicit val fa: Foo[Any] = ??? + implicit val ba: Bar[Int] = ??? + summon[Foo[Int]] // is OK since we fall back to old rules for old-style implicits as a tie breaker } From b7bff9a2b04c10b4bf2b00de1dad82f31145c57e Mon Sep 17 00:00:00 2001 From: odersky Date: Fri, 26 Jul 2024 16:54:18 +0200 Subject: [PATCH 05/13] Adapt scala-collection-compat to priority changes --- community-build/community-projects/scala-collection-compat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/community-build/community-projects/scala-collection-compat b/community-build/community-projects/scala-collection-compat index 2bf3fea914b2..a81b5bc7661a 160000 --- a/community-build/community-projects/scala-collection-compat +++ b/community-build/community-projects/scala-collection-compat @@ -1 +1 @@ -Subproject commit 2bf3fea914b2f13e4805b3e7b519bdf0e595e4c9 +Subproject commit a81b5bc7661a22d2a91328e1a1ff1a006c1e5336 From 43bf127b361205adc4f428bc6c57524afaec0239 Mon Sep 17 00:00:00 2001 From: odersky Date: Fri, 26 Jul 2024 16:56:13 +0200 Subject: [PATCH 06/13] Revert PPRint changes Hopefully we don't need them anymore --- community-build/community-projects/PPrint | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/community-build/community-projects/PPrint b/community-build/community-projects/PPrint index 34a777f687bc..2203dc6081f5 160000 --- a/community-build/community-projects/PPrint +++ b/community-build/community-projects/PPrint @@ -1 +1 @@ -Subproject commit 34a777f687bc851953e682f99edcae9d2875babc +Subproject commit 2203dc6081f5e8fa89f552b155724b0a8fdcec03 From 7895b54d0e4470ddf1e12489d008b7332c998be9 Mon Sep 17 00:00:00 2001 From: odersky Date: Fri, 26 Jul 2024 17:06:13 +0200 Subject: [PATCH 07/13] Cleanup: Drop isNotGivenClass check This check was always false since a TermRef cannot have the NotGiven class as a symbol. --- .../src/dotty/tools/dotc/typer/Applications.scala | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index cb379a6a3b62..7e323e246a7d 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1830,16 +1830,13 @@ trait Applications extends Compatibility { isAsGood(alt1, tp1.instantiate(tparams.map(_.typeRef)), alt2, tp2) } case _ => // (3) - def compareValues(tp1: Type, tp2: Type)(using Context) = - extension (alt: TermRef) def isNotGivenClass: Boolean = alt.symbol == defn.NotGivenClass - isAsGoodValueType(tp1, tp2, alt1.isNotGivenClass, alt2.isNotGivenClass) tp2 match case tp2: MethodType => true // (3a) case tp2: PolyType if tp2.resultType.isInstanceOf[MethodType] => true // (3a) case tp2: PolyType => // (3b) - explore(compareValues(tp1, instantiateWithTypeVars(tp2))) + explore(isAsGoodValueType(tp1, instantiateWithTypeVars(tp2))) case _ => // 3b) - compareValues(tp1, tp2) + isAsGoodValueType(tp1, tp2) } /** Test whether value type `tp1` is as good as value type `tp2`. @@ -1869,7 +1866,6 @@ trait Applications extends Compatibility { * for overloading resolution (when `preferGeneral is false), and the opposite relation * `U <: T` or `U convertible to `T` for implicit disambiguation between givens * (when `preferGeneral` is true). For old-style implicit values, the 3.4 behavior is kept. - * If one of the alternatives is a NotGivenClass, and the other is not, then the NotGivenClass loses. * * - In Scala 3.5 and Scala 3.6-migration, we issue a warning if the result under * Scala 3.6 differ wrt to the old behavior up to 3.5. @@ -1877,7 +1873,7 @@ trait Applications extends Compatibility { * Also and only for given resolution: If a compared type refers to a given or its module class, use * the intersection of its parent classes instead. */ - def isAsGoodValueType(tp1: Type, tp2: Type, alt1IsNotGivenClass: Boolean, alt2IsNotGivenClass: Boolean)(using Context): Boolean = + def isAsGoodValueType(tp1: Type, tp2: Type)(using Context): Boolean = val oldResolution = ctx.mode.is(Mode.OldImplicitResolution) if !preferGeneral || Feature.migrateTo3 && oldResolution then // Normal specificity test for overloading resolution (where `preferGeneral` is false) @@ -1907,9 +1903,8 @@ trait Applications extends Compatibility { case _ => mapOver(t) (flip(tp1p) relaxed_<:< flip(tp2p)) || viewExists(tp1, tp2) else - // New rules: better means generalize, except `NotGivenClass` which is given lower priority - if alt1IsNotGivenClass != alt2IsNotGivenClass then alt2IsNotGivenClass - else (tp2p relaxed_<:< tp1p) || viewExists(tp2, tp1) + // New rules: better means generalize + (tp2p relaxed_<:< tp1p) || viewExists(tp2, tp1) end isAsGoodValueType /** Widen the result type of synthetic given methods from the implementation class to the From 9348538e9c5b50c0400f33456c1b511ecf051ca0 Mon Sep 17 00:00:00 2001 From: odersky Date: Sat, 27 Jul 2024 11:06:18 +0200 Subject: [PATCH 08/13] Use owner score over type score only ss last fallback --- .../dotty/tools/dotc/typer/Applications.scala | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 7e323e246a7d..09e7fd5ce668 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1965,29 +1965,29 @@ trait Applications extends Compatibility { def compareWithTypes(tp1: Type, tp2: Type) = val ownerScore = compareOwner(alt1.symbol.maybeOwner, alt2.symbol.maybeOwner) - if preferGeneral && ownerScore != 0 then - // For implicit resolution, take ownerscre as more significat than type resoltion - // Reason: People use owner hierarchies to explicitly prioritize, we should not - // break that by changing implicit priority of types. - ownerScore - else - val winsType1 = isAsGood(alt1, tp1, alt2, tp2) - val winsType2 = isAsGood(alt2, tp2, alt1, tp1) - - overload.println(i"compare($alt1, $alt2)? $tp1 $tp2 $ownerScore $winsType1 $winsType2") - if winsType1 && winsType2 - && alt1.widenExpr.isStable && (alt1.widenExpr frozen_=:= alt2.widenExpr) - then - // alternatives are the same after following ExprTypes, pick one of them - // (prefer the one that is not a method, but that's arbitrary). - if alt1.widenExpr =:= alt2 then -1 else 1 - else ownerScore match - case 1 => if winsType1 || !winsType2 then 1 else 0 - case -1 => if winsType2 || !winsType1 then -1 else 0 - case 0 => - if winsType1 != winsType2 then if winsType1 then 1 else -1 - else if alt1.symbol == alt2.symbol then comparePrefixes - else 0 + val winsType1 = isAsGood(alt1, tp1, alt2, tp2) + val winsType2 = isAsGood(alt2, tp2, alt1, tp1) + + overload.println(i"compare($alt1, $alt2)? $tp1 $tp2 $ownerScore $winsType1 $winsType2") + if winsType1 && winsType2 + && alt1.widenExpr.isStable && (alt1.widenExpr frozen_=:= alt2.widenExpr) + then + // alternatives are the same after following ExprTypes, pick one of them + // (prefer the one that is not a method, but that's arbitrary). + if alt1.widenExpr =:= alt2 then -1 else 1 + else ownerScore match + case 1 => if winsType1 || !winsType2 then 1 else 0 + case -1 => if winsType2 || !winsType1 then -1 else 0 + case 0 => + if winsType1 != winsType2 then if winsType1 then 1 else -1 + else if alt1.symbol == alt2.symbol then comparePrefixes + else if preferGeneral then + // For implicit resolution, take ownerscore as more significat than type resoltion + // Reason: People use owner hierarchies to explicitly prioritize, we should not + // break that by changing implicit priority of types. On the other hand we do want + // to comparePrefixes if there is a draw; StringFormaterTest breaks if we don't do that. + ownerScore + else 0 end compareWithTypes if alt1.symbol.is(ConstructorProxy) && !alt2.symbol.is(ConstructorProxy) then -1 From 36a74b06c2e1e4e7ca049a18297c4880693117c9 Mon Sep 17 00:00:00 2001 From: Eugene Flesselle Date: Sun, 28 Jul 2024 21:47:00 +0200 Subject: [PATCH 09/13] Reuse `compareAlternatives#prev` computation --- compiler/src/dotty/tools/dotc/typer/Implicits.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 37ebeec08b73..6e008957aafa 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1330,13 +1330,13 @@ trait Implicits: if alt1.ref eq alt2.ref then 0 else if alt1.level != alt2.level then alt1.level - alt2.level else - var cmp = comp(using searchContext()) - if cmp == 0 && alt1.ref.symbol.is(Implicit) && alt2.ref.symbol.is(Implicit) then + lazy val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution)) + val cmp = comp(using searchContext()) match // if we get an ambiguity with new rules for a pair of old-style implicits, fall back to old rules - cmp = comp(using searchContext().addMode(Mode.OldImplicitResolution)) + case 0 if alt1.ref.symbol.is(Implicit) && alt2.ref.symbol.is(Implicit) => prev + case cmp => cmp val sv = Feature.sourceVersion if isWarnPriorityChangeVersion(sv) then - val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution)) if disambiguate && cmp != prev then def warn(msg: Message) = val critical = alt1.ref :: alt2.ref :: Nil From e1cc065f2ee38a520e32406f97a2d11e53a2cccb Mon Sep 17 00:00:00 2001 From: Eugene Flesselle Date: Sun, 28 Jul 2024 21:51:33 +0200 Subject: [PATCH 10/13] Drop using owner score as fallback over type score since it was under the case matching ownerScore to 0 --- compiler/src/dotty/tools/dotc/typer/Applications.scala | 6 ------ 1 file changed, 6 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 09e7fd5ce668..0134e40c7457 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1981,12 +1981,6 @@ trait Applications extends Compatibility { case 0 => if winsType1 != winsType2 then if winsType1 then 1 else -1 else if alt1.symbol == alt2.symbol then comparePrefixes - else if preferGeneral then - // For implicit resolution, take ownerscore as more significat than type resoltion - // Reason: People use owner hierarchies to explicitly prioritize, we should not - // break that by changing implicit priority of types. On the other hand we do want - // to comparePrefixes if there is a draw; StringFormaterTest breaks if we don't do that. - ownerScore else 0 end compareWithTypes From 39c49ad8a2194b5d1c736b0ba8980f553848ba52 Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 29 Jul 2024 10:57:21 +0200 Subject: [PATCH 11/13] Fix ownerscore disambiguation --- .../dotty/tools/dotc/typer/Applications.scala | 20 ++++++++++------ tests/pos/given-priority.scala | 24 +++++++++++++++++++ 2 files changed, 37 insertions(+), 7 deletions(-) create mode 100644 tests/pos/given-priority.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 0134e40c7457..3fe25c402ec7 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1975,13 +1975,19 @@ trait Applications extends Compatibility { // alternatives are the same after following ExprTypes, pick one of them // (prefer the one that is not a method, but that's arbitrary). if alt1.widenExpr =:= alt2 then -1 else 1 - else ownerScore match - case 1 => if winsType1 || !winsType2 then 1 else 0 - case -1 => if winsType2 || !winsType1 then -1 else 0 - case 0 => - if winsType1 != winsType2 then if winsType1 then 1 else -1 - else if alt1.symbol == alt2.symbol then comparePrefixes - else 0 + else + // For implicit resolution, take ownerscore as more significant than type resolution + // Reason: People use owner hierarchies to explicitly prioritize, we should not + // break that by changing implicit priority of types. On the other hand we do want + // to comparePrefixes if there is a draw; StringFormaterTest breaks if we don't do that. + def drawOrOwner = if preferGeneral then ownerScore else 0 + ownerScore match + case 1 => if winsType1 || !winsType2 then 1 else drawOrOwner + case -1 => if winsType2 || !winsType1 then -1 else drawOrOwner + case 0 => + if winsType1 != winsType2 then if winsType1 then 1 else -1 + else if alt1.symbol == alt2.symbol then comparePrefixes + else 0 end compareWithTypes if alt1.symbol.is(ConstructorProxy) && !alt2.symbol.is(ConstructorProxy) then -1 diff --git a/tests/pos/given-priority.scala b/tests/pos/given-priority.scala new file mode 100644 index 000000000000..048e063eff35 --- /dev/null +++ b/tests/pos/given-priority.scala @@ -0,0 +1,24 @@ +/* These tests show various mechanisms available for implicit prioritization. + */ +import language.`3.6` + +class A // The type for which we infer terms below +class B extends A + +/* First, two schemes that require a pre-planned architecture for how and + * where given instances are defined. + * + * Traditional scheme: prioritize with location in class hierarchy + */ +class LowPriorityImplicits: + given g1: A() + +object NormalImplicits extends LowPriorityImplicits: + given g2: B() + +def test1 = + import NormalImplicits.given + val x = summon[A] + val _: B = x + val y = summon[B] + val _: B = y From 1ea5485b984c8355b6645f2c45c58c7a6a788277 Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 29 Jul 2024 14:10:25 +0200 Subject: [PATCH 12/13] Use "prefer owner score over type score" trick only under new resolution rules That way, we see the difference if something changes. --- compiler/src/dotty/tools/dotc/typer/Applications.scala | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 3fe25c402ec7..d341581a656a 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1980,7 +1980,9 @@ trait Applications extends Compatibility { // Reason: People use owner hierarchies to explicitly prioritize, we should not // break that by changing implicit priority of types. On the other hand we do want // to comparePrefixes if there is a draw; StringFormaterTest breaks if we don't do that. - def drawOrOwner = if preferGeneral then ownerScore else 0 + def drawOrOwner = + if preferGeneral && !ctx.mode.is(Mode.OldImplicitResolution) then ownerScore + else 0 ownerScore match case 1 => if winsType1 || !winsType2 then 1 else drawOrOwner case -1 => if winsType2 || !winsType1 then -1 else drawOrOwner From 8f85da55c138f81a8303f6b7fc091dc48a18d304 Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 29 Jul 2024 20:04:55 +0200 Subject: [PATCH 13/13] Fix Formatting.Show for types X | Null. Once we saw what got changed we got priority errors like this: ```scala [warn] -- Warning: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/reporting/messages.scala:2145:15 [warn] 2145 | | ${Magenta("case a @ C()")} => 2 [warn] | ^ [warn] |Change in given search preference for dotty.tools.dotc.printing.Formatting.ShownDef.Show[Product | Null] between alternatives (dotty.tools.dotc.printing.Formatting.ShownDef.Show.given_Show_Product : [warn] | dotty.tools.dotc.printing.Formatting.ShownDef.Show[Product]) and (dotty.tools.dotc.printing.Formatting.ShownDef.Show.oldShowNull : [warn] | [X] [warn] | (implicit evidence$1: dotty.tools.dotc.printing.Formatting.ShownDef.Show[X]) [warn] | : dotty.tools.dotc.printing.Formatting.ShownDef.Show.oldShowNull[X] [warn] |) [warn] |Previous choice : the first alternative [warn] |New choice from Scala 3.6: the second alternative ``` I believe there is some funky thing going on with nulls. Probably unsafe nulls somewhere. Anyway what seems to happen is that before Product's given was selected for an argument of `Product | Null`, which seems to indicate that the two types were seen as equivalent. Anyway, if we compare `Show[Product]` with `Show[X | Null]` it seems that Show[Product] won the type score and lost the owner score, so it was a draw. But then the second criterion kicked in which rules that alternatives without any implicit arguments take priority over alternatives with explicit arguments. The given for Product was unconditional but the given for X | Null has a context bound. So Show[Product] won. But once we switch to more general, we have that Show[Product] loses both type and owner score, so Show[X | Null] won. And that seems to have caused the classcast exceptions. I am not sure why, the logic for Shown was too hard for me to follow. The fix is to move Show[X | Null] to have lowest priority. --- .../tools/dotc/printing/Formatting.scala | 20 +++++++++---------- .../tools/dotc/StringFormatterTest.scala | 1 + 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/printing/Formatting.scala b/compiler/src/dotty/tools/dotc/printing/Formatting.scala index 6f1c32beb822..34db29575ca3 100644 --- a/compiler/src/dotty/tools/dotc/printing/Formatting.scala +++ b/compiler/src/dotty/tools/dotc/printing/Formatting.scala @@ -2,8 +2,6 @@ package dotty.tools package dotc package printing -import scala.language.unsafeNulls - import scala.collection.mutable import core.* @@ -50,9 +48,14 @@ object Formatting { /** The base implementation, passing the argument to StringFormatter which will try to `.show` it. */ object ShowAny extends Show[Any]: - def show(x: Any): Shown = x + def show(x: Any): Shown = + if x == null then "null" else x // defensive action in case `showNull` does not trigger because of unsafeNulls + + class ShowImplicit4: + given showNull[X: Show]: Show[X | Null] with + def show(x: X | Null) = if x == null then "null" else CtxShow(toStr(x.nn)) - class ShowImplicits3: + class ShowImplicits3 extends ShowImplicit4: given Show[Product] = ShowAny class ShowImplicits2 extends ShowImplicits3: @@ -77,15 +80,10 @@ object Formatting { given [K: Show, V: Show]: Show[Map[K, V]] with def show(x: Map[K, V]) = CtxShow(x.map((k, v) => s"${toStr(k)} => ${toStr(v)}")) - end given given [H: Show, T <: Tuple: Show]: Show[H *: T] with def show(x: H *: T) = CtxShow(toStr(x.head) *: toShown(x.tail).asInstanceOf[Tuple]) - end given - - given [X: Show]: Show[X | Null] with - def show(x: X | Null) = if x == null then "null" else CtxShow(toStr(x.nn)) given Show[FlagSet] with def show(x: FlagSet) = x.flagsString @@ -148,8 +146,8 @@ object Formatting { private def treatArg(arg: Shown, suffix: String)(using Context): (String, String) = arg.runCtxShow match { case arg: Seq[?] if suffix.indexOf('%') == 0 && suffix.indexOf('%', 1) != -1 => val end = suffix.indexOf('%', 1) - val sep = StringContext.processEscapes(suffix.substring(1, end)) - (arg.mkString(sep), suffix.substring(end + 1)) + val sep = StringContext.processEscapes(suffix.substring(1, end).nn) + (arg.mkString(sep), suffix.substring(end + 1).nn) case arg: Seq[?] => (arg.map(showArg).mkString("[", ", ", "]"), suffix) case arg => diff --git a/compiler/test/dotty/tools/dotc/StringFormatterTest.scala b/compiler/test/dotty/tools/dotc/StringFormatterTest.scala index 4dfc08cc7e9b..b0ff8b8fc03e 100644 --- a/compiler/test/dotty/tools/dotc/StringFormatterTest.scala +++ b/compiler/test/dotty/tools/dotc/StringFormatterTest.scala @@ -23,6 +23,7 @@ class StringFormatterTest extends AbstractStringFormatterTest: @Test def flagsTup = check("(,final)", i"${(JavaStatic, Final)}") @Test def seqOfTup2 = check("(final,given), (private,lazy)", i"${Seq((Final, Given), (Private, Lazy))}%, %") @Test def seqOfTup3 = check("(Foo,given, (right is approximated))", i"${Seq((Foo, Given, TypeComparer.ApproxState.None.addHigh))}%, %") + @Test def tupleNull = check("(1,null)", i"${(1, null: String | Null)}") class StorePrinter extends Printer: var string: String = ""