Read RuntimeInvisible*Annotations in Indexer

[MikeEdgar]

Fixes #120 - I'm hoping they were not excluded deliberately originally :-) This also includes a refactoring of processAttributes to use a switch statement for readability.

[Ladicek]

My understanding is that this was a deliberate decision :-) Perhaps also to make the index smaller. Not sure if these annotations should be accessible by default, or if opt-in should be required...

Side note: case clauses in switch should be indented :-)

[MikeEdgar]

Thanks for the feedback. I fixed the indentation in the switch. I'll run a few tests to see what kind of memory impact this has to help inform whether an opt-in could help.

[MikeEdgar]

I ran a test indexing the quarkus-app/lib jars for a "typical" Quarkus application with Resteasy, Hibernate, and some Camel libraries. The runtime-invisible annotations results in about an additional 1 MB of heap (56M before -> 57M). That could be mostly eliminated with an opt-in switch that could be used to selectively scan the invisible annotations, e.g. only for the application classes and not for dependencies.

Does that sound reasonable? If so, do you have any opinion on the form of the option/switch? Using a system property is my suggestion. Something like a boolean named jandex.runtime-invisible-annotations.

[n1hility]

This is something we should definitely do, since we have had real asks for this. A great example is the Kotlin null annotations. That said, I have been unsure about how we handle the "surprise" of annotations showing up in Jandex but not via reflection, which could lead to framework errors. There probably should be a flag on the annotation to mark whether or not it's visible, but do we need to go a step further and propvide mechanisms to filter (e.g. boolean includeInvisible)? If we filter on the top map API entry points (E.g. calls via Index.), what about people browsing from the class to the annotations.

My instinct is that its not worth API filtering, and we should just present them all with some flag to know the visibility. A good time to make this change would be in 3 since its a significant behavioral difference (new stuff showing up that didnt before).

Thoughts?

[Ladicek]

Yea I agree. AnnotationInstance could have a flag (isRuntimeVisible or something?), and I'd definitely consider this 3.0 material.

[n1hility]

�(BTW just for some background that's why it was not originally introduced in Jandex, there was no demand at the time, and the reflection inconsistency was a concern)

[MikeEdgar]

we have had real asks for this. A great example is the Kotlin null annotations

That's the exact impetus for this PR :-). I did some experimentation with making AnnotationInstance abstract and have a visible and invisible subclass (to avoid adding a flag to the memory footprint). The flag takes the form of a boolean method in each subclass, and of course a new value written/read in the index file.

If that sounds like a reasonable approach, I'll clean up what I have and update this PR with the commit.

[MikeEdgar]

One additional question - if this is for 3.0, does anyone still think there is a need to enable the invisible annotations via a system property, or some other mechanism like what is in this PR currently?

[Ladicek]

I think I'd probably prefer to keep the AnnotationInstance class final, so that people can't hack into internals by subclassing (and putting the subclass into org.jboss.jandex -- which I know I did once, with some other Jandex non-final class).

We could probably cheat and store a marker value into the values array (say, if the 0th item of the array is null, then this is a runtime-invisible annotation, otherwise there's no null value in there), or something like that :-) But if we add all the class-only annotations to the index and then load them into memory, that itself will probably overshadow the impact of adding a byte-sized field to AnnotationInstance.

[Ladicek]

Also, no, I don't think we need an opt-in anymore.

I originally thought it doesn't matter so much whether runtime-invisible annotations are present in the index or not. What does matter is the set of annotations returned by the API. But if we put this into 3.0, it sounds palatable.

[MikeEdgar]

Rebased on master. The final commit includes a new runtimeVisible field in AnnotationInstance, along with the reader/writer changes for it. Additionally, it includes a bit of re-factoring to make version a field in the readers and writers (rather than passing it around) and also to consolidate some duplication for binary searching of AnnotationInstance arrays.

[MikeEdgar]

I saw your comment in #124 and I'll rebase this and target the smallrye branch.

[Ladicek]

Ah I didn't expect you to react so quickly :-) Well, maybe I can merge this as is then :-) I'll take a look. Thanks!

[Ladicek]

Making version a field in readers/writers makes it impossible to reuse a single instance to read/write multiple versions, but we don't need that anywhere. And having version as field makes it unnecessary to pass it around as parameter, which is nice. So that's fine. Also the de-duplication of binary search of AnnotationInstance arrays is nice!

Out of the 3 comments I added, 2 of them are I believe logical errors that need to be fixed. The other is a matter of style, so... :-)

[Ladicek]

Noticed one more mistake, which slipped through because a there wasn't a test. The test proposal I suggested should cover that.

[Ladicek]

This is interesting, but correct I guess? Since the annotation has ElementType.PARAMETER, ElementType.TYPE_USE.

[MikeEdgar]

Yeah, exactly. I wasn't expecting it initially, but it makes sense.

[Ladicek]

Looks good, thanks! I added one commit to your branch to somewhat simplify how AnnotationInstance is constructed. I changed the class to only have 1 private constructor, all callers now must use the static create() methods.

Please let me know if that looks OK to you, and I'll merge this.

[MikeEdgar]

The AnnotationInstance changes look good to me - thanks @Ladicek