Simplify local class name serialization, don't go through extension

Also fix it for the case of a class in a non-default package

compiler/backend/src/org/jetbrains/kotlin/codegen/JvmSerializerExtension.java

@@ -25,7 +25,10 @@ import org.jetbrains.kotlin.codegen.state.JetTypeMapper;
 import org.jetbrains.kotlin.descriptors.*;
 import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptor;
 import org.jetbrains.kotlin.load.java.lazy.types.RawTypeCapabilities;
-import org.jetbrains.kotlin.serialization.*;
+import org.jetbrains.kotlin.serialization.AnnotationSerializer;
+import org.jetbrains.kotlin.serialization.ProtoBuf;
+import org.jetbrains.kotlin.serialization.SerializerExtension;
+import org.jetbrains.kotlin.serialization.StringTable;
 import org.jetbrains.kotlin.serialization.deserialization.NameResolver;
 import org.jetbrains.kotlin.serialization.deserialization.descriptors.DeserializedPropertyDescriptor;
 import org.jetbrains.kotlin.serialization.deserialization.descriptors.DeserializedSimpleFunctionDescriptor;
@@ -34,18 +37,17 @@ import org.jetbrains.kotlin.types.JetType;
 import org.jetbrains.org.objectweb.asm.Type;
 import org.jetbrains.org.objectweb.asm.commons.Method;
 
-import static org.jetbrains.kotlin.codegen.AsmUtil.shortNameByAsmType;
 import static org.jetbrains.kotlin.codegen.JvmSerializationBindings.*;
 
 public class JvmSerializerExtension extends SerializerExtension {
     private final JvmSerializationBindings bindings;
-    private final JetTypeMapper typeMapper;
+    private final StringTable stringTable;
-    private final StringTable stringTable = new JvmStringTable(this);
+    private final AnnotationSerializer annotationSerializer;
-    private final AnnotationSerializer annotationSerializer = new AnnotationSerializer(stringTable);
 
     public JvmSerializerExtension(@NotNull JvmSerializationBindings bindings, @NotNull JetTypeMapper typeMapper) {
         this.bindings = bindings;
-        this.typeMapper = typeMapper;
+        this.stringTable = new JvmStringTable(typeMapper);
+        this.annotationSerializer = new AnnotationSerializer(stringTable);
     }
 
     @NotNull
@@ -91,12 +93,6 @@ public class JvmSerializerExtension extends SerializerExtension {
         }
     }
 
