Skip to content

[Experiment] Drop prioritization of givens over implicits #21273

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

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 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
2 changes: 1 addition & 1 deletion community-build/community-projects/PPrint
19 changes: 6 additions & 13 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1830,15 +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) =
isAsGoodValueType(tp1, tp2, alt1.symbol.is(Implicit), alt2.symbol.is(Implicit))
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`.
Expand Down Expand Up @@ -1868,15 +1866,14 @@ 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.
*
* - 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.
*
* 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)(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)
Expand All @@ -1892,10 +1889,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.
Expand All @@ -1909,9 +1903,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
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
Expand Down
7 changes: 5 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/Implicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1330,10 +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())
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
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
Expand Down
28 changes: 28 additions & 0 deletions tests/neg/i2974.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@

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:
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
}
12 changes: 0 additions & 12 deletions tests/pos/i2974.scala

This file was deleted.

2 changes: 1 addition & 1 deletion tests/run/enrich-gentraversable.check
Original file line number Diff line number Diff line change
Expand Up @@ -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)
3 changes: 2 additions & 1 deletion tests/run/enrich-gentraversable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/run/implicits_poly.check
Original file line number Diff line number Diff line change
@@ -1 +1 @@
barFoo
fooFoo
8 changes: 4 additions & 4 deletions tests/warn/i15503a.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}

Expand All @@ -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 */
Expand Down
Loading