A list of issues about Classfile API

[-]

Short conclusion: we have agreed on two actions so far,
1. Hide writeTo(BufWriter) in classfile.instruction.LocalVariable(Type)
2. Add typeSymbol for the 5 entries that have Field/Method types

And for the rest, here are my responses, inlined:

On Wed, Jun 28, 2023 at 10:00 PM Brian Goetz <brian.goetz at oracle.com> wrote:

> Thanks for gathering this list.  Some quick thoughts inline.
>
> On 6/28/2023 2:32 AM, - wrote:
>
> Hello, I just looked over the Classfile API and found a few design issues,
> aside from simple typos addressed in
> https://github.com/openjdk/jdk/pull/14686:
>
> 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.

>
> But I do agree that the inclusion of `writeTo(BufWriter)` is
> implementation that should be encapsulated.
>
> 2.
> ConstantPoolBuilder::constantValueEntry/loadableConstantEntry/annotationConstantValueEntry
> should accept the 4 subword types: Byte, Boolean, Char, Short, which should
> simply map to int entries.
>
>
> If we think this is needed, we'll need an overload for
> constantValueEntry(Object), since Byte and friends are not ConstantDesc,
> and there is unfortunately no common supertype for the wrapper classes.  If
> you have a sharp type in hand, you can also use intEntry for
> Byte/Char/Short.
>
Oops, never noticed these 4 subword primitive types are not ConstantDesc.
This suggestion is moot.

>
>
> 3. BufWriter::asByteBuffer is weird: it is a write-allowed buffer that can
> modify the buf until the old array is discarded when the backing buf is
> expanded. Currently used by stack map generation and stack counting, we
> should probably refit this to be user-safe or make this an internal API.
>
>
> 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.

>
> 4. FieldRefEntry, MethodRefEntry, InterfaceMethodRefEntry,
> ConstantDynamicEntry, InvokeDynamicEntry should have typeSymbol() to easily
> access the shared field and method type symbols.
>
>
> Agree
>
> 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.

>
>
> 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.

>
>
>     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. It's possible that
some 3rd party user introduces a custom attribute that should be parsed
differently when present in a Field vs in a Method; don't think our API
handles such cases at all.

>
> 2. For nullable references, currently Classfile API accepts an
> Optional<Xxx> argument instead of a nullable argument. Should we switch
> those to nullable arguments instead, like parameter name for
> MethodParameterInfo?
>
>
> 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.

>
>
> 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

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

Chen Liang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20230628/ea40eaf7/attachment.htm>