RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]

[Serguei Spitsyn]

On Wed, 20 Mar 2024 03:25:39 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Alex Menkov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Updated the fix and the test for multiple annotations
>
> test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 107:
> 
>> 105:         newClassBytes = classBytes;
>> 106:         fInst.retransformClasses(targetClass);
>> 107:         assertTrue(targetClass.getName() + " was not seen by transform()", seenClassBytes != null);
> 
> Nit: I guess, the `targetClassName` was intended to be used in place of `targetClass.getName()`.

The `retransform()` method returns an array of bytes. There is also a comment about it at the line 101. But the result has never been checked (please, see lines: 116, 123, 129, 135). This creates some confusion and questions why the return value is needed.
The parameter `null` at lines 123, 129, 135 is also confusing and need some explanation.
As I read the code, the method  `retransform()` is saving the parameter `classBytes` in the `newClassBytes` field. The field `newClassBytes` value is then returned by the `transform()` method.
But the `ClassFileTransformer.html#transform` states this:
`Returns: a well-formed class file buffer (the result of the transform), or null if no transform is performed`.

So that, when the `null` is returned by the `transform()` then it means there was no real transform.
Could you comment on this? Do I miss anything here?

Please, see: [ClassFileTransformer.html#transform](https://docs.oracle.com/en/java/javase/21/docs/api/java.instrument/java/lang/instrument/ClassFileTransformer.html#transform(java.lang.Module,java.lang.ClassLoader,java.lang.String,java.lang.Class,java.security.ProtectionDomain,byte%5B%5D))

Also, the comment at line 122 is not needed at this form:

            // Ensure retransformation does not fail with ClassFormatError.

Sorry, I was not clear when I've asked to add a comment at this point. I wanted a clarification about what does passing the `null` mean in terms of transformation.
My understanding is that there is no real transformation when the `null` is returned but the `Instrumentation` mechanism is still working and can fail and we want to test/verify it.  So, passing a Null-transformer is enough for our testing. Is it correct?
Please, keep in mind that few people have some knowledge about the re-transformation details and would appreciate any clarification here.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1531502998