From f36c21ed9f79a1fc41e36d7c5221a070d83e61a7 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 1 May 2016 09:22:17 +0200 Subject: [PATCH 01/16] Avoid merging denotations of different symbols in same class #1240 shows that we need to detect ambiguous overloads of methods coming from the same base class (with different signatures there) that have the same signature in some deriving class. This was undetected before because the two methods were simply merged into one overloaded alternative. --- src/dotty/tools/dotc/core/Denotations.scala | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/core/Denotations.scala b/src/dotty/tools/dotc/core/Denotations.scala index 946738d73378..8003e68ac960 100644 --- a/src/dotty/tools/dotc/core/Denotations.scala +++ b/src/dotty/tools/dotc/core/Denotations.scala @@ -282,8 +282,15 @@ object Denotations { val info2 = denot2.info val sym1 = denot1.symbol val sym2 = denot2.symbol - val sym2Accessible = sym2.isAccessibleFrom(pre) + if (sym1.exists && sym2.exists && + (sym1 ne sym2) && (sym1.owner eq sym2.owner) && + !sym1.is(Bridge) && !sym2.is(Bridge)) + // double definition of two methods with same signature in one class; + // don't merge them. + return NoDenotation + + val sym2Accessible = sym2.isAccessibleFrom(pre) /** Does `sym1` come before `sym2` in the linearization of `pre`? */ def precedes(sym1: Symbol, sym2: Symbol) = { def precedesIn(bcs: List[ClassSymbol]): Boolean = bcs match { From cc0f62954e814200f47f978b857abf6ab039c9f0 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 1 May 2016 09:23:53 +0200 Subject: [PATCH 02/16] Don't copy Any constructor to Object in Erasure Once we do not merge methods with same signature anymore we got an ambiguous overload between the constructors of Any and Object after erasure (when all Any methods are moved to Object). To avoid this, we map the Any constructor to the Object constructor after erasure. --- src/dotty/tools/dotc/transform/Erasure.scala | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/dotty/tools/dotc/transform/Erasure.scala b/src/dotty/tools/dotc/transform/Erasure.scala index 0b3a07f65833..ee8040168a17 100644 --- a/src/dotty/tools/dotc/transform/Erasure.scala +++ b/src/dotty/tools/dotc/transform/Erasure.scala @@ -41,13 +41,19 @@ class Erasure extends Phase with DenotTransformer { thisTransformer => // Aftre erasure, all former Any members are now Object members val ClassInfo(pre, _, ps, decls, selfInfo) = ref.info val extendedScope = decls.cloneScope - defn.AnyClass.classInfo.decls.foreach(extendedScope.enter) + for (decl <- defn.AnyClass.classInfo.decls) + if (!decl.isConstructor) extendedScope.enter(decl) ref.copySymDenotation( info = transformInfo(ref.symbol, ClassInfo(pre, defn.ObjectClass, ps, extendedScope, selfInfo)) ) } else { + val oldSymbol = ref.symbol + val newSymbol = + if ((oldSymbol.owner eq defn.AnyClass) && oldSymbol.isConstructor) + defn.ObjectClass.primaryConstructor + else oldSymbol val oldOwner = ref.owner val newOwner = if (oldOwner eq defn.AnyClass) defn.ObjectClass else oldOwner val oldInfo = ref.info @@ -55,10 +61,10 @@ class Erasure extends Phase with DenotTransformer { thisTransformer => val oldFlags = ref.flags val newFlags = ref.flags &~ Flags.HasDefaultParams // HasDefaultParams needs to be dropped because overriding might become overloading // TODO: define derivedSymDenotation? - if ((oldOwner eq newOwner) && (oldInfo eq newInfo) && (oldFlags == newFlags)) ref + if ((oldSymbol eq newSymbol) && (oldOwner eq newOwner) && (oldInfo eq newInfo) && (oldFlags == newFlags)) ref else { assert(!ref.is(Flags.PackageClass), s"trans $ref @ ${ctx.phase} oldOwner = $oldOwner, newOwner = $newOwner, oldInfo = $oldInfo, newInfo = $newInfo ${oldOwner eq newOwner} ${oldInfo eq newInfo}") - ref.copySymDenotation(owner = newOwner, initFlags = newFlags, info = newInfo) + ref.copySymDenotation(symbol = newSymbol, owner = newOwner, initFlags = newFlags, info = newInfo) } } case ref => From d0f05ad6c756355bb6ea863e10a554f54f145907 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 1 May 2016 09:25:01 +0200 Subject: [PATCH 03/16] ResolveOverloaded should handle alternatives that are the same TermRef This happens once we do not merge methods with the same signature coming from the same class. --- src/dotty/tools/dotc/typer/Applications.scala | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/dotty/tools/dotc/typer/Applications.scala b/src/dotty/tools/dotc/typer/Applications.scala index 390ecaee97e3..4f7a1c22077d 100644 --- a/src/dotty/tools/dotc/typer/Applications.scala +++ b/src/dotty/tools/dotc/typer/Applications.scala @@ -867,8 +867,6 @@ trait Applications extends Compatibility { self: Typer => */ def isAsGood(alt1: TermRef, alt2: TermRef)(implicit ctx: Context): Boolean = track("isAsGood") { ctx.traceIndented(i"isAsGood($alt1, $alt2)", overload) { - assert(alt1 ne alt2) - /** Is class or module class `sym1` derived from class or module class `sym2`? * Module classes also inherit the relationship from their companions. */ @@ -975,14 +973,20 @@ trait Applications extends Compatibility { self: Typer => bestSoFar } val best = winner(alt, alts1) - def asGood(alts: List[TermRef]): List[TermRef] = alts match { + // A tricky corner case is where we have two methods that have the same signature + // when seen from a particulatr prefix. In that case the TermRefs of two + // alternatives are identical. Hence, we have to make sure (using `altSeen`) that we + // eliminate only one alternative that's identical to `best`. + // A test case is in neg/i1240.scala + def asGood(alts: List[TermRef], altSeen: Boolean): List[TermRef] = alts match { case alt :: alts1 => - if ((alt eq best) || !isAsGood(alt, best)) asGood(alts1) - else alt :: asGood(alts1) + if (!altSeen && (alt eq best)) asGood(alts1, altSeen = true) + else if (!isAsGood(alt, best)) asGood(alts1, altSeen) + else alt :: asGood(alts1, altSeen) case nil => Nil } - best :: asGood(alts) + best :: asGood(alts, altSeen = false) } } From 2dd6a7ae07cca7b03372d6b9f1fb30e0fcc975b7 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 1 May 2016 09:25:11 +0200 Subject: [PATCH 04/16] Test case --- test/dotc/tests.scala | 3 ++- tests/neg/customArgs/i1240.scala | 40 ++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 tests/neg/customArgs/i1240.scala diff --git a/test/dotc/tests.scala b/test/dotc/tests.scala index b646c72d5573..97c872bb6eb2 100644 --- a/test/dotc/tests.scala +++ b/test/dotc/tests.scala @@ -119,10 +119,11 @@ class tests extends CompilerTest { @Test def neg_typedIdents() = compileDir(negDir, "typedIdents") val negCustomArgs = negDir + "customArgs/" - @Test def neg_typers() = compileFile(negCustomArgs, "typers")(allowDoubleBindings) + @Test def neg_typers = compileFile(negCustomArgs, "typers")(allowDoubleBindings) @Test def neg_overrideClass = compileFile(negCustomArgs, "overrideClass", List("-language:Scala2")) @Test def neg_autoTupling = compileFile(negCustomArgs, "autoTuplingTest", args = "-language:noAutoTupling" :: Nil) @Test def neg_i1050 = compileFile(negCustomArgs, "i1050", List("-strict")) + @Test def neg_i1240 = compileFile(negCustomArgs, "i1240")(allowDoubleBindings) val negTailcallDir = negDir + "tailcall/" @Test def neg_tailcall_t1672b = compileFile(negTailcallDir, "t1672b") diff --git a/tests/neg/customArgs/i1240.scala b/tests/neg/customArgs/i1240.scala new file mode 100644 index 000000000000..6235e8815d27 --- /dev/null +++ b/tests/neg/customArgs/i1240.scala @@ -0,0 +1,40 @@ +package test + +class C[T] { + + def foo(x: D) = { System.out.println("D foo"); } + def foo(x: T) = { System.out.println("T foo"); } +} + +object C { + def main(args: Array[String]) = + new C[D]().foo(new D()) // error: ambiguous +} +/* +class C1[T] { + def foo(x: D) = { System.out.println("D foo"); } +} +class C2[T] { + def foo(x: D) = { System.out.println("D foo"); } +} + +class D {} + +// more complicated example +abstract class A { + type C[X] + def foo[B](x: C[B]): C[B] = {println("A.C"); x} + def foo[B](x: List[B]): List[B] = {println("A.List"); x} + def give[X]: C[X] +} + +class B extends A { + type C[X] = List[X] + override def give[X] = Nil + override def foo[B](x: C[B]): C[B] = {println("B.C"); x} + // which method is overriden? + // should any bridges be generated? + val a: A = this + a.foo(a.give[Int]) // what method should be called here in runtime? +} +*/ From 968f1ab0cd706de8833112741407f94a6a0b2677 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 2 May 2016 11:34:38 +0200 Subject: [PATCH 05/16] Fix test case --- tests/neg/customArgs/i1240.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/neg/customArgs/i1240.scala b/tests/neg/customArgs/i1240.scala index 6235e8815d27..438199b4a63b 100644 --- a/tests/neg/customArgs/i1240.scala +++ b/tests/neg/customArgs/i1240.scala @@ -10,7 +10,7 @@ object C { def main(args: Array[String]) = new C[D]().foo(new D()) // error: ambiguous } -/* + class C1[T] { def foo(x: D) = { System.out.println("D foo"); } } @@ -37,4 +37,4 @@ class B extends A { val a: A = this a.foo(a.give[Int]) // what method should be called here in runtime? } -*/ + From 48b716012bd72486dbf4a2bd3b293ef212f4addd Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 2 May 2016 15:51:41 +0200 Subject: [PATCH 06/16] Issue MergeError exception for double def situations When finding two symbols in the same class that have the same signature as seen from some prefix, issue a merge error. This is simpler and more robust than the alternative of producing an overloaded denotation and dealing with it afterwards. --- src/dotty/tools/dotc/core/Denotations.scala | 37 ++++++++--- src/dotty/tools/dotc/transform/Erasure.scala | 68 +++++++++++--------- tests/neg/customArgs/i1240.scala | 20 +----- tests/neg/i1240a.scala | 17 +++++ 4 files changed, 85 insertions(+), 57 deletions(-) create mode 100644 tests/neg/i1240a.scala diff --git a/src/dotty/tools/dotc/core/Denotations.scala b/src/dotty/tools/dotc/core/Denotations.scala index 8003e68ac960..0bcb3198b615 100644 --- a/src/dotty/tools/dotc/core/Denotations.scala +++ b/src/dotty/tools/dotc/core/Denotations.scala @@ -283,12 +283,7 @@ object Denotations { val sym1 = denot1.symbol val sym2 = denot2.symbol - if (sym1.exists && sym2.exists && - (sym1 ne sym2) && (sym1.owner eq sym2.owner) && - !sym1.is(Bridge) && !sym2.is(Bridge)) - // double definition of two methods with same signature in one class; - // don't merge them. - return NoDenotation + if (isDoubleDef(sym1, sym2)) doubleDefError(denot1, denot2, pre) val sym2Accessible = sym2.isAccessibleFrom(pre) /** Does `sym1` come before `sym2` in the linearization of `pre`? */ @@ -425,8 +420,12 @@ object Denotations { final def validFor = denot1.validFor & denot2.validFor final def isType = false final def signature(implicit ctx: Context) = Signature.OverloadedSignature - def atSignature(sig: Signature, site: Type, relaxed: Boolean)(implicit ctx: Context): SingleDenotation = - denot1.atSignature(sig, site, relaxed) orElse denot2.atSignature(sig, site, relaxed) + def atSignature(sig: Signature, site: Type, relaxed: Boolean)(implicit ctx: Context): SingleDenotation = { + val atSig1 = denot1.atSignature(sig, site, relaxed) + val atSig2 = denot2.atSignature(sig, site, relaxed) + if (isDoubleDef(atSig1.symbol, atSig2.symbol)) doubleDefError(atSig1, atSig2, site) + atSig1.orElse(atSig2) + } def currentIfExists(implicit ctx: Context): Denotation = derivedMultiDenotation(denot1.currentIfExists, denot2.currentIfExists) def current(implicit ctx: Context): Denotation = @@ -907,6 +906,28 @@ object Denotations { */ case class NoQualifyingRef(alts: List[SingleDenotation])(implicit ctx: Context) extends ErrorDenotation + /** A double defifinition + */ + def isDoubleDef(sym1: Symbol, sym2: Symbol)(implicit ctx: Context): Boolean = + (sym1.exists && sym2.exists && + (sym1 ne sym2) && (sym1.owner eq sym2.owner) && + !sym1.is(Bridge) && !sym2.is(Bridge)) + + def doubleDefError(denot1: SingleDenotation, denot2: SingleDenotation, pre: Type)(implicit ctx: Context): Unit = { + val sym1 = denot1.symbol + val sym2 = denot2.symbol + def fromWhere = if (pre == NoPrefix) "" else i"\nwhen seen as members of $pre" + throw new MergeError( + i"""cannot merge + | $sym1: ${sym1.info} and + | $sym2: ${sym2.info}; + |they are both defined in ${sym1.owner} but have matching signatures + | ${denot1.info} and + | ${denot2.info}$fromWhere""".stripMargin, + denot2.info, denot2.info) + } + + // --------------- PreDenotations ------------------------------------------------- /** A PreDenotation represents a group of single denotations diff --git a/src/dotty/tools/dotc/transform/Erasure.scala b/src/dotty/tools/dotc/transform/Erasure.scala index ee8040168a17..24dea5118e7d 100644 --- a/src/dotty/tools/dotc/transform/Erasure.scala +++ b/src/dotty/tools/dotc/transform/Erasure.scala @@ -559,43 +559,47 @@ object Erasure extends TypeTestsCasts{ before match { case Nil => emittedBridges.toList case (oldMember: untpd.DefDef) :: oldTail => - val oldSymbol = oldMember.symbol(beforeCtx) - val newSymbol = member.symbol(ctx) - assert(oldSymbol.name(beforeCtx) == newSymbol.name, - s"${oldSymbol.name(beforeCtx)} bridging with ${newSymbol.name}") - val newOverridden = oldSymbol.denot.allOverriddenSymbols.toSet // TODO: clarify new <-> old in a comment; symbols are swapped here - val oldOverridden = newSymbol.allOverriddenSymbols(beforeCtx).toSet // TODO: can we find a more efficient impl? newOverridden does not have to be a set! - def stillInBaseClass(sym: Symbol) = ctx.owner derivesFrom sym.owner - val neededBridges = (oldOverridden -- newOverridden).filter(stillInBaseClass) - - var minimalSet = Set[Symbol]() - // compute minimal set of bridges that are needed: - for (bridge <- neededBridges) { - val isRequired = minimalSet.forall(nxtBridge => !(bridge.info =:= nxtBridge.info)) - - if (isRequired) { - // check for clashes - val clash: Option[Symbol] = oldSymbol.owner.info.decls.lookupAll(bridge.name).find { - sym => - (sym.name eq bridge.name) && sym.info.widen =:= bridge.info.widen - }.orElse( + try { + val oldSymbol = oldMember.symbol(beforeCtx) + val newSymbol = member.symbol(ctx) + assert(oldSymbol.name(beforeCtx) == newSymbol.name, + s"${oldSymbol.name(beforeCtx)} bridging with ${newSymbol.name}") + val newOverridden = oldSymbol.denot.allOverriddenSymbols.toSet // TODO: clarify new <-> old in a comment; symbols are swapped here + val oldOverridden = newSymbol.allOverriddenSymbols(beforeCtx).toSet // TODO: can we find a more efficient impl? newOverridden does not have to be a set! + def stillInBaseClass(sym: Symbol) = ctx.owner derivesFrom sym.owner + val neededBridges = (oldOverridden -- newOverridden).filter(stillInBaseClass) + + var minimalSet = Set[Symbol]() + // compute minimal set of bridges that are needed: + for (bridge <- neededBridges) { + val isRequired = minimalSet.forall(nxtBridge => !(bridge.info =:= nxtBridge.info)) + + if (isRequired) { + // check for clashes + val clash: Option[Symbol] = oldSymbol.owner.info.decls.lookupAll(bridge.name).find { + sym => + (sym.name eq bridge.name) && sym.info.widen =:= bridge.info.widen + }.orElse( emittedBridges.find(stat => (stat.name == bridge.name) && stat.tpe.widen =:= bridge.info.widen) - .map(_.symbol) - ) - clash match { - case Some(cl) => - ctx.error(i"bridge for method ${newSymbol.showLocated(beforeCtx)} of type ${newSymbol.info(beforeCtx)}\n" + - i"clashes with ${cl.symbol.showLocated(beforeCtx)} of type ${cl.symbol.info(beforeCtx)}\n" + - i"both have same type after erasure: ${bridge.symbol.info}") - case None => minimalSet += bridge + .map(_.symbol)) + clash match { + case Some(cl) => + ctx.error(i"bridge for method ${newSymbol.showLocated(beforeCtx)} of type ${newSymbol.info(beforeCtx)}\n" + + i"clashes with ${cl.symbol.showLocated(beforeCtx)} of type ${cl.symbol.info(beforeCtx)}\n" + + i"both have same type after erasure: ${bridge.symbol.info}") + case None => minimalSet += bridge + } } } - } - val bridgeImplementations = minimalSet.map { - sym => makeBridgeDef(member, sym)(ctx) + val bridgeImplementations = minimalSet.map { + sym => makeBridgeDef(member, sym)(ctx) + } + emittedBridges ++= bridgeImplementations + } catch { + case ex: MergeError => ctx.error(ex.getMessage, member.pos) } - emittedBridges ++= bridgeImplementations + traverse(newTail, oldTail, emittedBridges) case notADefDef :: oldTail => traverse(after, oldTail, emittedBridges) diff --git a/tests/neg/customArgs/i1240.scala b/tests/neg/customArgs/i1240.scala index 438199b4a63b..3f4d1e210592 100644 --- a/tests/neg/customArgs/i1240.scala +++ b/tests/neg/customArgs/i1240.scala @@ -20,21 +20,7 @@ class C2[T] { class D {} -// more complicated example -abstract class A { - type C[X] - def foo[B](x: C[B]): C[B] = {println("A.C"); x} - def foo[B](x: List[B]): List[B] = {println("A.List"); x} - def give[X]: C[X] +class X { + def foo(x: D): D + def foo(x: D): D // error: already defined } - -class B extends A { - type C[X] = List[X] - override def give[X] = Nil - override def foo[B](x: C[B]): C[B] = {println("B.C"); x} - // which method is overriden? - // should any bridges be generated? - val a: A = this - a.foo(a.give[Int]) // what method should be called here in runtime? -} - diff --git a/tests/neg/i1240a.scala b/tests/neg/i1240a.scala new file mode 100644 index 000000000000..dc711454e0a6 --- /dev/null +++ b/tests/neg/i1240a.scala @@ -0,0 +1,17 @@ + +// more complicated example +abstract class A { + type C[X] + def foo[B](x: C[B]): C[B] = {println("A.C"); x} + def foo[B](x: List[B]): List[B] = {println("A.List"); x} + def give[X]: C[X] +} + +class B extends A { + type C[X] = List[X] + override def give[X] = Nil + override def foo[B](x: C[B]): C[B] = {println("B.C"); x} // error: merge error during erasure + val a: A = this + a.foo(a.give[Int]) // what method should be called here in runtime? +} + From f722de7c6131b544345dbc6745b5d219de5831e7 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 2 May 2016 15:56:52 +0200 Subject: [PATCH 07/16] A test case for overloading/overriding interactions This showcases a tricky interaction between overloading and overriding. See discussion of #1240 for context. --- tests/run/i1240.scala | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 tests/run/i1240.scala diff --git a/tests/run/i1240.scala b/tests/run/i1240.scala new file mode 100644 index 000000000000..860806ba5705 --- /dev/null +++ b/tests/run/i1240.scala @@ -0,0 +1,27 @@ +// A tricky case of overriding behavior +// Note: It would be acceptable if this produced an error instead. +// Bit testing this is tricky. +abstract class Base[T] { + def foo(x: T): String +} + +class C[T] extends Base[T] { + + def foo(x: D): String = "D foo" + def foo(x: T): String = "T foo" +} + +object Test { + def main(args: Array[String]) = { + val b1: Base[D] = new C[D] // which of the two foo's in C overrides the one in B? + assert(b1.foo(new D) == "T foo") + val b2: Base[D] = new C[D] {} + // In Java, this gives an error like this: + // methods foo(A) from C[D] and foo(String) from C[D] are inherited with the same signature + // But the analogous example with `b1` compiles OK in Java. + assert(b2.foo(new D) == "T foo") + } +} + +class D + From fe5f4f3963d2d6b2f6514a362fb312bc2e7d7f94 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 2 May 2016 15:58:44 +0200 Subject: [PATCH 08/16] Revert: ResolveOverloaded should handle alternatives that are the same TermRef This happens once we do not merge methods with the same signature coming from the same class. (reverted from commit 83262d090a98e2374c9b3e5a1480892397d695d3) This case no longer applies as such a situation will now give a MergeError instead. --- src/dotty/tools/dotc/typer/Applications.scala | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/dotty/tools/dotc/typer/Applications.scala b/src/dotty/tools/dotc/typer/Applications.scala index 4f7a1c22077d..390ecaee97e3 100644 --- a/src/dotty/tools/dotc/typer/Applications.scala +++ b/src/dotty/tools/dotc/typer/Applications.scala @@ -867,6 +867,8 @@ trait Applications extends Compatibility { self: Typer => */ def isAsGood(alt1: TermRef, alt2: TermRef)(implicit ctx: Context): Boolean = track("isAsGood") { ctx.traceIndented(i"isAsGood($alt1, $alt2)", overload) { + assert(alt1 ne alt2) + /** Is class or module class `sym1` derived from class or module class `sym2`? * Module classes also inherit the relationship from their companions. */ @@ -973,20 +975,14 @@ trait Applications extends Compatibility { self: Typer => bestSoFar } val best = winner(alt, alts1) - // A tricky corner case is where we have two methods that have the same signature - // when seen from a particulatr prefix. In that case the TermRefs of two - // alternatives are identical. Hence, we have to make sure (using `altSeen`) that we - // eliminate only one alternative that's identical to `best`. - // A test case is in neg/i1240.scala - def asGood(alts: List[TermRef], altSeen: Boolean): List[TermRef] = alts match { + def asGood(alts: List[TermRef]): List[TermRef] = alts match { case alt :: alts1 => - if (!altSeen && (alt eq best)) asGood(alts1, altSeen = true) - else if (!isAsGood(alt, best)) asGood(alts1, altSeen) - else alt :: asGood(alts1, altSeen) + if ((alt eq best) || !isAsGood(alt, best)) asGood(alts1) + else alt :: asGood(alts1) case nil => Nil } - best :: asGood(alts, altSeen = false) + best :: asGood(alts) } } From 9aa800f8a9905de059b6e34554cb166a9776dff3 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 2 May 2016 19:57:57 +0200 Subject: [PATCH 09/16] Refined handling of atSignature We cannot throw a merge error if atSignature does not give a unique single denotation. Counter example is compiling dotty itself, where we get a false negative during bridge generation. Instead, atSigature needs to return a normal denotation, and we need to check separately where required that a denotation is in fact a SingleDenotation. --- src/dotty/tools/dotc/core/Denotations.scala | 48 ++++++++++--------- src/dotty/tools/dotc/core/Types.scala | 4 +- src/dotty/tools/dotc/transform/Splitter.scala | 2 +- 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/dotty/tools/dotc/core/Denotations.scala b/src/dotty/tools/dotc/core/Denotations.scala index 0bcb3198b615..080e8a39ba58 100644 --- a/src/dotty/tools/dotc/core/Denotations.scala +++ b/src/dotty/tools/dotc/core/Denotations.scala @@ -122,11 +122,11 @@ object Denotations { /** The signature of the denotation. */ def signature(implicit ctx: Context): Signature - /** Resolve overloaded denotation to pick the one with the given signature + /** Resolve overloaded denotation to pick the ones with the given signature * when seen from prefix `site`. * @param relaxed When true, consider only parameter signatures for a match. */ - def atSignature(sig: Signature, site: Type = NoPrefix, relaxed: Boolean = false)(implicit ctx: Context): SingleDenotation + def atSignature(sig: Signature, site: Type = NoPrefix, relaxed: Boolean = false)(implicit ctx: Context): Denotation /** The variant of this denotation that's current in the given context, or * `NotDefinedHereDenotation` if this denotation does not exist at current phase, but @@ -157,7 +157,10 @@ object Denotations { * or NoDenotation if no satisfying alternative exists. * @throws TypeError if there is at more than one alternative that satisfies `p`. */ - def suchThat(p: Symbol => Boolean): SingleDenotation + def suchThat(p: Symbol => Boolean)(implicit ctx: Context): SingleDenotation + + /** If this is a SingleDenotation, return it, otherwise throw a TypeError */ + def checkUnique(implicit ctx: Context): SingleDenotation = suchThat(alwaysTrue) /** Does this denotation have an alternative that satisfies the predicate `p`? */ def hasAltWith(p: SingleDenotation => Boolean): Boolean @@ -227,13 +230,17 @@ object Denotations { /** The alternative of this denotation that has a type matching `targetType` when seen * as a member of type `site`, `NoDenotation` if none exists. */ - def matchingDenotation(site: Type, targetType: Type)(implicit ctx: Context): SingleDenotation = - if (isOverloaded) - atSignature(targetType.signature, site, relaxed = true).matchingDenotation(site, targetType) - else if (exists && !site.memberInfo(symbol).matchesLoosely(targetType)) - NoDenotation - else - asSingleDenotation + def matchingDenotation(site: Type, targetType: Type)(implicit ctx: Context): SingleDenotation = { + def qualifies(sym: Symbol) = site.memberInfo(sym).matchesLoosely(targetType) + if (isOverloaded) { + atSignature(targetType.signature, site, relaxed = true) match { + case sd: SingleDenotation => sd.matchingDenotation(site, targetType) + case md => md.suchThat(qualifies(_)) + } + } + else if (exists && !qualifies(symbol)) NoDenotation + else asSingleDenotation + } /** Form a denotation by conjoining with denotation `that`. * @@ -420,23 +427,21 @@ object Denotations { final def validFor = denot1.validFor & denot2.validFor final def isType = false final def signature(implicit ctx: Context) = Signature.OverloadedSignature - def atSignature(sig: Signature, site: Type, relaxed: Boolean)(implicit ctx: Context): SingleDenotation = { - val atSig1 = denot1.atSignature(sig, site, relaxed) - val atSig2 = denot2.atSignature(sig, site, relaxed) - if (isDoubleDef(atSig1.symbol, atSig2.symbol)) doubleDefError(atSig1, atSig2, site) - atSig1.orElse(atSig2) - } + def atSignature(sig: Signature, site: Type, relaxed: Boolean)(implicit ctx: Context): Denotation = + derivedMultiDenotation(denot1.atSignature(sig, site, relaxed), denot2.atSignature(sig, site, relaxed)) def currentIfExists(implicit ctx: Context): Denotation = derivedMultiDenotation(denot1.currentIfExists, denot2.currentIfExists) def current(implicit ctx: Context): Denotation = derivedMultiDenotation(denot1.current, denot2.current) def altsWith(p: Symbol => Boolean): List[SingleDenotation] = denot1.altsWith(p) ++ denot2.altsWith(p) - def suchThat(p: Symbol => Boolean): SingleDenotation = { + def suchThat(p: Symbol => Boolean)(implicit ctx: Context): SingleDenotation = { val sd1 = denot1.suchThat(p) val sd2 = denot2.suchThat(p) if (sd1.exists) - if (sd2.exists) throw new TypeError(s"failure to disambiguate overloaded reference $this") + if (sd2.exists) + if (isDoubleDef(denot1.symbol, denot2.symbol)) doubleDefError(denot1, denot2) + else throw new TypeError(s"failure to disambiguate overloaded reference $this") else sd1 else sd2 } @@ -486,7 +491,7 @@ object Denotations { def altsWith(p: Symbol => Boolean): List[SingleDenotation] = if (exists && p(symbol)) this :: Nil else Nil - def suchThat(p: Symbol => Boolean): SingleDenotation = + def suchThat(p: Symbol => Boolean)(implicit ctx: Context): SingleDenotation = if (exists && p(symbol)) this else NoDenotation def hasAltWith(p: SingleDenotation => Boolean): Boolean = @@ -906,14 +911,14 @@ object Denotations { */ case class NoQualifyingRef(alts: List[SingleDenotation])(implicit ctx: Context) extends ErrorDenotation - /** A double defifinition + /** A double definition */ def isDoubleDef(sym1: Symbol, sym2: Symbol)(implicit ctx: Context): Boolean = (sym1.exists && sym2.exists && (sym1 ne sym2) && (sym1.owner eq sym2.owner) && !sym1.is(Bridge) && !sym2.is(Bridge)) - def doubleDefError(denot1: SingleDenotation, denot2: SingleDenotation, pre: Type)(implicit ctx: Context): Unit = { + def doubleDefError(denot1: Denotation, denot2: Denotation, pre: Type = NoPrefix)(implicit ctx: Context): Nothing = { val sym1 = denot1.symbol val sym2 = denot2.symbol def fromWhere = if (pre == NoPrefix) "" else i"\nwhen seen as members of $pre" @@ -927,7 +932,6 @@ object Denotations { denot2.info, denot2.info) } - // --------------- PreDenotations ------------------------------------------------- /** A PreDenotation represents a group of single denotations diff --git a/src/dotty/tools/dotc/core/Types.scala b/src/dotty/tools/dotc/core/Types.scala index 8ef0e9fd140a..3ad43a05e519 100644 --- a/src/dotty/tools/dotc/core/Types.scala +++ b/src/dotty/tools/dotc/core/Types.scala @@ -1437,7 +1437,7 @@ object Types { asMemberOf(prefix) match { case NoDenotation => d.current case newd: SingleDenotation => newd - case newd => newd.atSignature(d.signature).orElse(d.current) + case newd => newd.atSignature(d.signature).checkUnique.orElse(d.current) } private def denotOfSym(sym: Symbol)(implicit ctx: Context): Denotation = { @@ -1729,7 +1729,7 @@ object Types { override def loadDenot(implicit ctx: Context): Denotation = { val d = super.loadDenot if (sig eq Signature.OverloadedSignature) d - else d.atSignature(sig) + else d.atSignature(sig).checkUnique } override def newLikeThis(prefix: Type)(implicit ctx: Context): TermRef = { diff --git a/src/dotty/tools/dotc/transform/Splitter.scala b/src/dotty/tools/dotc/transform/Splitter.scala index 410b412e05d9..efcf95ede016 100644 --- a/src/dotty/tools/dotc/transform/Splitter.scala +++ b/src/dotty/tools/dotc/transform/Splitter.scala @@ -46,7 +46,7 @@ class Splitter extends MiniPhaseTransform { thisTransform => val mbr = tp.member(name) if (!mbr.isOverloaded) mbr.asSingleDenotation else tree.tpe match { - case tref: TermRefWithSignature => mbr.atSignature(tref.sig) + case tref: TermRefWithSignature => mbr.atSignature(tref.sig).checkUnique case _ => def alts = mbr.alternatives.map(alt => i"$alt: ${alt.info}").mkString(", ") ctx.error(s"cannot disambiguate overloaded members $alts", tree.pos) From 3a97b3fb546bc57afb8cbfadb27eb9e66b0842d1 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 3 May 2016 11:00:29 +0200 Subject: [PATCH 10/16] Another test case involving super accessors --- tests/neg/i1240b.scala | 17 +++++++++++++++++ tests/run/i1240.scala | 4 ++-- 2 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 tests/neg/i1240b.scala diff --git a/tests/neg/i1240b.scala b/tests/neg/i1240b.scala new file mode 100644 index 000000000000..6987dbc96d06 --- /dev/null +++ b/tests/neg/i1240b.scala @@ -0,0 +1,17 @@ +// yet another variant, testing super accessors + +trait T { + def foo[B](x: C[B]): C[B] +} +abstract class A extends T { + type C[X] + def foo[B](x: C[B]): C[B] = {println("A.C"); x} + def foo[B](x: List[B]): List[B] = {println("A.List"); x} +} +trait U extends T { + def foo[B](x: C[B]): C[B] = super.foo[B](x) +} +object Test extends A with U { + type C[X] = List[X] + def main(args: Array[String]) = foo(List("")) +} diff --git a/tests/run/i1240.scala b/tests/run/i1240.scala index 860806ba5705..7092d91314e4 100644 --- a/tests/run/i1240.scala +++ b/tests/run/i1240.scala @@ -1,6 +1,6 @@ // A tricky case of overriding behavior -// Note: It would be acceptable if this produced an error instead. -// Bit testing this is tricky. +// Note: It might be acceptable if this produced an error instead. +// But testing this is tricky. abstract class Base[T] { def foo(x: T): String } From b26b725ef25a55a325b2c8a44c1ac67272632f14 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 3 May 2016 11:23:24 +0200 Subject: [PATCH 11/16] Handle MergeErrors in RefChecks Used to throw an uncaught merge error in checkAllOverrides when compiling i1240c.scala. --- src/dotty/tools/dotc/typer/RefChecks.scala | 6 +++++- tests/neg/i1240b.scala | 23 +++++++++------------- tests/neg/i1240c.scala | 17 ++++++++++++++++ 3 files changed, 31 insertions(+), 15 deletions(-) create mode 100644 tests/neg/i1240c.scala diff --git a/src/dotty/tools/dotc/typer/RefChecks.scala b/src/dotty/tools/dotc/typer/RefChecks.scala index 37c2fe48f90a..a654bb08f734 100644 --- a/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/src/dotty/tools/dotc/typer/RefChecks.scala @@ -837,7 +837,7 @@ class RefChecks extends MiniPhase { thisTransformer => if (tree.symbol is Macro) EmptyTree else tree } - override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo) = { + override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo) = try { val cls = ctx.owner checkOverloadedRestrictions(cls) checkParents(cls) @@ -845,6 +845,10 @@ class RefChecks extends MiniPhase { thisTransformer => checkAllOverrides(cls) checkDerivedValueClass(cls, tree.body) tree + } catch { + case ex: MergeError => + ctx.error(ex.getMessage, tree.pos) + tree } override def transformTypeTree(tree: TypeTree)(implicit ctx: Context, info: TransformerInfo) = { diff --git a/tests/neg/i1240b.scala b/tests/neg/i1240b.scala index 6987dbc96d06..2d23db61470b 100644 --- a/tests/neg/i1240b.scala +++ b/tests/neg/i1240b.scala @@ -1,17 +1,12 @@ -// yet another variant, testing super accessors - -trait T { - def foo[B](x: C[B]): C[B] +// yet another variant, testing type parameters +trait T[X] { + def foo(x: X): X } -abstract class A extends T { - type C[X] - def foo[B](x: C[B]): C[B] = {println("A.C"); x} - def foo[B](x: List[B]): List[B] = {println("A.List"); x} +abstract class A[X] extends T[X] { + def foo(x: X): X = {println("A.X"); x} + def foo(x: String): String = {println("A.String"); x} } -trait U extends T { - def foo[B](x: C[B]): C[B] = super.foo[B](x) -} -object Test extends A with U { - type C[X] = List[X] - def main(args: Array[String]) = foo(List("")) +trait U[X] extends T[X] { + abstract override def foo(x: X): X = super.foo(x) } +object Test extends A[String] with U[String] // error: accidental override diff --git a/tests/neg/i1240c.scala b/tests/neg/i1240c.scala new file mode 100644 index 000000000000..89fded2927ca --- /dev/null +++ b/tests/neg/i1240c.scala @@ -0,0 +1,17 @@ +// yet another variant, testing super accessors +// (but exhibited a crash in RefChecks). + +trait T { + def foo[B](x: C[B]): C[B] +} +abstract class A extends T { + type C[X] + def foo[B](x: C[B]): C[B] = {println("A.C"); x} + def foo[B](x: List[B]): List[B] = {println("A.List"); x} +} +trait U extends T { + abstract override def foo[B](x: C[B]): C[B] = super.foo[B](x) +} +object Test extends A with U { + type C[X] = List[X] +} From c9ac3d7267fe53dfbd3f57658049637ab218b3d5 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 16 May 2016 18:47:39 +0200 Subject: [PATCH 12/16] Remove stray test Real test is in neg/customargs --- tests/neg/i1240c.scala | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100644 tests/neg/i1240c.scala diff --git a/tests/neg/i1240c.scala b/tests/neg/i1240c.scala deleted file mode 100644 index 89fded2927ca..000000000000 --- a/tests/neg/i1240c.scala +++ /dev/null @@ -1,17 +0,0 @@ -// yet another variant, testing super accessors -// (but exhibited a crash in RefChecks). - -trait T { - def foo[B](x: C[B]): C[B] -} -abstract class A extends T { - type C[X] - def foo[B](x: C[B]): C[B] = {println("A.C"); x} - def foo[B](x: List[B]): List[B] = {println("A.List"); x} -} -trait U extends T { - abstract override def foo[B](x: C[B]): C[B] = super.foo[B](x) -} -object Test extends A with U { - type C[X] = List[X] -} From c29e9754b94cc352337791c9e36131f5b8be385d Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 19 May 2016 11:32:44 +0200 Subject: [PATCH 13/16] Fix dotc bootstrap failure During an attempted dotty bootstrap it was noted that Types.scala did not compile anymore, because `checkUnique` threw a `TypeError` during erasure. The issue was an overloaded member `name` in TermrefWithSig. In NamedType: def name: Name In TermRef: def name: TermName Before erasure, there's one member `name`, after erasure there are two (because after erasure result type counts). The error arose when trying to recompute a member of a `TermRefWithSig` where the name is `name` and the expected signature is `(Nil, ?)`. Since there are two members that match the name and the signature, `checkUnique` triggered a `TypeError`. Before adding `checkUnique`, the previous `atSignature` call would just have returned an arbitrary choice among the two alternative definitions of `name`. The fix is not to use `checkUnique` but to fall back to `d.current` in the case where several alternatives appear. Interestingly, the failure only triggers when -Ycheck options are *disabled*. I added a new test that compiles Types.scala without checks, so we catch this and possibly similar bugs in the future. --- src/dotty/tools/dotc/core/Types.scala | 6 +++++- test/dotc/tests.scala | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/core/Types.scala b/src/dotty/tools/dotc/core/Types.scala index 3ad43a05e519..114e6c9082c4 100644 --- a/src/dotty/tools/dotc/core/Types.scala +++ b/src/dotty/tools/dotc/core/Types.scala @@ -1437,7 +1437,11 @@ object Types { asMemberOf(prefix) match { case NoDenotation => d.current case newd: SingleDenotation => newd - case newd => newd.atSignature(d.signature).checkUnique.orElse(d.current) + case newd => + newd.atSignature(d.signature) match { + case newd1: SingleDenotation if newd1.exists => newd1 + case _ => d.current + } } private def denotOfSym(sym: Symbol)(implicit ctx: Context): Denotation = { diff --git a/test/dotc/tests.scala b/test/dotc/tests.scala index 97c872bb6eb2..de35d281b28c 100644 --- a/test/dotc/tests.scala +++ b/test/dotc/tests.scala @@ -157,6 +157,7 @@ class tests extends CompilerTest { @Test def dotc_ast = compileDir(dotcDir, "ast") @Test def dotc_config = compileDir(dotcDir, "config") @Test def dotc_core = compileDir(dotcDir, "core")("-Yno-double-bindings" :: allowDeepSubtypes)// twice omitted to make tests run faster + @Test def dotc_core_Types = compileFile(dotcDir, "core/Types")(noCheckOptions) // This directory doesn't exist anymore // @Test def dotc_core_pickling = compileDir(coreDir, "pickling")(allowDeepSubtypes)// twice omitted to make tests run faster From f1d95c286c802504b00b469bc924a39760fc9e73 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 19 May 2016 17:14:11 +0200 Subject: [PATCH 14/16] Fix test The previous additional test messed up partest in that file Types.scala was copied twice into the partest-generated directory and then the pos/core tests would compile both copies. This gave a double definition which manifested itself under -Yno-double-bindings as an assertion error. Ideally, partest generation would guard against this situation. For now I avoid the problem by compiling the whole of core without -Ycheck, not jst Types.scala. --- test/dotc/tests.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/dotc/tests.scala b/test/dotc/tests.scala index de35d281b28c..3528dfa72edf 100644 --- a/test/dotc/tests.scala +++ b/test/dotc/tests.scala @@ -156,8 +156,8 @@ class tests extends CompilerTest { @Test def dotc_ast = compileDir(dotcDir, "ast") @Test def dotc_config = compileDir(dotcDir, "config") - @Test def dotc_core = compileDir(dotcDir, "core")("-Yno-double-bindings" :: allowDeepSubtypes)// twice omitted to make tests run faster - @Test def dotc_core_Types = compileFile(dotcDir, "core/Types")(noCheckOptions) + @Test def dotc_core = compileDir(dotcDir, "core")(allowDeepSubtypes)// twice omitted to make tests run faster + @Test def dotc_core_nocheck = compileDir(dotcDir, "core")(noCheckOptions) // This directory doesn't exist anymore // @Test def dotc_core_pickling = compileDir(coreDir, "pickling")(allowDeepSubtypes)// twice omitted to make tests run faster From c87c0303e40ef7e0103b2606c744bc7a2b61ece4 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 19 May 2016 17:15:18 +0200 Subject: [PATCH 15/16] Better doc comment --- src/dotty/annotation/internal/Child.scala | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/dotty/annotation/internal/Child.scala b/src/dotty/annotation/internal/Child.scala index 23ff2a97c00c..4f3ceb6c09c6 100644 --- a/src/dotty/annotation/internal/Child.scala +++ b/src/dotty/annotation/internal/Child.scala @@ -2,5 +2,14 @@ package dotty.annotation.internal import scala.annotation.Annotation -/** An annotation to indicate a child class or object of the annotated class. */ +/** An annotation to indicate a child class or object of the annotated class. + * E.g. if we have + * + * sealed class A + * case class B() extends A + * case class C() extends A + * + * Then `A` would carry the annotations `@Child[B] @Child[C]` where + * `B`, `C` are TypeRefs. + */ class Child[T] extends Annotation From 77642b925cdea436755ce6e26e794e48c37b3b1a Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 19 May 2016 17:15:51 +0200 Subject: [PATCH 16/16] Two more tests Unrelated to other commits but useful to get in. --- tests/pos/chan.scala | 20 ++++++++++++++++++++ tests/run/t7685-class-simple.scala | 10 ++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/pos/chan.scala create mode 100644 tests/run/t7685-class-simple.scala diff --git a/tests/pos/chan.scala b/tests/pos/chan.scala new file mode 100644 index 000000000000..ea8eb2b547cb --- /dev/null +++ b/tests/pos/chan.scala @@ -0,0 +1,20 @@ +trait Comparator { + type T // abstract type member, to be filled in by concrete classes + def ordering: Ordering[T] + def compare(a: T, b: T): Int = ordering.compare(a, b) +} + +object IntComparator extends Comparator { + type T = Int + def ordering: Ordering[Int] = Ordering.Int +} +object Test { + def process(c: Comparator)(items: Seq[c.T]): Int = { + c.compare(items(0), items(1)) + } +} +class Processor[K](c: Comparator { type T = K }) { + def process(items: Seq[K]): Int = { + c.compare(items(0), items(1)) + } +} diff --git a/tests/run/t7685-class-simple.scala b/tests/run/t7685-class-simple.scala new file mode 100644 index 000000000000..5147a66c0543 --- /dev/null +++ b/tests/run/t7685-class-simple.scala @@ -0,0 +1,10 @@ +object Test { + final class Foo(val x: String) extends AnyVal { override def toString = "" + x } + final class Bar(val f: Foo) extends AnyVal { override def toString = "" + f } + def main(args: Array[String]) = { + val x = "abc" + val f = new Foo(x) + val b = new Bar(f) + assert(b.toString == "abc") + } +}