Merge pull request #5604 from dotty-staging/fix-super-mixin-2

Fix #5433: check that the implemented super-accessor is valid

Author: smarter

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() {

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 = ""
+  }
 }

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.
@@ -101,9 +102,18 @@ object ResolveSuper {
     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)
-      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
+        .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 = other.asSeenFrom(base.typeRef).info
+        val accTp = acc.asSeenFrom(base.typeRef).info
+        if (!(otherTp <:< accTp))
+          ctx.error(IllegalSuperAccessor(base, memberName, acc, accTp, other.symbol, otherTp), base.sourcePos)
+      }
+
       bcs = bcs.tail
     }
     assert(sym.exists)

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

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 (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

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)
+  }
+}

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)
   }
 }