Merge pull request #778 from jeroentervoorde/private-constructors

Ignore private constructors

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

@@ -10,12 +10,14 @@ sealed abstract class MemberInfo(val owner: ClassInfo, val bytecodeName: String,
   final var isDeprecated: Boolean  = false
   final var signature: Signature   = Signature.none // Includes generics. 'descriptor' is the erased version.
   final var scopedPrivate: Boolean = false
+  final var classPrivate: Boolean = false
 
   def nonAccessible: Boolean
 
   final def fullName: String          = s"${owner.formattedFullName}.$decodedName"
   final def abstractPrefix            = if (isDeferred && !owner.isTrait) "abstract " else ""
-  final def scopedPrivatePrefix       = if (scopedPrivate) "private[..] " else ""
+  final def scopedPrivatePrefix       = "private[..] "
+  final def classPrivatePrefix        = "private "
   final def staticPrefix: String      = if (isStatic) "static " else ""
   final def tpe: Type                 = owner.owner.definitions.fromDescriptor(descriptor)
   final def hasSyntheticName: Boolean = decodedName.contains('$')
@@ -44,7 +46,16 @@ private[mima] final class MethodInfo(owner: ClassInfo, bytecodeName: String, fla
   def shortMethodString: String = {
     val prefix = if (hasSyntheticName) if (isExtensionMethod) "extension " else "synthetic " else ""
     val deprecated = if (isDeprecated) "deprecated " else ""
-    s"${scopedPrivatePrefix}${abstractPrefix}$prefix${deprecated}${staticPrefix}method $decodedName$tpe"
+
+    val privatePrefix = if (isScopedPrivate) {
+      scopedPrivatePrefix
+    } else if (isClassPrivate) {
+      classPrivatePrefix
+    } else {
+      ""
+    }
+
+    s"${privatePrefix}${abstractPrefix}$prefix${deprecated}${staticPrefix}method $decodedName$tpe"
   }
 
   lazy val paramsCount: Int = {
@@ -67,10 +78,12 @@ private[mima] final class MethodInfo(owner: ClassInfo, bytecodeName: String, fla
     decodedName.substring(0, i + 1).endsWith("$extension")
   }
   def nonAccessible: Boolean = {
-    !isPublic || isScopedPrivate || isSynthetic ||
+    !isPublic || isScopedPrivate || isClassPrivate || isSynthetic ||
       (hasSyntheticName && !(isExtensionMethod || isDefaultGetter || isTraitInit))
   }
   def isScopedPrivate: Boolean = scopedPrivate
 
+  def isClassPrivate: Boolean = classPrivate
+
   override def toString = s"def $bytecodeName: $descriptor"
 }

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

@@ -1,6 +1,6 @@
 package com.typesafe.tools.mima.core
 
-import PickleFormat._
+import com.typesafe.tools.mima.core.PickleFormat.*
 
 object MimaUnpickler {
   def unpickleClass(buf: PickleBuffer, clazz: ClassInfo, path: String): Unit = {
@@ -174,7 +174,6 @@ object MimaUnpickler {
     def doMethods(clazz: ClassInfo, methods: List[SymbolInfo]) = {
       methods.iterator
         .filter(!_.isParam)
-        .filter(_.name.value != CONSTRUCTOR) // TODO support package private constructors
         .toSeq.groupBy(_.name).foreach { case (name, pickleMethods) =>
           doMethodOverloads(clazz, name, pickleMethods)
         }
@@ -192,9 +191,10 @@ object MimaUnpickler {
       // then implementing the rules of erasure, so that you can then match the pickle
       // types with the erased types.  Meanwhile we'll just ignore them, worst case users
       // need to add a filter like they have for years.
-      if (pickleMethods.size == bytecodeMethods.size && pickleMethods.exists(_.isScopedPrivate)) {
+      if (pickleMethods.size == bytecodeMethods.size && pickleMethods.exists(m => m.isScopedPrivate || m.isClassPrivate)) {
         bytecodeMethods.zip(pickleMethods).foreach { case (bytecodeMeth, pickleMeth) =>
           bytecodeMeth.scopedPrivate = pickleMeth.isScopedPrivate
+          bytecodeMeth.classPrivate = pickleMeth.isClassPrivate
         }
       }
     }
@@ -262,6 +262,7 @@ object MimaUnpickler {
     def hasFlag(flag: Long): Boolean = (flags & flag) != 0L
     def isModuleOrModuleClass        = hasFlag(Flags.MODULE_PKL)
     def isParam                      = hasFlag(Flags.PARAM)
+    def isClassPrivate               = hasFlag(Flags.PRIVATE)
   }
   val NoSymbol: SymbolInfo = SymbolInfo(NONEsym, nme.NoSymbol, null, 0, false)
 
@@ -289,6 +290,7 @@ object MimaUnpickler {
   }
 
   object Flags {
+    final val PRIVATE    = 1L << 2
     final val MODULE_PKL = 1L << 10
     final val PARAM      = 1L << 13
   }

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

@@ -19,7 +19,7 @@ object TastyUnpickler {
     val names = readNames(in)
     val tree  = unpickleTree(getTreeReader(in, names), names)
 
-    copyPrivateWithin(tree, clazz.owner)
+    copyPrivateFlags(tree, clazz.owner)
     copyAnnotations(tree, clazz.owner)
   }
 
@@ -73,7 +73,7 @@ object TastyUnpickler {
     }
   }.traverse(tree)
 
-  def copyPrivateWithin(tree: Tree, pkgInfo: PackageInfo): Unit = new ClassTraverser(pkgInfo) {
+  def copyPrivateFlags(tree: Tree, pkgInfo: PackageInfo): Unit = new ClassTraverser(pkgInfo) {
     override def forEachClass(clsDef: ClsDef, cls: ClassInfo): Unit =
       clsDef.privateWithin.foreach(_ => cls.module._scopedPrivate = true)
 
@@ -85,17 +85,21 @@ object TastyUnpickler {
     def doMethods(tmpl: Template) = {
       val clazz = currentClass
       (tmpl.fields ::: tmpl.meths).iterator
-        .filter(_.name != Name.Constructor) // TODO support package private constructors
         .toSeq.groupBy(_.name).foreach { case (name, pickleMethods) =>
           doMethodOverloads(clazz, name, pickleMethods)
         }
     }
 
     def doMethodOverloads(clazz: ClassInfo, name: Name, pickleMethods: Seq[TermMemberDef]) = {
       val bytecodeMethods = clazz.methods.get(name.source).filter(!_.isBridge).toList
-      if (pickleMethods.size == bytecodeMethods.size && pickleMethods.exists(_.privateWithin.isDefined)) {
-        bytecodeMethods.zip(pickleMethods).foreach { case (bytecodeMeth, pickleMeth) =>
-          bytecodeMeth.scopedPrivate = pickleMeth.privateWithin.isDefined
+
+      if (pickleMethods.size == bytecodeMethods.size) {
+        if (pickleMethods.exists(t => t.privateWithin.isDefined || t.classPrivate)) {
+          bytecodeMethods.zip(pickleMethods).foreach { case (bytecodeMeth, pickleMeth) =>
+
+            bytecodeMeth.scopedPrivate = pickleMeth.privateWithin.isDefined
+            bytecodeMeth.classPrivate = pickleMeth.classPrivate
+          }
         }
       }
     }
@@ -144,8 +148,8 @@ object TastyUnpickler {
           val name = readName()
           skipTree(readByte()) // type
           if (!nothingButMods(end)) skipTree(readByte()) // rhs
-          val (privateWithin, annots) = readMods(end)
-          ValDef(name, privateWithin, annots)
+          val (privateWithin, classPrivate, annots) = readMods(end)
+          ValDef(name, privateWithin, classPrivate, annots)
         }
 
         def readDefDef() = {
@@ -156,8 +160,8 @@ object TastyUnpickler {
           while (nextByte == TYPEPARAM || nextByte == PARAM || nextByte == EMPTYCLAUSE || nextByte == SPLITCLAUSE) skipTree(readByte()) // params
           skipTree(readByte()) // returnType
           if (!nothingButMods(end)) skipTree(readByte()) // rhs
-          val (privateWithin, annots) = readMods(end)
-          DefDef(name, privateWithin, annots)
+          val (privateWithin, classPrivate, annots) = readMods(end)
+          DefDef(name, privateWithin, classPrivate, annots)
         }
 
         def readTemplate(): Template = {
@@ -188,25 +192,27 @@ object TastyUnpickler {
         def readClassDef(name: Name, end: Addr) = {
           // NameRef Template Modifier* -- modifiers class name template
           val template = readTemplate()
-          val (privateWithin, annots) = readMods(end)
+          val (privateWithin, classPrivate, annots) = readMods(end)
           ClsDef(name.toTypeName, template, privateWithin, annots)
         }
 
         def readTypeMemberDef(end: Addr) = { goto(end); UnknownTree(TYPEDEF) } // NameRef type_Term Modifier* -- modifiers type name (= type | bounds)
 
-        def readMods(end: Addr): (Option[Type], List[Annot]) = {
+        def readMods(end: Addr): (Option[Type], Boolean, List[Annot]) = {
           //   PRIVATEqualified qualifier_Type --   private[qualifier]
           // PROTECTEDqualified qualifier_Type -- protected[qualifier]
           var privateWithin = Option.empty[Type]
+          var classPrivate = false
           val annots = new ListBuffer[Annot]
           doUntil(end)(readByte() match {
             case ANNOTATION                => annots += readAnnot()
             case PRIVATEqualified          => privateWithin = Some(readType())
             case PROTECTEDqualified        => privateWithin = Some(readType())
+            case PRIVATE                   => classPrivate = true
             case tag if isModifierTag(tag) => skipTree(tag)
             case tag                       => assert(false, s"illegal modifier tag ${astTagToString(tag)} at ${currentAddr.index - 1}, end = $end")
           })
-          (privateWithin, annots.toList)
+          (privateWithin, classPrivate, annots.toList)
         }
 
         def readTypeDef() = {
@@ -271,11 +277,11 @@ object TastyUnpickler {
     def show = s"${showXs(annots, end = " ")}${showPrivateWithin(privateWithin)}class $name$template"
   }
   final case class Template(classes: List[ClsDef], fields: List[ValDef], meths: List[DefDef]) extends Tree { def show = s"${(classes ::: meths).map("\n  " + _).mkString}" }
-  final case class ValDef(name: Name, privateWithin: Option[Type], annots: List[Annot] = Nil) extends TermMemberDef
-  final case class DefDef(name: Name, privateWithin: Option[Type], annots: List[Annot] = Nil) extends TermMemberDef
+  final case class ValDef(name: Name, privateWithin: Option[Type], classPrivate: Boolean, annots: List[Annot] = Nil) extends TermMemberDef
+  final case class DefDef(name: Name, privateWithin: Option[Type], classPrivate: Boolean, annots: List[Annot] = Nil) extends TermMemberDef
 
   sealed trait TermMemberDef extends Tree {
-    def name: Name; def privateWithin: Option[Type]; def annots: List[Annot]
+    def name: Name; def privateWithin: Option[Type]; def classPrivate: Boolean; def annots: List[Annot]
     def show = s"${showXs(annots, end = " ")}${showPrivateWithin(privateWithin)}def $name"
   }
 
@@ -311,9 +317,11 @@ object TastyUnpickler {
     def traversePkg(pkg: Pkg)                              = { val Pkg(path, trees) = pkg; traverse(path); trees.foreach(traverse) }
     def traverseClsDef(clsDef: ClsDef)                     = { val ClsDef(name, tmpl, privateWithin, annots) = clsDef; traverseName(name); traverseTemplate(tmpl); traversePrivateWithin(privateWithin); annots.foreach(traverse) }
     def traverseTemplate(tmpl: Template)                   = { val Template(classes, fields, meths) = tmpl; classes.foreach(traverse); fields.foreach(traverse); meths.foreach(traverse) }
-    def traverseValDef(valDef: ValDef)                     = { val ValDef(name, privateWithin, annots) = valDef; traverseName(name); traversePrivateWithin(privateWithin); annots.foreach(traverse) }
-    def traverseDefDef(defDef: DefDef)                     = { val DefDef(name, privateWithin, annots) = defDef; traverseName(name); traversePrivateWithin(privateWithin); annots.foreach(traverse) }
+    def traverseValDef(valDef: ValDef)                     = { val ValDef(name, privateWithin, _, annots) = valDef; traverseName(name); traversePrivateWithin(privateWithin); annots.foreach(traverse) }
+    def traverseDefDef(defDef: DefDef)                     = { val DefDef(name, privateWithin, _, annots) = defDef; traverseName(name); traversePrivateWithin(privateWithin); annots.foreach(traverse) }
     def traversePrivateWithin(privateWithin: Option[Type]) = { privateWithin.foreach(traverseType) }
+
+    //def traverseClassPrivate(classPrivate: Boolean)        =     { privateWithin.foreach(traverseType) }
     def traverseAnnot(annot: Annot)                        = { val Annot(tycon, fullAnnotation) = annot; traverse(tycon); traverse(fullAnnotation) }
 
     def traversePath(path: Path) = path match {
@@ -552,10 +560,10 @@ object TastyUnpickler {
   }
 
   final case class Header(
-      val header: (Int, Int, Int, Int),
-      val version: (Int, Int, Int),
-      val toolingVersion: String,
-      val uuid: UUID,
+    header: (Int, Int, Int, Int),
+    version: (Int, Int, Int),
+    toolingVersion: String,
+    uuid: UUID,
   )
 
   def readHeader(in: TastyReader): Header = {

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

@@ -1,23 +1,26 @@
 package com.typesafe.tools.mima.lib
 
-import java.io.{ ByteArrayOutputStream, PrintStream }
-import java.net.{ URI, URLClassLoader }
-import javax.tools._
+import com.typesafe.tools.mima.core.ClassPath
 
+import java.io.{ByteArrayOutputStream, PrintStream}
+import java.net.{URI, URLClassLoader}
+import java.nio.charset.StandardCharsets
+import java.nio.file.Files
+import javax.tools._
 import scala.annotation.tailrec
 import scala.collection.JavaConverters._
 import scala.collection.mutable
 import scala.reflect.internal.util.BatchSourceFile
-import scala.reflect.io.{ Directory, Path, PlainFile }
-import scala.util.{ Failure, Success, Try }
-
-import com.typesafe.tools.mima.core.ClassPath
+import scala.reflect.io.{Directory, Path, PlainFile}
+import scala.util.{Failure, Success, Try}
 
 final class TestCase(val baseDir: Directory, val scalaCompiler: ScalaCompiler, val javaCompiler: JavaCompiler) {
   def name               = baseDir.name
   def scalaBinaryVersion = if (scalaCompiler.isScala3) "3" else scalaCompiler.version.take(4)
   def scalaJars          = scalaCompiler.jars
 
+  def skip: Boolean      = (baseDir / s"skip-${scalaBinaryVersion}.txt").exists
+
   val srcV1  = (baseDir / "v1").toDirectory
   val srcV2  = (baseDir / "v2").toDirectory
   val srcApp = (baseDir / "app").toDirectory
@@ -44,8 +47,14 @@ final class TestCase(val baseDir: Directory, val scalaCompiler: ScalaCompiler, v
     if (sourceFiles.forall(_.isJava)) return Success(())
     val bootcp = ClassPath.join(scalaJars.map(_.getPath))
     val cpOpt  = if (cp.isEmpty) Nil else List("-classpath", ClassPath.join(cp.map(_.path)))
+    val optsFile = baseDir / s"scalac-options-${scalaBinaryVersion}.txt"
+    val testOpts = if (optsFile.exists) {
+      Files.readAllLines(optsFile.jfile.toPath, StandardCharsets.UTF_8).asScala.filterNot(_.trim.startsWith("#")).toList
+    } else {
+      List.empty
+    }
     val paths = sourceFiles.map(_.path)
-    val args = "-bootclasspath" :: bootcp :: cpOpt ::: "-d" :: s"$out" :: paths
+    val args = "-bootclasspath" :: bootcp :: testOpts ::: cpOpt ::: "-d" :: s"$out" :: paths
     scalaCompiler.compile(args)
   }
 

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

@@ -13,7 +13,7 @@ object UnitTests {
   } yield ()
 
   def runTestCaseIf(testCase: TestCase, direction: Direction): Try[Unit] = {
-    if ((testCase.baseDir / direction.oracleFile).exists)
+    if ((testCase.baseDir / direction.oracleFile).exists && !testCase.skip)
       runTestCase1(testCase, direction)
     else
       Success(())

functional-tests/src/test/case-class-private-constructor-changes-ok/app/App.scala

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

functional-tests/src/test/case-class-private-constructor-changes-ok/scalac-options-2.13.txt

@@ -0,0 +1,2 @@
+-Xsource:3
+-Wconf:cat=scala3-migration:s

functional-tests/src/test/case-class-private-constructor-changes-ok/skip-2.11.txt

@@ -0,0 +1 @@
+Code does not compile on 2.11 because unapply cannot be redefined

functional-tests/src/test/case-class-private-constructor-changes-ok/skip-2.12.txt

@@ -0,0 +1 @@
+Code does not compile on 2.11 because unapply cannot be redefined

functional-tests/src/test/case-class-private-constructor-changes-ok/v1/A.scala

@@ -0,0 +1,9 @@
+package foo
+
+case class Foo private (a: String)
+
+object Foo {
+  def apply() = new Foo("a")
+
+  private def unapply(f: Foo) = f
+}

functional-tests/src/test/case-class-private-constructor-changes-ok/v2/A.scala

@@ -0,0 +1,9 @@
+package foo
+
+case class Foo private (a: String, b: String) {}
+object Foo {
+  def apply() = new Foo("a", "b")
+  def apply(a: String) = new Foo(a, "b")
+
+  private def unapply(f: Foo) = f
+}

functional-tests/src/test/class-private-constructor-changes-ok/app/App.scala

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

functional-tests/src/test/class-private-constructor-changes-ok/v1/A.scala

@@ -0,0 +1,7 @@
+package foo
+
+class Foo private (val a: String)
+
+object Foo {
+  def apply() = new Foo("a")
+}

functional-tests/src/test/class-private-constructor-changes-ok/v2/A.scala

@@ -0,0 +1,7 @@
+package foo
+
+class Foo private (val a: String, val b: String) {}
+object Foo {
+  def apply() = new Foo("a", "b")
+  def apply(a: String) = new Foo(a, "b")
+}

functional-tests/src/test/package-nested-private-constructor-changes-ok/app/App.scala

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

functional-tests/src/test/package-nested-private-constructor-changes-ok/v1/A.scala

@@ -0,0 +1,9 @@
+package foo
+
+object Foo {
+  class Bar private[foo](a: String) {}
+
+  object Bar {
+    def apply() = new Bar("a")
+  }
+}

functional-tests/src/test/package-nested-private-constructor-changes-ok/v2/A.scala

@@ -0,0 +1,10 @@
+package foo
+
+object Foo {
+  class Bar private[foo](a: Int, val b: String) {}
+
+  object Bar {
+    def apply() = new Bar(123, "b")
+    def apply(a: String) = new Bar(a.toInt, "b")
+  }
+}

functional-tests/src/test/package-private-constructor-changes-ok/app/App.scala

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

functional-tests/src/test/package-private-constructor-changes-ok/v1/A.scala

@@ -0,0 +1,7 @@
+package foo
+
+class Foo private[foo](a: String) {}
+
+object Foo {
+  def apply() = new Foo("a")
+}