diff --git a/build.sbt b/build.sbt index 9fd4f9173..1569ec94e 100644 --- a/build.sbt +++ b/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" diff --git a/compile/api/src/main/scala/sbt/ClassToAPI.scala b/compile/api/src/main/scala/sbt/ClassToAPI.scala index fbfe1e930..c2f47c231 100644 --- a/compile/api/src/main/scala/sbt/ClassToAPI.scala +++ b/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) = - { - val methods = mergeMap(c, c.getDeclaredMethods, c.getMethods, methodToDef(enclPkg)) - val fields = mergeMap(c, c.getDeclaredFields, c.getFields, fieldToDef(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 all = methods ++ fields ++ constructors ++ classes - val parentJavaTypes = allSuperTypes(c) - if (!Modifier.isPrivate(c.getModifiers)) - cmap.inherited ++= parentJavaTypes.collect { case c: Class[_] => c } - val parentTypes = types(parentJavaTypes) - 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)) - (staticStructure, instanceStructure) - } + 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 fields = mergeMap(c, c.getDeclaredFields, c.getFields, fieldToDef(c, cf, 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 all = methods ++ fields ++ constructors ++ classes + val parentJavaTypes = allSuperTypes(c) + if (!Modifier.isPrivate(c.getModifiers)) + cmap.inherited ++= parentJavaTypes.collect { case c: Class[_] => c } + val parentTypes = types(parentJavaTypes) + 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)) + (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)) - if (mods.isFinal) new api.Val(tpe, name, accs, mods, annots) else new api.Var(tpe, name, accs, mods, annots) + val fieldTpe = reference(returnType(f)) + // 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) diff --git a/compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java b/compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java new file mode 100644 index 000000000..818ca5954 --- /dev/null +++ b/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; +} diff --git a/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala b/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala index aadfab796..4eef68b22 100644 --- a/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala +++ b/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)} - issue errors and warnings ${findsErrors(local)} + compile a java file ${works(local)} + issue errors and warnings ${findsErrors(local)} Compiling a file with forked javac should - compile a java file ${works(forked)} - issue errors and warnings ${findsErrors(forked)} - yield the same errors as local javac $forkSameAsLocal + compile a java file ${works(forked)} + issue errors and warnings ${findsErrors(forked)} + yield the same errors as local javac $forkSameAsLocal Documenting a file with forked javadoc should - document a java file ${docWorks(forked)} - find errors in a java file ${findsDocErrors(forked)} + document a java file ${docWorks(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) -} \ No newline at end of file + + def hasStaticFinalFile = + new java.io.File(getClass.getResource("hasstaticfinal.java").toURI) + +} diff --git a/notes/0.13.9/java-constant-fields.markdown b/notes/0.13.9/java-constant-fields.markdown new file mode 100644 index 000000000..50bd193f2 --- /dev/null +++ b/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] diff --git a/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala b/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala index 32ab93d6b..d230dbfe9 100644 --- a/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala +++ b/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 { @@ -61,4 +84,4 @@ private[sbt] object Constants { final val ConstantMethodType = 16 final val ConstantInvokeDynamic = 18 final val ClassDescriptor = 'L' -} \ No newline at end of file +} diff --git a/util/classfile/src/main/scala/sbt/classfile/Parser.scala b/util/classfile/src/main/scala/sbt/classfile/Parser.scala index 97edaf12d..16c5bf656 100644 --- a/util/classfile/src/main/scala/sbt/classfile/Parser.scala +++ b/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) }