API suggestions

[-]

Alternatively, we can probably use

var annos = classModel.findAttribute(Attributes.RUNTIME_VISIBLE_ANNOTATIONS);
if (annos.isPresent()) transform =
transform.andThen(ClassTransform.dropping(ce -> ce instanceof
RuntimeVisibleAnnotationsAttribute));
// prepare the updated annotations

Back to the first question, I still believe something like

CodeTransform.startHandler(cb -> {
  cb.constantInstruction(index);
  cb.invokestatic(CD_MyClass, "checkpoint", MTD_void_int);
});

is more concise than:

new CodeTransform() {
  @Override
  public void atStart(CodeBuilder cb) {
    cb.constantInstruction(index);
    cb.invokestatic(CD_MyClass, "checkpoint", MTD_void_int);
  }

  @Override
  public void accept(CodeBuilder cb, CodeElement ce) {
    cb.with(ce);
  }
}

I still believe a CodeTransform.startHandler(Consumer<CodeBuilder>)
should be added for the frequency of such call tracking patterns and
the convenience, unlike the attribute update pattern, which is much
less applicable.

On Wed, Mar 22, 2023 at 8:29 AM Brian Goetz <brian.goetz at oracle.com> wrote:
>
> 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
>
>>
>>
>> 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
>>
>>
>