Merge pull request #505 from raboof/compare-signature-independent-of-…

…name

Parse signatures to avoid false positives

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

@@ -1,5 +1,7 @@
 package com.typesafe.tools.mima.lib.analyze.method
 
+import scala.annotation.tailrec
+
 import com.typesafe.tools.mima.core._
 
 private[analyze] object MethodChecker {
@@ -47,12 +49,62 @@ private[analyze] object MethodChecker {
         hasMatchingSignature(oldmeth.signature, newmeth.signature, newmeth.bytecodeName)
   }
 
-  private def hasMatchingSignature(oldsig: String, newsig: String, bytecodeName: String): Boolean = {
-    oldsig == newsig || // Special case for scala#7975
+  def hasMatchingSignature(oldsig: String, newsig: String, bytecodeName: String): Boolean = {
+    def canonicalize(signature: String): String = {
+      signature.headOption match {
+        case None | Some('(') => signature
+        case _ =>
+            val (formalTypeParameters, rest) = FormalTypeParameter.parseList(signature.drop(1))
+            val replacements = formalTypeParameters.map(_.identifier).zipWithIndex
+            replacements.foldLeft(signature) { case (sig, (from, to)) =>
+              sig
+                .replace(s"<${from}:", s"<__${to}__:")
+                .replace(s";${from}:", s";__${to}__:")
+                .replace(s"T${from};", s"__${to}__") }
+      }
+    }
+
+    oldsig == newsig ||
+      canonicalize(oldsig) == canonicalize(newsig) || // Special case for scala#7975
       bytecodeName == MemberInfo.ConstructorName && hasMatchingCtorSig(oldsig, newsig)
   }
 
-  private[analyze] def hasMatchingCtorSig(oldsig: String, newsig: String): Boolean =
+  case class FormalTypeParameter(identifier: String, bound: String)
+  object FormalTypeParameter {
+    def parseList(in: String, listSoFar: List[FormalTypeParameter] = Nil): (List[FormalTypeParameter], String) = {
+      in(0) match {
+        case '>' => (listSoFar, in.drop(1))
+        case o => {
+          val (next, rest) = parseOne(in)
+          parseList(rest, listSoFar :+ next)
+        }
+      }
+    }
+    def parseOne(in: String): (FormalTypeParameter, String) = {
+      val identifier = in.takeWhile(_ != ':')
+      val boundAndRest = in.dropWhile(_ != ':').drop(1)
+      val (bound, rest) = splitBoundAndRest(boundAndRest)
+      (FormalTypeParameter(identifier, bound), rest)
+    }
+    @tailrec
+    private def splitBoundAndRest(in: String, boundSoFar: String = "", depth: Int = 0): (String, String) = {
+      if (depth > 0) {
+        in(0) match {
+          case '>' => splitBoundAndRest(in.drop(1), boundSoFar + '>', depth - 1)
+          case '<' => splitBoundAndRest(in.drop(1), boundSoFar + '<', depth + 1)
+          case o => splitBoundAndRest(in.drop(1), boundSoFar + o, depth)
+        }
+      } else {
+        in(0) match {
+          case '<' => splitBoundAndRest(in.drop(1), boundSoFar + '<', depth + 1)
+          case ';' => (boundSoFar, in.drop(1))
+          case o => splitBoundAndRest(in.drop(1), boundSoFar + o, depth)
+        }
+      }
+    }
+  }
+
+  private def hasMatchingCtorSig(oldsig: String, newsig: String): Boolean =
     newsig.isEmpty ||              // ignore losing signature on constructors
       oldsig.endsWith(newsig.tail) // ignore losing the 1st (outer) param (.tail drops the leading '(')
 

core/src/test/scala/com/typesafe/tools/mima/lib/analyze/method/MethodCheckerSpec.scala

@@ -1,5 +1,7 @@
 package com.typesafe.tools.mima.lib.analyze.method
 
+import com.typesafe.tools.mima.core.MemberInfo
+
 import org.scalatest.matchers.should.Matchers
 import org.scalatest.wordspec.AnyWordSpec
 
@@ -21,11 +23,31 @@ final class MethodCheckerSpec extends AnyWordSpec with Matchers {
       // dropping the first parameter of the Signature attribute
       // can only be explained by going from a Scala version that does not have
       // the fix in scala#7975 (2.12.8, 2.13.0) to one that does
-      assert(MethodChecker.hasMatchingCtorSig(`signature_in_2.12.8`, `signature_in_2.12.9`))
+      assert(MethodChecker.hasMatchingSignature(`signature_in_2.12.8`, `signature_in_2.12.9`, MemberInfo.ConstructorName))
     }
 
     "reject adding the first parameter of the Signature attribute of a constructor back" in {
-      assert(!MethodChecker.hasMatchingCtorSig(`signature_in_2.12.9`, `signature_in_2.12.8`))
+      assert(!MethodChecker.hasMatchingSignature(`signature_in_2.12.9`, `signature_in_2.12.8`, MemberInfo.ConstructorName))
+    }
+
+    "allow renaming a generic parameter" in {
+      assert(MethodChecker.hasMatchingSignature(
+        "<U:Ljava/lang/Object;>(TU;Lscala/collection/immutable/List<TU;>;)Lscala/Option<TU;>;",
+        "<T:Ljava/lang/Object;>(TT;Lscala/collection/immutable/List<TT;>;)Lscala/Option<TT;>;",
+        "foo"))
+    }
+  }
+
+  "The signature parser" should {
+    "parse a signature with generic bounds that themselves have generics" in {
+      val (types, rest) = MethodChecker.FormalTypeParameter.parseList(
+        "T:Ljava/lang/Object;U:Lscala/collection/immutable/List<TT;>;>(TT;Lscala/collection/immutable/List<TU;>;)Lscala/Option<TT;>;>")
+      types.length should be(2)
+      types(0).identifier should be("T")
+      types(0).bound should be("Ljava/lang/Object")
+      types(1).identifier should be("U")
+      types(1).bound should be("Lscala/collection/immutable/List<TT;>")
+      rest should be("(TT;Lscala/collection/immutable/List<TU;>;)Lscala/Option<TT;>;>")
     }
   }
 }

