Merge pull request #541 from dwijnand/forward-bincompat

Catch forward-incompatible method overriding in traits

Author: dwijnand

core/src/main/scala/com/typesafe/tools/mima/core/ClassInfo.scala

@@ -175,22 +175,36 @@ private[mima] sealed abstract class ClassInfo(val owner: PackageInfo) extends In
       case _: SyntheticClassInfo   => false
       case impl: ConcreteClassInfo =>
         assert(impl.isImplClass, impl)
-        impl.methods.get(m.bytecodeName).exists(im => hasImplSig(im.descriptor, m.descriptor))
+        impl.methods.get(m.bytecodeName).exists { im =>
+          val isig = im.descriptor
+          val tsig = m.descriptor
+          assert(isig(0) == '(' && isig(1) == 'L' && tsig(0) == '(', s"isig=[$isig] tsig=[$tsig]")
+          hasMatchingSig(isig, tsig)
+        }
     }
   }
 
-  // Does `isig` correspond to `tsig` if seen as the signature of the static
-  // implementation method of a trait method with signature `tsig`?
-  private def hasImplSig(isig: String, tsig: String): Boolean = {
-    assert(isig(0) == '(' && isig(1) == 'L' && tsig(0) == '(')
-    val ilen = isig.length
+  /** Does the given method have a static mixin forwarder? */
+  final def hasMixinForwarder(m: MethodInfo): Boolean = {
+    methods.get(m.bytecodeName + "$").exists { fm =>
+      val fsig = fm.descriptor
+      val tsig = m.descriptor
+      assert(fsig(0) == '(' && tsig(0) == '(', s"fsig=[$fsig] tsig=[$tsig]")
+      hasMatchingSig(fsig, tsig)
+    }
+  }
+
+  // Does `sig` correspond to `tsig` if seen as the signature of the static
+  // implementation method or the mixin forwarder method of a trait method with signature `tsig`?
+  private def hasMatchingSig(sig: String, tsig: String): Boolean = {
+    val ilen = sig.length
     val tlen = tsig.length
     var i = 2
-    while (isig(i) != ';')
+    while (sig(i) != ';')
       i += 1
     i += 1
     var j = 1
-    while (i < ilen && j < tlen && isig(i) == tsig(j)) {
+    while (i < ilen && j < tlen && sig(i) == tsig(j)) {
       i += 1
       j += 1
     }

core/src/main/scala/com/typesafe/tools/mima/core/Problems.scala

@@ -45,6 +45,7 @@ sealed abstract class Problem extends ProblemRef {
     case DirectAbstractMethodProblem(ref)                 => s"${ref.methodString} does not have a correspondent in $affectedVersion version"
     case ReversedAbstractMethodProblem(ref)               => s"in $affectedVersion version there is ${ref.methodString}, which does not have a correspondent"
     case UpdateForwarderBodyProblem(ref)                  => s"in $affectedVersion version, classes mixing ${ref.owner.fullName} needs to update body of ${ref.shortMethodString}"
+    case NewMixinForwarderProblem(ref)                    => s"in $affectedVersion version, classes mixing ${ref.owner.fullName} need be recompiled to wire to the new static mixin forwarder method all super calls to ${ref.shortMethodString}"
     case InheritedNewAbstractMethodProblem(absmeth, ref)  => s"${absmeth.methodString} is inherited by class ${ref.owner.bytecodeName} in $affectedVersion version."
   }
 }
@@ -84,4 +85,5 @@ sealed abstract class AbstractMethodProblem(memb: MemberInfo)
 final case class DirectAbstractMethodProblem(newmeth: MethodInfo)                             extends AbstractMethodProblem(newmeth)
 final case class ReversedAbstractMethodProblem(newmeth: MethodInfo)                           extends MemberProblem(newmeth)
 final case class UpdateForwarderBodyProblem(oldmeth: MethodInfo)                              extends MemberProblem(oldmeth)
+final case class NewMixinForwarderProblem(oldmeth: MethodInfo)                                extends MemberProblem(oldmeth)
 final case class InheritedNewAbstractMethodProblem(absmeth: MethodInfo, newmeth: MethodInfo)  extends AbstractMethodProblem(newmeth)

core/src/main/scala/com/typesafe/tools/mima/lib/analyze/method/MethodChecker.scala

@@ -29,6 +29,8 @@ private[analyze] object MethodChecker {
     } else {
       if (oldmeth.owner.hasStaticImpl(oldmeth))
         checkStaticImplMethod(oldmeth, newclazz)
+      else if (oldmeth.owner.hasMixinForwarder(oldmeth))
+        checkStaticMixinForwarderMethod(oldmeth, newclazz)
       else
         checkExisting1Impl(oldmeth, newclazz, _.lookupMethods(oldmeth))
     }
@@ -79,6 +81,19 @@ private[analyze] object MethodChecker {
     }
   }
 
+  private def checkStaticMixinForwarderMethod(oldmeth: MethodInfo, newclazz: ClassInfo) = {
+    if (newclazz.hasMixinForwarder(oldmeth)) {
+      None // then it's ok, the method it is still there
+    } else {
+      if (newclazz.allTraits.exists(_.hasMixinForwarder(oldmeth))) {
+        Some(NewMixinForwarderProblem(oldmeth))
+      } else {
+        val methsLookup = (_: ClassInfo).lookupConcreteTraitMethods(oldmeth)
+        Some(missingOrIncompatible(oldmeth, methsLookup(newclazz).toList, methsLookup))
+      }
+    }
+  }
+
   private def missingOrIncompatible(oldmeth: MethodInfo, newmeths: List[MethodInfo], methsLookup: ClassInfo => Iterator[MethodInfo]) = {
     val newmethAndBridges = newmeths.filter(oldmeth.matchesType(_)) // _the_ corresponding new method + its bridges
     newmethAndBridges.find(_.tpe.resultType == oldmeth.tpe.resultType) match {

functional-tests/src/it/scala-library-2-13/problems.txt

@@ -0,0 +1 @@
+in new version, classes mixing scala.collection.MapView need be recompiled to wire to the new static mixin forwarder method all super calls to method className()java.lang.String

functional-tests/src/main/scala/com/typesafe/tools/mima/lib/CollectProblemsTest.scala

@@ -13,7 +13,10 @@ object CollectProblemsTest {
     () <- testCase.compileBoth
     problems = collectProblems(direction.lhs(testCase).jfile, direction.rhs(testCase).jfile)
     expected = readOracleFile(testCase.versionedFile(direction.oracleFile).jfile)
-    () <- diffProblems(problems, expected)
+    () <- direction match {
+      case Backwards => diffProblems(problems, expected)
+      case Forwards  => diffProblems(problems, expected, "other")
+    }
   } yield ()
 
   val mimaLib: MiMaLib = new MiMaLib(cp = Nil)

functional-tests/src/test/trait-adding-a-method-override-is-nok/app/App.scala

@@ -0,0 +1,5 @@
+object App {
+  def main(args: Array[String]): Unit = {
+    println(new B {}.n)
+  }
+}

functional-tests/src/test/trait-adding-a-method-override-is-nok/forwards.txt

@@ -0,0 +1 @@
+in other version, classes mixing B need be recompiled to wire to the new static mixin forwarder method all super calls to method n()java.lang.String

functional-tests/src/test/trait-adding-a-method-override-is-nok/v1/A.scala

@@ -0,0 +1,8 @@
+trait A {
+  def m = "a"
+  def n = "a"
+}
+
+trait B extends A {
+  override def m = "b"
+}

functional-tests/src/test/trait-adding-a-method-override-is-nok/v2/A.scala

@@ -0,0 +1,9 @@
+trait A {
+  def m = "a"
+  def n = "a"
+}
+
+trait B extends A {
+  override def m = "b"
+  override def n = "b"
+}

functional-tests/src/test/trait-deleting-concrete-methods-is-nok/problems.txt

@@ -1 +1,2 @@
 synthetic static method $init$(B)Unit in interface B does not have a correspondent in new version
+in new version, classes mixing B need be recompiled to wire to the new static mixin forwarder method all super calls to method foo()Int