Skip to content

Commit

Permalink
Change rules for given prioritization
Browse files Browse the repository at this point in the history 
Consider the following program:

```scala
class A
class B extends A
class C extends A

given A = A()
given B = B()
given C = C()

def f(using a: A, b: B, c: C) =
  println(a.getClass)
  println(b.getClass)
  println(c.getClass)

@main def Test = f
```
With the current rules, this would fail with an ambiguity error between B and C when
trying to synthesize the A parameter. This is a problem without an easy remedy.

We can fix this problem by flipping the priority for implicit arguments. Instead of
requiring an argument to be most _specific_, we now require it to be most _general_
while still conforming to the formal parameter.

There are three justifications for this change, which at first glance seems quite drastic:

 - It gives us a natural way to deal with inheritance triangles like the one in the code above.
   Such triangles are quite common.
 - Intuitively, we want to get the closest possible match between required formal parameter type and
   synthetisized argument. The "most general" rule provides that.
 - We already do a crucial part of this. Namely, with current rules we interpolate all
   type variables in an implicit argument downwards, no matter what their variance is.
   This makes no sense in theory, but solves hairy problems with contravariant typeclasses
   like `Comparable`. Instead of this hack, we now do something more principled, by
   flipping the direction everywhere, preferring general over specific, instead of just
   flipping contravariant type parameters.

The behavior is dependent on the Scala version

 - Old behavior: up to 3.4
 - New behavior: from 3.5, 3.5-migration warns on behavior change

The CB builds under the new rules. One fix was needed for a shapeless 3 deriving test.
There was a typo: mkInstances instead of mkProductInstances, which previously got healed
by accident because of the most specific rule.

Also: Don't flip contravariant type arguments for overloading resolution

Flipping contravariant type arguments was needed for implicit search
where it will be replaced by a more general scheme. But it makes no
sense for overloading resolution. For overloading resolution, we want
to pick the most specific alternative, analogous to us picking the
most specific instantiation when we force a fully defined type.

Also: Disable implicit search everywhere for disambiaguation

Previously, one disambiguation step missed that, whereas implicits were
turned off everywhere else.
  • Loading branch information
