A list of issues about Classfile API

[Brian Goetz]

On 6/28/2023 11:02 AM, - wrote:
> And for the rest, here are my responses, inlined:
> Specific issues:
>
>>     1. LocalVariable and LocalVariableType: They are missing factory
>>     methods. In addition, they shouldn't have writeTo(BufWriter)
>>     method exposed in the public API.
>
>     I see two factory methods in LocalVariable and the same in
>     LocalVariableType, both called "of".  Are there other versions
>     missing?
>
> Sorry, I meant classfile.attribute.LocalVariableInfo and 
> LocalVariableTypeInfo.

Since these are purely internal classes, I think instantiating through 
constructors is fine here.

>
>
>     Yes, this seems to have been added in to support the stack map
>     generator.  I agree it should move internal and/or be refitted.
>
> I think we will move this to internal for now, as its behavior as a 
> public API is up to debate. We can always add it in the future shall 
> we need it.

This works for me.

>
>
>>     5. AttributeMapper::validSince should be removed, as mentioned by
>>     Adam a while ago; it's unused and its implementations are
>>     sometimes incorrect.
>
>     I have mixed feelings about this.  The JVMS has a table of when
>     each attribute was defined, and this is a sensible and
>     authoritative place to put this information.  On the other hand,
>     it is not enforced when writing classfiles, and for attributes
>     that are not defined by JVMS, this quantity is basically undefined.
>
>     Perhaps it should be renamed to something that doesn't imply
>     validation, such as "introducedInVersion"?
>
>     If we're to remove it, we should ask a similar question about
>     allowMultiple(), though we do enforce this one and it seems useful
>     as a way of catching silly errors.
>
> Introduction version is more like a Classfile version-based toggle to 
> treat an attribute too new as unknown (default behavior per JVMS). 
> Maybe users want more fine-grained control to parse attributes based 
> on class reader context, which I think may be better done by allowing 
> mapper to return Optional.empty/null for readAttribute; and Classfile 
> API may emit an UnknownAttribute instead based on options.

Having options to determine whether to filter inapplicable attributes 
seems a configuration mess for little value.  I'm OK with removing 
validSince(), since we are unlikely to act on it substantially.

>
>
>>     General issues:
>>     1. What about the attributes in Code? Currently, attributes in
>>     code aren't delivered in Streaming besides the few hardcoded
>>     ones, which means that custom attributes will be lost after a
>>     code model undergoes an accept-all code transform. Is that the
>>     intended behavior?
>
>     Not the intended behavior.  The definition of CodeElement says:
>
>     public sealed interface CodeElement extends ClassfileElement
>             permits Instruction, PseudoInstruction,
>                     CustomAttribute,
>     RuntimeVisibleTypeAnnotationsAttribute,
>     RuntimeInvisibleTypeAnnotationsAttribute,
>                     StackMapTableAttribute {
>     }
>
>     so CustomAttribute should definitely be expected by consumers.  We
>     eat a number of other attributes, such as line numbers, stack map,
>     etc, presenting their contents as pseudo-instructions.  And we
>     definitely eat stackmaps (similar to how we treat
>     BootstrapMethodsTable as part of the CP, not an attribute.)  But
>     the residue of attributes (which is usually empty) should be
>     delivered as custom attributes.  I suspect we never noticed
>     because so few classfile have these.
>
> In addition, if we do stream custom attributes, we might declare 
> UnknownAttribute a subtype of CodeElement and stream them as well, if 
> the process unknown attribute option allows.

Yes, that seems right.

Separately: we need better documentation for attribute handing. This 
could go in a section of the package Javadoc, but there are a number of 
conceptual things to capture: how some attributes are considered part of 
some other classfile element, ho those attributes are presented as 
elements, how to use AttributeMapper, what happens with unknown 
attributes, compability considerations when adapting classfiles with 
unknown attributes, the horrible special story of the type annotation 
attributes, etc.

>
>
>>         1.1. If CodeElement is only for streamable elements, then
>>     StackMapTableAttribute shouldn't be a CodeElement, as it's never
>>     delivered in code streams.
>
>     Yes, probably true. I think we could definitely do better at
>     making it clear which attributes are considered "part of something
>     else", and under what conditions they will show up anyway (e.g.,
>     if you ask for CodeModel::attributes you will see the stack map
>     table.)  Not clear whether the way to do this is with code (such
>     as a NotHandledAsElement supertype) or a section of the user's
>     guide (what user's guide?) about attributes.
>
> The ClassfileElement is currently only restricting streaming; 
> attributes themselves can appear on any AttributedElement without type 
> restriction, and attribute mappers work across all contexts as well.

Yes, this is how it was intended -- the attributes bytes are always 
available if you ask the right way, but the element view shows you a 
logical, if not physical, view of the classfile.

>
>     Is there a third choice?  :)   Both are kind of sad.
>
>     We should certainly be consistent.  Thankfully, I think there are
>     relatively few cases here to handle?
>
> It's said that Optional wrapper would bring extra overhead even when 
> value types drop, especially if the wrapped type could have been 
> inlined. Code wise, it does look somewhat verbose (compared to the 
> traditional null-based approach used by familiar ASM), so it's more 
> like a personal habit thing, and I slightly favor nullable due to IDE 
> warnings and preexisting ASM usage. I think we can find most use cases 
> already by searching for Optional in Classfile API.

OK, let's make a list of both nullable and Optional parameters (this is 
a useful doc item to have anyway), and make the least bad choice we can 
given the context.

>
>>     3. We can add @sealedGraph tag to sealed interfaces in the
>>     Classfile API, which will render an svg image of the sealed class
>>     hierarchy (exists in classes like j.l.i.CallSite already)
>
>     For types where all the subtypes are private (and there are a lot
>     of these), naming the subtypes is probably just teasing the user. 
>     Does the SVG renderer remove non-public permitted subtypes?  Some
>     of the public types permit both public and private subtypes, which
>     would be confusing.
>
> It does filter and only render types in public API: see the FFI API's 
> MemoryLayout example: 
> https://download.java.net/java/early_access/jdk21/docs/api/java.base/java/lang/foreign/MemoryLayout-sealed-graph.svg 
> <https://urldefense.com/v3/__https://download.java.net/java/early_access/jdk21/docs/api/java.base/java/lang/foreign/MemoryLayout-sealed-graph.svg__;!!ACWV5N9M2RV99hQ!PA1jju5DwgMm1CKiRbiacC3UCYN1J8nVqRQqAdY29KdBfWS1MjQVL05xA50Dd81m77aRsG0F5i7mL6wtlHYiBPNz$>

OK, so I think we can apply this to interfaces that have any public 
subtypes.

> Again, thank you for your time and this detailed review!

Thanks for your diligent help with this project, your efforts are 
definitely improving both the implementation and API.

Cheers,
-Brian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20230628/19d51669/attachment-0001.htm>