opentelemetry-java-instrumentation icon indicating copy to clipboard operation
opentelemetry-java-instrumentation copied to clipboard

Force dynamic typing on AssignReturned annotations

Open JonasKunz opened this issue 7 months ago • 0 comments

When using invokedynamic-Advices, changes to the original method arguments, instrumented class fields etc have to be done via the bytebuddy @Advice.AssignReturned annotations.

These annotations all have a typing parameter, which defaults to STATIC. This means bytebuddy will attempt to assign the returned value without a cast to the target, which can very easily yield unloadable bytecode: E.g. assigning an entry from an Object[] to an non-Object parameter will cause the JVM bytecode verifier to reject the instrumented class. These error messages are hard to find the root cause for unless you deeply know how the instrumentations work and you know to look at the @AssignReturned.* annotations. This will get even worse due to the type-erasure performed in #11873.

The alternative is to use DYNAMIC for typing, which means bytebuddy will insert a cast before doing the assignment.

This change proposes to alter the behaviour so that we always use DYNAMIC, independent of what is specified in the annotations. This should make writing instrumentations easier for beginners and prevent cluttering the annotations with typing = Assigner.Typing.DYNAMIC, making them more readable due to removing noise.

Note that this is just a proposal based on my "taste" and what I personally think of whats best for ease-of-use, if you prefer the explicit typing via the annotations feel free to reject this PR.

JonasKunz avatar Jul 23 '24 10:07 JonasKunz