Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mapToJvmSignature crash on local class #1335

Open
bubenheimer opened this issue Mar 2, 2023 · 7 comments
Open

mapToJvmSignature crash on local class #1335

bubenheimer opened this issue Mar 2, 2023 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@bubenheimer
Copy link

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 neetopia added the bug Something isn't working label Mar 2, 2023
@neetopia neetopia self-assigned this Mar 2, 2023
@neetopia
Copy link
Collaborator

neetopia commented Mar 8, 2023

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
Copy link
Author

bubenheimer commented Mar 8, 2023

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
Copy link
Collaborator

neetopia commented Mar 8, 2023

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
Copy link
Author

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
Copy link
Author

bubenheimer commented Mar 10, 2023

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
Copy link
Collaborator

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
Copy link
Author

bubenheimer commented Mar 10, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants