Improve annotation literals generation in ArC

[Ladicek]

This commit adds support for emitting annotation literal classes with all
possible annotation member types: all primitive types, strings, enums,
classes, nested annotations, and arrays of previously mentioned types.
A test for the annotation literal class generator is added, too.

Additionally, this commit removes the ability to generate "one-off" annotation
literal classes. They are now always shared. This allows simplification of
the AnnotationLiteralProcessor and AnnotationLiteralGenerator classes.
These classes now maintain separation between generating an annotation
literal class and generating a bytecode sequence to instantiate it.

The previous user-visible APIs that dealed with "one-off" annotation literals
are now deprecated.

[Ladicek]

Marking as draft for now, because I want to discuss one thing and don't want to pollute CI until that is settled.

@mkouba There's one thing that technically breaks compatibility: where the [semi-]public API used to expose AnnotationLiteralProcessor, it now exposes AnnotationLiteralGenerator. I kinda like the 2nd name better (the class also extends AbstractGenerator), but if you want to maintain true compatibility, I see 2 ways to restore it: 1. rename AnnotationLiteralGenerator to AnnotationLiteralProcessor; 2. create an empty subclass of AnnotationLiteralGenerator called AnnotationLiteralProcessor that would be deprecated (plus expose that in the API, also as deprecated method). WDYT?

[Ladicek]

Maybe renaming AnnotationLiteralGenerator [back] to AnnotationLiteralProcessor would also make this PR a bit smaller and get rid of small distracting changes...

[Ladicek]

Okay, I tried reverting those changes that break compatibility, even though I believe AnnotationLiteralGenerator.create() is a better name than AnnotationLiteralProcessor.process(). It does make the diff a bit smaller, but only the trivial changes are gone -- those gritty changes resulting from merging the two classes obviously remain :-)

[mkouba]

I have to admit that I don't fully understand the need to merge those two classes. In fact, I created two separate classes intentionally because they do different things. AnnotationLiteralGenerator is generating the bytecode (like other generators do) whereas the AnnotationLiteralProcessor.process() handles the call sites and caching (note that in quarkus the literals are always shared). In other words, the clients of AnnotationLiteralProcessor.process() should not care about generation at all - it's the responsibility of the integration layer (which knows whether the literals are shared or not).

[Ladicek]

These two classes share so many internal assumptions and data structures it made sense to me to merge them. That was the only way I could understand the code and extend it to support all types of annotation members.

I admit the javadoc as present in this PR is focused more on the generation part, and I can improve that probably :-) But if you look at the process method, even as present in current main branch, it shows that the caller has to be aware of annotation literal sharing and generation. The caller literally has to provide a ClassOutput to which the annotation literal class may be generated if sharing is disabled. If we got rid of the possibility of oneoff annotation literals and made sharing the only option, then sure, that would allow clean separation at least on the API level. I still think that implementation-wise, it makes sense to merge the 2 classes.

[Ladicek]

I tried to improve the AnnotationLiteralProcessor javadoc per above.

Now, if you insist the merge is a bad idea, I can split them again. I would probably be able to do that, now that I understand how the code is supposed to work :-)

[mkouba]

If we got rid of the possibility of oneoff annotation literals and made sharing the only option, then sure, that would allow clean separation at least on the API level.

I like this idea. I don't remember the details why we kept the possibility of one-off annotation literals but given the fact that it's shared by default (sharedAnnotationLiterals=true) and Quarkus is the only ArC integrator I tend to support the idea of removing that functionality.

[Ladicek]

That sounds good to me. I'll try to prepare a (2nd?) PR.

For backward compatibility, we'll have to keep the old process method with useless parameters in place for a while, as well as the BeanProcessor.Builder.setSharedAnnotationLiterals() method (not sure if there's anything more, I think not), but that shouldn't be a big deal. These are all technically public APIs, but I doubt very much anyone outside Quarkus uses them.

[mkouba]

For backward compatibility, we'll have to keep the old process method with useless parameters in place for a while, as well as the BeanProcessor.Builder.setSharedAnnotationLiterals() method (not sure if there's anything more, I think not), but that shouldn't be a big deal. These are all technically public APIs, but I doubt very much anyone outside Quarkus uses them.

I'd keep the old process() method and mark it as deprecated but I'd remove the BeanProcessor.Builder.setSharedAnnotationLiterals() method or mark it as deprecated and throw an exception in the body.

[Ladicek]

OK, just got back to this, sorry it took me so long (see smallrye/jandex#206 if you wanna know why :-) ).

I started splitting the class back into two and found there's an actual need to merge them: nested annotations. If I use AnnotationLiteralProcessor with an annotation that contains a nested annotation, then AnnotationLiteralGenerator needs to call AnnotationLiteralProcessor to instantiate the nested annotation literal. At this point, I believe it just doesn't make sense to keep the two classes separate, because they are just so closely tied together.

(Note that this entails an interesting problem. AnnotationLiteralGenerator calling AnnotationLiteralProcessor to instantiate the nested annotation literal may include generating its class. This means I'm modifying the annotation literal cache, i.e. a Map, during iteration. I need to think about how to fix this, but this doesn't really change the reasoning above.)

[Ladicek]

Okay, the problem I mentioned above only appears for one-off annotation literals. For shared ones, the nested annotation classes are generated during the initial AnnotationLiteralProcessor.process(), so that should be OK. I'm not entirely sure the inter-dependency still exists if I remove one-off annotation literals, let me investigate more.

[Ladicek]

OK, when I remove one-off annotation literals, pretty clean separation between AnnotationLiteranProcessor and AnnotationLiteralGenerator exists, so I can split them back. (Note that present AnnotationLiteranProcessor calls a bunch of static methods on AnnotationLiteralGenerator which are not used by AnnotationLiteralGenerator otherwise, so I'll just move them around.)

[mkouba]

Ok, nested annotations were not supported before (because the AnnotationLiteralGenerator was originally used only for qualifiers and annotation-valued members should be annotated @Nonbinding in CDI and from my experience are rarely used... and nobody has complained so far ;-).

OK, when I remove one-off annotation literals, pretty clean separation between AnnotationLiteranProcessor and AnnotationLiteralGenerator exists, so I can split them back.

I do trust your judgment and if you believe it would be better to split them, go ahead! ;-)

[Ladicek]

I'll keep them separate, because after removing one-off annotation literals, the separation is actually possible and meaningful.

Just FYI, we need nested annotations for CDI Lite, or Build Compatible Extensions specifically. Synthetic beans in BCE allow passing arbitrary annotations from build time to runtime. (Which I think is a reasonable compromise between expressivity and simplicity.) More PRs related to that will be coming :-)

[Ladicek]

I think this is ready now.

[Ladicek]

Rebased + got rid of a duplicate assertion in the test.

[mkouba]

Merged, thanks!

[Ladicek]

Thank you!