Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes and tweaks to implicit priority change scheme #21328

Closed
wants to merge 5 commits into from
Closed

Fixes and tweaks to implicit priority change scheme #21328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35b5f7c
A left-biased variant for implicit/given pairs
odersky Aug 5, 2024
7c4bd67
Compensate loss of transitivity
odersky Aug 5, 2024
f62e141
Delay priority change until 3.7
odersky Aug 5, 2024
87f5ca0
Fix ranking logic
odersky Aug 6, 2024
f5f390e
Make priority change warning messages stable
odersky Aug 6, 2024
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
Open in desktop
Submodule PPrint updated 1 files
+1 −1 pprint/src-3/TPrintImpl.scala
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/ast/Desugar.scala
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if scheme == CompareScheme.Intermediate || alt1IsImplicit then
if scheme == CompareScheme.Intermediate then

Is it intentional that we still do the CompareScheme.Intermediate when specifically alt1 is an implicit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! The idea is that for implicit/givens pairs we do the rules according to the left operand. Say an implicit I is more specialized than a given G. Then I wins by type against G but G also wins by type against I. So it's a draw in the end.
But in more complex situations it could be (for instance) that G doe not win against I because there are some variable's to instantiate on I but not on G. So I wins. (or G, vice versa). The PR comment mentions this.

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
Loading