From 7e7aca5f55a8d383f6fb19ce004710dd8f5481ce Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 13 Jul 2015 21:29:14 -0700 Subject: [PATCH] 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 {