PR feedback

Author: SolalPirelli

compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala

@@ -585,7 +585,7 @@ trait BCodeBodyBuilder(val primitives: DottyPrimitives)(using ctx: Context) exte
 
         case StringTag  =>
           assert(const.value != null, const) // TODO this invariant isn't documented in `case class Constant`
-          if BCodeUtils.checkConstantStringLength(null, const.stringValue) then
+          if BCodeUtils.checkConstantStringLength(const.stringValue) then
             mnode.visitLdcInsn(const.stringValue) // `stringValue` special-cases null, but not for a const with StringTag
           else
             report.error("String constant is too long for the JVM", pos)
@@ -604,7 +604,7 @@ trait BCodeBodyBuilder(val primitives: DottyPrimitives)(using ctx: Context) exte
             )
           else
             val toASM = tp.toASMType
-            if BCodeUtils.checkConstantStringLength(null, toASM.getInternalName) then
+            if BCodeUtils.checkConstantStringLength(toASM.getInternalName) then
               mnode.visitLdcInsn(toASM)
             else
               report.error("Type name is too long for the JVM", pos)

compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala

@@ -394,7 +394,7 @@ trait BCodeHelpers(val backendUtils: BackendUtils)(using ctx: Context) extends B
      *         Machine Specification, §4.3.4, or `null` if `sym` doesn't need a generic signature.
      * @see https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.3.4
      */
-    def getGenericSignature(sym: Symbol, owner: Symbol): String = {
+    def getGenericSignature(sym: Symbol, owner: Symbol): String | Null = {
       atPhase(erasurePhase) {
         def computeMemberTpe(): Type =
           if (sym.is(Method)) sym.denot.info
@@ -447,7 +447,9 @@ trait BCodeHelpers(val backendUtils: BackendUtils)(using ctx: Context) extends B
       val jReturnType = toTypeKind(methodInfo.resultType)
       val mdesc = MethodBType(paramJavaTypes, jReturnType).descriptor
       val mirrorMethodName = m.javaSimpleName
-      if !BCodeUtils.checkConstantStringLength(jgensig, mirrorMethodName, mdesc) then
+      val lengthOk = if jgensig ne null then BCodeUtils.checkConstantStringLength(jgensig)
+                                        else BCodeUtils.checkConstantStringLength(mirrorMethodName, mdesc)
+      if !lengthOk then
         report.error("Mirror method signature is too long for the JVM", m.srcPos)
         return
       val mirrorMethod: asm.MethodVisitor = jclass.visitMethod(
@@ -607,7 +609,7 @@ trait BCodeHelpers(val backendUtils: BackendUtils)(using ctx: Context) extends B
       val moduleName = internalName(moduleClass) // + "$"
       val mirrorName = bType.internalName
       val mirrorClass = new asm.tree.ClassNode
-      if !BCodeUtils.checkConstantStringLength(null, mirrorName) then
+      if !BCodeUtils.checkConstantStringLength(mirrorName) then
         report.error("Mirror class name is too long for the JVM", moduleClass.srcPos)
         return mirrorClass // not filled, but we cannot create it, and we just reported an error
       mirrorClass.visit(

compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala

@@ -339,7 +339,9 @@ trait BCodeSkelBuilder(using ctx: Context) extends BCodeHelpers {
       val flags = BCodeUtils.javaFlags(claszSymbol)
 
       val thisSignature = getGenericSignature(claszSymbol, claszSymbol.owner)
-      if !BCodeUtils.checkConstantStringLength(thisSignature, thisName) then
+      val lengthOk = if thisSignature ne null then BCodeUtils.checkConstantStringLength(thisSignature)
+                                              else BCodeUtils.checkConstantStringLength(thisName)
+      if !lengthOk then
         report.error("Class name is too long for the JVM", claszSymbol.srcPos)
         return
       cnode.visit(backendUtils.classfileVersion, flags,
@@ -745,7 +747,9 @@ trait BCodeSkelBuilder(using ctx: Context) extends BCodeHelpers {
         else jMethodName
 
       val mdesc = asmMethodType(methSymbol).descriptor
-      if !BCodeUtils.checkConstantStringLength(jgensig, bytecodeName, mdesc) then
+      val lengthOk = if jgensig ne null then BCodeUtils.checkConstantStringLength(jgensig)
+                                        else BCodeUtils.checkConstantStringLength(bytecodeName, mdesc)
+      if !lengthOk then
         report.error("Method signature is too long for the JVM", methSymbol.srcPos)
         return
       mnode = cnode.visitMethod(

compiler/src/dotty/tools/backend/jvm/BCodeUtils.scala

@@ -22,36 +22,35 @@ import scala.tools.asm
 //      (one can argue we should reuse one of the other existing helper objects, but that's for a later cleanup)
 
 object BCodeUtils {
-  val MAX_BYTES_PER_UTF8_CONSTANT = 65535
-  /** Checks that the given signature if present, or the concatenation of the given name and descriptor, do not exceed the JVM's UTF-8 text size limits. */
-  def checkConstantStringLength(sig: String | Null, name: String, desc: String = ""): Boolean = {
-    // The JVM enforces a max length of 65535 bytes per UTF-8 constant.
-    // In practice the set of UTF-8 that Java uses can't be more than 4 bytes per char.
-    def count(str: String, startAt: Int): Int =
-      var byteCount = startAt
-      for
-        i <- 0 until str.length
-      do
+  // The JVM enforces a max length of 65535 bytes per UTF-8 constant.
+  // Java uses "Modified UTF-8", in which the null character specifically is two bytes,
+  // and the rest of the BMP is as usual, see https://docs.oracle.com/javase/8/docs/api/java/io/DataInput.html#modified-utf-8
+  // Outside the BMP, characters are represented as surrogate pairs, i.e., 2+2 bytes, but `charAt` sees them as separate "characters".
+  // This means if we see a surrogate pair in a string, we should count each half as 2 bytes, since the encoded UTF-8 character will be 4 bytes.
+  // One consequence of this is that the maximum number of UTF-8 bytes for a single Java `char` (not codepoint!) is 3.
+  private val MAX_BYTES_PER_UTF8_CONSTANT = 65535
+  private val MAX_BYTES_PER_CHAR = 3
+
+  /** Checks that the given name, or the concatenation of the given two names and descriptor if present, do not exceed the JVM's UTF-8 text size limits. */
+  def checkConstantStringLength(name: String, other: String = ""): Boolean = {
+    var byteCount = 0
+    def check(str: String): Boolean =
+      var i = 0
+      while i < str.length do
         val c = str.charAt(i)
         byteCount += (
-          if c <= 0x7F then 1
+          if c == 0x00 then 2
+          else if c <= 0x7F then 1
           else if c <= 0x7FF then 2
           else if Character.isHighSurrogate(c) || Character.isLowSurrogate(c) then 2
           else 3
         )
-      byteCount
-
+        if byteCount > MAX_BYTES_PER_UTF8_CONSTANT then
+          return false
+        i += 1
+      true
     // For performance, since we expect few large strings, check if the string is obviously fine first.
-    val totalCount =
-      if sig eq null then
-        if name.length + desc.length < MAX_BYTES_PER_UTF8_CONSTANT / 4 then 0
-        else count(desc, count(name, 0))
-      else if sig.length >= MAX_BYTES_PER_UTF8_CONSTANT then
-        count(sig, 0)
-      else
-        0
-
-    totalCount <= MAX_BYTES_PER_UTF8_CONSTANT
+    name.length + other.length <= MAX_BYTES_PER_UTF8_CONSTANT / MAX_BYTES_PER_CHAR || (check(name) && check(other))
   }
 
   /**