API suggestions

[Brian Goetz]

Here's the combinator I had in mind:

     static<T extends CodeElement> CodeTransform 
statefulReplace(Class<T> target,
Function<T, T> replacer,
Supplier<T> supplier) {
         return new CodeTransform() {
             boolean found = false;

             @Override
             public void accept(CodeBuilder builder, CodeElement element) {
                 if (target.isAssignableFrom(element.getClass())) {
                     found = true;
                     builder.accept(replacer.apply((T) element));
                 }
             }

             @Override
             public void atEnd(CodeBuilder builder) {
                 if (!found)
                     builder.accept(supplier.get());
             }
         };
     }

So then you could say

     CodeTransform = statefulReplace(RVAA.class,
                                     rvaa -> RVAA.of(...),
                                     () -> RVAA.of(...));

It's cute, but I don't think it carries its weight in the API. I think 
the real gap here is one of documentation, not abstraction; finding the 
low-energy-state solution is not obvious enough.  Once you find the 
solution, making it incrementally smaller with a combinator (or ten) is 
not as important.

Public Service Announcement: people think that code is the important 
contribution, but sometimes doc is a more valuable contribution.  A 
"transform writers guide" would be an awesome contribution.


On 3/22/2023 8:42 AM, Dan Heidinga wrote:
>
>
> On Wed, Mar 22, 2023 at 8:19 AM Brian Goetz <brian.goetz at oracle.com> 
> wrote:
>
>     Before we get into what API changes are needed, let's sync on the
>     problem first.
>
>     1.  CodeTransform has an `atStart` method just as it has an
>     `atEnd` method, and I presume overriding `atStart(builder)` is
>     adequate to your need.  Is the issue simply that there is no
>     canned combinator for startHandler as there is for endHandler?
>
>     2.  Adding an annotation is complicated, and we explored a number
>     of solutions before settling in the current place.  There are
>     three cases:
>
>      - There is no RVAA, so you have to add one
>      - There is an RVAA, but it already has your annotation, so
>     there's nothing to do
>      - There is an RVAA, and it doesn't have your annotation, so you
>     have to transform it
>
>     (Actually there is a fourth case: it has your annotation, but with
>     _different properties_; there is @Foo(3) but you want to add
>     @Foo(4).  Now what do you do?  Replace it?  Leave it?  Add both?)
>
>     The trickiness is compounded by the fact that the latter two cases
>     can be handled by transforming the element for RVAA, but the first
>     case can't be handled until the end handler, and you have to
>     remember whether you saw an RVAA or not.  So the process is
>     inherently stateful.  Some options were covered in this thread
>     (which talked about NestMembers, but the story is the same for
>     annotations.)
>
>     https://mail.openjdk.org/pipermail/classfile-api-dev/2022-August/000108.html
>
>     Here's the recommended code for that situation:
>
>          () -> new ClassTransform() {
>              boolean foundNM = false;
>
>              void accept(ClassBuilder b, ClassElement e) {
>                  switch (e) {
>                      case NestMembersAttribute a -> { transform a;
>     foundNM =
>     true; }
>                      ... other transform stuff ...
>                  }
>              }
>
>              void atEnd(ClassBuilder b) {
>                  if (!foundNM)
>     builder.accept(NestMembersAttribute.of(...));
>              }
>          }
>
>     It's irritating because you have to do three things -- keep track
>     of some state, rewrite an attribute if you see it, and generate an
>     attribute if you don't, but it's really not so lengthy or hard to
>     follow.
>
>
> Here's a link to my various attempts to find a better way to express 
> this [0] when exploring NestMember attribute changes.  Wrapping 
> Brian's suggested pattern in a named class makes it a little easier to 
> read at the point of use but there's no single pattern for this (that 
> I've found) that results in significantly cleaner code.
>
> TLDR: Brian's proposed code is the best you can do for this.
>
> --Dan
>
> [0] 
> https://github.com/DanHeidinga/jdk-sandbox/blob/9c5bf15522e76ebc106a433758c87623dec4827b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/GenerateLambdaClassesPlugin.java#L238-L385 
> <https://urldefense.com/v3/__https://github.com/DanHeidinga/jdk-sandbox/blob/9c5bf15522e76ebc106a433758c87623dec4827b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/GenerateLambdaClassesPlugin.java*L238-L385__;Iw!!ACWV5N9M2RV99hQ!J-O_EVx6bVisxcY4VatFAgX4IqHCqDFZv9lj0hkNJQT9QCbPXVxQCCzwbWh-hHP8ZbPTFRJrET9BugBPlJc$>
>
>
>     I can imagine capturing this in a combinator, where you provide
>     two lambdas, one a transform on your favorite attribute, and one a
>     supplier of that attribute, but I don't think it will be that much
>     easier to use, it just automate managing and acting on the boolean.
>
>
>
>
>     On 3/21/2023 11:38 PM, - wrote:
>>     Hi,
>>     After housekeeping the few tests after the latest integration of
>>     additional constants in ConstantDescs, I have found two potentially
>>     common patterns in bytecode manipulation and wish to suggest new APIs:
>>
>>     1. Injection of code in the beginning of methods
>>     Currently, our CodeTransform only has an endHandler; however, this is
>>     less useful, for a method can have multiple exit points. Usually to
>>     track method calls, bytecode manipulation, such as those in jfr and
>>     instrumentation, inject at the beginning of methods, which is
>>     guaranteed to be on the code path.
>>
>>     Thus, I suggest a `startHandler(Consumer<CodeBuilder>)` for
>>     CodeTransform for this purpose, instead of having to explicitly
>>     override atStart(CodeBuilder) in non-lambda CodeTransform.
>>
>>     2. Adding new annotations
>>     In ASM, annotation injection is done simply via extra annotation
>>     visitor calls, which makes it compatible for either single or multiple
>>     annotations. In contrast, in the Classfile API, if we want to add an
>>     annotation, feeding a new RuntimeVisibleAnnotationAttribute will
>>     destroy the previous RuntimeVisibleAnnotationAttribute, and handling
>>     via a stateful ClassTransform is a bit too lengthy for users.
>>
>>     I don't know if we should have an API like Map::computeIfAbsent for
>>     attributes or if we will simplify it via other means.
>>
>>     Chen Liang
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20230322/c7ee4af4/attachment-0001.htm>