A list of issues about Classfile API

[Brian Goetz]

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?

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.

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

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

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

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

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

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

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