Skip to content

Commit

Permalink
Fixes and tweaks to implicit priority change scheme (#21339)
Browse files Browse the repository at this point in the history
Based on #21328: We now use a left-biased scheme, as follows.

From 3.7 on:

A given x: X is better than a given or implicit y: Y if y can be instantiated/widened to X.
An implicit x: X is better than a given or implicit y: Y if y can be instantiated to a supertype of X.
Use owner score for givens as a tie breaker if after all other tests we still have an ambiguity.
This is not transitive, but the PR implements a scheme to work around that.

Other changes:
- Drop special handling of NotGiven in prioritization. The previous logic pretended to do so, but was ineffective.
- Fix healAmbiguous to compareAlternatives with disambiguate = true, to account for changes made in #21045
  • Loading branch information
EugeneFlesselle authored Aug 7, 2024
2 parents 40b8c7a + 0361991 commit 590b496
Show file tree
Hide file tree
Showing 39 changed files with 581 additions and 115 deletions.
2 changes: 1 addition & 1 deletion community-build/community-projects/PPrint
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/ast/Desugar.scala
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ object desugar {
params.map: param =>
val normFlags = param.mods.flags &~ GivenOrImplicit | (mparam.mods.flags & (GivenOrImplicit))
param.withMods(param.mods.withFlags(normFlags))
.showing(i"ADAPTED PARAM $result ${result.mods.flags} for ${meth.name}")
.showing(i"adapted param $result ${result.mods.flags} for ${meth.name}", Printers.desugar)
else params
(normParams ++ mparams) :: Nil
case mparams :: mparamss1 =>
Expand Down
17 changes: 7 additions & 10 deletions compiler/src/dotty/tools/dotc/printing/Formatting.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ package dotty.tools
package dotc
package printing

import scala.language.unsafeNulls

import scala.collection.mutable

import core.*
Expand Down Expand Up @@ -52,7 +50,11 @@ object Formatting {
object ShowAny extends Show[Any]:
def show(x: Any): Shown = x

class ShowImplicits3:
class ShowImplicits4:
given [X: Show]: Show[X | Null] with
def show(x: X | Null) = if x == null then "null" else CtxShow(toStr(x.nn))

class ShowImplicits3 extends ShowImplicits4:
given Show[Product] = ShowAny

class ShowImplicits2 extends ShowImplicits3:
Expand All @@ -77,15 +79,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
Expand Down Expand Up @@ -148,8 +145,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 =>
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2988,7 +2988,7 @@ class MissingImplicitArgument(

/** Default error message for non-nested ambiguous implicits. */
def defaultAmbiguousImplicitMsg(ambi: AmbiguousImplicits) =
s"Ambiguous given instances: ${ambi.explanation}${location("of")}"
s"Ambiguous given instances: ${ambi.explanation}${location("of")}${ambi.priorityChangeWarningNote}"

/** Default error messages for non-ambiguous implicits, or nested ambiguous
* implicits.
Expand Down
75 changes: 46 additions & 29 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1762,6 +1762,17 @@ trait Applications extends Compatibility {
else if sym2.is(Module) then compareOwner(sym1, cls2)
else 0

enum CompareScheme:
case Old // Normal specificity test for overloading resolution (where `preferGeneral` is false)
// and in mode Scala3-migration when we compare with the old Scala 2 rules.

case Intermediate // Intermediate rules: better means specialize, but map all type arguments downwards
// These are enabled for 3.0-3.5, or if OldImplicitResolution
// is specified, and also for all comparisons between old-style implicits,

case New // New rules: better means generalize, givens (and extensions) always beat implicits
end CompareScheme

/** Compare two alternatives of an overloaded call or an implicit search.
*
* @param alt1, alt2 Non-overloaded references indicating the two choices
Expand All @@ -1788,6 +1799,15 @@ trait Applications extends Compatibility {
*/
def compare(alt1: TermRef, alt2: TermRef, preferGeneral: Boolean = false)(using Context): Int = trace(i"compare($alt1, $alt2)", overload) {
record("resolveOverloaded.compare")
val scheme =
val oldResolution = ctx.mode.is(Mode.OldImplicitResolution)
if !preferGeneral || Feature.migrateTo3 && oldResolution then
CompareScheme.Old
else if Feature.sourceVersion.isAtMost(SourceVersion.`3.5`)
|| oldResolution
|| alt1.symbol.is(Implicit) && alt2.symbol.is(Implicit)
then CompareScheme.Intermediate
else CompareScheme.New

/** Is alternative `alt1` with type `tp1` as good as alternative
* `alt2` with type `tp2` ?
Expand Down Expand Up @@ -1830,15 +1850,15 @@ 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))
def compareValues(tp2: Type)(using Context) =
isAsGoodValueType(tp1, tp2, alt1.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(compareValues(instantiateWithTypeVars(tp2)))
case _ => // 3b)
compareValues(tp1, tp2)
compareValues(tp2)
}

/** Test whether value type `tp1` is as good as value type `tp2`.
Expand All @@ -1849,7 +1869,7 @@ trait Applications extends Compatibility {
* available in 3.0-migration if mode `Mode.OldImplicitResolution` is turned on as well.
* It is used to highlight differences between Scala 2 and 3 behavior.
*
* - In Scala 3.0-3.5, the behavior is as follows: `T <:p U` iff there is an implicit conversion
* - In Scala 3.0-3.6, the behavior is as follows: `T <:p U` iff there is an implicit conversion
* from `T` to `U`, or
*
* flip(T) <: flip(U)
Expand All @@ -1864,21 +1884,20 @@ trait Applications extends Compatibility {
* of parameters are not affected. So `T <: U` would imply `Set[Cmp[U]] <:p Set[Cmp[T]]`,
* as usual, because `Set` is non-variant.
*
* - From Scala 3.6, `T <:p U` means `T <: U` or `T` convertible to `U`
* - From Scala 3.7, `T <:p U` means `T <: U` or `T` convertible to `U`
* 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.
* (when `preferGeneral` is true). For old-style implicit values, the 3.5 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.
* - In Scala 3.6 and Scala 3.7-migration, we issue a warning if the result under
* Scala 3.7 differs wrt to the old behavior up to 3.6.
*
* 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 =
val oldResolution = ctx.mode.is(Mode.OldImplicitResolution)
if !preferGeneral || Feature.migrateTo3 && oldResolution then
def isAsGoodValueType(tp1: Type, tp2: Type, alt1IsImplicit: Boolean)(using Context): Boolean =
if scheme == CompareScheme.Old then
// Normal specificity test for overloading resolution (where `preferGeneral` is false)
// and in mode Scala3-migration when we compare with the old Scala 2 rules.
isCompatible(tp1, tp2)
Expand All @@ -1892,13 +1911,7 @@ trait Applications extends Compatibility {
val tp1p = prepare(tp1)
val tp2p = prepare(tp2)

if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`)
|| oldResolution
|| alt1IsImplicit && alt2IsImplicit
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.
if scheme == CompareScheme.Intermediate || alt1IsImplicit then
val flip = new TypeMap:
def apply(t: Type) = t match
case t @ AppliedType(tycon, args) =>
Expand All @@ -1909,9 +1922,7 @@ 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)
(tp2p relaxed_<:< tp1p) || viewExists(tp2, tp1)
end isAsGoodValueType

/** Widen the result type of synthetic given methods from the implementation class to the
Expand Down Expand Up @@ -1982,13 +1993,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 new 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.
def drawOrOwner =
if scheme == CompareScheme.New 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
Expand Down
Loading

0 comments on commit 590b496

Please sign in to comment.