@odersky
odersky committed Apr 16, 2024
1 parent 4c2305d commit 09f6913
Show file tree
Hide file tree
Showing 15 changed files with 254 additions and 78 deletions.
17 changes: 10 additions & 7 deletions compiler/src/dotty/tools/dotc/core/Mode.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ object Mode {
val Pattern: Mode = newMode(0, "Pattern")
val Type: Mode = newMode(1, "Type")

val PatternOrTypeBits: Mode = Pattern | Type

val ImplicitsEnabled: Mode = newMode(2, "ImplicitsEnabled")
val InferringReturnType: Mode = newMode(3, "InferringReturnType")

Expand Down Expand Up @@ -101,16 +103,19 @@ object Mode {
*/
val CheckBoundsOrSelfType: Mode = newMode(14, "CheckBoundsOrSelfType")

/** Use Scala2 scheme for overloading and implicit resolution */
val OldOverloadingResolution: Mode = newMode(15, "OldOverloadingResolution")
/** Use previous Scheme for implicit resolution. Currently significant
* in 3.0-migration where we use Scala-2's scheme instead and in 3.5-migration
* where we use the previous scheme up to 3.4 instead.
*/
val OldImplicitResolution: Mode = newMode(15, "OldImplicitResolution")

/** Treat CapturingTypes as plain AnnotatedTypes even in phase CheckCaptures.
* Reuses the value of OldOverloadingResolution to save Mode bits.
* This is OK since OldOverloadingResolution only affects implicit search, which
* Reuses the value of OldImplicitResolution to save Mode bits.
* This is OK since OldImplicitResolution only affects implicit search, which
* is done during phases Typer and Inlinig, and IgnoreCaptures only has an
* effect during phase CheckCaptures.
*/
val IgnoreCaptures = OldOverloadingResolution
val IgnoreCaptures = OldImplicitResolution

/** Allow hk applications of type lambdas to wildcard arguments;
* used for checking that such applications do not normally arise
Expand All @@ -120,8 +125,6 @@ object Mode {
/** Read original positions when unpickling from TASTY */
val ReadPositions: Mode = newMode(17, "ReadPositions")

val PatternOrTypeBits: Mode = Pattern | Type

/** We are elaborating the fully qualified name of a package clause.
* In this case, identifiers should never be imported.
*/
Expand Down
132 changes: 79 additions & 53 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import ProtoTypes.*
import Inferencing.*
import reporting.*
import Nullables.*, NullOpsDecorator.*
import config.Feature
import config.{Feature, SourceVersion}

import collection.mutable
import config.Printers.{overload, typr, unapp}
Expand Down Expand Up @@ -1665,6 +1665,12 @@ trait Applications extends Compatibility {
/** Compare two alternatives of an overloaded call or an implicit search.
*
* @param alt1, alt2 Non-overloaded references indicating the two choices
* @param preferGeneral When comparing two value types, prefer the more general one
* over the more specific one iff `preferGeneral` is true.
* `preferGeneral` is set to `true` when we compare two given values, since
* then we want the most general evidence that matches the target
* type. It is set to `false` for overloading resolution, when we want the
* most specific type instead.
* @return 1 if 1st alternative is preferred over 2nd
* -1 if 2nd alternative is preferred over 1st
* 0 if neither alternative is preferred over the other
Expand All @@ -1680,27 +1686,28 @@ trait Applications extends Compatibility {
* an alternative that takes more implicit parameters wins over one
* that takes fewer.
*/
def compare(alt1: TermRef, alt2: TermRef)(using Context): Int = trace(i"compare($alt1, $alt2)", overload) {
def compare(alt1: TermRef, alt2: TermRef, preferGeneral: Boolean = false)(using Context): Int = trace(i"compare($alt1, $alt2)", overload) {
record("resolveOverloaded.compare")

/** Is alternative `alt1` with type `tp1` as specific as alternative
val compareGivens = alt1.symbol.is(Given) || alt2.symbol.is(Given)

/** Is alternative `alt1` with type `tp1` as good as alternative
* `alt2` with type `tp2` ?
*
* 1. A method `alt1` of type `(p1: T1, ..., pn: Tn)U` is as specific as `alt2`
* 1. A method `alt1` of type `(p1: T1, ..., pn: Tn)U` is as good as `alt2`
* if `alt1` is nullary or `alt2` is applicable to arguments (p1, ..., pn) of
* types T1,...,Tn. If the last parameter `pn` has a vararg type T*, then
* `alt1` must be applicable to arbitrary numbers of `T` parameters (which
* implies that it must be a varargs method as well).
* 2. A polymorphic member of type [a1 >: L1 <: U1, ..., an >: Ln <: Un]T is as
* specific as `alt2` of type `tp2` if T is as specific as `tp2` under the
* good as `alt2` of type `tp2` if T is as good as `tp2` under the
* assumption that for i = 1,...,n each ai is an abstract type name bounded
* from below by Li and from above by Ui.
* 3. A member of any other type `tp1` is:
* a. always as specific as a method or a polymorphic method.
* b. as specific as a member of any other type `tp2` if `tp1` is compatible
* with `tp2`.
* a. always as good as a method or a polymorphic method.
* b. as good as a member of any other type `tp2` is `asGoodValueType(tp1, tp2) = true`
*/
def isAsSpecific(alt1: TermRef, tp1: Type, alt2: TermRef, tp2: Type): Boolean = trace(i"isAsSpecific $tp1 $tp2", overload) {
def isAsGood(alt1: TermRef, tp1: Type, alt2: TermRef, tp2: Type): Boolean = trace(i"isAsSpecific $tp1 $tp2", overload) {
tp1 match
case tp1: MethodType => // (1)
tp1.paramInfos.isEmpty && tp2.isInstanceOf[LambdaType]
Expand All @@ -1722,69 +1729,89 @@ trait Applications extends Compatibility {
fullyDefinedType(tp1Params, "type parameters of alternative", alt1.symbol.srcPos)

val tparams = newTypeParams(alt1.symbol, tp1.paramNames, EmptyFlags, tp1.instantiateParamInfos(_))
isAsSpecific(alt1, tp1.instantiate(tparams.map(_.typeRef)), alt2, tp2)
isAsGood(alt1, tp1.instantiate(tparams.map(_.typeRef)), alt2, tp2)
}
case _ => // (3)
tp2 match
case tp2: MethodType => true // (3a)
case tp2: PolyType if tp2.resultType.isInstanceOf[MethodType] => true // (3a)
case tp2: PolyType => // (3b)
explore(isAsSpecificValueType(tp1, instantiateWithTypeVars(tp2)))
explore(isAsGoodValueType(tp1, instantiateWithTypeVars(tp2)))
case _ => // 3b)
isAsSpecificValueType(tp1, tp2)
isAsGoodValueType(tp1, tp2)
}

/** Test whether value type `tp1` is as specific as value type `tp2`.
* Let's abbreviate this to `tp1 <:s tp2`.
* Previously, `<:s` was the same as `<:`. This behavior is still
* available under mode `Mode.OldOverloadingResolution`. The new behavior
* is different, however. Here, `T <:s U` iff
/** Test whether value type `tp1` is as good as value type `tp2`.
* Let's abbreviate this to `tp1 <:p tp2`. The behavior depends on the Scala version
* and mode.
*
* flip(T) <: flip(U)
* - In Scala 2, `<:p` was the same as `<:`. This behavior is still
* 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.
*
* where `flip` changes covariant occurrences of contravariant type parameters to
* covariant ones. Intuitively `<:s` means subtyping `<:`, except that all arguments
* to contravariant parameters are compared as if they were covariant. E.g. given class
* - In Scala 3.0-3.4, the behavior is as follows: `T <:p U` iff there is an impliit conversion
* from `T` to `U`, or
*
* class Cmp[-X]
* flip(T) <: flip(U)
*
* `Cmp[T] <:s Cmp[U]` if `T <: U`. On the other hand, non-variant occurrences
* of parameters are not affected. So `T <: U` would imply `Set[Cmp[U]] <:s Set[Cmp[T]]`,
* as usual, because `Set` is non-variant.
* where `flip` changes covariant occurrences of contravariant type parameters to
* covariant ones. Intuitively `<:p` means subtyping `<:`, except that all arguments
* to contravariant parameters are compared as if they were covariant. E.g. given class
*
* This relation might seem strange, but it models closely what happens for methods.
* Indeed, if we integrate the existing rules for methods into `<:s` we have now that
* class Cmp[-X]
*
* (T)R <:s (U)R
* `Cmp[T] <:p Cmp[U]` if `T <: U`. On the other hand, non-variant occurrences
* of parameters are not affected. So `T <: U` would imply `Set[Cmp[U]] <:p Set[Cmp[T]]`,
* as usual, because `Set` is non-variant.
*
* iff
* - From Scala 3.5, `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.
*
* T => R <:s U => R
* - In Scala 3.5-migration, use the 3.5 scheme normally, and the 3.4 scheme if
* `Mode.OldImplicitResolution` is on. This is used to highlight differences in the
* two resolution schemes.
*
* Also: If a compared type refers to a given or its module class, use
* 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 isAsSpecificValueType(tp1: Type, tp2: Type)(using Context) =
if (ctx.mode.is(Mode.OldOverloadingResolution))
def isAsGoodValueType(tp1: Type, tp2: Type)(using Context) =
val oldResolution = ctx.mode.is(Mode.OldImplicitResolution)
if !preferGeneral || Feature.migrateTo3 && oldResolution 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)
else {
val flip = new TypeMap {
def apply(t: Type) = t match {
case t @ AppliedType(tycon, args) =>
def mapArg(arg: Type, tparam: TypeParamInfo) =
if (variance > 0 && tparam.paramVarianceSign < 0) defn.FunctionNOf(arg :: Nil, defn.UnitType)
else arg
mapOver(t.derivedAppliedType(tycon, args.zipWithConserve(tycon.typeParams)(mapArg)))
case _ => mapOver(t)
}
}
def prepare(tp: Type) = tp.stripTypeVar match {
else
def prepare(tp: Type) = tp.stripTypeVar match
case tp: NamedType if tp.symbol.is(Module) && tp.symbol.sourceModule.is(Given) =>
flip(tp.widen.widenToParents)
case _ => flip(tp)
}
(prepare(tp1) relaxed_<:< prepare(tp2)) || viewExists(tp1, tp2)
}
tp.widen.widenToParents
case _ =>
tp

val tp1p = prepare(tp1)
val tp2p = prepare(tp2)

if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`)
|| oldResolution
|| !compareGivens
then
// Intermediate rules: better means specialize, but map all type arguments downwards
// These are enabled for 3.0-3.4, and for all comparisons between old-style implicits,
// and in 3.5-migration when we compare with previous rules.
val flip = new TypeMap:
def apply(t: Type) = t match
case t @ AppliedType(tycon, args) =>
def mapArg(arg: Type, tparam: TypeParamInfo) =
if (variance > 0 && tparam.paramVarianceSign < 0) defn.FunctionNOf(arg :: Nil, defn.UnitType)
else arg
mapOver(t.derivedAppliedType(tycon, args.zipWithConserve(tycon.typeParams)(mapArg)))
case _ => mapOver(t)
(flip(tp1p) relaxed_<:< flip(tp2p)) || viewExists(tp1, tp2)
else
// 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
* type that's implemented. Example
Expand Down Expand Up @@ -1844,9 +1871,8 @@ trait Applications extends Compatibility {

def compareWithTypes(tp1: Type, tp2: Type) =
val ownerScore = compareOwner(alt1.symbol.maybeOwner, alt2.symbol.maybeOwner)

val winsType1 = isAsSpecific(alt1, tp1, alt2, tp2)
val winsType2 = isAsSpecific(alt2, tp2, alt1, tp1)
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
Expand Down
29 changes: 22 additions & 7 deletions compiler/src/dotty/tools/dotc/typer/Implicits.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1105,8 +1105,8 @@ trait Implicits:
case result: SearchFailure if result.isAmbiguous =>
val deepPt = pt.deepenProto
if (deepPt ne pt) inferImplicit(deepPt, argument, span)
else if (migrateTo3 && !ctx.mode.is(Mode.OldOverloadingResolution))
withMode(Mode.OldOverloadingResolution)(inferImplicit(pt, argument, span)) match {
else if (migrateTo3 && !ctx.mode.is(Mode.OldImplicitResolution))
withMode(Mode.OldImplicitResolution)(inferImplicit(pt, argument, span)) match {
case altResult: SearchSuccess =>
report.migrationWarning(
result.reason.msg
Expand Down Expand Up @@ -1221,7 +1221,7 @@ trait Implicits:
assert(argument.isEmpty || argument.tpe.isValueType || argument.tpe.isInstanceOf[ExprType],
em"found: $argument: ${argument.tpe}, expected: $pt")

private def nestedContext() =
private def searchContext() =
ctx.fresh.setMode(ctx.mode &~ Mode.ImplicitsEnabled)

private def isCoherent = pt.isRef(defn.CanEqualClass)
Expand Down Expand Up @@ -1265,7 +1265,7 @@ trait Implicits:
else
val history = ctx.searchHistory.nest(cand, pt)
val typingCtx =
nestedContext().setNewTyperState().setFreshGADTBounds.setSearchHistory(history)
searchContext().setNewTyperState().setFreshGADTBounds.setSearchHistory(history)
val result = typedImplicit(cand, pt, argument, span)(using typingCtx)
result match
case res: SearchSuccess =>
Expand All @@ -1290,9 +1290,24 @@ trait Implicits:
* 0 if neither alternative is preferred over the other
*/
def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel): Int =
def comp(using Context) = explore(compare(alt1.ref, alt2.ref, preferGeneral = true))
if alt1.ref eq alt2.ref then 0
else if alt1.level != alt2.level then alt1.level - alt2.level
else explore(compare(alt1.ref, alt2.ref))(using nestedContext())
else
val cmp = comp(using searchContext())
if Feature.sourceVersion == SourceVersion.`3.5-migration` then
val prev = comp(using searchContext().addMode(Mode.OldImplicitResolution))
if cmp != prev then
def choice(c: Int) = c match
case -1 => "the second alternative"
case 1 => "the first alternative"
case _ => "none - it's ambiguous"
report.warning(
em"""Change in given search preference for $pt between alternatives ${alt1.ref} and ${alt2.ref}
|Previous choice: ${choice(prev)}
|New choice : ${choice(cmp)}""", srcPos)
cmp
end compareAlternatives

/** If `alt1` is also a search success, try to disambiguate as follows:
* - If alt2 is preferred over alt1, pick alt2, otherwise return an
Expand Down Expand Up @@ -1328,8 +1343,8 @@ trait Implicits:
else
ctx.typerState

diff = inContext(ctx.withTyperState(comparisonState)):
compare(ref1, ref2)
diff = inContext(searchContext().withTyperState(comparisonState)):
compare(ref1, ref2, preferGeneral = true)
else // alt1 is a conversion, prefer extension alt2 over it
diff = -1
if diff < 0 then alt2
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/ImportSuggestions.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ trait ImportSuggestions:
var i = 0
var diff = 0
while i < filled && diff == 0 do
diff = compare(ref, top(i))(using noImplicitsCtx)
diff = compare(ref, top(i), preferGeneral = true)(using noImplicitsCtx)
if diff > 0 then
rest += top(i)
top(i) = ref
Expand Down
17 changes: 16 additions & 1 deletion docs/_docs/reference/changed-features/implicit-resolution.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,22 @@ Condition (*) is new. It is necessary to ensure that the defined relation is tra

[//]: # todo: expand with precise rules

**9.** The following change is currently enabled in `-source future`:

**9.** Given disambiguation has changed. When comparing two givens that both match an expected type, we used to pick the most specific one, in alignment with
overloading resolution. From Scala 3.5 on, we pick the most general one instead. Compiling with Scala 3.5-migration will print a warning in all cases where the preference has changed. Example:
```scala
class A
class B extends A
class C extends A

given A = A()
given B = B()
given C = C()

summon[A] // was ambiguous, will now return `given_A`
```

**10.** The following change is currently enabled in `-source future`:

Implicit resolution now avoids generating recursive givens that can lead to an infinite loop at runtime. Here is an example:

Expand Down
Loading

0 comments on commit 09f6913

Please sign in to comment.