Skip to content

Commit d4de2cb

Browse files
committed
Fix shapeless 3 deriving test.
Typo: mkInstances instead of mkProductInstances, previously got healed by accident because if most specific rule. Change rules for given prioritization 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. 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. Disable implicit search everywhere for disambiaguation Previously, one disambiguation step missed that, whereas implicits were turned off everywhere else.
1 parent 084ab1a commit d4de2cb

13 files changed

+135
-27
lines changed

compiler/src/dotty/tools/dotc/core/Mode.scala

+4-2
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ object Mode {
4141
val Pattern: Mode = newMode(0, "Pattern")
4242
val Type: Mode = newMode(1, "Type")
4343

44+
val PatternOrTypeBits: Mode = Pattern | Type
45+
4446
val ImplicitsEnabled: Mode = newMode(2, "ImplicitsEnabled")
4547
val InferringReturnType: Mode = newMode(3, "InferringReturnType")
4648

@@ -120,8 +122,6 @@ object Mode {
120122
/** Read original positions when unpickling from TASTY */
121123
val ReadPositions: Mode = newMode(17, "ReadPositions")
122124

123-
val PatternOrTypeBits: Mode = Pattern | Type
124-
125125
/** We are elaborating the fully qualified name of a package clause.
126126
* In this case, identifiers should never be imported.
127127
*/
@@ -133,6 +133,8 @@ object Mode {
133133
/** We are typing the body of an inline method */
134134
val InlineableBody: Mode = newMode(21, "InlineableBody")
135135

136+
val NewGivenRules: Mode = newMode(22, "NewGivenRules")
137+
136138
/** We are synthesizing the receiver of an extension method */
137139
val SynthesizeExtMethodReceiver: Mode = newMode(23, "SynthesizeExtMethodReceiver")
138140

compiler/src/dotty/tools/dotc/typer/Applications.scala

+21-9
Original file line numberDiff line numberDiff line change
@@ -1724,9 +1724,12 @@ trait Applications extends Compatibility {
17241724
* an alternative that takes more implicit parameters wins over one
17251725
* that takes fewer.
17261726
*/
1727-
def compare(alt1: TermRef, alt2: TermRef)(using Context): Int = trace(i"compare($alt1, $alt2)", overload) {
1727+
def compare(alt1: TermRef, alt2: TermRef, preferGeneral: Boolean = false)(using Context): Int = trace(i"compare($alt1, $alt2)", overload) {
17281728
record("resolveOverloaded.compare")
17291729

1730+
val newGivenRules =
1731+
ctx.mode.is(Mode.NewGivenRules) && alt1.symbol.is(Given)
1732+
17301733
/** Is alternative `alt1` with type `tp1` as specific as alternative
17311734
* `alt2` with type `tp2` ?
17321735
*
@@ -1809,9 +1812,11 @@ trait Applications extends Compatibility {
18091812
* the intersection of its parent classes instead.
18101813
*/
18111814
def isAsSpecificValueType(tp1: Type, tp2: Type)(using Context) =
1812-
if (ctx.mode.is(Mode.OldOverloadingResolution))
1815+
if !preferGeneral || ctx.mode.is(Mode.OldOverloadingResolution) then
1816+
// Normal specificity test for overloading resultion (where `preferGeneral` is false)
1817+
// and in mode Scala3-migration when we compare with the old Scala 2 rules.
18131818
isCompatible(tp1, tp2)
1814-
else {
1819+
else
18151820
val flip = new TypeMap {
18161821
def apply(t: Type) = t match {
18171822
case t @ AppliedType(tycon, args) =>
@@ -1822,13 +1827,20 @@ trait Applications extends Compatibility {
18221827
case _ => mapOver(t)
18231828
}
18241829
}
1825-
def prepare(tp: Type) = tp.stripTypeVar match {
1830+
1831+
def prepare(tp: Type) = tp.stripTypeVar match
18261832
case tp: NamedType if tp.symbol.is(Module) && tp.symbol.sourceModule.is(Given) =>
1827-
flip(tp.widen.widenToParents)
1828-
case _ => flip(tp)
1829-
}
1830-
(prepare(tp1) relaxed_<:< prepare(tp2)) || viewExists(tp1, tp2)
1831-
}
1833+
tp.widen.widenToParents
1834+
case _ =>
1835+
tp
1836+
1837+
val tp1p = prepare(tp1)
1838+
val tp2p = prepare(tp2)
1839+
if newGivenRules then
1840+
(tp2p relaxed_<:< tp1p) || viewExists(tp2, tp1)
1841+
else
1842+
(flip(tp1p) relaxed_<:< flip(tp2p)) || viewExists(tp1, tp2)
1843+
end isAsSpecificValueType
18321844

18331845
/** Widen the result type of synthetic given methods from the implementation class to the
18341846
* type that's implemented. Example

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+10-5
Original file line numberDiff line numberDiff line change
@@ -1226,7 +1226,7 @@ trait Implicits:
12261226
assert(argument.isEmpty || argument.tpe.isValueType || argument.tpe.isInstanceOf[ExprType],
12271227
em"found: $argument: ${argument.tpe}, expected: $pt")
12281228

1229-
private def nestedContext() =
1229+
private def searchContext() =
12301230
ctx.fresh.setMode(ctx.mode &~ Mode.ImplicitsEnabled)
12311231

12321232
private def isCoherent = pt.isRef(defn.CanEqualClass)
@@ -1270,7 +1270,7 @@ trait Implicits:
12701270
else
12711271
val history = ctx.searchHistory.nest(cand, pt)
12721272
val typingCtx =
1273-
nestedContext().setNewTyperState().setFreshGADTBounds.setSearchHistory(history)
1273+
searchContext().setNewTyperState().setFreshGADTBounds.setSearchHistory(history)
12741274
val result = typedImplicit(cand, pt, argument, span)(using typingCtx)
12751275
result match
12761276
case res: SearchSuccess =>
@@ -1297,7 +1297,12 @@ trait Implicits:
12971297
def compareAlternatives(alt1: RefAndLevel, alt2: RefAndLevel): Int =
12981298
if alt1.ref eq alt2.ref then 0
12991299
else if alt1.level != alt2.level then alt1.level - alt2.level
1300-
else explore(compare(alt1.ref, alt2.ref))(using nestedContext())
1300+
else
1301+
val was = explore(compare(alt1.ref, alt2.ref, preferGeneral = true))(using searchContext())
1302+
val now = explore(compare(alt1.ref, alt2.ref, preferGeneral = true))(using searchContext().addMode(Mode.NewGivenRules))
1303+
if was != now then
1304+
println(i"change in preference for $pt between ${alt1.ref} and ${alt2.ref}, was: $was, now: $now at $srcPos")
1305+
now
13011306

13021307
/** If `alt1` is also a search success, try to disambiguate as follows:
13031308
* - If alt2 is preferred over alt1, pick alt2, otherwise return an
@@ -1333,8 +1338,8 @@ trait Implicits:
13331338
else
13341339
ctx.typerState
13351340

1336-
diff = inContext(ctx.withTyperState(comparisonState)):
1337-
compare(ref1, ref2)
1341+
diff = inContext(searchContext().withTyperState(comparisonState)):
1342+
compare(ref1, ref2, preferGeneral = true)
13381343
else // alt1 is a conversion, prefer extension alt2 over it
13391344
diff = -1
13401345
if diff < 0 then alt2

compiler/src/dotty/tools/dotc/typer/ImportSuggestions.scala

+1-1
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ trait ImportSuggestions:
296296
var i = 0
297297
var diff = 0
298298
while i < filled && diff == 0 do
299-
diff = compare(ref, top(i))(using noImplicitsCtx)
299+
diff = compare(ref, top(i), preferGeneral = true)(using noImplicitsCtx)
300300
if diff > 0 then
301301
rest += top(i)
302302
top(i) = ref

tests/neg/i15264.scala

+58
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
object priority:
2+
// lower number = higher priority
3+
class Prio0 extends Prio1
4+
object Prio0 { given Prio0() }
5+
6+
class Prio1 extends Prio2
7+
object Prio1 { given Prio1() }
8+
9+
class Prio2
10+
object Prio2 { given Prio2() }
11+
12+
object repro:
13+
// analogous to cats Eq, Hash, Order:
14+
class A[V]
15+
class B[V] extends A[V]
16+
class C[V] extends A[V]
17+
18+
class Q[V]
19+
20+
object context:
21+
// prios work here, which is cool
22+
given[V](using priority.Prio0): C[V] = new C[V]
23+
given[V](using priority.Prio1): B[V] = new B[V]
24+
given[V](using priority.Prio2): A[V] = new A[V]
25+
26+
object exports:
27+
// so will these exports
28+
export context.given
29+
30+
// if you import these don't import from 'context' above
31+
object qcontext:
32+
// base defs, like what you would get from cats
33+
given gb: B[Int] = new B[Int]
34+
given gc: C[Int] = new C[Int]
35+
36+
// these seem like they should work but don't
37+
given gcq[V](using p0: priority.Prio0)(using c: C[V]): C[Q[V]] = new C[Q[V]]
38+
given gbq[V](using p1: priority.Prio1)(using b: B[V]): B[Q[V]] = new B[Q[V]]
39+
given gaq[V](using p2: priority.Prio2)(using a: A[V]): A[Q[V]] = new A[Q[V]]
40+
41+
object test1:
42+
import repro.*
43+
import repro.exports.given
44+
45+
// these will work
46+
val a = summon[A[Int]]
47+
48+
object test2:
49+
import repro.*
50+
import repro.qcontext.given
51+
52+
// This one will fail as ambiguous - prios aren't having an effect.
53+
// Priorities indeed don't have an effect if the result is already decided
54+
// without using clauses, they onyl act as a tie breaker.
55+
// With the new resolution rules, it's ambiguous since we pick `gaq` for
56+
// summon, and that needs an A[Int], but there are only the two competing choices
57+
// qb and qc.
58+
val a = summon[A[Q[Int]]] // error: ambiguous between qb and qc for A[Int]

tests/pos/i15264.scala

+3-2
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ object repro:
3030
// if you import these don't import from 'context' above
3131
object qcontext:
3232
// base defs, like what you would get from cats
33+
given ga: A[Int] = new B[Int] // added so that we don't get an ambiguity in test2
3334
given gb: B[Int] = new B[Int]
3435
given gc: C[Int] = new C[Int]
3536

@@ -45,9 +46,9 @@ object test1:
4546
// these will work
4647
val a = summon[A[Int]]
4748

49+
4850
object test2:
4951
import repro.*
5052
import repro.qcontext.given
5153

52-
// this one will fail as ambiguous - prios aren't having an effect
53-
val a = summon[A[Q[Int]]]
54+
val a = summon[A[Q[Int]]]
+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
class A
2+
class B
3+
class C[-T]
4+
5+
def foo(using A): C[Any] = ???
6+
def foo(using B): C[Int] = ???
7+
8+
9+
@main def Test =
10+
given A = A()
11+
given B = B()
12+
val x = foo
13+
val _: C[Any] = x

tests/run/given-triangle.check

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
class A
2+
class B
3+
class C

tests/run/given-triangle.scala

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
class A
2+
class B extends A
3+
class C extends A
4+
5+
given A = A()
6+
given B = B()
7+
given C = C()
8+
9+
def f(using a: A, b: B, c: C) =
10+
println(a.getClass)
11+
println(b.getClass)
12+
println(c.getClass)
13+
14+
@main def Test = f

tests/run/implicit-specifity.scala

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,5 +38,5 @@ object Test extends App {
3838
assert(Show[Int] == 0)
3939
assert(Show[String] == 1)
4040
assert(Show[Generic] == 1) // showGen loses against fallback due to longer argument list
41-
assert(Show[Generic2] == 2) // ... but the opaque type intersection trick works.
41+
assert(Show[Generic2] == 1) // ... and the opaque type intersection trick no longer works with new resolution rules.
4242
}

tests/run/implied-for.scala

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ object Test extends App {
2020
val x2: T = t
2121
val x3: D[Int] = d
2222

23-
assert(summon[T].isInstanceOf[B])
23+
assert(summon[T].isInstanceOf[T])
2424
assert(summon[D[Int]].isInstanceOf[D[_]])
2525
}
2626

tests/run/implied-priority.scala

+5-5
Original file line numberDiff line numberDiff line change
@@ -72,16 +72,16 @@ def test2a = {
7272
}
7373

7474
/* If that solution is not applicable, we can define an override by refining the
75-
* result type of the given instance, e.g. like this:
75+
* result type of all lower-priority instances, e.g. like this:
7676
*/
7777
object Impl3 {
78-
given t1[T]: E[T]("low")
78+
trait LowPriority // A marker trait to indicate a lower priority
79+
given t1[T]: E[T]("low") with LowPriority
7980
}
8081

8182
object Override {
82-
trait HighestPriority // A marker trait to indicate a higher priority
8383

84-
given over[T]: E[T]("hi") with HighestPriority()
84+
given over[T]: E[T]("hi") with {}
8585
}
8686

8787
def test3 = {
@@ -90,7 +90,7 @@ def test3 = {
9090

9191
{ import Override.given
9292
import Impl3.given
93-
assert(summon[E[String]].str == "hi") // `over` takes priority since its result type is a subtype of t1's.
93+
assert(summon[E[String]].str == "hi", summon[E[String]].str) // `Impl3` takes priority since its result type is a subtype of t1's.
9494
}
9595
}
9696

0 commit comments

Comments
 (0)