POC: JDK ClassModel -> ASM ClassReader

[Brian Goetz]

Just some quick comments, more when I get back from vacation.  

> Thanks for merging my patches, the class reader API works more or less equivalent to the ASM version. The one thing I have not working so far are attributes that the JDK knows but ASM does not. I was trying to implement the "contents" method upwards, but this is difficult with the unbound attributes as they normally require a BufWriter which in turn needs a ConstantPoolBuilder. Somehow, I need to pipe the constant resolution to ASM which cannot be done without unsealing some interfaces. I will try to prototype a solution here, too, but I wanted to get the writer working first.

I’ll take a look at this when I get back.

> For the type annotations on instructions: Would it be an option to add "getVisibleAnnotations" and "getInvisibleAnnotations" to the relevant CodeElement types? This way, I could for example query the "ExceptionCatch" value for its annotations. 

I’d like to slice this in two parts; one to query the BCI for a (bound) CodeElement, and one to use that BCI to lookup annotations.  Getting the BCI is an essential bit of functionality, but given that elements can be bound or unbound, such functionality will be necessarily partial, so we have to communicate “no BCI” as a possible return value.  

> StackMapFrames could on the other hand just be added at their position in the CodeElement iteration to receive them where they become relevant. This way, one would not need to keep track of the current offset. This would also allow for an easier write model where ASM does not allow you to know the offset of a stack map. I assume that the current model is very much modeled after the needs of the javap tool. Ideally, the frame objects would be reduced to the information that is contained in a class file and the consumer could track implicit information such as the "effective" stack and locals.

There are two concerns with this, one minor and one major.  The minor one is that this has a significant cost, and most users don’t want this information.  So we would surely want to gate this with an option whose default is false.  (We care the most about transformation, and most transformations make only light changes, so we don’t want to add costs that most users won’t want to bear.). 

The major one is how it perturbs the model.  The element stream delivered by a model, and the one consumed by a builder, should be duals.  What should a builder do when handed a frame?  Switch responsibility over to the user for correct frame generation?  Ignore it and regenerate stack maps anyway?  

I think we need a better picture of “who is the audience for this sub feature” before designing the API.  

> At last, some notes I made while working with the writer API:
> - ClassBuilder does not require flags when creating a field using "withField", but does so for "withMethod". Should this not be consistent?

One can always set flags through the AccessFlags element; anything else is just a convenience.  The convenience is there for fields because they almost never have any other elements — but methods almost always do.  I don’t necessarily object to conveniences, but I don’t want to confuse users with too many ways to do the same thing.  (I think the flags-accepting overload for fields does NOT accept a Consumer, so there’s always only one way to do it within each withField overload — which is a good thing.). 

> - EnclosingMethodAttribute requires optional parameters in its "of" method. I find that a bit awkward and would rather see an overload or the parameters to accept "null”.

This is a deeper question of API design than it first appears.  (BTW originally we had the null-accepting/delivering version, and we found it to be error-prone.). 

We want for the _data model_ to be the source of truth, and derive the API from that.  One reason for doing so is that it means that the reading API (accessors on the Element) and the writing API (withXxx methods on Builders) will be free of gratuitous inconsistencies, so you can always take things apart to whatever level you want, clone/modify/keep each individual datum, and feed them back to the corresponding builder without having to convert, wrap, unwrap, etc.  So the return type of `enclosingMethod` returns an Optional, there’s a strong argument that the of() method should take one.  Otherwise, users have to make up the difference in ad-hoc and error-prone ways when adapting classifies.  

Both Optional and null are reasonable ways to represent possibly-not-there values in an API like this, but mixing the two in the same API multiplies the chance for error.  So I prefer to address this question at a higher level (“should anything ever return null”) rather than what a particular accessor or factory should deal in.  

Cheers,
-Brian