search

google / ksp

Kotlin Symbol Processing API
https://github.com/google/ksp
Apache License 2.0
2.91k stars 280 forks source link

mapToJvmSignature crash on local class #1335

Open bubenheimer opened 1 year ago

bubenheimer commented 1 year ago

Given this top-level function and local class:

private fun simple() {
    class VerySimple
}

Calling Resolver.mapToJvmSignature() on the KSClassDeclaration corresponding to class VerySimple crashes ksp:

java.lang.IllegalArgumentException: Unexpected container: private fun simple(): kotlin.Unit defined in com.example in file Sample.kt[SimpleFunctionDescriptorImpl@6668c51f] for class VerySimple
    at org.jetbrains.kotlin.load.kotlin.DescriptorBasedTypeSignatureMappingKt.computeInternalName(descriptorBasedTypeSignatureMapping.kt:190)
    at org.jetbrains.kotlin.load.kotlin.DescriptorBasedTypeSignatureMappingKt.mapType(descriptorBasedTypeSignatureMapping.kt:141)
    at org.jetbrains.kotlin.codegen.state.KotlinTypeMapper.mapType(KotlinTypeMapper.kt:276)
    at org.jetbrains.kotlin.codegen.state.KotlinTypeMapper.mapType$default(KotlinTypeMapper.kt:267)
    at org.jetbrains.kotlin.codegen.state.KotlinTypeMapper.mapType(KotlinTypeMapper.kt:263)
    at com.google.devtools.ksp.processing.impl.ResolverImpl.mapToJvmSignatureInternal$compiler_plugin(ResolverImpl.kt:356)
    at com.google.devtools.ksp.processing.impl.ResolverImpl.mapToJvmSignature(ResolverImpl.kt:353)
    at org.bubenheimer.instancelimit.InstanceLimitProcessor.createClassInfo(InstanceLimitProcessor.kt:111)
    at org.bubenheimer.instancelimit.InstanceLimitProcessor.addInstanceLimit(InstanceLimitProcessor.kt:64)
    at org.bubenheimer.instancelimit.InstanceLimitProcessor.process(InstanceLimitProcessor.kt:214)
    at org.bubenheimer.instancelimit.InstanceLimitProcessor.process(InstanceLimitProcessor.kt:140)
    at com.google.devtools.ksp.AbstractKotlinSymbolProcessingExtension$doAnalysis$6$1.invoke(KotlinSymbolProcessingExtension.kt:291)
    at com.google.devtools.ksp.AbstractKotlinSymbolProcessingExtension$doAnalysis$6$1.invoke(KotlinSymbolProcessingExtension.kt:289)
    at com.google.devtools.ksp.AbstractKotlinSymbolProcessingExtension.handleException(KotlinSymbolProcessingExtension.kt:394)
    at com.google.devtools.ksp.AbstractKotlinSymbolProcessingExtension.doAnalysis(KotlinSymbolProcessingExtension.kt:289)
    at org.jetbrains.kotlin.cli.jvm.compiler.TopDownAnalyzerFacadeForJVM.analyzeFilesWithJavaIntegration(TopDownAnalyzerFacadeForJVM.kt:123)
    at org.jetbrains.kotlin.cli.jvm.compiler.TopDownAnalyzerFacadeForJVM.analyzeFilesWithJavaIntegration$default(TopDownAnalyzerFacadeForJVM.kt:99)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler$analyze$1.invoke(KotlinToJVMBytecodeCompiler.kt:257)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler$analyze$1.invoke(KotlinToJVMBytecodeCompiler.kt:42)
    at org.jetbrains.kotlin.cli.common.messages.AnalyzerWithCompilerReport.analyzeAndReport(AnalyzerWithCompilerReport.kt:115)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler.analyze(KotlinToJVMBytecodeCompiler.kt:248)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler.compileModules$cli(KotlinToJVMBytecodeCompiler.kt:88)
    at org.jetbrains.kotlin.cli.jvm.compiler.KotlinToJVMBytecodeCompiler.compileModules$cli$default(KotlinToJVMBytecodeCompiler.kt:47)
    at org.jetbrains.kotlin.cli.jvm.K2JVMCompiler.doExecute(K2JVMCompiler.kt:167)
    at org.jetbrains.kotlin.cli.jvm.K2JVMCompiler.doExecute(K2JVMCompiler.kt:53)
    at org.jetbrains.kotlin.cli.common.CLICompiler.execImpl(CLICompiler.kt:101)
    at org.jetbrains.kotlin.cli.common.CLICompiler.execImpl(CLICompiler.kt:47)
    at org.jetbrains.kotlin.cli.common.CLITool.exec(CLITool.kt:101)
    at org.jetbrains.kotlin.daemon.CompileServiceImpl.compile(CompileServiceImpl.kt:1645)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
    at java.base/java.lang.reflect.Method.invoke(Unknown Source)
    at java.rmi/sun.rmi.server.UnicastServerRef.dispatch(Unknown Source)
    at java.rmi/sun.rmi.transport.Transport$1.run(Unknown Source)

