Classfile API CodeBuilder::transforming clarification

[Paul Sandoz]

ClassfileBuilder::transform and CodeBuilder::transforming are closely connected in behavior (with different implementation performance profiles); they differ in the source of events that are pumped through the transformer, which then pumps the flat-mapped results into the receiving builder.

ClassfileBuilder::transform could be generalized to accept Iterable<E>. AFAICT its behavior is not tied to the specifics of CompoundElement<E> and could more generally accept a sequence of elements of a compound element or produced by something else.

For code, ClassfileBuilder::transform could be implemented using CodeBuilder::transforming, where the handler is iter::forEach, and where iter is a variable of type Iterable<CodeElement>.

From this perspective CodeBuilder::transforming seems like the more general builder-based transform primitive.

However, I don’t have a strong sense whether this generality is useful for anything accept the building of code. In that respect associating the transform with a block seems very useful (I agree with the argument order). Overriding block may not do it justice, it seems very powerful, which makes we wanna keep the transform name e.g, transformBlock.

Paul.

> On Mar 9, 2023, at 10:51 AM, Brian Goetz <brian.goetz at oracle.com> wrote:
> 
> Coming back to this ...
> 
> At the top level, we have ClassModel::transform; this takes a materialized ClassModel and applies a ClassTransform, returning the new byte[].  
> 
> This method does not have analogues in {Method,Field,Code}Model, nor does it override ClassfileBuilder::transform; it takes a whole class in and produces a whole class out.  Therefore, I think this method lives in the wrong place; it should live in Classfile, alongside parse/build, where all the other "operate on an entire classfile" operations live.  The following overloads make sense:
> 
>     byte[] transformClass(byte[] classfile, ClassTransform transform)
>     byte[] transformClass(ClassModel classModel, ClassTransform transform)
> 
> This also makes transformation more discoverable, since it lives alongside of build and parse.  
> 
> We have _static_ methods transforming{Code,Fields,Methods,MethodBodies} which are lifting operations on transforms.  These seem fine.  
> 
> The confusing ones are CodeBuilder::transform and ClassfileBuilder::transforming.  
> 
>     // ClassfileBuilder
>     /**
>      * Apply a transform to a model, directing results to this builder.
>      * @param model the model to transform
>      * @param transform the transform to apply
>      */
>     default void transform(CompoundElement<E> model, ClassfileTransform<?, E, B> transform) {
>         @SuppressWarnings("unchecked")
>         B builder = (B) this;
>         var resolved = transform.resolve(builder);
>         resolved.startHandler().run();
>         model.forEachElement(resolved.consumer());
>         resolved.endHandler().run();
>     }
> 
> The purpose of this method is to run a transform on a compound element, with the output of the transform bound to the (receiver) builder.  This is not so much intended to be called directly by users, as much as an implementation convenience for methods like MethodBuilder::transformCode, because the dance of resolving and applying a transform is easy to get wrong.  Arguably this method doesn't belong as an instance method on Builder, as much as a static helper method somewhere that takes (model, transform, builder).  If we can't find any uses of ClassfileBuilder::transform outside of the classfile API implementation itself, perhaps it should be moved to a static helper somewhere, such as in ClassfileTransform, and give it a name like applyTransform(model, transform, builder).
> 
> Which leaves us with CodeBuilder::transforming.  I don't find any uses of it in the implementation itself, and I had to piece together what it does...  
> 
>     // CodeBuilder
>     /**
>      * Apply a transform to the code built by a handler, directing results to this builder.
>      *
>      * @param transform the transform to apply to the code built by the handler
>      * @param handler the handler that receives a {@linkplain CodeBuilder} to
>      * build the code.
>      * @return this builder
>      */
>     default CodeBuilder transforming(CodeTransform transform, Consumer<CodeBuilder> handler) {
>         var resolved = transform.resolve(this);
>         resolved.startHandler().run();
>         handler.accept(new TransformingCodeBuilder(this, resolved.consumer()));
>         resolved.endHandler().run();
>         return this;
>     }
> 
> What it does is combine generating all or part of a method body with CodeBuilder, with transforming that code with a transform, binding the output of the transform to the current builder.  The use case for this is when we have captured useful logic in a transform, and we want this to be applied equally to new methods (or parts of methods) we generate as to existing methods we find in the classfile.  
> 
> Now that I've swapped in what this does ... I agree with Adam's suggestion that it is misnamed.  My first choice is to overload block():
> 
>     CodeBuilder block(Consumer<CodeBuilder> handler)
>     CodeBuilder block(CodeTransform transform, Consumer<CodeBuilder> handler)
> 
> Both are primarily block-building, the latter transforms the block on the fly.  
> 
> I prefer ordering the arguments (transform, block) rather than the other way, because anyone who uses this has likely already captured the transform in a variable, whereas the handler is usually an ad-hoc lambda.  
> 
> 
> 
> 
> 
> On 3/7/2023 5:09 AM, Adam Sotona wrote:
>> Hi,
>> During the Classfile API reviews there have been raised concerns about `CodeBuilder::transforming` method, for details see:
>> https://github.com/openjdk/jdk/pull/10982/files/074dd30401a68638a24c157595caeb96b3511614#r1123858513
>>  
>> I would like to (re-)open this discussion here to find the best suitable form of the following method:
>>     /**
>>      * Apply a transform to the code built by a handler, directing results to this builder.
>>      *
>>      * @param transform the transform to apply to the code built by the handler
>>      * @param handler the handler that receives a {@linkplain CodeBuilder} to
>>      * build the code.
>>      * @return this builder
>>      */
>>     default CodeBuilder transforming(CodeTransform transform, Consumer<CodeBuilder> handler)
>>  
>> My proposal is to align it more with `CodeBuilder::block` method and emphasize more the bytecode block than the transformation itself.
>> I propose to change the method name and arguments to:
>>     /**
>>      * Add a transformed lexical block to the method being built.
>>      * <p>
>>      * Within this block, the {@link #startLabel()} and {@link #endLabel()} correspond
>>      * to the start and end of the block, and the {@link BlockCodeBuilder#breakLabel()}
>>      * also corresponds to the end of the block.
>>      *
>>      * @param handler handler that receives a {@linkplain BlockCodeBuilder} to
>>      * generate the body of the lexical block.
>>      * @param transform the transform to apply to the lexical block generated by handler
>>      * @return this builder
>>      */
>>     default CodeBuilder transformedBlock(Consumer<BlockCodeBuilder> handler, CodeTransform transform)
>>  
>> Or alternatively:
>>     default CodeBuilder transformBlock(Consumer<BlockCodeBuilder> handler, CodeTransform transform)
>>  
>> or:
>>     default CodeBuilder transformingBlock(Consumer<BlockCodeBuilder> handler, CodeTransform transform)
>>  
>> or just simple:
>>     default CodeBuilder block(Consumer<BlockCodeBuilder> handler, CodeTransform transform)
>>  
>>  
>> Please let me know which version do you prefer or propose alternatives.
>>  
>> Thanks,
>> Adam