From 5cebc76da6d602698eb2744f5236ed24c6572560 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Wed, 12 Dec 2018 16:19:54 +0100 Subject: [PATCH 1/4] Fix #5433: check that the implemented super-accessor is valid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems had the same bug (but at least in Dotty this is detected by -Ycheck:all), now fixed based on the logic in this PR by https://github.com/scala/scala/pull/7641. For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m. --- .../tools/dotc/transform/ResolveSuper.scala | 61 ++++++++++++++++++- tests/neg/i5433.check | 26 ++++++++ tests/neg/i5433.scala | 25 ++++++++ 3 files changed, 110 insertions(+), 2 deletions(-) create mode 100644 tests/neg/i5433.check create mode 100644 tests/neg/i5433.scala diff --git a/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala b/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala index be3661259e55..c224e3d97e4c 100644 --- a/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala +++ b/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala @@ -100,10 +100,67 @@ object ResolveSuper { val SuperAccessorName(memberName) = acc.name.unexpandedName ctx.debuglog(i"starting rebindsuper from $base of ${acc.showLocated}: ${acc.info} in $bcs, name = $memberName") while (bcs.nonEmpty && sym == NoSymbol) { - val other = bcs.head.info.nonPrivateDecl(memberName) + val cur = bcs.head + val other = cur.info.nonPrivateDecl(memberName) if (ctx.settings.Ydebug.value) ctx.log(i"rebindsuper ${bcs.head} $other deferred = ${other.symbol.is(Deferred)}") - sym = other.matchingDenotation(base.thisType, base.thisType.memberInfo(acc)).symbol + val otherMember = other.matchingDenotation(base.thisType, base.thisType.memberInfo(acc)) + if (otherMember.exists) { + sym = otherMember.symbol + // Having a matching denotation is not enough: it should also be a subtype + // of the superaccessor's type, see i5433.scala for an example where this matters + val otherTp = otherMember.asSeenFrom(base.typeRef).info + val accTp = acc.asSeenFrom(base.typeRef).info + if (!(otherTp <:< accTp)) { + // The mixin containing a super-call that requires a super-accessor + val mixin = acc.owner + // The super-call in `mixin` + val superCall = i"super.$memberName" + // The super-call that we end up trying to call + val resolvedSuperCall = i"super[${cur.name}].$memberName" + // The super-call that we would have called if `super` in traits behaved like it + // does in classes, i.e. followed the linearization of the trait itself. + val staticSuperCall = { + val staticSuper = mixin.asClass.info.parents.reverse + .find(_.nonPrivateMember(memberName).matchingDenotation(mixin.thisType, acc.info).exists) + val staticSuperName = staticSuper match { + case Some(parent) => + parent.classSymbol.name.show + case None => // Might be reachable under separate compilation + "SomeParent" + } + i"super[$staticSuperName].$memberName" + } + ctx.error( + hl"""$base cannot be defined due to a conflict between its parents when + |implementing a super-accessor for $memberName in $mixin: + | + |1. One of its parent ($mixin) contains a call $superCall in its body, + | and when a super-call in a trait is written without an explicit parent + | listed in brackets, it is implemented by a generated super-accessor in + | the class that extends this trait based on the linearization order of + | the class. + |2. Because ${cur.name} comes before ${mixin.name} in the linearization + | order of ${base.name}, and because ${cur.name} overrides $memberName, + | the super-accessor in ${base.name} is implemented as a call to + | $resolvedSuperCall. + |3. However, + | ${otherTp.widenExpr} (the type of $resolvedSuperCall in ${base.name}) + | is not a subtype of + | ${accTp.widenExpr} (the type of $memberName in $mixin). + | Hence, the super-accessor that needs to be generated in ${base.name} + | is illegal. + | + |Here are two possible ways to resolve this: + | + |1. Change the linearization order of ${base.name} such that + | ${mixin.name} comes before ${cur.name}. + |2. Alternatively, replace $superCall in the body of $mixin by a + | super-call to a specific parent, e.g. $staticSuperCall + |""".stripMargin, base.sourcePos) + } + } + bcs = bcs.tail } assert(sym.exists) diff --git a/tests/neg/i5433.check b/tests/neg/i5433.check new file mode 100644 index 000000000000..60edf7a03d8c --- /dev/null +++ b/tests/neg/i5433.check @@ -0,0 +1,26 @@ +<298..298> in i5433.scala +class Fail cannot be defined due to a conflict between its parents when +implementing a super-accessor for foo in trait C: + +1. One of its parent (trait C) contains a call super.foo in its body, + and when a super-call in a trait is written without an explicit parent + listed in brackets, it is implemented by a generated super-accessor in + the class that extends this trait based on the linearization order of + the class. +2. Because B comes before C in the linearization + order of Fail, and because B overrides foo, + the super-accessor in Fail is implemented as a call to + super[B].foo. +3. However, + X (the type of super[B].foo in Fail) + is not a subtype of + Y (the type of foo in trait C). + Hence, the super-accessor that needs to be generated in Fail + is illegal. + +Here are two possible ways to resolve this: + +1. Change the linearization order of Fail such that + C comes before B. +2. Alternatively, replace super.foo in the body of trait C by a + super-call to a specific parent, e.g. super[A].foo diff --git a/tests/neg/i5433.scala b/tests/neg/i5433.scala new file mode 100644 index 000000000000..5459f16c5623 --- /dev/null +++ b/tests/neg/i5433.scala @@ -0,0 +1,25 @@ +class X +class Y extends X + +trait A[+T] { + def foo: T = null.asInstanceOf[T] +} + +trait B extends A[X] { + override def foo: X = new X +} + +trait C extends A[Y] { + override def foo: Y = new Y + def superFoo: Y = super.foo // C will have an abstract `def C$$super$foo: Y` because of this call +} + +class Fail extends B with C // error + // Generated `def C$$super$foo: Y = super[B].foo` + +object Test { + def main(args: Array[String]): Unit = { + val y: Y = (new Fail).superFoo // Used to fail with a ClassCastException because of `Fail#C$$super$foo` being incorrect above + assert(y == null) + } +} From fc7c19009de291c785ae6e945fee15709a78862c Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Sat, 2 Mar 2019 19:09:00 +0100 Subject: [PATCH 2/4] Avoid non-determinism in stdlib test by sorting files For some mysterious reason I still need to investigate, some invalid super-calls in the stdlib that should be detected by the previous commit are only detected when the files are compiled in a specific order, this is made more confusing by the order of listed files being non-deterministic by default. As a first step towards sanity, sort the files to avoid non-determinism and consistently detect the invalid super-calls. --- compiler/test/dotty/tools/TestSources.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/test/dotty/tools/TestSources.scala b/compiler/test/dotty/tools/TestSources.scala index d570bf07d3e2..034d67466135 100644 --- a/compiler/test/dotty/tools/TestSources.scala +++ b/compiler/test/dotty/tools/TestSources.scala @@ -21,7 +21,7 @@ object TestSources { val acc2 = files.foldLeft(acc)((acc1, file) => if (file.isFile && file.getPath.endsWith(".scala")) file.getPath :: acc1 else acc1) files.foldLeft(acc2)((acc3, file) => if (file.isDirectory) collectAllFilesInDir(file, acc3) else acc3) } - collectAllFilesInDir(new File(stdLibPath), Nil) + collectAllFilesInDir(new File(stdLibPath), Nil).sorted } // pos tests lists From ce97789c863260d65b48a652947b7332dfa2ba4a Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Sat, 2 Mar 2019 20:29:50 +0100 Subject: [PATCH 3/4] Avoid super-calls leading to illegal superaccessors ...in the 2.12 stdlib which is part of our test suite. --- .../scala/collection/IndexedSeqOptimized.scala | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/scala2-library/src/library/scala/collection/IndexedSeqOptimized.scala b/tests/scala2-library/src/library/scala/collection/IndexedSeqOptimized.scala index 320725c30e63..cae2e000af95 100644 --- a/tests/scala2-library/src/library/scala/collection/IndexedSeqOptimized.scala +++ b/tests/scala2-library/src/library/scala/collection/IndexedSeqOptimized.scala @@ -70,11 +70,11 @@ trait IndexedSeqOptimized[+A, +Repr] extends Any with IndexedSeqLike[A, Repr] { override /*TraversableLike*/ def reduceLeft[B >: A](op: (B, A) => B): B = - if (length > 0) foldl(1, length, this(0), op) else super.reduceLeft(op) + if (length > 0) foldl(1, length, this(0), op) else super[IndexedSeqLike].reduceLeft(op) override /*IterableLike*/ def reduceRight[B >: A](op: (A, B) => B): B = - if (length > 0) foldr(0, length - 1, this(length - 1), op) else super.reduceRight(op) + if (length > 0) foldr(0, length - 1, this(length - 1), op) else super[IndexedSeqLike].reduceRight(op) override /*IterableLike*/ def zip[A1 >: A, B, That](that: GenIterable[B])(implicit bf: CanBuildFrom[Repr, (A1, B), That]): That = that match { @@ -89,7 +89,7 @@ trait IndexedSeqOptimized[+A, +Repr] extends Any with IndexedSeqLike[A, Repr] { } b.result() case _ => - super.zip[A1, B, That](that)(bf) + super[IndexedSeqLike].zip[A1, B, That](that)(bf) } override /*IterableLike*/ @@ -122,16 +122,16 @@ trait IndexedSeqOptimized[+A, +Repr] extends Any with IndexedSeqLike[A, Repr] { } override /*IterableLike*/ - def head: A = if (isEmpty) super.head else this(0) + def head: A = if (isEmpty) super[IndexedSeqLike].head else this(0) override /*TraversableLike*/ - def tail: Repr = if (isEmpty) super.tail else slice(1, length) + def tail: Repr = if (isEmpty) super[IndexedSeqLike].tail else slice(1, length) override /*TraversableLike*/ - def last: A = if (length > 0) this(length - 1) else super.last + def last: A = if (length > 0) this(length - 1) else super[IndexedSeqLike].last override /*IterableLike*/ - def init: Repr = if (length > 0) slice(0, length - 1) else super.init + def init: Repr = if (length > 0) slice(0, length - 1) else super[IndexedSeqLike].init override /*TraversableLike*/ def take(n: Int): Repr = slice(0, n) @@ -167,7 +167,7 @@ trait IndexedSeqOptimized[+A, +Repr] extends Any with IndexedSeqLike[A, Repr] { i == len } case _ => - super.sameElements(that) + super[IndexedSeqLike].sameElements(that) } override /*IterableLike*/ @@ -274,7 +274,7 @@ trait IndexedSeqOptimized[+A, +Repr] extends Any with IndexedSeqLike[A, Repr] { true } case _ => - super.endsWith(that) + super[IndexedSeqLike].endsWith(that) } } From 8bad055e3520446960bfedea4fced6af03f70c9c Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 5 Mar 2019 13:36:59 +0100 Subject: [PATCH 4/4] Make the illegal super accessor message a case class --- .../reporting/diagnostic/ErrorMessageID.java | 3 +- .../dotc/reporting/diagnostic/messages.scala | 56 ++++++++++++++++ .../tools/dotc/transform/ResolveSuper.scala | 65 +++---------------- tests/neg/i5433.check | 2 +- 4 files changed, 68 insertions(+), 58 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java b/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java index d1e62425cc85..ebeea4a5598b 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java @@ -144,7 +144,8 @@ public enum ErrorMessageID { OverloadInRefinementID, NoMatchingOverloadID, StableIdentPatternID, - StaticFieldsShouldPrecedeNonStaticID + StaticFieldsShouldPrecedeNonStaticID, + IllegalSuperAccessorID ; public int errorNumber() { diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala index ba7c4de7f2d9..0185b1785544 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala @@ -2265,4 +2265,60 @@ object messages { hl"""Stable identifier required, but ${tree.show} found""" override def explanation: String = "" } + + case class IllegalSuperAccessor(base: Symbol, memberName: Name, + acc: Symbol, accTp: Type, + other: Symbol, otherTp: Type)(implicit val ctx: Context) extends Message(IllegalSuperAccessorID) { + val kind: String = "Reference" + val msg: String = { + // The mixin containing a super-call that requires a super-accessor + val accMixin = acc.owner + // The class or trait that the super-accessor should resolve too in `base` + val otherMixin = other.owner + // The super-call in `accMixin` + val superCall = i"super.$memberName" + // The super-call that the super-accesors in `base` forwards to + val resolvedSuperCall = i"super[${otherMixin.name}].$memberName" + // The super-call that we would have called if `super` in traits behaved like it + // does in classes, i.e. followed the linearization of the trait itself. + val staticSuperCall = { + val staticSuper = accMixin.asClass.info.parents.reverse + .find(_.nonPrivateMember(memberName).matchingDenotation(accMixin.thisType, acc.info).exists) + val staticSuperName = staticSuper match { + case Some(parent) => + parent.classSymbol.name.show + case None => // Might be reachable under separate compilation + "SomeParent" + } + i"super[$staticSuperName].$memberName" + } + hl"""$base cannot be defined due to a conflict between its parents when + |implementing a super-accessor for $memberName in $accMixin: + | + |1. One of its parent (${accMixin.name}) contains a call $superCall in its body, + | and when a super-call in a trait is written without an explicit parent + | listed in brackets, it is implemented by a generated super-accessor in + | the class that extends this trait based on the linearization order of + | the class. + |2. Because ${otherMixin.name} comes before ${accMixin.name} in the linearization + | order of ${base.name}, and because ${otherMixin.name} overrides $memberName, + | the super-accessor in ${base.name} is implemented as a call to + | $resolvedSuperCall. + |3. However, + | ${otherTp.widenExpr} (the type of $resolvedSuperCall in ${base.name}) + | is not a subtype of + | ${accTp.widenExpr} (the type of $memberName in $accMixin). + | Hence, the super-accessor that needs to be generated in ${base.name} + | is illegal. + | + |Here are two possible ways to resolve this: + | + |1. Change the linearization order of ${base.name} such that + | ${accMixin.name} comes before ${otherMixin.name}. + |2. Alternatively, replace $superCall in the body of $accMixin by a + | super-call to a specific parent, e.g. $staticSuperCall + |""".stripMargin + } + val explanation: String = "" + } } diff --git a/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala b/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala index c224e3d97e4c..970984bd380d 100644 --- a/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala +++ b/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala @@ -12,6 +12,7 @@ import DenotTransformers._ import NameOps._ import NameKinds._ import ResolveSuper._ +import reporting.diagnostic.messages.IllegalSuperAccessor /** This phase adds super accessors and method overrides where * linearization differs from Java's rule for default methods in interfaces. @@ -100,65 +101,17 @@ object ResolveSuper { val SuperAccessorName(memberName) = acc.name.unexpandedName ctx.debuglog(i"starting rebindsuper from $base of ${acc.showLocated}: ${acc.info} in $bcs, name = $memberName") while (bcs.nonEmpty && sym == NoSymbol) { - val cur = bcs.head - val other = cur.info.nonPrivateDecl(memberName) - if (ctx.settings.Ydebug.value) - ctx.log(i"rebindsuper ${bcs.head} $other deferred = ${other.symbol.is(Deferred)}") - val otherMember = other.matchingDenotation(base.thisType, base.thisType.memberInfo(acc)) - if (otherMember.exists) { - sym = otherMember.symbol + val other = bcs.head.info.nonPrivateDecl(memberName) + .matchingDenotation(base.thisType, base.thisType.memberInfo(acc)) + ctx.debuglog(i"rebindsuper ${bcs.head} $other deferred = ${other.symbol.is(Deferred)}") + if (other.exists) { + sym = other.symbol // Having a matching denotation is not enough: it should also be a subtype // of the superaccessor's type, see i5433.scala for an example where this matters - val otherTp = otherMember.asSeenFrom(base.typeRef).info + val otherTp = other.asSeenFrom(base.typeRef).info val accTp = acc.asSeenFrom(base.typeRef).info - if (!(otherTp <:< accTp)) { - // The mixin containing a super-call that requires a super-accessor - val mixin = acc.owner - // The super-call in `mixin` - val superCall = i"super.$memberName" - // The super-call that we end up trying to call - val resolvedSuperCall = i"super[${cur.name}].$memberName" - // The super-call that we would have called if `super` in traits behaved like it - // does in classes, i.e. followed the linearization of the trait itself. - val staticSuperCall = { - val staticSuper = mixin.asClass.info.parents.reverse - .find(_.nonPrivateMember(memberName).matchingDenotation(mixin.thisType, acc.info).exists) - val staticSuperName = staticSuper match { - case Some(parent) => - parent.classSymbol.name.show - case None => // Might be reachable under separate compilation - "SomeParent" - } - i"super[$staticSuperName].$memberName" - } - ctx.error( - hl"""$base cannot be defined due to a conflict between its parents when - |implementing a super-accessor for $memberName in $mixin: - | - |1. One of its parent ($mixin) contains a call $superCall in its body, - | and when a super-call in a trait is written without an explicit parent - | listed in brackets, it is implemented by a generated super-accessor in - | the class that extends this trait based on the linearization order of - | the class. - |2. Because ${cur.name} comes before ${mixin.name} in the linearization - | order of ${base.name}, and because ${cur.name} overrides $memberName, - | the super-accessor in ${base.name} is implemented as a call to - | $resolvedSuperCall. - |3. However, - | ${otherTp.widenExpr} (the type of $resolvedSuperCall in ${base.name}) - | is not a subtype of - | ${accTp.widenExpr} (the type of $memberName in $mixin). - | Hence, the super-accessor that needs to be generated in ${base.name} - | is illegal. - | - |Here are two possible ways to resolve this: - | - |1. Change the linearization order of ${base.name} such that - | ${mixin.name} comes before ${cur.name}. - |2. Alternatively, replace $superCall in the body of $mixin by a - | super-call to a specific parent, e.g. $staticSuperCall - |""".stripMargin, base.sourcePos) - } + if (!(otherTp <:< accTp)) + ctx.error(IllegalSuperAccessor(base, memberName, acc, accTp, other.symbol, otherTp), base.sourcePos) } bcs = bcs.tail diff --git a/tests/neg/i5433.check b/tests/neg/i5433.check index 60edf7a03d8c..32df8fc58c8b 100644 --- a/tests/neg/i5433.check +++ b/tests/neg/i5433.check @@ -2,7 +2,7 @@ class Fail cannot be defined due to a conflict between its parents when implementing a super-accessor for foo in trait C: -1. One of its parent (trait C) contains a call super.foo in its body, +1. One of its parent (C) contains a call super.foo in its body, and when a super-call in a trait is written without an explicit parent listed in brackets, it is implemented by a generated super-accessor in the class that extends this trait based on the linearization order of