I got this on the 1.0.10-release branch, guessing it's not specific to just that branch.

neetopia commented 1 year ago

Tried to reproduce your case, the stacktrace is same, actually it comes from kotlin compiler, where there is an explicit check to ensure a class is contained in another class, i.e. local class is not allowed to query jvm signature. Which leads me to a question, is this a documented behavior for JVM in general? Also if it is, the fix might need to happen in compiler which will take a much longer time to land.

bubenheimer commented 1 year ago

I'm not sure about JVM signature. What I actually need is the somewhat equivalent functionality for binary name #1336. This does look well-defined: Class.forName() takes the binary name as an argument, and I can pass it all sorts of local class binary names to load them. Wouldn't the compiler need a way to build these names?

neetopia commented 1 year ago

That's my concern point, I tried to find a spec on JVM names of a local class but couldn't find one, if there is no such thing it makes sense for compiler to not support it, if there is one, it should be considered as a compiler bug and file a corresponding issue in YouTrack.

bubenheimer commented 1 year ago

The fact that I can load local classes via reflection using Class.forName() suggests to me that local class JVM names are well-defined. To figure out the name string in my manual exploration I used Class.getTypeName(). Can't rule out that reflection class lookup by name is a recursive process, and compiler may not have complete local class names, or leaves it to ASM to figure out these Java-specific details somehow.

Would you be able to file a YouTrack issue to get clarification? I'm really out of my league on Kotlin compiler internals.

bubenheimer commented 1 year ago

If there is no spec for local JVM class names then I guess it could be just ASM and Kotlin compiler using a custom algorithm to figure out a globally unique (and stable) class name of their choosing, and we need a way to get that name from them. Does that make sense?

neetopia commented 1 year ago

Loading via reflection is ok, no matter what, the class has to have a name after compiled, my concern is that as you suggested in #1338 , that the name itself might be tied to something else like kotlin compiler version, then we can't determine a name for symbols loaded from libraries because we don't know what's the compiler version used to compile that library.

This is not an issue for compiler because local classes are meant not to be visible out of the scope, therefore any attempt, including using reflection to load the class when out of scope, is unsupported therefore the compiler doesn't care about changing the name mangling logic since it is not breaking compatibility.

Having said that, I will try to reach out to JB about this issue, let's see what we can get.

bubenheimer commented 1 year ago

Right. This also means that as long as we can always get the name directly from the compiler we're golden. The name may change if the compiler changes, but the processor's name will always be in sync with the compiler's. If user code does not depend on stable names in precompiled code the result will be correct. This is true for my processor: user code never gets to see the actual names, they are not exposed in public API; names will be regenerated whenever the compiler runs, and that's ok.

mapToJvmSignature() and mapToJvmClassName() would need to carry the disclaimer that the returned name cannot be considered stable across runs, if it is local, and should not be exposed in public code.

Thank you for reaching out!