Skip to content

Commit 3e3df65

Browse files
oderskyodersky
committed
Merge pull request #1243 from dotty-staging/fix-#1240
Fix overriding problems
2 parents 134ad7a + 77642b9 commit 3e3df65

File tree

13 files changed

+231
-58
lines changed

13 files changed

+231
-58
lines changed

‎src/dotty/annotation/internal/Child.scala

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,14 @@ package dotty.annotation.internal
22

33
import scala.annotation.Annotation
44

5-
/** An annotation to indicate a child class or object of the annotated class. */
5+
/** An annotation to indicate a child class or object of the annotated class.
6+
* E.g. if we have
7+
*
8+
* sealed class A
9+
* case class B() extends A
10+
* case class C() extends A
11+
*
12+
* Then `A` would carry the annotations `@Child[B] @Child[C]` where
13+
* `B`, `C` are TypeRefs.
14+
*/
615
class Child[T] extends Annotation

‎src/dotty/tools/dotc/core/Denotations.scala

Lines changed: 48 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,11 @@ object Denotations {
122122
/** The signature of the denotation. */
123123
def signature(implicit ctx: Context): Signature
124124

125-
/** Resolve overloaded denotation to pick the one with the given signature
125+
/** Resolve overloaded denotation to pick the ones with the given signature
126126
* when seen from prefix `site`.
127127
* @param relaxed When true, consider only parameter signatures for a match.
128128
*/
129-
def atSignature(sig: Signature, site: Type = NoPrefix, relaxed: Boolean = false)(implicit ctx: Context): SingleDenotation
129+
def atSignature(sig: Signature, site: Type = NoPrefix, relaxed: Boolean = false)(implicit ctx: Context): Denotation
130130

131131
/** The variant of this denotation that's current in the given context, or
132132
* `NotDefinedHereDenotation` if this denotation does not exist at current phase, but
@@ -157,7 +157,10 @@ object Denotations {
157157
* or NoDenotation if no satisfying alternative exists.
158158
* @throws TypeError if there is at more than one alternative that satisfies `p`.
159159
*/
160-
def suchThat(p: Symbol => Boolean): SingleDenotation
160+
def suchThat(p: Symbol => Boolean)(implicit ctx: Context): SingleDenotation
161+
162+
/** If this is a SingleDenotation, return it, otherwise throw a TypeError */
163+
def checkUnique(implicit ctx: Context): SingleDenotation = suchThat(alwaysTrue)
161164

162165
/** Does this denotation have an alternative that satisfies the predicate `p`? */
163166
def hasAltWith(p: SingleDenotation => Boolean): Boolean
@@ -227,13 +230,17 @@ object Denotations {
227230
/** The alternative of this denotation that has a type matching `targetType` when seen
228231
* as a member of type `site`, `NoDenotation` if none exists.
229232
*/
230-
def matchingDenotation(site: Type, targetType: Type)(implicit ctx: Context): SingleDenotation =
231-
if (isOverloaded)
232-
atSignature(targetType.signature, site, relaxed = true).matchingDenotation(site, targetType)
233-
else if (exists && !site.memberInfo(symbol).matchesLoosely(targetType))
234-
NoDenotation
235-
else
236-
asSingleDenotation
233+
def matchingDenotation(site: Type, targetType: Type)(implicit ctx: Context): SingleDenotation = {
234+
def qualifies(sym: Symbol) = site.memberInfo(sym).matchesLoosely(targetType)
235+
if (isOverloaded) {
236+
atSignature(targetType.signature, site, relaxed = true) match {
237+
case sd: SingleDenotation => sd.matchingDenotation(site, targetType)
238+
case md => md.suchThat(qualifies(_))
239+
}
240+
}
241+
else if (exists && !qualifies(symbol)) NoDenotation
242+
else asSingleDenotation
243+
}
237244

238245
/** Form a denotation by conjoining with denotation `that`.
239246
*
@@ -282,8 +289,10 @@ object Denotations {
282289
val info2 = denot2.info
283290
val sym1 = denot1.symbol
284291
val sym2 = denot2.symbol
285-
val sym2Accessible = sym2.isAccessibleFrom(pre)
286292

293+
if (isDoubleDef(sym1, sym2)) doubleDefError(denot1, denot2, pre)
294+
295+
val sym2Accessible = sym2.isAccessibleFrom(pre)
287296
/** Does `sym1` come before `sym2` in the linearization of `pre`? */
288297
def precedes(sym1: Symbol, sym2: Symbol) = {
289298
def precedesIn(bcs: List[ClassSymbol]): Boolean = bcs match {
@@ -418,19 +427,21 @@ object Denotations {
418427
final def validFor = denot1.validFor & denot2.validFor
419428
final def isType = false
420429
final def signature(implicit ctx: Context) = Signature.OverloadedSignature
421-
def atSignature(sig: Signature, site: Type, relaxed: Boolean)(implicit ctx: Context): SingleDenotation =
422-
denot1.atSignature(sig, site, relaxed) orElse denot2.atSignature(sig, site, relaxed)
430+
def atSignature(sig: Signature, site: Type, relaxed: Boolean)(implicit ctx: Context): Denotation =
431+
derivedMultiDenotation(denot1.atSignature(sig, site, relaxed), denot2.atSignature(sig, site, relaxed))
423432
def currentIfExists(implicit ctx: Context): Denotation =
424433
derivedMultiDenotation(denot1.currentIfExists, denot2.currentIfExists)
425434
def current(implicit ctx: Context): Denotation =
426435
derivedMultiDenotation(denot1.current, denot2.current)
427436
def altsWith(p: Symbol => Boolean): List[SingleDenotation] =
428437
denot1.altsWith(p) ++ denot2.altsWith(p)
429-
def suchThat(p: Symbol => Boolean): SingleDenotation = {
438+
def suchThat(p: Symbol => Boolean)(implicit ctx: Context): SingleDenotation = {
430439
val sd1 = denot1.suchThat(p)
431440
val sd2 = denot2.suchThat(p)
432441
if (sd1.exists)
433-
if (sd2.exists) throw new TypeError(s"failure to disambiguate overloaded reference $this")
442+
if (sd2.exists)
443+
if (isDoubleDef(denot1.symbol, denot2.symbol)) doubleDefError(denot1, denot2)
444+
else throw new TypeError(s"failure to disambiguate overloaded reference $this")
434445
else sd1
435446
else sd2
436447
}
@@ -480,7 +491,7 @@ object Denotations {
480491
def altsWith(p: Symbol => Boolean): List[SingleDenotation] =
481492
if (exists && p(symbol)) this :: Nil else Nil
482493

483-
def suchThat(p: Symbol => Boolean): SingleDenotation =
494+
def suchThat(p: Symbol => Boolean)(implicit ctx: Context): SingleDenotation =
484495
if (exists && p(symbol)) this else NoDenotation
485496

486497
def hasAltWith(p: SingleDenotation => Boolean): Boolean =
@@ -900,6 +911,27 @@ object Denotations {
900911
*/
901912
case class NoQualifyingRef(alts: List[SingleDenotation])(implicit ctx: Context) extends ErrorDenotation
902913

914+
/** A double definition
915+
*/
916+
def isDoubleDef(sym1: Symbol, sym2: Symbol)(implicit ctx: Context): Boolean =
917+
(sym1.exists && sym2.exists &&
918+
(sym1 ne sym2) && (sym1.owner eq sym2.owner) &&
919+
!sym1.is(Bridge) && !sym2.is(Bridge))
920+
921+
def doubleDefError(denot1: Denotation, denot2: Denotation, pre: Type = NoPrefix)(implicit ctx: Context): Nothing = {
922+
val sym1 = denot1.symbol
923+
val sym2 = denot2.symbol
924+
def fromWhere = if (pre == NoPrefix) "" else i"\nwhen seen as members of $pre"
925+
throw new MergeError(
926+
i"""cannot merge
927+
| $sym1: ${sym1.info} and
928+
| $sym2: ${sym2.info};
929+
|they are both defined in ${sym1.owner} but have matching signatures
930+
| ${denot1.info} and
931+
| ${denot2.info}$fromWhere""".stripMargin,
932+
denot2.info, denot2.info)
933+
}
934+
903935
// --------------- PreDenotations -------------------------------------------------
904936

905937
/** A PreDenotation represents a group of single denotations

‎src/dotty/tools/dotc/core/Types.scala

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1437,7 +1437,11 @@ object Types {
14371437
asMemberOf(prefix) match {
14381438
case NoDenotation => d.current
14391439
case newd: SingleDenotation => newd
1440-
case newd => newd.atSignature(d.signature).orElse(d.current)
1440+
case newd =>
1441+
newd.atSignature(d.signature) match {
1442+
case newd1: SingleDenotation if newd1.exists => newd1
1443+
case _ => d.current
1444+
}
14411445
}
14421446

14431447
private def denotOfSym(sym: Symbol)(implicit ctx: Context): Denotation = {
@@ -1729,7 +1733,7 @@ object Types {
17291733
override def loadDenot(implicit ctx: Context): Denotation = {
17301734
val d = super.loadDenot
17311735
if (sig eq Signature.OverloadedSignature) d
1732-
else d.atSignature(sig)
1736+
else d.atSignature(sig).checkUnique
17331737
}
17341738

17351739
override def newLikeThis(prefix: Type)(implicit ctx: Context): TermRef = {

‎src/dotty/tools/dotc/transform/Erasure.scala

Lines changed: 45 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -41,24 +41,30 @@ class Erasure extends Phase with DenotTransformer { thisTransformer =>
4141
// Aftre erasure, all former Any members are now Object members
4242
val ClassInfo(pre, _, ps, decls, selfInfo) = ref.info
4343
val extendedScope = decls.cloneScope
44-
defn.AnyClass.classInfo.decls.foreach(extendedScope.enter)
44+
for (decl <- defn.AnyClass.classInfo.decls)
45+
if (!decl.isConstructor) extendedScope.enter(decl)
4546
ref.copySymDenotation(
4647
info = transformInfo(ref.symbol,
4748
ClassInfo(pre, defn.ObjectClass, ps, extendedScope, selfInfo))
4849
)
4950
}
5051
else {
52+
val oldSymbol = ref.symbol
53+
val newSymbol =
54+
if ((oldSymbol.owner eq defn.AnyClass) && oldSymbol.isConstructor)
55+
defn.ObjectClass.primaryConstructor
56+
else oldSymbol
5157
val oldOwner = ref.owner
5258
val newOwner = if (oldOwner eq defn.AnyClass) defn.ObjectClass else oldOwner
5359
val oldInfo = ref.info
5460
val newInfo = transformInfo(ref.symbol, oldInfo)
5561
val oldFlags = ref.flags
5662
val newFlags = ref.flags &~ Flags.HasDefaultParams // HasDefaultParams needs to be dropped because overriding might become overloading
5763
// TODO: define derivedSymDenotation?
58-
if ((oldOwner eq newOwner) && (oldInfo eq newInfo) && (oldFlags == newFlags)) ref
64+
if ((oldSymbol eq newSymbol) && (oldOwner eq newOwner) && (oldInfo eq newInfo) && (oldFlags == newFlags)) ref
5965
else {
6066
assert(!ref.is(Flags.PackageClass), s"trans $ref @ ${ctx.phase} oldOwner = $oldOwner, newOwner = $newOwner, oldInfo = $oldInfo, newInfo = $newInfo ${oldOwner eq newOwner} ${oldInfo eq newInfo}")
61-
ref.copySymDenotation(owner = newOwner, initFlags = newFlags, info = newInfo)
67+
ref.copySymDenotation(symbol = newSymbol, owner = newOwner, initFlags = newFlags, info = newInfo)
6268
}
6369
}
6470
case ref =>
@@ -553,43 +559,47 @@ object Erasure extends TypeTestsCasts{
553559
before match {
554560
case Nil => emittedBridges.toList
555561
case (oldMember: untpd.DefDef) :: oldTail =>
556-
val oldSymbol = oldMember.symbol(beforeCtx)
557-
val newSymbol = member.symbol(ctx)
558-
assert(oldSymbol.name(beforeCtx) == newSymbol.name,
559-
s"${oldSymbol.name(beforeCtx)} bridging with ${newSymbol.name}")
560-
val newOverridden = oldSymbol.denot.allOverriddenSymbols.toSet // TODO: clarify new <-> old in a comment; symbols are swapped here
561-
val oldOverridden = newSymbol.allOverriddenSymbols(beforeCtx).toSet // TODO: can we find a more efficient impl? newOverridden does not have to be a set!
562-
def stillInBaseClass(sym: Symbol) = ctx.owner derivesFrom sym.owner
563-
val neededBridges = (oldOverridden -- newOverridden).filter(stillInBaseClass)
564-
565-
var minimalSet = Set[Symbol]()
566-
// compute minimal set of bridges that are needed:
567-
for (bridge <- neededBridges) {
568-
val isRequired = minimalSet.forall(nxtBridge => !(bridge.info =:= nxtBridge.info))
569-
570-
if (isRequired) {
571-
// check for clashes
572-
val clash: Option[Symbol] = oldSymbol.owner.info.decls.lookupAll(bridge.name).find {
573-
sym =>
574-
(sym.name eq bridge.name) && sym.info.widen =:= bridge.info.widen
575-
}.orElse(
562+
try {
563+
val oldSymbol = oldMember.symbol(beforeCtx)
564+
val newSymbol = member.symbol(ctx)
565+
assert(oldSymbol.name(beforeCtx) == newSymbol.name,
566+
s"${oldSymbol.name(beforeCtx)} bridging with ${newSymbol.name}")
567+
val newOverridden = oldSymbol.denot.allOverriddenSymbols.toSet // TODO: clarify new <-> old in a comment; symbols are swapped here
568+
val oldOverridden = newSymbol.allOverriddenSymbols(beforeCtx).toSet // TODO: can we find a more efficient impl? newOverridden does not have to be a set!
569+
def stillInBaseClass(sym: Symbol) = ctx.owner derivesFrom sym.owner
570+
val neededBridges = (oldOverridden -- newOverridden).filter(stillInBaseClass)
571+
572+
var minimalSet = Set[Symbol]()
573+
// compute minimal set of bridges that are needed:
574+
for (bridge <- neededBridges) {
575+
val isRequired = minimalSet.forall(nxtBridge => !(bridge.info =:= nxtBridge.info))
576+
577+
if (isRequired) {
578+
// check for clashes
579+
val clash: Option[Symbol] = oldSymbol.owner.info.decls.lookupAll(bridge.name).find {
580+
sym =>
581+
(sym.name eq bridge.name) && sym.info.widen =:= bridge.info.widen
582+
}.orElse(
576583
emittedBridges.find(stat => (stat.name == bridge.name) && stat.tpe.widen =:= bridge.info.widen)
577-
.map(_.symbol)
578-
)
579-
clash match {
580-
case Some(cl) =>
581-
ctx.error(i"bridge for method ${newSymbol.showLocated(beforeCtx)} of type ${newSymbol.info(beforeCtx)}\n" +
582-
i"clashes with ${cl.symbol.showLocated(beforeCtx)} of type ${cl.symbol.info(beforeCtx)}\n" +
583-
i"both have same type after erasure: ${bridge.symbol.info}")
584-
case None => minimalSet += bridge
584+
.map(_.symbol))
585+
clash match {
586+
case Some(cl) =>
587+
ctx.error(i"bridge for method ${newSymbol.showLocated(beforeCtx)} of type ${newSymbol.info(beforeCtx)}\n" +
588+
i"clashes with ${cl.symbol.showLocated(beforeCtx)} of type ${cl.symbol.info(beforeCtx)}\n" +
589+
i"both have same type after erasure: ${bridge.symbol.info}")
590+
case None => minimalSet += bridge
591+
}
585592
}
586593
}
587-
}
588594

589-
val bridgeImplementations = minimalSet.map {
590-
sym => makeBridgeDef(member, sym)(ctx)
595+
val bridgeImplementations = minimalSet.map {
596+
sym => makeBridgeDef(member, sym)(ctx)
597+
}
598+
emittedBridges ++= bridgeImplementations
599+
} catch {
600+
case ex: MergeError => ctx.error(ex.getMessage, member.pos)
591601
}
592-
emittedBridges ++= bridgeImplementations
602+
593603
traverse(newTail, oldTail, emittedBridges)
594604
case notADefDef :: oldTail =>
595605
traverse(after, oldTail, emittedBridges)

‎src/dotty/tools/dotc/transform/Splitter.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class Splitter extends MiniPhaseTransform { thisTransform =>
4646
val mbr = tp.member(name)
4747
if (!mbr.isOverloaded) mbr.asSingleDenotation
4848
else tree.tpe match {
49-
case tref: TermRefWithSignature => mbr.atSignature(tref.sig)
49+
case tref: TermRefWithSignature => mbr.atSignature(tref.sig).checkUnique
5050
case _ =>
5151
def alts = mbr.alternatives.map(alt => i"$alt: ${alt.info}").mkString(", ")
5252
ctx.error(s"cannot disambiguate overloaded members $alts", tree.pos)

‎src/dotty/tools/dotc/typer/RefChecks.scala

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -837,14 +837,18 @@ class RefChecks extends MiniPhase { thisTransformer =>
837837
if (tree.symbol is Macro) EmptyTree else tree
838838
}
839839

840-
override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo) = {
840+
override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo) = try {
841841
val cls = ctx.owner
842842
checkOverloadedRestrictions(cls)
843843
checkParents(cls)
844844
checkCompanionNameClashes(cls)
845845
checkAllOverrides(cls)
846846
checkDerivedValueClass(cls, tree.body)
847847
tree
848+
} catch {
849+
case ex: MergeError =>
850+
ctx.error(ex.getMessage, tree.pos)
851+
tree
848852
}
849853

850854
override def transformTypeTree(tree: TypeTree)(implicit ctx: Context, info: TransformerInfo) = {

‎test/dotc/tests.scala

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,10 +119,11 @@ class tests extends CompilerTest {
119119
@Test def neg_typedIdents() = compileDir(negDir, "typedIdents")
120120

121121
val negCustomArgs = negDir + "customArgs/"
122-
@Test def neg_typers() = compileFile(negCustomArgs, "typers")(allowDoubleBindings)
122+
@Test def neg_typers = compileFile(negCustomArgs, "typers")(allowDoubleBindings)
123123
@Test def neg_overrideClass = compileFile(negCustomArgs, "overrideClass", List("-language:Scala2"))
124124
@Test def neg_autoTupling = compileFile(negCustomArgs, "autoTuplingTest", args = "-language:noAutoTupling" :: Nil)
125125
@Test def neg_i1050 = compileFile(negCustomArgs, "i1050", List("-strict"))
126+
@Test def neg_i1240 = compileFile(negCustomArgs, "i1240")(allowDoubleBindings)
126127

127128
val negTailcallDir = negDir + "tailcall/"
128129
@Test def neg_tailcall_t1672b = compileFile(negTailcallDir, "t1672b")
@@ -155,7 +156,8 @@ class tests extends CompilerTest {
155156

156157
@Test def dotc_ast = compileDir(dotcDir, "ast")
157158
@Test def dotc_config = compileDir(dotcDir, "config")
158-
@Test def dotc_core = compileDir(dotcDir, "core")("-Yno-double-bindings" :: allowDeepSubtypes)// twice omitted to make tests run faster
159+
@Test def dotc_core = compileDir(dotcDir, "core")(allowDeepSubtypes)// twice omitted to make tests run faster
160+
@Test def dotc_core_nocheck = compileDir(dotcDir, "core")(noCheckOptions)
159161

160162
// This directory doesn't exist anymore
161163
// @Test def dotc_core_pickling = compileDir(coreDir, "pickling")(allowDeepSubtypes)// twice omitted to make tests run faster

‎tests/neg/customArgs/i1240.scala

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package test
2+
3+
class C[T] {
4+
5+
def foo(x: D) = { System.out.println("D foo"); }
6+
def foo(x: T) = { System.out.println("T foo"); }
7+
}
8+
9+
object C {
10+
def main(args: Array[String]) =
11+
new C[D]().foo(new D()) // error: ambiguous
12+
}
13+
14+
class C1[T] {
15+
def foo(x: D) = { System.out.println("D foo"); }
16+
}
17+
class C2[T] {
18+
def foo(x: D) = { System.out.println("D foo"); }
19+
}
20+
21+
class D {}
22+
23+
class X {
24+
def foo(x: D): D
25+
def foo(x: D): D // error: already defined
26+
}

‎tests/neg/i1240a.scala

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
2+
// more complicated example
3+
abstract class A {
4+
type C[X]
5+
def foo[B](x: C[B]): C[B] = {println("A.C"); x}
6+
def foo[B](x: List[B]): List[B] = {println("A.List"); x}
7+
def give[X]: C[X]
8+
}
9+
10+
class B extends A {
11+
type C[X] = List[X]
12+
override def give[X] = Nil
13+
override def foo[B](x: C[B]): C[B] = {println("B.C"); x} // error: merge error during erasure
14+
val a: A = this
15+
a.foo(a.give[Int]) // what method should be called here in runtime?
16+
}
17+

‎tests/neg/i1240b.scala

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// yet another variant, testing type parameters
2+
trait T[X] {
3+
def foo(x: X): X
4+
}
5+
abstract class A[X] extends T[X] {
6+
def foo(x: X): X = {println("A.X"); x}
7+
def foo(x: String): String = {println("A.String"); x}
8+
}
9+
trait U[X] extends T[X] {
10+
abstract override def foo(x: X): X = super.foo(x)
11+
}
12+
object Test extends A[String] with U[String] // error: accidental override

0 commit comments

Comments
 (0)