From fa6a25792254404b659dd8ce2b949447187ef813 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 29 Jun 2021 23:34:10 +0200 Subject: [PATCH 1/8] Refine overriding pairs in RefChecks We now exclude a pair of overriding / overridden symbols that have a common subclass parent of the base class only under the additional condition that their signatures also match. #12828 shows a case where this matters: ```scala trait Foo[A]: def foo(x: A): Unit trait Bar[A] extends Foo[A]: def foo(x: A & String): Unit = println(x.toUpperCase) object Baz extends Bar[Int] // error overriding foo: incompatible type @main def run() = Baz.foo(42) ``` When checking `Baz`, there is a common subclass parent (namely `Bar`), but the signatures of the two `foo` definitions as seen from `Bar` are different, so we cannot assume that the pair has already been checked. Fixes #12828 --- .../dotc/transform/OverridingPairs.scala | 54 ++++++++++--------- .../dotty/tools/dotc/typer/RefChecks.scala | 41 ++++++++------ tests/neg/i12828.check | 7 +++ tests/neg/i12828.scala | 9 ++++ 4 files changed, 70 insertions(+), 41 deletions(-) create mode 100644 tests/neg/i12828.check create mode 100644 tests/neg/i12828.scala diff --git a/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala b/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala index b2851fd92619..acd4285e85f2 100644 --- a/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala @@ -1,8 +1,9 @@ -package dotty.tools.dotc +package dotty.tools +package dotc package transform import core._ -import Flags._, Symbols._, Contexts._, Scopes._, Decorators._ +import Flags._, Symbols._, Contexts._, Scopes._, Decorators._, Types.Type import collection.mutable import collection.immutable.BitSet import scala.annotation.tailrec @@ -35,9 +36,9 @@ object OverridingPairs { */ protected def parents: Array[Symbol] = base.info.parents.toArray.map(_.typeSymbol) - /** Does `sym1` match `sym2` so that it qualifies as overriding. - * Types always match. Term symbols match if their membertypes - * relative to .this do + /** Does `sym1` match `sym2` so that it qualifies as overriding when both symbols are + * seen as members of `self`? Types always match. Term symbols match if their membertypes + * relative to `self` do. */ protected def matches(sym1: Symbol, sym2: Symbol): Boolean = sym1.isType || sym1.asSeenFrom(self).matches(sym2.asSeenFrom(self)) @@ -85,11 +86,22 @@ object OverridingPairs { then bits += i subParents(bc) = bits - private def hasCommonParentAsSubclass(cls1: Symbol, cls2: Symbol): Boolean = - (subParents(cls1) intersect subParents(cls2)).nonEmpty + /** Is the override of `sym1` and `sym2` already handled when checking + * a parent of `self`? + */ + private def isHandledByParent(sym1: Symbol, sym2: Symbol): Boolean = + val commonParents = subParents(sym1.owner).intersect(subParents(sym2.owner)) + commonParents.nonEmpty + && commonParents.exists(i => canBeHandledByParent(sym1, sym2, parents(i).thisType)) + + /** Can pair `sym1`/`sym2` be handled by parent `parentType` which is a common subtype + * of both symbol's owners? Assumed to be true by default, but overridden in RefChecks. + */ + protected def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parentType: Type): Boolean = + true /** The scope entries that have already been visited as overridden - * (maybe excluded because of hasCommonParentAsSubclass). + * (maybe excluded because of already handled by a parent). * These will not appear as overriding */ private val visited = util.HashSet[Symbol]() @@ -134,28 +146,22 @@ object OverridingPairs { * overridden = overridden member of the pair, provided hasNext is true */ @tailrec final def next(): Unit = - if (nextEntry ne null) { + if nextEntry != null then nextEntry = decls.lookupNextEntry(nextEntry) - if (nextEntry ne null) - try { + if nextEntry != null then + try overridden = nextEntry.sym - if (overriding.owner != overridden.owner && matches(overriding, overridden)) { + if overriding.owner != overridden.owner && matches(overriding, overridden) then visited += overridden - if (!hasCommonParentAsSubclass(overriding.owner, overridden.owner)) return - } - } - catch { - case ex: TypeError => - // See neg/i1750a for an example where a cyclic error can arise. - // The root cause in this example is an illegal "override" of an inner trait - report.error(ex, base.srcPos) - } - else { + if !isHandledByParent(overriding, overridden) then return + catch case ex: TypeError => + // See neg/i1750a for an example where a cyclic error can arise. + // The root cause in this example is an illegal "override" of an inner trait + report.error(ex, base.srcPos) + else curEntry = curEntry.prev nextOverriding() - } next() - } nextOverriding() next() diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index 976607c79f1e..baf425f5aa63 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -370,10 +370,13 @@ object RefChecks { //Console.println(infoString(member) + " shadows1 " + infoString(other) " in " + clazz);//DEBUG return val parentSymbols = clazz.info.parents.map(_.typeSymbol) - if (parentSymbols exists (p => subOther(p) && subMember(p) && deferredCheck)) + def matchIn(parent: Symbol): Boolean = considerMatching(member, other, parent.thisType) + if parentSymbols.exists(p => + subOther(p) && subMember(p) && deferredCheck && matchIn(p)) + then //Console.println(infoString(member) + " shadows2 " + infoString(other) + " in " + clazz);//DEBUG return - if (parentSymbols forall (p => subOther(p) == subMember(p))) + if parentSymbols.forall(p => subOther(p) == subMember(p) && matchIn(p)) then //Console.println(infoString(member) + " shadows " + infoString(other) + " in " + clazz);//DEBUG return } @@ -496,26 +499,30 @@ object RefChecks { }*/ } - val opc = new OverridingPairs.Cursor(clazz): - - /** We declare a match if either we have a full match including matching names - * or we have a loose match with different target name but the types are the same. - * This leaves two possible sorts of discrepancies to be reported as errors - * in `checkOveride`: - * - * - matching names, target names, and signatures but different types - * - matching names and types, but different target names - */ - override def matches(sym1: Symbol, sym2: Symbol): Boolean = - !(sym1.owner.is(JavaDefined, butNot = Trait) && sym2.owner.is(JavaDefined, butNot = Trait)) && // javac already handles these checks + /** We declare a match if either we have a full match including matching names + * or we have a loose match with different target name but the types are the same. + * This leaves two possible sorts of discrepancies to be reported as errors + * in `checkOveride`: + * + * - matching names, target names, and signatures but different types + * - matching names and types, but different target names + */ + def considerMatching(sym1: Symbol, sym2: Symbol, self: Type): Boolean = + !(sym1.owner.is(JavaDefined, butNot = Trait) && sym2.owner.is(JavaDefined, butNot = Trait)) && // javac already handles these checks (sym1.isType || { - val sd1 = sym1.asSeenFrom(clazz.thisType) - val sd2 = sym2.asSeenFrom(clazz.thisType) + val sd1 = sym1.asSeenFrom(self) + val sd2 = sym2.asSeenFrom(self) sd1.matchesLoosely(sd2) && (sym1.hasTargetName(sym2.targetName) || compatibleTypes(sym1, sd1.info, sym2, sd2.info)) }) - end opc + + val opc = new OverridingPairs.Cursor(clazz): + override def matches(sym1: Symbol, sym2: Symbol): Boolean = + considerMatching(sym1, sym2, self) + override def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parentType: Type): Boolean = + considerMatching(sym1, sym2, parentType) + .showing(i"already handled ${sym1.showLocated}: ${sym1.asSeenFrom(parentType).signature}, ${sym2.showLocated}: ${sym2.asSeenFrom(parentType).signature} = $result", refcheck) while opc.hasNext do checkOverride(opc.overriding, opc.overridden) diff --git a/tests/neg/i12828.check b/tests/neg/i12828.check new file mode 100644 index 000000000000..5278ceb5d16b --- /dev/null +++ b/tests/neg/i12828.check @@ -0,0 +1,7 @@ +-- [E163] Declaration Error: tests/neg/i12828.scala:7:7 ---------------------------------------------------------------- +7 |object Baz extends Bar[Int] // error overriding foo: incompatible type + | ^ + | error overriding method foo in trait Foo of type (x: Int): Unit; + | method foo in trait Bar of type (x: Int & String): Unit has incompatible type + +longer explanation available when compiling with `-explain` diff --git a/tests/neg/i12828.scala b/tests/neg/i12828.scala new file mode 100644 index 000000000000..b2cf5b698ab3 --- /dev/null +++ b/tests/neg/i12828.scala @@ -0,0 +1,9 @@ +trait Foo[A]: + def foo(x: A): Unit + +trait Bar[A] extends Foo[A]: + def foo(x: A & String): Unit = println(x.toUpperCase) + +object Baz extends Bar[Int] // error overriding foo: incompatible type + +@main def run() = Baz.foo(42) From e008f6e251049bef4a7dfdd85950b9f8d33c08e9 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 30 Jun 2021 11:29:06 +0200 Subject: [PATCH 2/8] Remove 2 out of 3 conditions that should be redundant --- .../dotty/tools/dotc/typer/RefChecks.scala | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index baf425f5aa63..5c89e5da039f 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -367,18 +367,22 @@ object RefChecks { def subMember(s: Symbol) = s derivesFrom member.owner if (subOther(member.owner) && deferredCheck) + //println(i"skip 1 ${member.showLocated}, ${other.showLocated}") //Console.println(infoString(member) + " shadows1 " + infoString(other) " in " + clazz);//DEBUG return + /* val parentSymbols = clazz.info.parents.map(_.typeSymbol) def matchIn(parent: Symbol): Boolean = considerMatching(member, other, parent.thisType) if parentSymbols.exists(p => subOther(p) && subMember(p) && deferredCheck && matchIn(p)) then + println(i"skip 2 ${member.showLocated}, ${other.showLocated}") //Console.println(infoString(member) + " shadows2 " + infoString(other) + " in " + clazz);//DEBUG return if parentSymbols.forall(p => subOther(p) == subMember(p) && matchIn(p)) then + println(i"skip 3 ${member.showLocated}, ${other.showLocated}") //Console.println(infoString(member) + " shadows " + infoString(other) + " in " + clazz);//DEBUG - return + return*/ } /* Is the intersection between given two lists of overridden symbols empty? */ @@ -508,21 +512,27 @@ object RefChecks { * - matching names and types, but different target names */ def considerMatching(sym1: Symbol, sym2: Symbol, self: Type): Boolean = - !(sym1.owner.is(JavaDefined, butNot = Trait) && sym2.owner.is(JavaDefined, butNot = Trait)) && // javac already handles these checks - (sym1.isType || { - val sd1 = sym1.asSeenFrom(self) - val sd2 = sym2.asSeenFrom(self) - sd1.matchesLoosely(sd2) + if sym1.owner.is(JavaDefined, butNot = Trait) + && sym2.owner.is(JavaDefined, butNot = Trait) + then false // javac already handles these checks + else if sym1.isType then true + else + val sd1 = sym1.asSeenFrom(self) + val sd2 = sym2.asSeenFrom(self) + sd1.matchesLoosely(sd2) && (sym1.hasTargetName(sym2.targetName) || compatibleTypes(sym1, sd1.info, sym2, sd2.info)) - }) val opc = new OverridingPairs.Cursor(clazz): override def matches(sym1: Symbol, sym2: Symbol): Boolean = considerMatching(sym1, sym2, self) + + // We can exclude pairs safely from checking only of they also matched in + // the parent class. See neg/i12828.scala for an example where this matters. override def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parentType: Type): Boolean = considerMatching(sym1, sym2, parentType) .showing(i"already handled ${sym1.showLocated}: ${sym1.asSeenFrom(parentType).signature}, ${sym2.showLocated}: ${sym2.asSeenFrom(parentType).signature} = $result", refcheck) + end opc while opc.hasNext do checkOverride(opc.overriding, opc.overridden) @@ -535,13 +545,11 @@ object RefChecks { // // class A { type T = B } // class B extends A { override type T } - for - dcl <- clazz.info.decls.iterator - if dcl.is(Deferred) - other <- dcl.allOverriddenSymbols - if !other.is(Deferred) - do - checkOverride(dcl, other) + for dcl <- clazz.info.decls.iterator do + if dcl.is(Deferred) then + for other <- dcl.allOverriddenSymbols do + if !other.is(Deferred) then + checkOverride(dcl, other) printMixinOverrideErrors() From 8138eb4ed6f957029b25a1f1315e88c429cc17d2 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 30 Jun 2021 16:36:43 +0200 Subject: [PATCH 3/8] RefChecks cleanuo Remove illogical conditions that suppressed certain checks. Replace them by a separate condition that certain override checks are suppressed if the types of the members do not match (i.e. it's just the signatures that match but not the types). In that case, we have arguably overshot with declaring an override. Bridge generation is also suppressed in this case. Scala 2 behaves effectively the same way, by never taking signatures into account. --- .../dotty/tools/dotc/transform/Bridges.scala | 10 ++++- .../dotty/tools/dotc/typer/RefChecks.scala | 43 ++++++------------- tests/neg/i12828c.scala | 12 ++++++ tests/neg/i12828d.scala | 18 ++++++++ tests/neg/varargs-annot-2.scala | 9 ++++ tests/neg/varargs-annot.scala | 2 +- tests/run/i12828a.scala | 12 ++++++ tests/run/i12828b.scala | 12 ++++++ 8 files changed, 86 insertions(+), 32 deletions(-) create mode 100644 tests/neg/i12828c.scala create mode 100644 tests/neg/i12828d.scala create mode 100644 tests/neg/varargs-annot-2.scala create mode 100644 tests/run/i12828a.scala create mode 100644 tests/run/i12828b.scala diff --git a/compiler/src/dotty/tools/dotc/transform/Bridges.scala b/compiler/src/dotty/tools/dotc/transform/Bridges.scala index 8cb78c258704..cc4147d7d622 100644 --- a/compiler/src/dotty/tools/dotc/transform/Bridges.scala +++ b/compiler/src/dotty/tools/dotc/transform/Bridges.scala @@ -36,7 +36,7 @@ class Bridges(root: ClassSymbol, thisPhase: DenotTransformer)(using Context) { !sym.isOneOf(MethodOrModule) || super.exclude(sym) } - //val site = root.thisType + val site = root.thisType private var toBeRemoved = immutable.Set[Symbol]() private val bridges = mutable.ListBuffer[Tree]() @@ -77,7 +77,13 @@ class Bridges(root: ClassSymbol, thisPhase: DenotTransformer)(using Context) { |clashes with definition of the member itself; both have erased type ${info(member)(using elimErasedCtx)}."""", bridgePosFor(member)) } - else if (!bridgeExists) + else if !inContext(preErasureCtx)(site.memberInfo(member).matches(site.memberInfo(other))) then + // Neither symbol signatures nor pre-erasure types seen from root match; this means + // according to Scala 2 semantics there is no override. + // A bridge might introduce a classcast exception. + // Example where this was observed: run/i12828a.scala and MapView in stdlib213 + report.log(i"suppress bridge in $root for ${member} in ${member.owner} and ${other.showLocated} since member infos ${site.memberInfo(member)} and ${site.memberInfo(other)} do not match") + else if !bridgeExists then addBridge(member, other) } diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index 5c89e5da039f..b700e4dfff59 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -323,6 +323,13 @@ object RefChecks { overrideErrorMsg("no longer has compatible type"), (if (member.owner == clazz) member else clazz).srcPos)) + /** Do types of `member` and `other` as seen from `self` match? + * If not we treat them as not a real override and don't issue certain + * error messages. Also, bridges are not generated in this case. + */ + def trueMatch: Boolean = + memberTp(self).matches(otherTp(self)) + def emitOverrideError(fullmsg: Message) = if (!(hasErrors && member.is(Synthetic) && member.is(Module))) { // suppress errors relating toi synthetic companion objects if other override @@ -360,31 +367,6 @@ object RefChecks { //Console.println(infoString(member) + " overrides " + infoString(other) + " in " + clazz);//DEBUG - // return if we already checked this combination elsewhere - if (member.owner != clazz) { - def deferredCheck = member.is(Deferred) || !other.is(Deferred) - def subOther(s: Symbol) = s derivesFrom other.owner - def subMember(s: Symbol) = s derivesFrom member.owner - - if (subOther(member.owner) && deferredCheck) - //println(i"skip 1 ${member.showLocated}, ${other.showLocated}") - //Console.println(infoString(member) + " shadows1 " + infoString(other) " in " + clazz);//DEBUG - return - /* - val parentSymbols = clazz.info.parents.map(_.typeSymbol) - def matchIn(parent: Symbol): Boolean = considerMatching(member, other, parent.thisType) - if parentSymbols.exists(p => - subOther(p) && subMember(p) && deferredCheck && matchIn(p)) - then - println(i"skip 2 ${member.showLocated}, ${other.showLocated}") - //Console.println(infoString(member) + " shadows2 " + infoString(other) + " in " + clazz);//DEBUG - return - if parentSymbols.forall(p => subOther(p) == subMember(p) && matchIn(p)) then - println(i"skip 3 ${member.showLocated}, ${other.showLocated}") - //Console.println(infoString(member) + " shadows " + infoString(other) + " in " + clazz);//DEBUG - return*/ - } - /* Is the intersection between given two lists of overridden symbols empty? */ def intersectionIsEmpty(syms1: Iterator[Symbol], syms2: Iterator[Symbol]) = { val set2 = syms2.toSet @@ -419,7 +401,7 @@ object RefChecks { overrideError("cannot be used here - class definitions cannot be overridden") else if (!other.is(Deferred) && member.isClass) overrideError("cannot be used here - classes can only override abstract types") - else if (other.isEffectivelyFinal) // (1.2) + else if other.isEffectivelyFinal && trueMatch then // (1.2) overrideError(i"cannot override final member ${other.showLocated}") else if (member.is(ExtensionMethod) && !other.is(ExtensionMethod)) // (1.3) overrideError("is an extension method, cannot override a normal method") @@ -440,15 +422,18 @@ object RefChecks { member.setFlag(Override) else if (member.isType && self.memberInfo(member) =:= self.memberInfo(other)) () // OK, don't complain about type aliases which are equal - else if (member.owner != clazz && other.owner != clazz && - !(other.owner derivesFrom member.owner)) + else if member.owner != clazz + && other.owner != clazz + && !other.owner.derivesFrom(member.owner) + && trueMatch + then emitOverrideError( s"$clazz inherits conflicting members:\n " + infoStringWithLocation(other) + " and\n " + infoStringWithLocation(member) + "\n(Note: this can be resolved by declaring an override in " + clazz + ".)") else if member.is(Exported) then overrideError("cannot override since it comes from an export") - else + else if trueMatch then overrideError("needs `override` modifier") else if (other.is(AbsOverride) && other.isIncompleteIn(clazz) && !member.is(AbsOverride)) overrideError("needs `abstract override` modifiers") diff --git a/tests/neg/i12828c.scala b/tests/neg/i12828c.scala new file mode 100644 index 000000000000..4db5d8462017 --- /dev/null +++ b/tests/neg/i12828c.scala @@ -0,0 +1,12 @@ +abstract class Foo[A] { + def foo(x: A): Unit +} +abstract class Bar[A] extends Foo[A] { + def foo(x: A with String): Unit = println(x.toUpperCase) +} +object Baz extends Bar[Int] // error overriding foo: incompatible type + // Scala 2 gives: object creation impossible. Missing implementation for `foo` + +object Test { + def main(args: Array[String]) = Baz.foo(42) +} diff --git a/tests/neg/i12828d.scala b/tests/neg/i12828d.scala new file mode 100644 index 000000000000..45a95501835d --- /dev/null +++ b/tests/neg/i12828d.scala @@ -0,0 +1,18 @@ +trait A[X] { + def foo(x: X): Unit = + println("A.foo") +} +trait B[X] extends A[X] { + def foo(x: Int): Unit = + println("B.foo") +} +object C extends B[Int] // error: conflicting members + // Scala 2: same + +object Test { + def main(args: Array[String]) = { + C.foo(1) + val a: A[Int] = C + a.foo(1) + } +} \ No newline at end of file diff --git a/tests/neg/varargs-annot-2.scala b/tests/neg/varargs-annot-2.scala new file mode 100644 index 000000000000..7b0fce1124ea --- /dev/null +++ b/tests/neg/varargs-annot-2.scala @@ -0,0 +1,9 @@ +import annotation.varargs + +trait C { + @varargs def v(i: Int*) = () +} + +class D extends C { // error: name clash between defined and inherited member + def v(i: Array[Int]) = () +} \ No newline at end of file diff --git a/tests/neg/varargs-annot.scala b/tests/neg/varargs-annot.scala index 490d2ab93695..d24b9d350d78 100644 --- a/tests/neg/varargs-annot.scala +++ b/tests/neg/varargs-annot.scala @@ -17,7 +17,7 @@ object Test { class D extends C { override def v(i: Int*) = () // error - def v(i: Array[Int]) = () // error + def v(i: Array[Int]) = () // ok, reported when used alone (see varargs-annot-2.scala) } @varargs def nov(a: Int) = 0 // error: A method without repeated parameters cannot be annotated with @varargs diff --git a/tests/run/i12828a.scala b/tests/run/i12828a.scala new file mode 100644 index 000000000000..459af156980c --- /dev/null +++ b/tests/run/i12828a.scala @@ -0,0 +1,12 @@ +trait Foo[A] { + def foo(x: A): Unit = () +} +trait Bar[A] extends Foo[A] { + def foo(x: A with String): Unit = println(x.toUpperCase) +} +object Baz extends Bar[Int] // was: error: Baz inherits conflicting members, now like Scala 2 + // Scala 2 compiles and runs + +object Test { + def main(args: Array[String]) = Baz.foo(42) +} diff --git a/tests/run/i12828b.scala b/tests/run/i12828b.scala new file mode 100644 index 000000000000..d1cd3e087a36 --- /dev/null +++ b/tests/run/i12828b.scala @@ -0,0 +1,12 @@ +class Foo[A] { + def foo(x: A): Unit = () +} +class Bar[A] extends Foo[A] { + def foo(x: A with String): Unit = println(x.toUpperCase) +} +object Baz extends Bar[Int] // was error: Baz inherits conflicting members, now like Scalac + // Scala 2 compiles and runs + +object Test { + def main(args: Array[String]) = Baz.foo(42) +} From 6a90e5e8a215ed597693b77c1f9f49e66a247303 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 30 Jun 2021 17:34:22 +0200 Subject: [PATCH 4/8] Systematize treatement of trueMatch Refchecks now decouples overriding from signature matching. A signature match is in essence a pre-check that two members might override, but if the two members are methods we also need to compare their parameter types. This has three consequences: 1. Before emitting an override error, test that parameter types match. If not, we have a false override and no error should be reported. 2. Before emitting a bridge, do the same test and omit the generation if the test fails 3. When checking whether a class is fully implemented, check that any implementation also agrees completely in its parameter types. --- .../src/dotty/tools/dotc/core/Denotations.scala | 4 ++-- .../src/dotty/tools/dotc/typer/RefChecks.scala | 17 +++++++++-------- tests/neg/OpaqueEscape.scala | 2 +- tests/neg/i11828.check | 8 ++++++++ tests/neg/i12828.check | 13 +++++++------ tests/neg/i12828.scala | 2 +- tests/neg/i7597.scala | 4 ++-- 7 files changed, 30 insertions(+), 20 deletions(-) create mode 100644 tests/neg/i11828.check diff --git a/compiler/src/dotty/tools/dotc/core/Denotations.scala b/compiler/src/dotty/tools/dotc/core/Denotations.scala index b8d1caf3d1b7..7bf4d17ddd69 100644 --- a/compiler/src/dotty/tools/dotc/core/Denotations.scala +++ b/compiler/src/dotty/tools/dotc/core/Denotations.scala @@ -1021,7 +1021,7 @@ object Denotations { * erasure (see i8615b, i9109b), Erasure takes care of adding any necessary * bridge to make this work at runtime. */ - def matchesLoosely(other: SingleDenotation)(using Context): Boolean = + def matchesLoosely(other: SingleDenotation, alwaysCompareParams: Boolean = false)(using Context): Boolean = if isType then true else val thisLanguage = SourceLanguage(symbol) @@ -1031,7 +1031,7 @@ object Denotations { val otherSig = other.signature(commonLanguage) sig.matchDegree(otherSig) match case FullMatch => - true + !alwaysCompareParams || info.matches(other.info) case MethodNotAMethodMatch => !ctx.erasedTypes && { // A Scala zero-parameter method and a Scala non-method always match. diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index b700e4dfff59..ae98faa09e68 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -323,12 +323,13 @@ object RefChecks { overrideErrorMsg("no longer has compatible type"), (if (member.owner == clazz) member else clazz).srcPos)) - /** Do types of `member` and `other` as seen from `self` match? + /** Do types of term members `member` and `other` as seen from `self` match? * If not we treat them as not a real override and don't issue certain * error messages. Also, bridges are not generated in this case. + * Type members are always assumed to match. */ def trueMatch: Boolean = - memberTp(self).matches(otherTp(self)) + member.isType || memberTp(self).matches(otherTp(self)) def emitOverrideError(fullmsg: Message) = if (!(hasErrors && member.is(Synthetic) && member.is(Module))) { @@ -340,7 +341,7 @@ object RefChecks { } def overrideError(msg: String, compareTypes: Boolean = false) = - if (noErrorType) + if trueMatch && noErrorType then emitOverrideError(overrideErrorMsg(msg, compareTypes)) def autoOverride(sym: Symbol) = @@ -401,7 +402,7 @@ object RefChecks { overrideError("cannot be used here - class definitions cannot be overridden") else if (!other.is(Deferred) && member.isClass) overrideError("cannot be used here - classes can only override abstract types") - else if other.isEffectivelyFinal && trueMatch then // (1.2) + else if other.isEffectivelyFinal then // (1.2) overrideError(i"cannot override final member ${other.showLocated}") else if (member.is(ExtensionMethod) && !other.is(ExtensionMethod)) // (1.3) overrideError("is an extension method, cannot override a normal method") @@ -425,15 +426,14 @@ object RefChecks { else if member.owner != clazz && other.owner != clazz && !other.owner.derivesFrom(member.owner) - && trueMatch then - emitOverrideError( + overrideError( s"$clazz inherits conflicting members:\n " + infoStringWithLocation(other) + " and\n " + infoStringWithLocation(member) + "\n(Note: this can be resolved by declaring an override in " + clazz + ".)") else if member.is(Exported) then overrideError("cannot override since it comes from an export") - else if trueMatch then + else overrideError("needs `override` modifier") else if (other.is(AbsOverride) && other.isIncompleteIn(clazz) && !member.is(AbsOverride)) overrideError("needs `abstract override` modifiers") @@ -578,7 +578,8 @@ object RefChecks { def isConcrete(sym: Symbol) = sym.exists && !sym.isOneOf(NotConcrete) clazz.nonPrivateMembersNamed(mbr.name) .filterWithPredicate( - impl => isConcrete(impl.symbol) && mbrDenot.matchesLoosely(impl)) + impl => isConcrete(impl.symbol) + && mbrDenot.matchesLoosely(impl, alwaysCompareParams = true)) .exists /** The term symbols in this class and its baseclasses that are diff --git a/tests/neg/OpaqueEscape.scala b/tests/neg/OpaqueEscape.scala index a6f701fd4e3b..1596003d4244 100644 --- a/tests/neg/OpaqueEscape.scala +++ b/tests/neg/OpaqueEscape.scala @@ -7,7 +7,7 @@ def unwrap(i:Wrapped):Int def wrap(i:Int):Wrapped } class Escaper extends EscaperBase{ // error: needs to be abstract - override def unwrap(i:Int):Int = i // error overriding method unwrap + override def unwrap(i:Int):Int = i // was error overriding method unwrap, now OK override def wrap(i:Int):Int = i // error overriding method wrap } val e = new Escaper:EscaperBase diff --git a/tests/neg/i11828.check b/tests/neg/i11828.check new file mode 100644 index 000000000000..80824e9b1334 --- /dev/null +++ b/tests/neg/i11828.check @@ -0,0 +1,8 @@ +-- Error: tests/neg/i12828.scala:7:7 ----------------------------------------------------------------------------------- +7 |object Baz extends Bar[Int] // error overriding foo: incompatible type + | ^ + | object creation impossible, since def foo(x: A): Unit in trait Foo is not defined + | (Note that + | parameter A in def foo(x: A): Unit in trait Foo does not match + | parameter Int & String in def foo(x: A & String): Unit in trait Bar + | ) diff --git a/tests/neg/i12828.check b/tests/neg/i12828.check index 5278ceb5d16b..070633fc35b3 100644 --- a/tests/neg/i12828.check +++ b/tests/neg/i12828.check @@ -1,7 +1,8 @@ --- [E163] Declaration Error: tests/neg/i12828.scala:7:7 ---------------------------------------------------------------- -7 |object Baz extends Bar[Int] // error overriding foo: incompatible type +-- Error: tests/neg/i12828.scala:7:7 ----------------------------------------------------------------------------------- +7 |object Baz extends Bar[Int] // error: not implemented | ^ - | error overriding method foo in trait Foo of type (x: Int): Unit; - | method foo in trait Bar of type (x: Int & String): Unit has incompatible type - -longer explanation available when compiling with `-explain` + | object creation impossible, since def foo(x: A): Unit in trait Foo is not defined + | (Note that + | parameter A in def foo(x: A): Unit in trait Foo does not match + | parameter Int & String in def foo(x: A & String): Unit in trait Bar + | ) diff --git a/tests/neg/i12828.scala b/tests/neg/i12828.scala index b2cf5b698ab3..d8d099b71d08 100644 --- a/tests/neg/i12828.scala +++ b/tests/neg/i12828.scala @@ -4,6 +4,6 @@ trait Foo[A]: trait Bar[A] extends Foo[A]: def foo(x: A & String): Unit = println(x.toUpperCase) -object Baz extends Bar[Int] // error overriding foo: incompatible type +object Baz extends Bar[Int] // error: not implemented @main def run() = Baz.foo(42) diff --git a/tests/neg/i7597.scala b/tests/neg/i7597.scala index 8b18b82b1db4..cc41a3c77e2d 100644 --- a/tests/neg/i7597.scala +++ b/tests/neg/i7597.scala @@ -6,8 +6,8 @@ object Test extends App { def apply(x: A): B } - class C[S <: String] extends Fn[String, Int] { - def apply(s: S): Int = 0 // error + class C[S <: String] extends Fn[String, Int] { // error + def apply(s: S): Int = 0 } foo("") From de523b3bb964353c86b9c7021617d2a88e961e30 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 30 Jun 2021 17:47:18 +0200 Subject: [PATCH 5/8] Avoid rechecking already handled pairs only if in same order --- .../tools/dotc/transform/OverridingPairs.scala | 17 +++++++++++++---- .../src/dotty/tools/dotc/typer/RefChecks.scala | 7 ++++--- tests/{run => neg}/i5094.scala | 2 +- 3 files changed, 18 insertions(+), 8 deletions(-) rename tests/{run => neg}/i5094.scala (81%) diff --git a/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala b/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala index acd4285e85f2..8887a0f83e08 100644 --- a/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala @@ -34,7 +34,7 @@ object OverridingPairs { * pair has already been treated in a parent class. * This may be refined in subclasses. @see Bridges for a use case. */ - protected def parents: Array[Symbol] = base.info.parents.toArray.map(_.typeSymbol) + protected def parents: Array[Symbol] = base.info.parents.toArray.map(_.classSymbol) /** Does `sym1` match `sym2` so that it qualifies as overriding when both symbols are * seen as members of `self`? Types always match. Term symbols match if their membertypes @@ -92,13 +92,22 @@ object OverridingPairs { private def isHandledByParent(sym1: Symbol, sym2: Symbol): Boolean = val commonParents = subParents(sym1.owner).intersect(subParents(sym2.owner)) commonParents.nonEmpty - && commonParents.exists(i => canBeHandledByParent(sym1, sym2, parents(i).thisType)) + && commonParents.exists(i => canBeHandledByParent(sym1, sym2, parents(i))) /** Can pair `sym1`/`sym2` be handled by parent `parentType` which is a common subtype * of both symbol's owners? Assumed to be true by default, but overridden in RefChecks. */ - protected def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parentType: Type): Boolean = - true + protected def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean = + val owner1 = sym1.owner + val owner2 = sym2.owner + def precedesIn(bcs: List[ClassSymbol]): Boolean = (bcs: @unchecked) match + case bc :: bcs1 => + if owner1 eq bc then true + else if owner2 eq bc then false + else precedesIn(bcs1) + case _ => + false + precedesIn(parent.asClass.baseClasses) /** The scope entries that have already been visited as overridden * (maybe excluded because of already handled by a parent). diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index ae98faa09e68..d12e24ebc811 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -514,9 +514,10 @@ object RefChecks { // We can exclude pairs safely from checking only of they also matched in // the parent class. See neg/i12828.scala for an example where this matters. - override def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parentType: Type): Boolean = - considerMatching(sym1, sym2, parentType) - .showing(i"already handled ${sym1.showLocated}: ${sym1.asSeenFrom(parentType).signature}, ${sym2.showLocated}: ${sym2.asSeenFrom(parentType).signature} = $result", refcheck) + override def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean = + considerMatching(sym1, sym2, parent.thisType) + .showing(i"already handled ${sym1.showLocated}: ${sym1.asSeenFrom(parent.thisType).signature}, ${sym2.showLocated}: ${sym2.asSeenFrom(parent.thisType).signature} = $result", refcheck) + && super.canBeHandledByParent(sym1, sym2, parent) end opc while opc.hasNext do diff --git a/tests/run/i5094.scala b/tests/neg/i5094.scala similarity index 81% rename from tests/run/i5094.scala rename to tests/neg/i5094.scala index 556a1f0f07df..755e81addf09 100644 --- a/tests/run/i5094.scala +++ b/tests/neg/i5094.scala @@ -9,7 +9,7 @@ trait SOIO extends IO { } trait SOSO extends SOIO with SO abstract class AS extends SO -class L extends AS with SOSO +class L extends AS with SOSO // error: cannot override final member object Test { def main(args: Array[String]): Unit = { new L From 8c405aca252d5a2df56a3d018ef3d5796bd5db34 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 30 Jun 2021 23:34:26 +0200 Subject: [PATCH 6/8] Apply linearization filter only to refchecks Apply linearization filter only to refchecks, not to other overriding pairs cursors. --- .../dotc/transform/OverridingPairs.scala | 11 +--------- .../dotty/tools/dotc/typer/RefChecks.scala | 21 ++++++++++++++++--- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala b/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala index 8887a0f83e08..6ccb06a2f9a6 100644 --- a/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala @@ -98,16 +98,7 @@ object OverridingPairs { * of both symbol's owners? Assumed to be true by default, but overridden in RefChecks. */ protected def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean = - val owner1 = sym1.owner - val owner2 = sym2.owner - def precedesIn(bcs: List[ClassSymbol]): Boolean = (bcs: @unchecked) match - case bc :: bcs1 => - if owner1 eq bc then true - else if owner2 eq bc then false - else precedesIn(bcs1) - case _ => - false - precedesIn(parent.asClass.baseClasses) + true /** The scope entries that have already been visited as overridden * (maybe excluded because of already handled by a parent). diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index d12e24ebc811..cc61102d26ef 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -512,12 +512,27 @@ object RefChecks { override def matches(sym1: Symbol, sym2: Symbol): Boolean = considerMatching(sym1, sym2, self) - // We can exclude pairs safely from checking only of they also matched in - // the parent class. See neg/i12828.scala for an example where this matters. + private def inLinearizationOrder(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean = + val owner1 = sym1.owner + val owner2 = sym2.owner + def precedesIn(bcs: List[ClassSymbol]): Boolean = (bcs: @unchecked) match + case bc :: bcs1 => + if owner1 eq bc then true + else if owner2 eq bc then false + else precedesIn(bcs1) + case _ => + false + precedesIn(parent.asClass.baseClasses) + + // We can exclude pairs safely from checking only under two additional conditions + // - their signatures also match in the parent class. + // See neg/i12828.scala for an example where this matters. + // - They overriding/overridden appear in linearization order. + // See neg/i5094.scala for an example where this matters. override def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean = considerMatching(sym1, sym2, parent.thisType) .showing(i"already handled ${sym1.showLocated}: ${sym1.asSeenFrom(parent.thisType).signature}, ${sym2.showLocated}: ${sym2.asSeenFrom(parent.thisType).signature} = $result", refcheck) - && super.canBeHandledByParent(sym1, sym2, parent) + && inLinearizationOrder(sym1, sym2, parent) end opc while opc.hasNext do From a78491531e058650665201e75d2cd8b81b5f126d Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 30 Jun 2021 23:51:57 +0200 Subject: [PATCH 7/8] Fix test comment --- tests/neg/i12828c.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/neg/i12828c.scala b/tests/neg/i12828c.scala index 4db5d8462017..d36e7a719984 100644 --- a/tests/neg/i12828c.scala +++ b/tests/neg/i12828c.scala @@ -4,7 +4,7 @@ abstract class Foo[A] { abstract class Bar[A] extends Foo[A] { def foo(x: A with String): Unit = println(x.toUpperCase) } -object Baz extends Bar[Int] // error overriding foo: incompatible type +object Baz extends Bar[Int] // error: not implemented (same as Scala 2) // Scala 2 gives: object creation impossible. Missing implementation for `foo` object Test { From c238f82bc159263a02e8610a049ea64cc51a8eb2 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 2 Jul 2021 16:45:03 +0200 Subject: [PATCH 8/8] Address review comments --- compiler/src/dotty/tools/dotc/core/Denotations.scala | 4 ++-- compiler/src/dotty/tools/dotc/typer/RefChecks.scala | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Denotations.scala b/compiler/src/dotty/tools/dotc/core/Denotations.scala index 7bf4d17ddd69..d97ae8ca2f6e 100644 --- a/compiler/src/dotty/tools/dotc/core/Denotations.scala +++ b/compiler/src/dotty/tools/dotc/core/Denotations.scala @@ -1021,7 +1021,7 @@ object Denotations { * erasure (see i8615b, i9109b), Erasure takes care of adding any necessary * bridge to make this work at runtime. */ - def matchesLoosely(other: SingleDenotation, alwaysCompareParams: Boolean = false)(using Context): Boolean = + def matchesLoosely(other: SingleDenotation, alwaysCompareTypes: Boolean = false)(using Context): Boolean = if isType then true else val thisLanguage = SourceLanguage(symbol) @@ -1031,7 +1031,7 @@ object Denotations { val otherSig = other.signature(commonLanguage) sig.matchDegree(otherSig) match case FullMatch => - !alwaysCompareParams || info.matches(other.info) + !alwaysCompareTypes || info.matches(other.info) case MethodNotAMethodMatch => !ctx.erasedTypes && { // A Scala zero-parameter method and a Scala non-method always match. diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index cc61102d26ef..af33ca2ff5c2 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -324,7 +324,7 @@ object RefChecks { (if (member.owner == clazz) member else clazz).srcPos)) /** Do types of term members `member` and `other` as seen from `self` match? - * If not we treat them as not a real override and don't issue certain + * If not we treat them as not a real override and don't issue override * error messages. Also, bridges are not generated in this case. * Type members are always assumed to match. */ @@ -595,7 +595,7 @@ object RefChecks { clazz.nonPrivateMembersNamed(mbr.name) .filterWithPredicate( impl => isConcrete(impl.symbol) - && mbrDenot.matchesLoosely(impl, alwaysCompareParams = true)) + && mbrDenot.matchesLoosely(impl, alwaysCompareTypes = true)) .exists /** The term symbols in this class and its baseclasses that are