From 6846bbd3ed01d98f78546de7d1843c3b2250d832 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Wed, 1 Jul 2015 21:37:32 -0700 Subject: [PATCH 01/10] Encode static-final constant fields as Singletons, so that when the api changes, they change. --- .../api/src/main/scala/sbt/ClassToAPI.scala | 51 +++++++++++++++++-- .../src/main/scala/sbt/classfile/Parser.scala | 3 +- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/compile/api/src/main/scala/sbt/ClassToAPI.scala b/compile/api/src/main/scala/sbt/ClassToAPI.scala index fbfe1e930..0a11686d0 100644 --- a/compile/api/src/main/scala/sbt/ClassToAPI.scala +++ b/compile/api/src/main/scala/sbt/ClassToAPI.scala @@ -70,7 +70,7 @@ object ClassToAPI { 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 fields = mergeMap(c, c.getDeclaredFields, c.getFields, fieldToDef(c, 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 @@ -127,14 +127,53 @@ 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 = + private def constantPoolConstantValue(cf: classfile.ClassFile, ai: classfile.AttributeInfo): AnyRef = { + assert(ai.name.exists(_ == "ConstantValue"), s"Non-ConstantValue attribute not supported: ${ai}") + import classfile.Constants._ + cf.constantPool(classfile.Parser.entryIndex(ai)) match { + case classfile.Constant(ConstantString, nextOffset, _, _) => + // follow the indirection from ConstantString to ConstantUTF8 + val nextConstant = cf.constantPool(nextOffset) + nextConstant.value.getOrElse { + throw new RuntimeException(s"Empty UTF8 value in constant pool: ${nextConstant}") + } + case constant @ classfile.Constant((ConstantFloat | ConstantLong | ConstantDouble | ConstantInteger), _, _, ref) => + ref.getOrElse { + throw new RuntimeException(s"Empty primitive value in constant pool: ${constant}") + } + } + } + + def fieldToDef(c: Class[_], 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 specificTpe: Option[api.Type] = + if (mods.isFinal) { + // TODO: need better way to get access to classfile-location/parsed-representation + // TODO: need to only bother for static fields + val file = new java.io.File(IO.classLocationFile(c), s"${c.getName.replace('.', '/')}.class") + val cf = classfile.Parser.apply(file) + val attributeInfos = cf.fields.find(_.name.exists(_ == name)).toSeq.flatMap(_.attributes) + // create a singleton type ending with the hash of the name-mangled ConstantValue of this field + attributeInfos.collectFirst { + case ai @ classfile.AttributeInfo(Some("ConstantValue"), _) => + val constantValue = constantPoolConstantValue(cf, ai) + c.getName.split("\\.").toSeq :+ (name + "$" + constantValue) + }.map { constantComponents => + new api.Singleton(pathFromStrings(constantComponents)) + } + } else { + None + } + val tpe = specificTpe.getOrElse(reference(returnType(f))) + if (mods.isFinal) { + new api.Val(tpe, name, accs, mods, annots) + } else { + new api.Var(tpe, name, accs, mods, annots) + } } def methodToDef(enclPkg: Option[String])(m: Method): api.Def = @@ -256,7 +295,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/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) } From a83f5eabf25132349f2b247a5b5eb3adb10aa10d Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 6 Jul 2015 11:11:06 -0700 Subject: [PATCH 02/10] Add missing dependency --- build.sbt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.sbt b/build.sbt index b6a48d173..dcfed8698 100644 --- a/build.sbt +++ b/build.sbt @@ -109,7 +109,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" From d52b6bfce74bfa0c07fc1fc374d7fde4e3f22a7d Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 6 Jul 2015 12:50:14 -0700 Subject: [PATCH 03/10] Add tests missed during initial patch port --- .../sbt/compiler/javac/hasstaticfinal.java | 4 ++ .../sbt/compiler/javac/JavaCompilerSpec.scala | 50 ++++++++++++++++--- 2 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java 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..00c21740a --- /dev/null +++ b/compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java @@ -0,0 +1,4 @@ +public class hasstaticfinal { + // the `TEMPLATE` string is replaced with various values during tests + public static final String HELLO = "TEMPLATE"; +} diff --git a/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala b/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala index aadfab796..f5a131674 100644 --- a/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala +++ b/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala @@ -5,6 +5,8 @@ import java.net.URLClassLoader import sbt._ import org.specs2.Specification +import xsbt.api.SameAPI +import xsbti.api.SourceAPI import xsbti.{ Severity, Problem } object JavaCompilerSpec extends Specification { @@ -14,17 +16,21 @@ 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)} + + // TODO: move somewhere analysis-specific + Analyzing classes generated by javac should + result in different APIs for static-final-primitive fields ${analyzeStaticDifference(local)} """ // TODO - write a test to ensure that java .class files wind up in the right spot, and we can call the compiled java code. @@ -64,6 +70,30 @@ object JavaCompilerSpec extends Specification { errored and foundErrorAndWarning and hasKnownErrors } + def analyzeStaticDifference(compiler: JavaTools) = { + def compileWithPrimitive(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(_.replace("TEMPLATE", templateValue)) + ) + + // then compile it + val (result, problems) = compile(compiler, 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're non-equal + val (prevCompiled, prevAPI) = compileWithPrimitive("A") + val (nextCompiled, nextAPI) = compileWithPrimitive("B") + val apisNonEqual = SameAPI(prevAPI, nextAPI) must beFalse + prevCompiled and nextCompiled and apisNonEqual + } + 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 +161,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) + +} From cc052b598c2821b85df6096db8d2571271f31bf4 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 6 Jul 2015 13:43:14 -0700 Subject: [PATCH 04/10] Add test for stable APIs with identical constants --- .../sbt/compiler/javac/JavaCompilerSpec.scala | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala b/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala index f5a131674..6d2494248 100644 --- a/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala +++ b/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala @@ -5,7 +5,7 @@ import java.net.URLClassLoader import sbt._ import org.specs2.Specification -import xsbt.api.SameAPI +import xsbt.api.{ SameAPI, DefaultShowAPI } import xsbti.api.SourceAPI import xsbti.{ Severity, Problem } @@ -20,20 +20,19 @@ object JavaCompilerSpec extends Specification { 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)} - // TODO: move somewhere analysis-specific Analyzing classes generated by javac should - result in different APIs for static-final-primitive fields ${analyzeStaticDifference(local)} + result in matching APIs for stable static-final fields ${analyzeStaticDifference(local, "A", "A")} + result in different APIs for changed static-final fields ${analyzeStaticDifference(local, "A", "B")} """ - // 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 @@ -70,7 +69,11 @@ object JavaCompilerSpec extends Specification { errored and foundErrorAndWarning and hasKnownErrors } - def analyzeStaticDifference(compiler: JavaTools) = { + /** + * Compiles with the given constant values, and confirms that if the strings are equal, then the + * the APIs are equal. + */ + def analyzeStaticDifference(compiler: JavaTools, left: String, right: String) = { def compileWithPrimitive(templateValue: String) = IO.withTemporaryDirectory { out => // copy the input file to a temporary location and change the templateValue @@ -87,11 +90,13 @@ object JavaCompilerSpec extends Specification { (origCompiled, ClassToAPI(Seq(clazzz))) } - // compile with two different primitive values, and confirm that they're non-equal - val (prevCompiled, prevAPI) = compileWithPrimitive("A") - val (nextCompiled, nextAPI) = compileWithPrimitive("B") - val apisNonEqual = SameAPI(prevAPI, nextAPI) must beFalse - prevCompiled and nextCompiled and apisNonEqual + // compile with two different primitive values, and confirm that they match if their + // values match + val (leftCompiled, leftAPI) = compileWithPrimitive(left) + val (rightCompiled, rightAPI) = compileWithPrimitive(right) + val apisExpectedMatch = SameAPI(leftAPI, rightAPI) must beEqualTo(left == right) + + leftCompiled and rightCompiled and apisExpectedMatch } def lineMatches(p: Problem, lineno: Int): Boolean = From 2982018e5a54566e27b85a3a1ffe875027d2a9ae Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 7 Jul 2015 09:59:45 -0700 Subject: [PATCH 05/10] Parse ClassFile as a lazy val, and provide it to fieldToDef --- .../api/src/main/scala/sbt/ClassToAPI.scala | 49 ++++++++++--------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/compile/api/src/main/scala/sbt/ClassToAPI.scala b/compile/api/src/main/scala/sbt/ClassToAPI.scala index 0a11686d0..aac3e396e 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,26 @@ 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(c, 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 = { + // TODO: need better way to get access to classfile-location + val file = new java.io.File(IO.classLocationFile(c), s"${c.getName.replace('.', '/')}.class") + classfile.Parser.apply(file) } + 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) + } + 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,7 +133,7 @@ object ClassToAPI { def upperBounds(ts: Array[Type]): api.Type = new api.Structure(lzy(types(ts)), lzyEmptyDefArray, lzyEmptyDefArray) - private def constantPoolConstantValue(cf: classfile.ClassFile, ai: classfile.AttributeInfo): AnyRef = { + private def constantPoolConstantValue(cf: ClassFile, ai: classfile.AttributeInfo): AnyRef = { assert(ai.name.exists(_ == "ConstantValue"), s"Non-ConstantValue attribute not supported: ${ai}") import classfile.Constants._ cf.constantPool(classfile.Parser.entryIndex(ai)) match { @@ -135,27 +141,26 @@ object ClassToAPI { // follow the indirection from ConstantString to ConstantUTF8 val nextConstant = cf.constantPool(nextOffset) nextConstant.value.getOrElse { - throw new RuntimeException(s"Empty UTF8 value in constant pool: ${nextConstant}") + throw new RuntimeException(s"Empty UTF8 value in constant pool: $nextConstant") } case constant @ classfile.Constant((ConstantFloat | ConstantLong | ConstantDouble | ConstantInteger), _, _, ref) => ref.getOrElse { - throw new RuntimeException(s"Empty primitive value in constant pool: ${constant}") + throw new RuntimeException(s"Empty primitive value in constant pool: $constant") } + case constant => + throw new IllegalStateException(s"Unsupported ConstantValue type: $constant") } } - def fieldToDef(c: Class[_], enclPkg: Option[String])(f: Field): api.FieldLike = + 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) + // if possible, generate a more specific type for constant fields val specificTpe: Option[api.Type] = if (mods.isFinal) { - // TODO: need better way to get access to classfile-location/parsed-representation - // TODO: need to only bother for static fields - val file = new java.io.File(IO.classLocationFile(c), s"${c.getName.replace('.', '/')}.class") - val cf = classfile.Parser.apply(file) val attributeInfos = cf.fields.find(_.name.exists(_ == name)).toSeq.flatMap(_.attributes) // create a singleton type ending with the hash of the name-mangled ConstantValue of this field attributeInfos.collectFirst { From 592b19b35e5d63a92c249663935e2f01a9742117 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 7 Jul 2015 10:25:27 -0700 Subject: [PATCH 06/10] Add non-string tests --- .../sbt/compiler/javac/hasstaticfinal.java | 4 ++-- .../sbt/compiler/javac/JavaCompilerSpec.scala | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java b/compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java index 00c21740a..818ca5954 100644 --- a/compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java +++ b/compile/src/test/resources/sbt/compiler/javac/hasstaticfinal.java @@ -1,4 +1,4 @@ public class hasstaticfinal { - // the `TEMPLATE` string is replaced with various values during tests - public static final String HELLO = "TEMPLATE"; + // 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 6d2494248..6901010e2 100644 --- a/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala +++ b/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala @@ -28,9 +28,10 @@ object JavaCompilerSpec extends Specification { 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(local, "A", "A")} - result in different APIs for changed static-final fields ${analyzeStaticDifference(local, "A", "B")} + Analyzing classes generated by javac should result in + matching APIs for stable static-final fields ${analyzeStaticDifference("String", "\"A\"", "\"A\"")} + different APIs for changed static-final fields ${analyzeStaticDifference("String", "\"A\"", "\"B\"")} + "safe" singleton type names ${analyzeStaticDifference("float", "0.123456789f", "0.123456789f")} """ def docWorks(compiler: JavaTools) = IO.withTemporaryDirectory { out => @@ -70,21 +71,23 @@ object JavaCompilerSpec extends Specification { } /** - * Compiles with the given constant values, and confirms that if the strings are equal, then the - * the APIs are equal. + * Compiles with the given constant values, and confirms that if the strings mismatch, then the + * the APIs mismatch. */ - def analyzeStaticDifference(compiler: JavaTools, left: String, right: String) = { + def analyzeStaticDifference(typeName: String, left: String, right: String) = { def compileWithPrimitive(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(_.replace("TEMPLATE", templateValue)) + IO.readLines(hasStaticFinalFile).map { line => + line.replace("TYPE", typeName).replace("VALUE", templateValue) + } ) // then compile it - val (result, problems) = compile(compiler, Seq(input), Seq("-d", out.getAbsolutePath)) + 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))) From aa11e9ca87636cef79fd39e5c9fd41f30687e470 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 7 Jul 2015 10:56:51 -0700 Subject: [PATCH 07/10] Improve comments --- compile/api/src/main/scala/sbt/ClassToAPI.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compile/api/src/main/scala/sbt/ClassToAPI.scala b/compile/api/src/main/scala/sbt/ClassToAPI.scala index aac3e396e..4d4a6bc5c 100644 --- a/compile/api/src/main/scala/sbt/ClassToAPI.scala +++ b/compile/api/src/main/scala/sbt/ClassToAPI.scala @@ -133,6 +133,7 @@ object ClassToAPI { def upperBounds(ts: Array[Type]): api.Type = new api.Structure(lzy(types(ts)), lzyEmptyDefArray, lzyEmptyDefArray) + /** Parses the constant value represented by the given ConstantValue AttributeInfo. */ private def constantPoolConstantValue(cf: ClassFile, ai: classfile.AttributeInfo): AnyRef = { assert(ai.name.exists(_ == "ConstantValue"), s"Non-ConstantValue attribute not supported: ${ai}") import classfile.Constants._ @@ -158,11 +159,12 @@ object ClassToAPI { val accs = access(f.getModifiers, enclPkg) val mods = modifiers(f.getModifiers) val annots = annotations(f.getDeclaredAnnotations) - // if possible, generate a more specific type for constant fields + // generate a more specific type for constant fields val specificTpe: Option[api.Type] = if (mods.isFinal) { val attributeInfos = cf.fields.find(_.name.exists(_ == name)).toSeq.flatMap(_.attributes) - // create a singleton type ending with the hash of the name-mangled ConstantValue of this field + // create a singleton type ending with the ConstantValue of this field. because this type + // is purely synthetic, it's fine that the name might contain filename-banned characters. attributeInfos.collectFirst { case ai @ classfile.AttributeInfo(Some("ConstantValue"), _) => val constantValue = constantPoolConstantValue(cf, ai) From a80c3e2bb8e7a543434fd66fbf025675ace35311 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Thu, 9 Jul 2015 22:01:05 -0700 Subject: [PATCH 08/10] Review feedback --- .../api/src/main/scala/sbt/ClassToAPI.scala | 48 +++++++++---------- .../main/scala/sbt/classfile/ClassFile.scala | 24 +++++++++- 2 files changed, 45 insertions(+), 27 deletions(-) diff --git a/compile/api/src/main/scala/sbt/ClassToAPI.scala b/compile/api/src/main/scala/sbt/ClassToAPI.scala index 4d4a6bc5c..5b3c231c5 100644 --- a/compile/api/src/main/scala/sbt/ClassToAPI.scala +++ b/compile/api/src/main/scala/sbt/ClassToAPI.scala @@ -69,11 +69,7 @@ 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) = { - lazy val cf = { - // TODO: need better way to get access to classfile-location - val file = new java.io.File(IO.classLocationFile(c), s"${c.getName.replace('.', '/')}.class") - classfile.Parser.apply(file) - } + 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)) @@ -88,6 +84,12 @@ object ClassToAPI { (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] = { @@ -133,27 +135,13 @@ object ClassToAPI { def upperBounds(ts: Array[Type]): api.Type = new api.Structure(lzy(types(ts)), lzyEmptyDefArray, lzyEmptyDefArray) - /** Parses the constant value represented by the given ConstantValue AttributeInfo. */ - private def constantPoolConstantValue(cf: ClassFile, ai: classfile.AttributeInfo): AnyRef = { - assert(ai.name.exists(_ == "ConstantValue"), s"Non-ConstantValue attribute not supported: ${ai}") - import classfile.Constants._ - cf.constantPool(classfile.Parser.entryIndex(ai)) match { - case classfile.Constant(ConstantString, nextOffset, _, _) => - // follow the indirection from ConstantString to ConstantUTF8 - val nextConstant = cf.constantPool(nextOffset) - nextConstant.value.getOrElse { - throw new RuntimeException(s"Empty UTF8 value in constant pool: $nextConstant") - } - case constant @ classfile.Constant((ConstantFloat | ConstantLong | ConstantDouble | ConstantInteger), _, _, ref) => - ref.getOrElse { - throw new RuntimeException(s"Empty primitive value in constant pool: $constant") - } - case constant => - throw new IllegalStateException(s"Unsupported ConstantValue type: $constant") - } + @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 = + def fieldToDef(c: Class[_], _cf: => ClassFile, enclPkg: Option[String])(f: Field): api.FieldLike = { val name = f.getName val accs = access(f.getModifiers, enclPkg) @@ -162,13 +150,21 @@ object ClassToAPI { // generate a more specific type for constant fields val specificTpe: Option[api.Type] = if (mods.isFinal) { + val cf = _cf val attributeInfos = cf.fields.find(_.name.exists(_ == name)).toSeq.flatMap(_.attributes) // create a singleton type ending with the ConstantValue of this field. because this type // is purely synthetic, it's fine that the name might contain filename-banned characters. attributeInfos.collectFirst { case ai @ classfile.AttributeInfo(Some("ConstantValue"), _) => - val constantValue = constantPoolConstantValue(cf, ai) - c.getName.split("\\.").toSeq :+ (name + "$" + constantValue) + try { + c.getName.split("\\.").toSeq :+ (name + "$" + cf.constantValue(ai)) + } 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 + ) + } }.map { constantComponents => new api.Singleton(pathFromStrings(constantComponents)) } diff --git a/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala b/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala index 32ab93d6b..ba70203ff 100644 --- a/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala +++ b/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala @@ -22,6 +22,28 @@ private[sbt] trait ClassFile { val sourceFile: Option[String] def types: Set[String] def stringValue(a: AttributeInfo): String + + /** Parses the constant value represented by the given ConstantValue AttributeInfo. */ + def constantValue(ai: AttributeInfo): AnyRef = { + assert( + ai.name.exists(_ == "ConstantValue"), + s"Non-ConstantValue attribute not supported: ${ai}" + ) + constantPool(Parser.entryIndex(ai)) match { + 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 +83,4 @@ private[sbt] object Constants { final val ConstantMethodType = 16 final val ConstantInvokeDynamic = 18 final val ClassDescriptor = 'L' -} \ No newline at end of file +} From 7e7aca5f55a8d383f6fb19ce004710dd8f5481ce Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 13 Jul 2015 21:29:14 -0700 Subject: [PATCH 09/10] Mismatch APIs on type changes as well --- .../api/src/main/scala/sbt/ClassToAPI.scala | 47 +++++++++++-------- .../sbt/compiler/javac/JavaCompilerSpec.scala | 21 +++++---- .../main/scala/sbt/classfile/ClassFile.scala | 17 +++---- 3 files changed, 50 insertions(+), 35 deletions(-) diff --git a/compile/api/src/main/scala/sbt/ClassToAPI.scala b/compile/api/src/main/scala/sbt/ClassToAPI.scala index 5b3c231c5..c2f47c231 100644 --- a/compile/api/src/main/scala/sbt/ClassToAPI.scala +++ b/compile/api/src/main/scala/sbt/ClassToAPI.scala @@ -141,37 +141,29 @@ object ClassToAPI { fieldToDef(c, classFileForClass(c), enclPkg)(f) } - def fieldToDef(c: Class[_], _cf: => ClassFile, enclPkg: Option[String])(f: Field): api.FieldLike = + 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 fieldTpe = reference(returnType(f)) // generate a more specific type for constant fields val specificTpe: Option[api.Type] = if (mods.isFinal) { - val cf = _cf - val attributeInfos = cf.fields.find(_.name.exists(_ == name)).toSeq.flatMap(_.attributes) - // create a singleton type ending with the ConstantValue of this field. because this type - // is purely synthetic, it's fine that the name might contain filename-banned characters. - attributeInfos.collectFirst { - case ai @ classfile.AttributeInfo(Some("ConstantValue"), _) => - try { - c.getName.split("\\.").toSeq :+ (name + "$" + cf.constantValue(ai)) - } 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 - ) - } - }.map { constantComponents => - new api.Singleton(pathFromStrings(constantComponents)) + 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(reference(returnType(f))) + val tpe = specificTpe.getOrElse(fieldTpe) if (mods.isFinal) { new api.Val(tpe, name, accs, mods, annots) } else { @@ -179,6 +171,23 @@ object ClassToAPI { } } + /** + * 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) diff --git a/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala b/compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala index 6901010e2..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,7 @@ 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 } @@ -29,9 +30,10 @@ object JavaCompilerSpec extends Specification { 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 changed static-final fields ${analyzeStaticDifference("String", "\"A\"", "\"B\"")} - "safe" singleton type names ${analyzeStaticDifference("float", "0.123456789f", "0.123456789f")} + 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")} """ def docWorks(compiler: JavaTools) = IO.withTemporaryDirectory { out => @@ -74,15 +76,18 @@ object JavaCompilerSpec extends Specification { * 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) = { - def compileWithPrimitive(templateValue: String) = + 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", typeName).replace("VALUE", templateValue) + line.replace("TYPE", templateType).replace("VALUE", templateValue) } ) @@ -95,8 +100,8 @@ object JavaCompilerSpec extends Specification { // compile with two different primitive values, and confirm that they match if their // values match - val (leftCompiled, leftAPI) = compileWithPrimitive(left) - val (rightCompiled, rightAPI) = compileWithPrimitive(right) + 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 diff --git a/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala b/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala index ba70203ff..d230dbfe9 100644 --- a/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala +++ b/util/classfile/src/main/scala/sbt/classfile/ClassFile.scala @@ -23,13 +23,15 @@ private[sbt] trait ClassFile { def types: Set[String] def stringValue(a: AttributeInfo): String - /** Parses the constant value represented by the given ConstantValue AttributeInfo. */ - def constantValue(ai: AttributeInfo): AnyRef = { - assert( - ai.name.exists(_ == "ConstantValue"), - s"Non-ConstantValue attribute not supported: ${ai}" - ) - constantPool(Parser.entryIndex(ai)) match { + /** + * 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) @@ -43,7 +45,6 @@ private[sbt] trait ClassFile { 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 { From e3ba86fdbbdb69b5026062f16aa669bc2d4ac1e6 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 13 Jul 2015 21:38:48 -0700 Subject: [PATCH 10/10] Add notes for the change --- notes/0.13.9/java-constant-fields.markdown | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 notes/0.13.9/java-constant-fields.markdown 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]