POC: JDK ClassModel -> ASM ClassReader

[Brian Goetz]

Thanks for test-driving this!

> This works really well, congratulations to such a well-working first 
> attempt. Some details stuck out however:

Good to hear.

> 1. It does not seem like it is possible to model “CROP” frames. The 
> frames that are read include zero additional local variables, but they 
> do not indicate how many local variables are cropped. I would suggest 
> adding subinterfaces for all frame types to allow for the same pattern 
> matching style that works well with the rest of the API. This way, the 
> declaredLocals and declaredStack methods would only be available if 
> relevant and the crop frame type could add a croppedLocals() : int method.

In general, the API for stack maps hasn't gotten a lot of attention, as 
there is not a lot of user code that accesses stack maps at all, and we 
generate stack maps as part of code generation.  So I'm not surprised 
there are missing bits or rough edges (same for next issue.)  Are you 
generating stack maps yourself?  Is the generator in the library not 
meeting your needs?

> 3. Debugging with toString works well for the most, but not all 
> classes, for example subclasses of CodeElement have representations. 
> It’s probably an oversight but it would be neat to add this quickly to 
> make exploring the code easier.

We should make a pass through all the implementations and make sure that 
toString is both present and uniform.  I am not surprised there are gaps 
here.  That would be a fine "starter bug" for someone to grab.

> 4. The JDK code knows an attribute named CharacterRange. I must 
> confess that I never heard of it and I could neither find 
> documentation. I assume it is a JDK20 concept? This made me however 
> think about how such “unknown” attributes can be handled. I would like 
> to find a way to treat all attributes that I do not know or care about 
> as an UnknownAttribute. This way, I could simply forward them as a 
> binary dump, as I currently do it for custom attributes, for example 
> to forward it to ASM. However, today there is no way to convert an 
> attribute payload back to an array.

It is not a JDK20 concept; it is what is used by the `jcov` coverage 
tool (and I believe others such as jacoco), from the CodeTools project.  
Javac will generate this attribute if you ask it nicely.

You should just ignore attributes you don't understand; that's how 
attributes were designed to work (there will always be new ones.) But 
your point about getting the raw contents is well taken. Essentially, 
your request is to raise `UnknownAttribute::contents` up to Attribute, 
perhaps renaming to `rawContents`.  This seems an easy and reasonable 
addition.

> 5. I think the “findAttribute” method will be invoked a lot. 
> Currently, it iterates over all attributes on each call. Ideally, this 
> would only be done once to create a map of attributes with O(1) lookup 
> speed. Of course, I could do this myself, but I think this would be 
> such a common need that I wanted to suggest it as implementation for 
> the official API, especially since the API feels like a map lookup. 
> This could be done as easily as by storing attributes in a map after 
> they are read for the first time as the attribute keys already are 
> compared by identity.

We considered this, and early performance tests suggested this was a 
loser.  However, that was a lot of API and workload churn ago, before 
AttributeMapper, which is interned, so we could try with an 
IdentityHashMap here and see whether it perturbs the benchmarks.

> 6. I found that TypeAnnotation.CatchTarget offers an index of the 
> exception table entry, additionally to being visited inline. I found 
> that model a bit awkward as there is no indication of the index in the 
> ExceptionCatch instruction that comes with the same pass. Also, there 
> is no guarantee on when the type annotations are visited in relation 
> to the try catch block. Ideally, it would be guaranteed that the 
> annotation is visited directly after the ExceptionCatch pseudo 
> instruction, to allow for an easy single-pass processing.

Yeah, everything about the type annotations attributes is a 
disaster...   Transforming while keeping the validity of the TA 
attributes is essentially impossible, because it includes indexes into 
things like exception tables, code attributes, tvar lists, tvar bounds 
lists, etc etc.  We can decode a TA attribute, we can give you a way to 
write one, but for a while we seriously considered just _dropping TA 
attributes on the floor during adaptation_ because they're so inherently 
unstable.

The visitation order guarantee you ask for is pretty expensive, and 
something that very very few people would care about.  So I surely 
wouldn't want to do that by default.

We've largely punted on doing anything better with type annotations, at 
least up until now, because they're such a disaster.  Still, we can 
probably do better here.  Open to suggestions.

> Finally, I have not yet started working on a ClassWriter equivalent. 
> Here, I found that the style of the Consumer<ClassBuilder> to be 
> incompatible with the way ASM works. This is of course a decision of 
> style, but I would consider this a major difficulty to migrate current 
> code at some point. Would it be an idea to offer both styles of class 
> creation? The internal one, additionally to exposing an interface with 
> the methods of DirectClassBuilder?

We've struggled with this as well when adapting the back end of 
`javac`.  This is a tricky issue.  In theory, allowing users to ask for 
a ClassBuilder is not hard, as you've sketched out.  But I am really 
worried it will become an attractive nuisance, where users will 
instinctively reach for that style because its familiar and seems 
"easier", and then will lose out on some features that the 
Consumer<ClassBuilder> approach offers, such as managing branch offset 
sizes.  Success would be if this were allowed, but only used 1% of the 
time.  Failure would be 3% :)

Q: in this case, is it enough if only ClassBuilder has this option, or 
do you need it for MethodBuilder and CodeBuilder as well?