-    @Override
-    @NotNull
-    public String getLocalClassName(@NotNull ClassDescriptor descriptor) {
-        return shortNameByAsmType(typeMapper.mapClass(descriptor));
-    }
-
     private void saveSignature(@NotNull CallableMemberDescriptor callable, @NotNull ProtoBuf.Callable.Builder proto) {
         SignatureSerializer signatureSerializer = new SignatureSerializer();
         if (callable instanceof FunctionDescriptor) {

compiler/backend/src/org/jetbrains/kotlin/codegen/serialization/JvmStringTable.kt

@@ -16,13 +16,12 @@
 
 package org.jetbrains.kotlin.codegen.serialization
 
+import org.jetbrains.kotlin.codegen.state.JetTypeMapper
 import org.jetbrains.kotlin.descriptors.ClassDescriptor
-import org.jetbrains.kotlin.descriptors.ClassOrPackageFragmentDescriptor
-import org.jetbrains.kotlin.descriptors.DeclarationDescriptor
 import org.jetbrains.kotlin.descriptors.PackageFragmentDescriptor
 import org.jetbrains.kotlin.load.kotlin.JvmNameResolver
-import org.jetbrains.kotlin.resolve.descriptorUtil.classId
+import org.jetbrains.kotlin.name.ClassId
-import org.jetbrains.kotlin.serialization.SerializerExtension
+import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.serialization.StringTable
 import org.jetbrains.kotlin.serialization.jvm.JvmProtoBuf
 import org.jetbrains.kotlin.serialization.jvm.JvmProtoBuf.StringTableTypes.Record
@@ -30,7 +29,7 @@ import org.jetbrains.kotlin.types.ErrorUtils
 import java.io.OutputStream
 import java.util.*
 
-class JvmStringTable(private val extension: SerializerExtension) : StringTable {
+class JvmStringTable(private val typeMapper: JetTypeMapper) : StringTable {
     public val strings = ArrayList<String>()
     private val records = ArrayList<Record>()
     private val map = HashMap<String, Int>()
@@ -52,43 +51,35 @@ class JvmStringTable(private val extension: SerializerExtension) : StringTable {
             throw IllegalStateException("Cannot get FQ name of error class: " + descriptor)
         }
 
-        val string: String
+        // We use the following format to encode ClassId: "pkg/Outer.Inner".
-        val storeAsLiteral: Boolean
+        // It represents a unique name, but such names don't usually appear in the constant pool, so we're writing "Lpkg/Outer$Inner;"
+        // instead and an instruction to drop the first and the last character in this string and replace all '$' with '.'.
+        // This works most of the time, except in two rare cases:
+        // - the name of the class or any of its outer classes contains dollars. In this case we're just storing the described
+        //   string literally: "pkg/Outer.Inner$with$dollars"
+        // - the class is local or nested in local. In this case we're also storing the literal string, and also storing the fact that
+        //   this name represents a local class in a separate list
 
-        val parent = sequence(descriptor, DeclarationDescriptor::getContainingDeclaration).first { it !is ClassDescriptor }
+        val classId = descriptor.classId
-        if (parent is PackageFragmentDescriptor) {
+        val string = classId.asString()
-            val classId = descriptor.classId
-            val packageName = classId.packageFqName
-            val className = classId.relativeClassName.asString()
-            string =
-                    if (packageName.isRoot) className
-                    else packageName.asString().replace('.', '/') + "/" + className
-
-            // If any of the class names contains '$', we can't simply replace all '$' with '.' upon deserialization.
-            // This case is rather rare, so we're storing a literal string
-            storeAsLiteral = '$' in string && classId.relativeClassName.pathSegments().any { '$' in it.asString() }
-        }
-        else {
-            storeAsLiteral = true
-            string = descriptor.localClassName()
-        }
-
-        val isLocal = descriptor.containingDeclaration !is ClassOrPackageFragmentDescriptor
 
         map[string]?.let { recordedIndex ->
-            if (isLocal == (recordedIndex in localNames)) {
+            // If we already recorded such string, we only return its index if it's local and our name is local
+            // OR it's not local and our name is not local as well
+            if (classId.isLocal == (recordedIndex in localNames)) {
                 return recordedIndex
             }
         }
 
         val index = strings.size()
-        if (isLocal) {
+        if (classId.isLocal) {
             localNames.add(index)
         }
 
         val record = Record.newBuilder()
 
-        if (storeAsLiteral) {
+        // If the class is local or any of its outer class names contains '$', store a literal string
+        if (classId.isLocal || '$' in string) {
             strings.add(string)
             // TODO: optimize, don't always store the operation
             record.setOperation(Record.Operation.NONE)
@@ -103,7 +94,7 @@ class JvmStringTable(private val extension: SerializerExtension) : StringTable {
             }
             else {
                 record.setOperation(Record.Operation.DESC_TO_CLASS_ID)
-                strings.add("L$string;")
+                strings.add("L${string.replace('.', '$')};")
             }
         }
 
@@ -114,13 +105,18 @@ class JvmStringTable(private val extension: SerializerExtension) : StringTable {
         return index
     }
 
-    private fun ClassDescriptor.localClassName(): String {
+    private val ClassDescriptor.classId: ClassId
-        val container = containingDeclaration
+        get() {
-        return when (container) {
+            val container = containingDeclaration
-            is ClassDescriptor -> container.localClassName() + "." + name.asString()
+            return when (container) {
-            else -> extension.getLocalClassName(this)
+                is ClassDescriptor -> container.classId.createNestedClassId(name)
+                is PackageFragmentDescriptor -> ClassId(container.fqName, name)
+                else -> {
+                    val fqName = FqName(typeMapper.mapClass(this).internalName.replace('/', '.'))
+                    ClassId(fqName.parent(), FqName.topLevel(fqName.shortName()), /* isLocal = */ true)
+                }
+            }
         }
-    }
 
     override fun serializeTo(output: OutputStream) {
         with(JvmProtoBuf.StringTableTypes.newBuilder()) {

compiler/builtins-serializer/src/org/jetbrains/kotlin/serialization/builtins/BuiltInsSerializerExtension.kt

@@ -22,7 +22,7 @@ import org.jetbrains.kotlin.resolve.scopes.DescriptorKindFilter
 import org.jetbrains.kotlin.serialization.*
 
 public class BuiltInsSerializerExtension : SerializerExtension() {
-    private val stringTable = StringTableImpl(this)
+    private val stringTable = StringTableImpl()
     private val annotationSerializer = AnnotationSerializer(stringTable)
 
     override fun getStringTable(): StringTable = stringTable

compiler/serialization/src/org/jetbrains/kotlin/serialization/SerializerExtension.java

@@ -46,9 +46,4 @@ public abstract class SerializerExtension {
 
     public void serializeType(@NotNull JetType type, @NotNull ProtoBuf.Type.Builder proto) {
     }
-
-    @NotNull
-    public String getLocalClassName(@NotNull ClassDescriptor descriptor) {
-        throw new UnsupportedOperationException("Local classes are unsupported: " + descriptor);
-    }
 }

compiler/serialization/src/org/jetbrains/kotlin/serialization/StringTableImpl.java

@@ -61,11 +61,6 @@ public class StringTableImpl implements StringTable {
 
     private final Interner<String> strings = new Interner<String>();
     private final Interner<FqNameProto> qualifiedNames = new Interner<FqNameProto>();
-    private final SerializerExtension extension;
-
-    public StringTableImpl(@NotNull SerializerExtension extension) {
-        this.extension = extension;
-    }
 
     public int getSimpleNameIndex(@NotNull Name name) {
         return getStringIndex(name.asString());
@@ -86,25 +81,21 @@ public class StringTableImpl implements StringTable {
         builder.setKind(QualifiedName.Kind.CLASS);
 
         DeclarationDescriptor containingDeclaration = descriptor.getContainingDeclaration();
-        String shortName;
         if (containingDeclaration instanceof PackageFragmentDescriptor) {
-            shortName = descriptor.getName().asString();
             FqName packageFqName = ((PackageFragmentDescriptor) containingDeclaration).getFqName();
             if (!packageFqName.isRoot()) {
                 builder.setParentQualifiedName(getPackageFqNameIndex(packageFqName));
             }
         }
         else if (containingDeclaration instanceof ClassDescriptor) {
-            shortName = descriptor.getName().asString();
             ClassDescriptor outerClass = (ClassDescriptor) containingDeclaration;
             builder.setParentQualifiedName(getFqNameIndex(outerClass));
         }
         else {
-            builder.setKind(QualifiedName.Kind.LOCAL);
+            throw new IllegalStateException("Cannot get FQ name of local class: " + descriptor);
-            shortName = extension.getLocalClassName(descriptor);
         }
 
-        builder.setShortName(getStringIndex(shortName));
+        builder.setShortName(getStringIndex(descriptor.getName().asString()));
 
         return qualifiedNames.intern(new FqNameProto(builder));
     }

compiler/testData/serialization/local/deepInnerChain.kt

@@ -1,3 +1,5 @@
+package test
+
 // CLASS_NAME_SUFFIX: Deepest
 
 fun main() {

compiler/testData/serialization/local/deepInnerChain.txt

@@ -1,7 +1,7 @@
 local final inner class Deepest
 
 public constructor Deepest()
-public final fun deep(): `DeepInnerChainKt$main$Local$Inner$prop$1$foo$1$DeepLocal`
+public final fun deep(): test.`DeepInnerChainKt$main$Local$Inner$prop$1$foo$1$DeepLocal`
-public final fun deepest(): `DeepInnerChainKt$main$Local$Inner$prop$1$foo$1$DeepLocal`.Deepest?
+public final fun deepest(): test.`DeepInnerChainKt$main$Local$Inner$prop$1$foo$1$DeepLocal`.Deepest?
-public final fun inner(): `DeepInnerChainKt$main$Local`.Inner
+public final fun inner(): test.`DeepInnerChainKt$main$Local`.Inner
-public final fun local(): `DeepInnerChainKt$main$Local`
+public final fun local(): test.`DeepInnerChainKt$main$Local`

compiler/tests/org/jetbrains/kotlin/serialization/AbstractLocalClassProtoTest.kt

@@ -22,11 +22,11 @@ import org.jetbrains.kotlin.cli.jvm.compiler.EnvironmentConfigFiles
 import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
 import org.jetbrains.kotlin.cli.jvm.config.getModuleName
 import org.jetbrains.kotlin.codegen.forTestCompile.ForTestCompileRuntime
+import org.jetbrains.kotlin.descriptors.PackagePartProvider
 import org.jetbrains.kotlin.frontend.java.di.createContainerForTopDownAnalyzerForJvm
 import org.jetbrains.kotlin.incremental.components.LookupTracker
 import org.jetbrains.kotlin.jvm.compiler.LoadDescriptorUtil
 import org.jetbrains.kotlin.load.java.JvmAnnotationNames
-import org.jetbrains.kotlin.descriptors.PackagePartProvider
 import org.jetbrains.kotlin.load.java.structure.reflect.classId
 import org.jetbrains.kotlin.resolve.jvm.TopDownAnalyzerFacadeForJVM
 import org.jetbrains.kotlin.resolve.lazy.declarations.FileBasedDeclarationProviderFactory
@@ -38,20 +38,20 @@ import java.net.URLClassLoader
 public abstract class AbstractLocalClassProtoTest : TestCaseWithTmpdir() {
     protected fun doTest(filename: String) {
         val source = File(filename)
-        LoadDescriptorUtil.compileKotlinToDirAndGetAnalysisResult(listOf(source), tmpdir, getTestRootDisposable(), ConfigurationKind.ALL)
+        LoadDescriptorUtil.compileKotlinToDirAndGetAnalysisResult(listOf(source), tmpdir, testRootDisposable, ConfigurationKind.ALL)
 
         val classNameSuffix = InTextDirectivesUtils.findStringWithPrefixes(source.readText(), "// CLASS_NAME_SUFFIX: ")
                               ?: error("CLASS_NAME_SUFFIX directive not found in test data")
 
         val classLoader = URLClassLoader(arrayOf(tmpdir.toURI().toURL()), ForTestCompileRuntime.runtimeAndReflectJarClassLoader())
 
-        val classFile = tmpdir.listFiles().singleOrNull { it.getPath().endsWith("$classNameSuffix.class") }
+        val classFile = tmpdir.walkTopDown().singleOrNull { it.path.endsWith("$classNameSuffix.class") }
                         ?: error("Local class with suffix `$classNameSuffix` is not found in: ${tmpdir.listFiles().toList()}")
-        val clazz = classLoader.loadClass(classFile.name.substringBeforeLast(".class"))
+        val clazz = classLoader.loadClass(classFile.relativeTo(tmpdir).substringBeforeLast(".class").replace('/', '.').replace('\\', '.'))
         assertHasAnnotationData(clazz)
 
         val environment = KotlinCoreEnvironment.createForTests(
-                getTestRootDisposable(),
+                testRootDisposable,
                 JetTestUtils.compilerConfigurationForTests(ConfigurationKind.ALL, TestJdkKind.MOCK_JDK, tmpdir),
                 EnvironmentConfigFiles.JVM_CONFIG_FILES
         )
@@ -79,7 +79,7 @@ public abstract class AbstractLocalClassProtoTest : TestCaseWithTmpdir() {
     private fun assertHasAnnotationData(clazz: Class<*>) {
         @Suppress("UNCHECKED_CAST")
         val annotation = clazz.getAnnotation(
-                clazz.getClassLoader().loadClass(JvmAnnotationNames.KOTLIN_CLASS.asString()) as Class<Annotation>
+                clazz.classLoader.loadClass(JvmAnnotationNames.KOTLIN_CLASS.asString()) as Class<Annotation>
         )
         assert(annotation != null) { "KotlinClass annotation is not found for class $clazz" }
 

core/descriptors/src/org/jetbrains/kotlin/name/ClassId.java

@@ -88,6 +88,15 @@ public final class ClassId {
         return new FqName(packageFqName.asString() + "." + relativeClassName.asString());
     }
 
+    /**
+     * @return a string where packages are delimited by '/' and classes by '.', e.g. "kotlin/Map.Entry"
+     */
+    @NotNull
+    public String asString() {
+        if (packageFqName.isRoot()) return relativeClassName.asString();
+        return packageFqName.asString().replace('.', '/') + "/" + relativeClassName.asString();
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
@@ -110,7 +119,6 @@ public final class ClassId {
 
     @Override
     public String toString() {
-        if (packageFqName.isRoot()) return "/" + relativeClassName;
+        return packageFqName.isRoot() ? "/" + asString() : asString();
-        return packageFqName.toString().replace('.', '/') + "/" + relativeClassName;
     }
 }

js/js.serializer/src/org/jetbrains/kotlin/serialization/js/KotlinJavascriptSerializerExtension.kt

@@ -28,7 +28,7 @@ import org.jetbrains.kotlin.serialization.StringTableImpl
 import org.jetbrains.kotlin.types.JetType
 
 public class KotlinJavascriptSerializerExtension : SerializerExtension() {
-    private val stringTable = StringTableImpl(this)
+    private val stringTable = StringTableImpl()
     private val annotationSerializer = AnnotationSerializer(stringTable)
 
     override fun getStringTable() = stringTable