Merge pull request #2261 from Duhemm/fix-1171

Consider signatures of methods before and after erasure in ExtractAPI

compile/interface/src/main/scala/xsbt/Compat.scala

@@ -45,6 +45,17 @@ abstract class Compat {
   val Nullary = global.NullaryMethodType
   val ScalaObjectClass = definitions.ScalaObjectClass
 
+  // `afterPostErasure` doesn't exist in Scala < 2.10
+  implicit def withAfterPostErasure(global: Global): WithAfterPostErasure = new WithAfterPostErasure(global)
+  class WithAfterPostErasure(global: Global) {
+    def afterPostErasure[T](op: => T): T = op
+  }
+  // `exitingPostErasure` was called `afterPostErasure` in 2.10
+  implicit def withExitingPostErasure(global: Global): WithExitingPostErasure = new WithExitingPostErasure(global)
+  class WithExitingPostErasure(global: Global) {
+    def exitingPostErasure[T](op: => T): T = global afterPostErasure op
+  }
+
   private[this] final class MiscCompat {
     // in 2.9, nme.LOCALCHILD was renamed to tpnme.LOCAL_CHILD
     def tpnme = nme
@@ -75,6 +86,7 @@ abstract class Compat {
 
     def enclosingTopLevelClass: Symbol = sym.toplevelClass
     def toplevelClass: Symbol = sourceCompatibilityOnly
+    def asMethod: MethodSymbol = sym.asInstanceOf[MethodSymbol]
   }
 
   val DummyValue = 0
@@ -89,6 +101,21 @@ abstract class Compat {
 
   private[this] final implicit def miscCompat(n: AnyRef): MiscCompat = new MiscCompat
 
+  object MirrorHelper {
+
+    private implicit def withRootMirror(x: Any): WithRootMirror = new WithRootMirror(x)
+    private class DummyMirror {
+      def getClassIfDefined(x: String): Symbol = NoSymbol
+    }
+    private class WithRootMirror(x: Any) {
+      def rootMirror: DummyMirror = new DummyMirror
+    }
+    lazy val AnyValClass = global.rootMirror.getClassIfDefined("scala.AnyVal")
+
+    def isAnyValSubtype(sym: Symbol): Boolean = sym.isNonBottomSubClass(AnyValClass)
+
+  }
+
   object MacroExpansionOf {
     def unapply(tree: Tree): Option[Tree] = {
 

compile/interface/src/main/scala/xsbt/ExtractAPI.scala

@@ -178,9 +178,32 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType,
 
   private def viewer(s: Symbol) = (if (s.isModule) s.moduleClass else s).thisType
   private def printMember(label: String, in: Symbol, t: Type) = println(label + " in " + in + " : " + t + " (debug: " + debugString(t) + " )")
-  private def defDef(in: Symbol, s: Symbol) =
+
+  private def defDef(in: Symbol, s: Symbol): List[xsbti.api.Def] =
     {
-      def build(t: Type, typeParams: Array[xsbti.api.TypeParameter], valueParameters: List[xsbti.api.ParameterList]): xsbti.api.Def =
+      import MirrorHelper._
+
+      val hasValueClassAsParameter: Boolean = {
+        import MirrorHelper._
+        s.asMethod.paramss.flatten map (_.info) exists (t => isAnyValSubtype(t.typeSymbol))
+      }
+
+      // Note: We only inspect the "outermost type" (i.e. no recursion) because we don't need to
+      // inspect after erasure a function that would, for instance, return a function that returns
+      // a subtype of AnyVal.
+      val hasValueClassAsReturnType: Boolean = {
+        val tpe = viewer(in).memberInfo(s)
+        tpe match {
+          case PolyType(_, base) => isAnyValSubtype(base.typeSymbol)
+          case MethodType(_, resultType) => isAnyValSubtype(resultType.typeSymbol)
+          case Nullary(resultType) => isAnyValSubtype(resultType.typeSymbol)
+          case resultType => isAnyValSubtype(resultType.typeSymbol)
+        }
+      }
+
+      val inspectPostErasure = hasValueClassAsParameter || hasValueClassAsReturnType
+
+      def build(t: Type, typeParams: Array[xsbti.api.TypeParameter], valueParameters: List[xsbti.api.ParameterList]): List[xsbti.api.Def] =
         {
           def parameterList(syms: List[Symbol]): xsbti.api.ParameterList =
             {
@@ -192,13 +215,56 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType,
               assert(typeParams.isEmpty)
               assert(valueParameters.isEmpty)
               build(base, typeParameters(in, typeParams0), Nil)
-            case MethodType(params, resultType) =>
-              build(resultType, typeParams, parameterList(params) :: valueParameters)
+            case mType @ MethodType(params, resultType) =>
+              // The types of a method's parameters change between phases: For instance, if a
+              // parameter is a subtype of AnyVal, then it won't have the same type before and after
+              // erasure. Therefore we record the type of parameters before AND after erasure to
+              // make sure that we don't miss some API changes.
+              //   class A(val x: Int) extends AnyVal
+              //   def foo(a: A): Int = A.x <- has type (LA)I before erasure
+              //                            <- has type (I)I after erasure
+              // If we change A from value class to normal class, we need to recompile all clients
+              // of def foo.
+              val beforeErasure =
+                build(resultType, typeParams, parameterList(params) :: valueParameters)
+              val afterErasure  =
+                if (inspectPostErasure)
+                  build(resultType, typeParams, global exitingPostErasure (parameterList(mType.params) :: valueParameters))
+                else
+                  Nil
+
+              beforeErasure ++ afterErasure
             case Nullary(resultType) => // 2.9 and later
               build(resultType, typeParams, valueParameters)
             case returnType =>
-              val t2 = processType(in, dropConst(returnType))
-              new xsbti.api.Def(valueParameters.reverse.toArray, t2, typeParams, simpleName(s), getAccess(s), getModifiers(s), annotations(in, s))
+              def makeDef(retTpe: xsbti.api.Type): xsbti.api.Def =
+                new xsbti.api.Def(
+                  valueParameters.reverse.toArray,
+                  retTpe,
+                  typeParams,
+                  simpleName(s),
+                  getAccess(s),
+                  getModifiers(s),
+                  annotations(in, s))
+
+              // The return type of a method may change before and after erasure. Consider the
+              // following method:
+              //   class A(val x: Int) extends AnyVal
+              //   def foo(x: Int): A = new A(x) <- has type (I)LA before erasure
+              //                                 <- has type (I)I after erasure
+              // If we change A from value class to normal class, we need to recompile all clients
+              // of def foo.
+              val beforeErasure = makeDef(processType(in, dropConst(returnType)))
+              val afterErasure =
+                if (inspectPostErasure) {
+                  val erasedReturn = dropConst(global exitingPostErasure viewer(in).memberInfo(s)) map {
+                    case MethodType(_, r) => r
+                    case other            => other
+                  }
+                  List(makeDef(processType(in, erasedReturn)))
+                } else Nil
+
+              beforeErasure :: afterErasure
           }
         }
       def parameterS(s: Symbol): xsbti.api.MethodParameter =
@@ -292,22 +358,22 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType,
     defs
   }
 
-  private def definition(in: Symbol, sym: Symbol): Option[xsbti.api.Definition] =
+  private def definition(in: Symbol, sym: Symbol): List[xsbti.api.Definition] =
     {
-      def mkVar = Some(fieldDef(in, sym, false, new xsbti.api.Var(_, _, _, _, _)))
-      def mkVal = Some(fieldDef(in, sym, true, new xsbti.api.Val(_, _, _, _, _)))
+      def mkVar = List(fieldDef(in, sym, false, new xsbti.api.Var(_, _, _, _, _)))
+      def mkVal = List(fieldDef(in, sym, true, new xsbti.api.Val(_, _, _, _, _)))
       if (isClass(sym))
-        if (ignoreClass(sym)) None else Some(classLike(in, sym))
+        if (ignoreClass(sym)) Nil else List(classLike(in, sym))
       else if (sym.isNonClassType)
-        Some(typeDef(in, sym))
+        List(typeDef(in, sym))
       else if (sym.isVariable)
-        if (isSourceField(sym)) mkVar else None
+        if (isSourceField(sym)) mkVar else Nil
       else if (sym.isStable)
-        if (isSourceField(sym)) mkVal else None
+        if (isSourceField(sym)) mkVal else Nil
       else if (sym.isSourceMethod && !sym.isSetter)
-        if (sym.isGetter) mkVar else Some(defDef(in, sym))
+        if (sym.isGetter) mkVar else defDef(in, sym)
       else
-        None
+        Nil
     }
   private def ignoreClass(sym: Symbol): Boolean =
     sym.isLocalClass || sym.isAnonymousClass || sym.fullName.endsWith(LocalChild.toString)
@@ -530,4 +596,4 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType,
       if (annots.isEmpty) processType(in, at.underlying) else annotated(in, annots, at.underlying)
     }
 
-}
+}

notes/0.13.10/consider-signatures-after-erasure.md

@@ -0,0 +1,12 @@
+
+  [@Duhemm]: http://github.com/Duhemm
+  [1171]: https://github.com/sbt/sbt/issues/1171
+  [2261]: https://github.com/sbt/sbt/pull/2261
+
+### Fixes with compatibility implications
+
+### Improvements
+- Register signatures of method before and after erasure if they involve value classes [#2261][2261] by [@Duhemm][@Duhemm]
+
+### Bug fixes
+- Incremental compiler misses change to value class, and results to NoSuchMethodError at runtime [#1171][1171]

sbt/src/sbt-test/source-dependencies/value-class/build.sbt

@@ -0,0 +1,2 @@
+incOptions := incOptions.value.withRecompileAllFraction(1.0)
+

sbt/src/sbt-test/source-dependencies/value-class/changes/A0.scala

@@ -0,0 +1 @@
+class A(val x: Int)

sbt/src/sbt-test/source-dependencies/value-class/changes/A1.scala

@@ -0,0 +1 @@
+class A(val x: Int) extends AnyVal

sbt/src/sbt-test/source-dependencies/value-class/changes/B0.scala

@@ -0,0 +1,3 @@
+class B {
+  def foo(a: A): Int = 1
+}

sbt/src/sbt-test/source-dependencies/value-class/changes/B1.scala

@@ -0,0 +1,3 @@
+class B {
+  def bar: A = new A(0)
+}

sbt/src/sbt-test/source-dependencies/value-class/changes/C0.scala

@@ -0,0 +1,3 @@
+object C extends App {
+  println(new B().foo(null))
+}

sbt/src/sbt-test/source-dependencies/value-class/changes/C1.scala

@@ -0,0 +1,3 @@
+object C extends App {
+  println(new B().bar.x)
+}

sbt/src/sbt-test/source-dependencies/value-class/test

@@ -0,0 +1,30 @@
+$ copy-file changes/A0.scala src/main/scala/A.scala
+$ copy-file changes/B0.scala src/main/scala/B.scala
+$ copy-file changes/C0.scala src/main/scala/C.scala
+
+# A is a normal class. B.foo accepts a parameter of type A. C calls B.foo, giving it `null`.
+> compile
+> run
+
+# Make A a value class.
+$ copy-file changes/A1.scala src/main/scala/A.scala
+
+# The code no longer compiles because B.foo no longer accepts `null` as an argument.
+# This means that we have invalidated C.scala, as expected!
+-> compile
+
+$ copy-file changes/A0.scala src/main/scala/A.scala
+$ copy-file changes/B1.scala src/main/scala/B.scala
+$ copy-file changes/C1.scala src/main/scala/C.scala
+
+# A is a normal class. B.bar takes no arguments and returns an instance of A. C calls B.bar.
+> compile
+> run
+
+# Make A a value class.
+$ copy-file changes/A1.scala src/main/scala/A.scala
+
+# The code compiles. It will run iff C is recompiled because the signature of B.bar has changed,
+# because A is now a value class.
+> run
+