Fix getJvmName for @JvmRecord data class properties#2813
Fix getJvmName for @JvmRecord data class properties#2813MariusVolkhart wants to merge 4 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
CLA violation is due to Claude Code. I didn't see anything in the CONTRIBUTING.md indicating agentic AI was not allowed, and wanted to be clear on the fact that it was used as part of this change. Please advise if you want me to make changes or are unable to accept the PR as a result. Thank you. |
MariusVolkhart
force-pushed
the
mv/record
branch
from
6cd15c8 to
86affbb
Compare
|
To me it seems like the tool requires all the owners of the commit (you and anthropic agent) need to sign the CLA. Obviously, the agent itself can't sign it so probably to the time being the workaround is not commit code using the agent |
MariusVolkhart
force-pushed
the
mv/record
branch
from
86affbb to
112045f
Compare
jaschdoc
left a comment
jaschdoc
left a comment
There was a problem hiding this comment.
Hey thank you for opening the PR and apologies for the slow response. I left some comments that aim to improve the test suite and readability. Cheers!
jaschdoc
added
awaiting author response
and removed
awaiting contributor response
labels
|
@MariusVolkhart, just pinging to see if you still want to contribute :) |
@JvmRecord data classes compile to Java records, whose component accessors use bare property names (e.g. name()) rather than bean-style getters (getName()). ResolverAAImpl.getJvmName unconditionally used JvmAbi.getterName() which always added the get prefix. Add a check for the @JvmRecord annotation alongside the existing annotation class check, since both use bare property names as accessor names. Fixes google#2812
The KSPAA17Test class existed only to run one test at JVM target 17, because a // JVM_TARGET: directive in a testData file collides with the hardcoded JVM_TARGET default in AbstractKSPTest: the test framework merges default directives with file-level ones instead of overriding, failing with "Too many values passed to JVM_TARGET: [1.8, 17]". Remove the hardcoded default and fall back to JvmTarget.DEFAULT at the point of consumption in AbstractKSPAATest, which is the same value the directive used to inject. Any test can now pick its JVM target with a one-line directive instead of a dedicated test class, and the record test moves into KSPAATest alongside the other jvmName tests.
The @JvmRecord annotation is not retained in class files, so the annotation-based check never matches a record class consumed from a dependency jar, and accessors were reported with the bean-style get prefix. The analysis API exposes java.lang.Record as a supertype of deserialized record classes, so recognize library records by that supertype, keeping the annotation check for source classes where the implicit supertype is not present. Covering this required two test-infrastructure fixes: library-module compilation now forwards the module's JVM target (records need 16+), and the compiler invocation now fails the test on a non-OK exit code instead of silently producing no class files, which previously surfaced as misleading downstream resolution failures. The testData also gains an @get:JvmName record property documenting that an explicit JvmName takes precedence over record accessor naming.
MariusVolkhart
force-pushed
the
mv/record
branch
from
410b0f9 to
dbcb5df
Compare
jaschdoc
left a comment
jaschdoc
left a comment
There was a problem hiding this comment.
Looks good. Only small nits :)
| @TestMetadata("jvmNameRecord.kt") | ||
| @Test | ||
| fun testJvmNameRecord() { | ||
| runTest("../kotlin-analysis-api/testData/jvmNameRecord.kt") |
There was a problem hiding this comment.
You can just use "$AA_PATH/jvmNameRecord.kt" here
|
|
||
| @TestMetadata("jvmNameRecord.kt") | ||
| @Test | ||
| fun testJvmNameRecord() { |
There was a problem hiding this comment.
It would be great if you could annotate the test with the @Bug annotation. That way, in the future we have some reference as to why the test is there. Example:
There was a problem hiding this comment.
Missing copyright header :)
There was a problem hiding this comment.
Can we add a test where one of the properties of the data class is also a data class / jvm record?
Also, missing copyright header :)
| val exitCode = execMethod.invoke(compiler, PrintStream(outStream), args.toTypedArray()) | ||
| check(exitCode.toString() == "OK") { | ||
| "Kotlin compilation failed with exit code $exitCode:\n$outStream" | ||
| } |
| cls.getAllProperties().map { property -> | ||
| val accessorNames = listOfNotNull( | ||
| property.getter?.let { resolver.getJvmName(it) }, | ||
| property.setter?.let { resolver.getJvmName(it) }, |
There was a problem hiding this comment.
I think we should also get the jvm name of the setter parameter.
| @JvmRecord | ||
| data class WithJvmName(@get:JvmName("customName") val n: Int) | ||
| // FILE: aliased.kt | ||
| import kotlin.jvm.JvmRecord as JR |
There was a problem hiding this comment.
In addition to this, can we also create a test where we use a type alias for the annotation?
3 participants
@JvmRecord data classes compile to Java records, whose component accessors use bare property names (e.g. name()) rather than bean-style getters (getName()). ResolverAAImpl.getJvmName unconditionally used JvmAbi.getterName() which always added the get prefix.
Add a check for the @JvmRecord annotation alongside the existing annotation class check, since both use bare property names as accessor names.
Fixes #2812