From f3f89399e3067c59f3d6bc52bc0e710a902bcbee Mon Sep 17 00:00:00 2001 From: Mirco Dotta Date: Sat, 1 Oct 2016 14:35:40 +0200 Subject: [PATCH 1/2] Exploring inner modules for binary incompatibilities. Fix #127 The former implementation of PackageInfo#accessibleClasses expects that for any module `Foo` compiled by scalac, both `Foo.class` and `Foo$.class` classfiles are produced. Because `Foo$.class` and `Foo.class` are essentially mirrors, and users never access directly `Foo$.class`, MiMa only traverses the `Foo.class`. This works well for top-level modules, but it breaks for inner modules, because no mirror class is created for inner modules (only the module class is created). The fix consist in treating inner modules differently, hence making sure inner modules are part of the set returned by PackageInfo#accessibleClasses. --- .../typesafe/tools/mima/core/ClassInfo.scala | 6 ++--- .../tools/mima/core/PackageInfo.scala | 26 +++++++++++++++++-- .../problems.txt | 2 ++ .../v1/A.scala | 5 ++++ .../v2/A.scala | 2 ++ .../problems.txt | 1 + .../v1/A.scala | 5 ++++ .../v2/A.scala | 2 ++ 8 files changed, 44 insertions(+), 5 deletions(-) create mode 100644 reporter/functional-tests/src/test/class-removing-inner-object-nok/problems.txt create mode 100644 reporter/functional-tests/src/test/class-removing-inner-object-nok/v1/A.scala create mode 100644 reporter/functional-tests/src/test/class-removing-inner-object-nok/v2/A.scala create mode 100644 reporter/functional-tests/src/test/object-removing-inner-object-nok/problems.txt create mode 100644 reporter/functional-tests/src/test/object-removing-inner-object-nok/v1/A.scala create mode 100644 reporter/functional-tests/src/test/object-removing-inner-object-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 ba05eb4a..ed3e4e93 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 @@ -65,10 +65,10 @@ abstract class ClassInfo(val owner: PackageInfo) extends HasDeclarationName with override def canEqual(other: Any) = other.isInstanceOf[ClassInfo] - def formattedFullName = formatClassName(if (isObject) fullName.init else fullName) + def formattedFullName = formatClassName(if (isModule) fullName.init else fullName) def declarationPrefix = { - if (isObject) "object" + if (isModule) "object" else if (isTrait) "trait" else if (loaded && isInterface) "interface" // java interfaces and traits with no implementation methods else "class" @@ -290,7 +290,7 @@ abstract class ClassInfo(val owner: PackageInfo) extends HasDeclarationName with ClassfileParser.isInterface(flags) } - def isObject: Boolean = bytecodeName.endsWith("$") + def isModule: Boolean = bytecodeName.endsWith("$") /** Is this class public? */ /* diff --git a/core/src/main/scala/com/typesafe/tools/mima/core/PackageInfo.scala b/core/src/main/scala/com/typesafe/tools/mima/core/PackageInfo.scala index e55c444c..a3530c19 100644 --- a/core/src/main/scala/com/typesafe/tools/mima/core/PackageInfo.scala +++ b/core/src/main/scala/com/typesafe/tools/mima/core/PackageInfo.scala @@ -64,8 +64,9 @@ abstract class PackageInfo(val owner: PackageInfo) { def classes: mutable.Map[String, ClassInfo] lazy val accessibleClasses: Set[ClassInfo] = { + // FIXME: Make this a tailrecursive implementation /** Fixed point iteration for finding all accessible classes. */ - def accessibleClassesUnder(prefix: Set[ClassInfo]): Set[ClassInfo] = { + def accessibleClassesUnder(prefix: Set[ClassInfo]): Set[ClassInfo] = { val vclasses = (classes.valuesIterator filter (isAccessible(_, prefix))).toSet if (vclasses.isEmpty) vclasses else vclasses union accessibleClassesUnder(vclasses) @@ -77,7 +78,28 @@ abstract class PackageInfo(val owner: PackageInfo) { else { val idx = clazz.decodedName.lastIndexOf("$") if (idx < 0) prefix.isEmpty // class name contains no $ - else prefix exists (_.decodedName == clazz.decodedName.substring(0, idx)) // prefix before dollar is an accessible class detected previously + else { + // A top-level module is compiled into a class plus a module class (for instance, + // for a top-level `object A`, scalac creates two classfiles `A.class` and `A$.class`.). + // While, for an inner module, scalac creates only one classfile, i.e., the module class + // classfile. + // + // `clazz` is an inner module if: + // 1) `clazz` is a module, and + // 2) `clazz` decoded name starts with one of the passed `prefix`. + def isInnerModule: Boolean = { + clazz.isModule && + prefix.exists { outer => + // this condition is to avoid looping indefinitely + clazz.decodedName != outer.decodedName && + // checks if `outer` is a prefix of `clazz` name + clazz.decodedName.startsWith(outer.decodedName) + } + } + // class, trait, and top-level module go through this condition + (prefix exists (_.decodedName == clazz.decodedName.substring(0, idx))) || // prefix before dollar is an accessible class detected previously + isInnerModule + } } } clazz.isPublic && isReachable diff --git a/reporter/functional-tests/src/test/class-removing-inner-object-nok/problems.txt b/reporter/functional-tests/src/test/class-removing-inner-object-nok/problems.txt new file mode 100644 index 00000000..57dea9bc --- /dev/null +++ b/reporter/functional-tests/src/test/class-removing-inner-object-nok/problems.txt @@ -0,0 +1,2 @@ +object A#B does not have a correspondent in new version +method B()A#B# in class A does not have a correspondent in new version \ No newline at end of file diff --git a/reporter/functional-tests/src/test/class-removing-inner-object-nok/v1/A.scala b/reporter/functional-tests/src/test/class-removing-inner-object-nok/v1/A.scala new file mode 100644 index 00000000..6e207331 --- /dev/null +++ b/reporter/functional-tests/src/test/class-removing-inner-object-nok/v1/A.scala @@ -0,0 +1,5 @@ +class A { + object B { + def foo: Int = 2 + } +} diff --git a/reporter/functional-tests/src/test/class-removing-inner-object-nok/v2/A.scala b/reporter/functional-tests/src/test/class-removing-inner-object-nok/v2/A.scala new file mode 100644 index 00000000..2e2439c3 --- /dev/null +++ b/reporter/functional-tests/src/test/class-removing-inner-object-nok/v2/A.scala @@ -0,0 +1,2 @@ +class A { +} diff --git a/reporter/functional-tests/src/test/object-removing-inner-object-nok/problems.txt b/reporter/functional-tests/src/test/object-removing-inner-object-nok/problems.txt new file mode 100644 index 00000000..e4037ccc --- /dev/null +++ b/reporter/functional-tests/src/test/object-removing-inner-object-nok/problems.txt @@ -0,0 +1 @@ +object A#B does not have a correspondent in new version \ No newline at end of file diff --git a/reporter/functional-tests/src/test/object-removing-inner-object-nok/v1/A.scala b/reporter/functional-tests/src/test/object-removing-inner-object-nok/v1/A.scala new file mode 100644 index 00000000..598070d7 --- /dev/null +++ b/reporter/functional-tests/src/test/object-removing-inner-object-nok/v1/A.scala @@ -0,0 +1,5 @@ +object A { + object B { + def foo: Int = 2 + } +} diff --git a/reporter/functional-tests/src/test/object-removing-inner-object-nok/v2/A.scala b/reporter/functional-tests/src/test/object-removing-inner-object-nok/v2/A.scala new file mode 100644 index 00000000..169ef293 --- /dev/null +++ b/reporter/functional-tests/src/test/object-removing-inner-object-nok/v2/A.scala @@ -0,0 +1,2 @@ +object A { +} From 615b35a583e629ed1bd759ebbc939fcecbce3844 Mon Sep 17 00:00:00 2001 From: Stefan Zeiger Date: Fri, 18 Nov 2016 16:50:52 +0100 Subject: [PATCH 2/2] Use `InnerClasses` attribute to find all reachable classes --- .../typesafe/tools/mima/core/ClassInfo.scala | 6 +++ .../tools/mima/core/ClassfileParser.scala | 11 +++++ .../tools/mima/core/PackageInfo.scala | 41 ++++--------------- .../problems.txt | 3 +- .../v1/A.scala | 1 + 5 files changed, 29 insertions(+), 33 deletions(-) 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 ed3e4e93..481248ae 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 @@ -56,6 +56,12 @@ abstract class ClassInfo(val owner: PackageInfo) extends HasDeclarationName with else owner.fullName + "." + bytecodeName } + var _innerClasses: Seq[String] = Seq.empty + def innerClasses = { ensureLoaded(); _innerClasses } + + var _isTopLevel = true + def isTopLevel = { ensureLoaded(); _isTopLevel } + final override def equals(other: Any): Boolean = other match { case that: ClassInfo => (that canEqual this) && this.fullName == that.fullName case _ => false diff --git a/core/src/main/scala/com/typesafe/tools/mima/core/ClassfileParser.scala b/core/src/main/scala/com/typesafe/tools/mima/core/ClassfileParser.scala index a6319476..837f27a4 100644 --- a/core/src/main/scala/com/typesafe/tools/mima/core/ClassfileParser.scala +++ b/core/src/main/scala/com/typesafe/tools/mima/core/ClassfileParser.scala @@ -267,6 +267,17 @@ abstract class ClassfileParser(definitions: Definitions) { val attrNameIndex = in.nextChar c.sourceFileName = pool.getName(attrNameIndex) } + } else if (attrName == "InnerClasses") { + val entries = in.nextChar.toInt + c._innerClasses = (0 until entries).map { _ => + val innerIndex, outerIndex, innerNameIndex = in.nextChar.toInt + in.skip(2) + if (innerIndex != 0 && outerIndex != 0 && innerNameIndex != 0) { + val n = pool.getClassName(innerIndex) + if (n == c.bytecodeName) c._isTopLevel = false // an inner class lists itself in InnerClasses + if (pool.getClassName(outerIndex) == c.bytecodeName) n else "" + } else "" + }.filterNot(_.isEmpty) } else if (attrName == "Scala" || attrName == "ScalaSig") { this.parsedClass.isScala = true } diff --git a/core/src/main/scala/com/typesafe/tools/mima/core/PackageInfo.scala b/core/src/main/scala/com/typesafe/tools/mima/core/PackageInfo.scala index a3530c19..18e22b3c 100644 --- a/core/src/main/scala/com/typesafe/tools/mima/core/PackageInfo.scala +++ b/core/src/main/scala/com/typesafe/tools/mima/core/PackageInfo.scala @@ -1,5 +1,6 @@ package com.typesafe.tools.mima.core +import scala.annotation.tailrec import scala.tools.nsc.io.AbstractFile import scala.tools.nsc.util.ClassPath import collection.mutable @@ -64,48 +65,24 @@ abstract class PackageInfo(val owner: PackageInfo) { def classes: mutable.Map[String, ClassInfo] lazy val accessibleClasses: Set[ClassInfo] = { - // FIXME: Make this a tailrecursive implementation /** Fixed point iteration for finding all accessible classes. */ - def accessibleClassesUnder(prefix: Set[ClassInfo]): Set[ClassInfo] = { - val vclasses = (classes.valuesIterator filter (isAccessible(_, prefix))).toSet - if (vclasses.isEmpty) vclasses - else vclasses union accessibleClassesUnder(vclasses) + @tailrec + def accessibleClassesUnder(prefix: Set[ClassInfo], found: Set[ClassInfo]): Set[ClassInfo] = { + val vclasses = (classes.valuesIterator.filter(isAccessible(_, prefix))).toSet + if (vclasses.isEmpty) found + else accessibleClassesUnder(vclasses, vclasses union found) } def isAccessible(clazz: ClassInfo, prefix: Set[ClassInfo]) = { def isReachable = { if (clazz.isSynthetic) false - else { - val idx = clazz.decodedName.lastIndexOf("$") - if (idx < 0) prefix.isEmpty // class name contains no $ - else { - // A top-level module is compiled into a class plus a module class (for instance, - // for a top-level `object A`, scalac creates two classfiles `A.class` and `A$.class`.). - // While, for an inner module, scalac creates only one classfile, i.e., the module class - // classfile. - // - // `clazz` is an inner module if: - // 1) `clazz` is a module, and - // 2) `clazz` decoded name starts with one of the passed `prefix`. - def isInnerModule: Boolean = { - clazz.isModule && - prefix.exists { outer => - // this condition is to avoid looping indefinitely - clazz.decodedName != outer.decodedName && - // checks if `outer` is a prefix of `clazz` name - clazz.decodedName.startsWith(outer.decodedName) - } - } - // class, trait, and top-level module go through this condition - (prefix exists (_.decodedName == clazz.decodedName.substring(0, idx))) || // prefix before dollar is an accessible class detected previously - isInnerModule - } - } + else if (prefix.isEmpty) clazz.isTopLevel && !clazz.bytecodeName.contains("$$") + else prefix.exists(_.innerClasses contains clazz.bytecodeName) } clazz.isPublic && isReachable } - accessibleClassesUnder(Set.empty) + accessibleClassesUnder(Set.empty, Set.empty) } /** All implementation classes of traits (classes that end in "$" followed by "class"). */ diff --git a/reporter/functional-tests/src/test/object-removing-inner-object-nok/problems.txt b/reporter/functional-tests/src/test/object-removing-inner-object-nok/problems.txt index e4037ccc..f32b7751 100644 --- a/reporter/functional-tests/src/test/object-removing-inner-object-nok/problems.txt +++ b/reporter/functional-tests/src/test/object-removing-inner-object-nok/problems.txt @@ -1 +1,2 @@ -object A#B does not have a correspondent in new version \ No newline at end of file +object A#B does not have a correspondent in new version +object A#B#C does not have a correspondent in new version diff --git a/reporter/functional-tests/src/test/object-removing-inner-object-nok/v1/A.scala b/reporter/functional-tests/src/test/object-removing-inner-object-nok/v1/A.scala index 598070d7..1d74855d 100644 --- a/reporter/functional-tests/src/test/object-removing-inner-object-nok/v1/A.scala +++ b/reporter/functional-tests/src/test/object-removing-inner-object-nok/v1/A.scala @@ -1,5 +1,6 @@ object A { object B { def foo: Int = 2 + object C } }