Merge pull request #2085 from twitter-forks/stuhood/java-static-final-fields-as-singletons

Encode static-final constant fields as Singletons

build.sbt

@@ -119,7 +119,7 @@ lazy val interfaceProj = (project in file("interface")).
 // defines operations on the API of a source, including determining whether it has changed and converting it to a string
 //   and discovery of Projclasses and annotations
 lazy val apiProj = (project in compilePath / "api").
-  dependsOn(interfaceProj).
+  dependsOn(interfaceProj, classfileProj).
   settings(
     testedBaseSettings,
     name := "API"

compile/api/src/main/scala/sbt/ClassToAPI.scala

@@ -3,6 +3,7 @@ package sbt
 import java.lang.reflect.{ Array => _, _ }
 import java.lang.annotation.Annotation
 import annotation.tailrec
+import sbt.classfile.ClassFile
 import xsbti.api
 import xsbti.SafeLazy
 import SafeLazy.strict
@@ -67,21 +68,28 @@ object ClassToAPI {
     }
 
   /** Returns the (static structure, instance structure, inherited classes) for `c`. */
-  def structure(c: Class[_], enclPkg: Option[String], cmap: ClassMap): (api.Structure, api.Structure) =
+  def structure(c: Class[_], enclPkg: Option[String], cmap: ClassMap): (api.Structure, api.Structure) = {
-    {
+    lazy val cf = classFileForClass(c)
-      val methods = mergeMap(c, c.getDeclaredMethods, c.getMethods, methodToDef(enclPkg))
+    val methods = mergeMap(c, c.getDeclaredMethods, c.getMethods, methodToDef(enclPkg))
-      val fields = mergeMap(c, c.getDeclaredFields, c.getFields, fieldToDef(enclPkg))
+    val fields = mergeMap(c, c.getDeclaredFields, c.getFields, fieldToDef(c, cf, enclPkg))
-      val constructors = mergeMap(c, c.getDeclaredConstructors, c.getConstructors, constructorToDef(enclPkg))
+    val constructors = mergeMap(c, c.getDeclaredConstructors, c.getConstructors, constructorToDef(enclPkg))
-      val classes = merge[Class[_]](c, c.getDeclaredClasses, c.getClasses, toDefinitions(cmap), (_: Seq[Class[_]]).partition(isStatic), _.getEnclosingClass != c)
+    val classes = merge[Class[_]](c, c.getDeclaredClasses, c.getClasses, toDefinitions(cmap), (_: Seq[Class[_]]).partition(isStatic), _.getEnclosingClass != c)
-      val all = methods ++ fields ++ constructors ++ classes
+    val all = methods ++ fields ++ constructors ++ classes
-      val parentJavaTypes = allSuperTypes(c)
+    val parentJavaTypes = allSuperTypes(c)
-      if (!Modifier.isPrivate(c.getModifiers))
+    if (!Modifier.isPrivate(c.getModifiers))
-        cmap.inherited ++= parentJavaTypes.collect { case c: Class[_] => c }
+      cmap.inherited ++= parentJavaTypes.collect { case c: Class[_] => c }
-      val parentTypes = types(parentJavaTypes)
+    val parentTypes = types(parentJavaTypes)
-      val instanceStructure = new api.Structure(lzyS(parentTypes.toArray), lzyS(all.declared.toArray), lzyS(all.inherited.toArray))
+    val instanceStructure = new api.Structure(lzyS(parentTypes.toArray), lzyS(all.declared.toArray), lzyS(all.inherited.toArray))
-      val staticStructure = new api.Structure(lzyEmptyTpeArray, lzyS(all.staticDeclared.toArray), lzyS(all.staticInherited.toArray))
+    val staticStructure = new api.Structure(lzyEmptyTpeArray, lzyS(all.staticDeclared.toArray), lzyS(all.staticInherited.toArray))
-      (staticStructure, instanceStructure)
+    (staticStructure, instanceStructure)
-    }
+  }
+
+  /** TODO: over time, ClassToAPI should switch the majority of access to the classfile parser */
+  private[this] def classFileForClass(c: Class[_]): ClassFile = {
+    val file = new java.io.File(IO.classLocationFile(c), s"${c.getName.replace('.', '/')}.class")
+    classfile.Parser.apply(file)
+  }
+
   private[this] def lzyS[T <: AnyRef](t: T): xsbti.api.Lazy[T] = lzy(t)
   def lzy[T <: AnyRef](t: => T): xsbti.api.Lazy[T] = xsbti.SafeLazy(t)
   private[this] def lzy[T <: AnyRef](t: => T, cmap: ClassMap): xsbti.api.Lazy[T] = {
@@ -127,16 +135,59 @@ object ClassToAPI {
   def upperBounds(ts: Array[Type]): api.Type =
     new api.Structure(lzy(types(ts)), lzyEmptyDefArray, lzyEmptyDefArray)
 
-  def fieldToDef(enclPkg: Option[String])(f: Field): api.FieldLike =
+  @deprecated("Use fieldToDef[4] instead", "0.13.9")
+  def fieldToDef(enclPkg: Option[String])(f: Field): api.FieldLike = {
+    val c = f.getDeclaringClass()
+    fieldToDef(c, classFileForClass(c), enclPkg)(f)
+  }
+
+  def fieldToDef(c: Class[_], cf: => ClassFile, enclPkg: Option[String])(f: Field): api.FieldLike =
     {
       val name = f.getName
       val accs = access(f.getModifiers, enclPkg)
       val mods = modifiers(f.getModifiers)
       val annots = annotations(f.getDeclaredAnnotations)
-      val tpe = reference(returnType(f))
+      val fieldTpe = reference(returnType(f))
-      if (mods.isFinal) new api.Val(tpe, name, accs, mods, annots) else new api.Var(tpe, name, accs, mods, annots)
+      // generate a more specific type for constant fields
+      val specificTpe: Option[api.Type] =
+        if (mods.isFinal) {
+          try {
+            cf.constantValue(name).map(singletonForConstantField(c, f, _))
+          } catch {
+            case e: Throwable =>
+              throw new IllegalStateException(
+                s"Failed to parse class $c: this may mean your classfiles are corrupted. Please clean and try again.",
+                e
+              )
+          }
+        } else {
+          None
+        }
+      val tpe = specificTpe.getOrElse(fieldTpe)
+      if (mods.isFinal) {
+        new api.Val(tpe, name, accs, mods, annots)
+      } else {
+        new api.Var(tpe, name, accs, mods, annots)
+      }
     }
 
+  /**
+   * Creates a Singleton type that includes both the type and ConstantValue for the given Field.
+   *
+   * Since java compilers are allowed to inline constant (static final primitive) fields in
+   * downstream classfiles, we generate a type that will cause APIs to match only when both
+   * the type and value of the field match. We include the classname mostly for readability.
+   *
+   * Because this type is purely synthetic, it's fine that the name might contain filename-
+   * banned characters.
+   */
+  private def singletonForConstantField(c: Class[_], field: Field, constantValue: AnyRef) =
+    new api.Singleton(
+      pathFromStrings(
+        c.getName.split("\\.").toSeq :+ (field.getName + "$" + returnType(field) + "$" + constantValue)
+      )
+    )
+
   def methodToDef(enclPkg: Option[String])(m: Method): api.Def =
     defLike(m.getName, m.getModifiers, m.getDeclaredAnnotations, typeParameterTypes(m), m.getParameterAnnotations, parameterTypes(m), Option(returnType(m)), exceptionTypes(m), m.isVarArgs, enclPkg)
 
@@ -256,7 +307,9 @@ object ClassToAPI {
     }
 
   def pathFromString(s: String): api.Path =
-    new api.Path(s.split("\\.").map(new api.Id(_)) :+ ThisRef)
+    pathFromStrings(s.split("\\."))
+  def pathFromStrings(ss: Seq[String]): api.Path =
+    new api.Path((ss.map(new api.Id(_)) :+ ThisRef).toArray)
   def packageName(c: Class[_]) = packageAndName(c)._1
   def packageAndName(c: Class[_]): (Option[String], String) =
     packageAndName(c.getName)

compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java

@@ -0,0 +1,4 @@
+public class hasstaticfinal {
+    // the `TYPE` and `VALUE` strings are replaced with various values during tests
+    public static final TYPE HELLO = VALUE;
+}

compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala

@@ -5,6 +5,9 @@ import java.net.URLClassLoader
 
 import sbt._
 import org.specs2.Specification
+import org.specs2.matcher.MatchResult
+import xsbt.api.{ SameAPI, DefaultShowAPI }
+import xsbti.api.SourceAPI
 import xsbti.{ Severity, Problem }
 
 object JavaCompilerSpec extends Specification {
@@ -14,20 +17,25 @@ object JavaCompilerSpec extends Specification {
 
 
   Compiling a java file with local javac should
-    compile a java file                      ${works(local)}
+    compile a java file                                             ${works(local)}
-    issue errors and warnings                ${findsErrors(local)}
+    issue errors and warnings                                       ${findsErrors(local)}
 
   Compiling a file with forked javac should
-     compile a java file                     ${works(forked)}
+    compile a java file                                             ${works(forked)}
-     issue errors and warnings               ${findsErrors(forked)}
+    issue errors and warnings                                       ${findsErrors(forked)}
-     yield the same errors as local javac    $forkSameAsLocal
+    yield the same errors as local javac                            $forkSameAsLocal
 
   Documenting a file with forked javadoc should
-      document a java file                  ${docWorks(forked)}
+    document a java file                                            ${docWorks(forked)}
-      find errors in a java file            ${findsDocErrors(forked)}
+    find errors in a java file                                      ${findsDocErrors(forked)}
+
+  Analyzing classes generated by javac should result in
+    matching APIs for stable static-final fields                    ${analyzeStaticDifference("String", "\"A\"", "\"A\"")}
+    different APIs for static-final fields with changed values      ${analyzeStaticDifference("String", "\"A\"", "\"B\"")}
+    different APIs for static-final fields with changed types       ${analyzeStaticDifference("String", "\"1\"", "int", "1")}
+    "safe" singleton type names                                     ${analyzeStaticDifference("float", "0.123456789f", "0.123456789f")}
   """
 
-  // TODO - write a test to ensure that java .class files wind up in the right spot, and we can call the compiled java code.
   def docWorks(compiler: JavaTools) = IO.withTemporaryDirectory { out =>
     val (result, problems) = doc(compiler, Seq(knownSampleGoodFile), Seq("-d", out.getAbsolutePath))
     val compiled = result must beTrue
@@ -64,6 +72,41 @@ object JavaCompilerSpec extends Specification {
     errored and foundErrorAndWarning and hasKnownErrors
   }
 
+  /**
+   * Compiles with the given constant values, and confirms that if the strings mismatch, then the
+   * the APIs mismatch.
+   */
+  def analyzeStaticDifference(typeName: String, left: String, right: String): MatchResult[Boolean] =
+    analyzeStaticDifference(typeName, left, typeName, right)
+
+  def analyzeStaticDifference(leftType: String, left: String, rightType: String, right: String): MatchResult[Boolean] = {
+    def compileWithPrimitive(templateType: String, templateValue: String) =
+      IO.withTemporaryDirectory { out =>
+        // copy the input file to a temporary location and change the templateValue
+        val input = new File(out, hasStaticFinalFile.getName())
+        IO.writeLines(
+          input,
+          IO.readLines(hasStaticFinalFile).map { line =>
+            line.replace("TYPE", templateType).replace("VALUE", templateValue)
+          }
+        )
+
+        // then compile it
+        val (result, problems) = compile(local, Seq(input), Seq("-d", out.getAbsolutePath))
+        val origCompiled = result must beTrue
+        val clazzz = new URLClassLoader(Array(out.toURI.toURL)).loadClass("hasstaticfinal")
+        (origCompiled, ClassToAPI(Seq(clazzz)))
+      }
+
+    // compile with two different primitive values, and confirm that they match if their
+    // values match
+    val (leftCompiled, leftAPI) = compileWithPrimitive(leftType, left)
+    val (rightCompiled, rightAPI) = compileWithPrimitive(rightType, right)
+    val apisExpectedMatch = SameAPI(leftAPI, rightAPI) must beEqualTo(left == right)
+
+    leftCompiled and rightCompiled and apisExpectedMatch
+  }
+
   def lineMatches(p: Problem, lineno: Int): Boolean =
     p.position.line.isDefined && (p.position.line.get == lineno)
   def isError(p: Problem): Boolean = p.severity == Severity.Error
@@ -131,4 +174,8 @@ object JavaCompilerSpec extends Specification {
 
   def knownSampleGoodFile =
     new java.io.File(getClass.getResource("good.java").toURI)
+
+  def hasStaticFinalFile =
+    new java.io.File(getClass.getResource("hasstaticfinal.java").toURI)
+
 }

notes/0.13.9/java-constant-fields.markdown

@@ -0,0 +1,17 @@
+  [@stuhood]: https://github.com/stuhood
+  [@jsuereth]: https://github.com/jsuereth
+  [@adriaanm]: https://github.com/adriaanm
+
+  [1967]: https://github.com/sbt/sbt/issues/1967
+  [2085]: https://github.com/sbt/sbt/pull/2085
+
+### Changes with compatibility implications
+
+### Improvements
+
+### Fixes
+
+- Changing the value of a constant (final-static-primitive) field will now
+  correctly trigger incremental compilation for downstream classes. This is to
+  account for the fact that java compilers may inline constant fields in
+  downstream classes. [#1967][1967]/[#2085][2085] by [@stuhood][@stuhood]

util/classfile/src/main/scala/sbt/classfile/ClassFile.scala

@@ -22,6 +22,29 @@ private[sbt] trait ClassFile {
   val sourceFile: Option[String]
   def types: Set[String]
   def stringValue(a: AttributeInfo): String
+
+  /**
+   * If the given fieldName represents a ConstantValue field, parses its representation from
+   * the constant pool and returns it.
+   */
+  def constantValue(fieldName: String): Option[AnyRef] =
+    this.fields.find(_.name.exists(_ == fieldName)).toSeq.flatMap(_.attributes).collectFirst {
+      case ai @ classfile.AttributeInfo(Some("ConstantValue"), _) =>
+        constantPool(Parser.entryIndex(ai))
+    }.map {
+      case Constant(ConstantString, nextOffset, _, _) =>
+        // follow the indirection from ConstantString to ConstantUTF8
+        val nextConstant = constantPool(nextOffset)
+        nextConstant.value.getOrElse {
+          throw new IllegalStateException(s"Empty UTF8 value in constant pool: $nextConstant")
+        }
+      case constant @ Constant((ConstantFloat | ConstantLong | ConstantDouble | ConstantInteger), _, _, ref) =>
+        ref.getOrElse {
+          throw new IllegalStateException(s"Empty primitive value in constant pool: $constant")
+        }
+      case constant =>
+        throw new IllegalStateException(s"Unsupported ConstantValue type: $constant")
+    }
 }
 
 private[sbt] final case class Constant(tag: Byte, nameIndex: Int, typeIndex: Int, value: Option[AnyRef]) extends NotNull {

util/classfile/src/main/scala/sbt/classfile/Parser.scala

@@ -157,8 +157,9 @@ private[sbt] object Parser {
     }
 
   private def toInt(v: Byte) = if (v < 0) v + 256 else v.toInt
-  private def entryIndex(a: AttributeInfo) =
+  def entryIndex(a: AttributeInfo) =
     {
+      require(a.value.length == 2, s"Expected two bytes for unsigned value; got: ${a.value.length}")
       val Array(v0, v1) = a.value
       toInt(v0) * 256 + toInt(v1)
     }