From f25269a43ca3c19d0b587f61d33655eeddb888aa Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Fri, 4 Sep 2020 21:33:55 +0100 Subject: [PATCH] Catch forward-incompatible method overriding in traits --- .../typesafe/tools/mima/core/ClassInfo.scala | 30 ++++++++++++++----- .../typesafe/tools/mima/core/Problems.scala | 2 ++ .../lib/analyze/method/MethodChecker.scala | 15 ++++++++++ .../src/it/scala-library-2-13/problems.txt | 1 + .../tools/mima/lib/CollectProblemsTest.scala | 5 +++- .../app/App.scala | 5 ++++ .../forwards.txt | 1 + .../v1/A.scala | 8 +++++ .../v2/A.scala | 9 ++++++ .../problems.txt | 1 + .../problems.txt | 2 +- .../problems.txt | 3 +- 12 files changed, 71 insertions(+), 11 deletions(-) create mode 100644 functional-tests/src/test/trait-adding-a-method-override-is-nok/app/App.scala create mode 100644 functional-tests/src/test/trait-adding-a-method-override-is-nok/forwards.txt create mode 100644 functional-tests/src/test/trait-adding-a-method-override-is-nok/v1/A.scala create mode 100644 functional-tests/src/test/trait-adding-a-method-override-is-nok/v2/A.scala diff --git a/core/src/main/scala/com/typesafe/tools/mima/core/ClassInfo.scala b/core/src/main/scala/com/typesafe/tools/mima/core/ClassInfo.scala index 821572b6..9858ea71 100644 --- a/core/src/main/scala/com/typesafe/tools/mima/core/ClassInfo.scala +++ b/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 } diff --git a/core/src/main/scala/com/typesafe/tools/mima/core/Problems.scala b/core/src/main/scala/com/typesafe/tools/mima/core/Problems.scala index 88c338ef..804a2cd7 100644 --- a/core/src/main/scala/com/typesafe/tools/mima/core/Problems.scala +++ b/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) diff --git a/core/src/main/scala/com/typesafe/tools/mima/lib/analyze/method/MethodChecker.scala b/core/src/main/scala/com/typesafe/tools/mima/lib/analyze/method/MethodChecker.scala index 620b008c..47e91906 100644 --- a/core/src/main/scala/com/typesafe/tools/mima/lib/analyze/method/MethodChecker.scala +++ b/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 { diff --git a/functional-tests/src/it/scala-library-2-13/problems.txt b/functional-tests/src/it/scala-library-2-13/problems.txt index e69de29b..5c068c3c 100644 --- a/functional-tests/src/it/scala-library-2-13/problems.txt +++ b/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 diff --git a/functional-tests/src/main/scala/com/typesafe/tools/mima/lib/CollectProblemsTest.scala b/functional-tests/src/main/scala/com/typesafe/tools/mima/lib/CollectProblemsTest.scala index 62ec6a30..d861ef59 100644 --- a/functional-tests/src/main/scala/com/typesafe/tools/mima/lib/CollectProblemsTest.scala +++ b/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) diff --git a/functional-tests/src/test/trait-adding-a-method-override-is-nok/app/App.scala b/functional-tests/src/test/trait-adding-a-method-override-is-nok/app/App.scala new file mode 100644 index 00000000..38120868 --- /dev/null +++ b/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) + } +} diff --git a/functional-tests/src/test/trait-adding-a-method-override-is-nok/forwards.txt b/functional-tests/src/test/trait-adding-a-method-override-is-nok/forwards.txt new file mode 100644 index 00000000..9eabd03d --- /dev/null +++ b/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 diff --git a/functional-tests/src/test/trait-adding-a-method-override-is-nok/v1/A.scala b/functional-tests/src/test/trait-adding-a-method-override-is-nok/v1/A.scala new file mode 100644 index 00000000..ed3f4a25 --- /dev/null +++ b/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" +} diff --git a/functional-tests/src/test/trait-adding-a-method-override-is-nok/v2/A.scala b/functional-tests/src/test/trait-adding-a-method-override-is-nok/v2/A.scala new file mode 100644 index 00000000..824f7812 --- /dev/null +++ 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" +} diff --git a/functional-tests/src/test/trait-deleting-concrete-methods-is-nok/problems.txt b/functional-tests/src/test/trait-deleting-concrete-methods-is-nok/problems.txt index a875dade..49252b58 100644 --- a/functional-tests/src/test/trait-deleting-concrete-methods-is-nok/problems.txt +++ 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 diff --git a/functional-tests/src/test/trait-inheriting-from-class-nok/problems.txt b/functional-tests/src/test/trait-inheriting-from-class-nok/problems.txt index 115fbb57..3299b0cb 100644 --- a/functional-tests/src/test/trait-inheriting-from-class-nok/problems.txt +++ b/functional-tests/src/test/trait-inheriting-from-class-nok/problems.txt @@ -1 +1 @@ -abstract method foo(Int)Int in interface A does not have a correspondent in new version \ No newline at end of file +method foo(Int)Int in interface A does not have a correspondent in new version diff --git a/functional-tests/src/test/trait-pushing-up-concrete-methods-is-nok/problems.txt b/functional-tests/src/test/trait-pushing-up-concrete-methods-is-nok/problems.txt index 22ed1402..49252b58 100644 --- a/functional-tests/src/test/trait-pushing-up-concrete-methods-is-nok/problems.txt +++ b/functional-tests/src/test/trait-pushing-up-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 \ No newline at end of file +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