functional-tests/src/test/class-method-generics-nok/problems.txt

@@ -1,4 +1,3 @@
-method genericWithChangingName()scala.Option in class A has a different generic signature in new version, where it is <T:Ljava/lang/Object;>()Lscala/Option<TT;>; rather than <U:Ljava/lang/Object;>()Lscala/Option<TU;>;. See https://github.com/lightbend/mima#incompatiblesignatureproblem
 method backwardsCompatibleNarrowing()scala.Option in class A has a different generic signature in new version, where it is ()Lscala/Option<Ljava/lang/String;>; rather than ()Lscala/Option<Ljava/lang/Object;>;. See https://github.com/lightbend/mima#incompatiblesignatureproblem
 #
 method cov1()java.lang.Object in class Api has a different generic signature in new version, where it is <T:Ljava/lang/Object;>()TT; rather than <missing>. See https://github.com/lightbend/mima#incompatiblesignatureproblem

functional-tests/src/test/class-method-generics-nok/v2/A.scala

@@ -1,6 +1,6 @@
 class A {
-  def genericWithChangingName[T]: Option[T] = ???
-  def backwardsCompatibleNarrowing: Option[String] = ???
+  def genericWithChangingName[T]: Option[T] = ???        // OK
+  def backwardsCompatibleNarrowing: Option[String] = ??? // KO
 }
 
 final class Api {

functional-tests/src/test/type-parameter-names-change-ok/app/App.scala

@@ -0,0 +1,6 @@
+object App {
+  def main(args: Array[String]): Unit = {
+    val x: Option[String] = new Tree().someMethod("foo", List(42))
+    println(x)
+  }
+}

functional-tests/src/test/type-parameter-names-change-ok/v1/Tree.scala

@@ -0,0 +1,3 @@
+class Tree {
+  def someMethod[T,U](e: T, l: List[U]): Option[T] = Some(e)
+}

functional-tests/src/test/type-parameter-names-change-ok/v2/Tree.scala

@@ -0,0 +1,3 @@
+class Tree {
+  def someMethod[X,Y](e: X, l: List[Y]): Option[X] = Some(e)
+}

project/MimaSettings.scala

@@ -34,6 +34,7 @@ object MimaSettings {
       exclude[Problem]("*mima.plugin.SbtLogger*"),
       exclude[Problem]("*mima.plugin.SbtMima.*"),
       exclude[Problem]("scala.tools.nsc.mima*"),
+      exclude[DirectMissingMethodProblem]("*mima.lib.analyze.method.MethodChecker.hasMatchingCtorSig"),
     ),
   )
 }