Review feedback

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))
           }

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'
-}
+}