Comments (13)
- Check
IndexView
fornull
eagerly, throw IAE ifnull
.
+1
- Always look for the container annotation, even if the "plain" annotation is present.
That's an interesting edge case. And I agree that we should always look for the container annotation as well. TBH I know about a few places in quarkus where this corner case is not handled "correctly" ;-).
from jandex.
+10 for unification.
annotationWithRepeatable()
should be probably annotationsWithRepeatable()
because it returns a collection. TBH I don't like the name very much. But the truth is that annotations(DotName repeatableName, IndexView index)
could be quite confusing.
I wonder if it would make sense to replace the "direct" variants of the method with composition, i.e. something like AnnotationTarget direct()
or AnnotationTarget directAnnotations()
.
Also I'm not 100% sure it makes sense to define both annotations()
and annotationsMap()
. I understand that the map is sometimes useful for ClassInfo
and MethodInfo
but the common use case "Is an annotation used somewhere on the class/method?" can be solved with hasAnnotation(DotName name)
...
from jandex.
I think annotationWithRepeatable
was probably a typo, not an intentional change. Jandex 2.4 calls it annotationsWithRepeatable
, so 3.0 should too. No reason to change that.
I didn't think of adding AnnotationTarget direct()
before. It's an interesting idea, but I think it would cause more harm than good. Suddenly, you would have 2 different AnnotationTarget
objects, both referring to the same declaration/type, but each returning different annotations. I thought having annotations()
and directAnnotations()
would be natural for users, as it's roughly similar to Reflection (e.g. Class.getMethods()
vs. Class.getDeclaredMethods()
). Not exactly happy about the total number of methods, but I think it's the best option.
And agree that annotationsMap()
is superfluous. I added it to the proposal because ClassInfo
currently presents its annotations in that form, but we could drop it for sure without loss of generality.
from jandex.
I just want to mention that AnnotationInstance annotation(DotName name)
should likely return a Collection
since it includes the nested instances. I think this is actually an issue with MethodInfo
currently.
from jandex.
MethodInfo
and FieldInfo
(and RecordComponentInfo
) have that problem. ClassInfo.classAnnotation
probably doesn't, as that doesn't return nested annotations.
from jandex.
I need to figure out a better name for the direct*
variants, because I realized that there's a notion of "directly" and "indirectly" present annotations on classes, and that's different from what I'm trying to do here. I originally thought I could use declared*
to mimic reflection, but that's probably bad too.
At the moment, I'm thinking element
would be a good choice, because elementAnnotations
is pretty clear in that it returns annotations only on an element (and not on other elements, nested in it). Problem is that Jandex doesn't use the term element
anywhere yet :-)
(For completeness, I'll add that yet another option would be to make annotations
return only annotations on given annotation target, and what's currently called annotations
would be renamed to something like annotationsWithNested
. But that kind of behavioral breakage is something I'm not willing to undertake.)
from jandex.
I started looking into this for good and there's a couple places in the Jandex API where the behavior is not exactly deterministic, Obviously, the prime example is the existing annotation(DotName)
method on MethodInfo
and FieldInfo
(and RecordComponentInfo
) [1]. Here's a simple example:
@MyAnnotation("f1")
List<@MyAnnotation("f2") String> field;
@MyAnnotation("m1")
void method(@MyAnnotation("m2") List<@MyAnnotation("m3") String> param) {
}
In this case, MyAnnotation
may be present on any target, including type usages, so field
has 3 occurences and method
has 4.
Obtaining a FieldInfo
for field
or MethodInfo
for method
and calling annotation(DotName.createSimple(MyAnnotation.class.getName()))
simply returns the first matching annotation the binary search algorithm finds in the sorted array of annotations.
This would obviously call for removing this method, but I'm afraid it is used a lot. And works just fine when only one annotation of given name is present, which is the most common case. So instead of removing, I'm thinking I'll do this:
- document that the method is non-deterministic when the annotation occurs more than once on the element and anything nested in it;
- add an overload of existing
annotations()
method looking like this:Collection<AnnotationInstance> annotations(DotName)
.
[1] Another example is ClassInfo.method(String, Type...)
method, because the JVM signature of a method includes its return type. It usually works as expected, because the most common situation where multiple methods are present that only differ in the return type is in case of bridge methods, and the method search prefers non-synthetic methods. It would still behave non-deterministically if there were multiple non-synthetic methods only differing in the return type.
from jandex.
I also just noticed that MethodInfo.annotationsWithRepeatable
is not consistent with {Field,RecordComponent}Info.annotationsWithRepeatable
in some subtle ways, and that should probably be rectified.
My original goal here is to break as little as possible, and if there's a breakage, it should be caught during compilation. This behavioral difference between annotationsWithRepeatable
and a potential fix unfortunately can't be source/binary incompatible, so... will think about that.
from jandex.
Regarding your first example agree it should be documented although this was intentional it’s a short-hand “findFirst” to cut down boiler plate when the results are unambiguous.
WRT to the second example, JLS semantics do not allow multiple different return values for the same argument signature (yet JVM semantics do allow it), (exploited by bridge methods). Jandex originally was focused on just plain Java so in some cases precision was sacrificed to cut boilerplate. I
from jandex.
Regarding your first example agree it should be documented although this was intentional it’s a short-hand “findFirst” to cut down boiler plate when the results are unambiguous.
Hm, I personally think that deterministic or not the annotation()
method should never be used for ambiguous results. On the other hand, it's useful if the annotation is not repeatable and the annotation target eliminates the ambiguity. That said, +1 for improving the javadoc ;-).
from jandex.
I actually found yesterday that MethodInfo
already has an annotations(DotName)
method, so I'll just add the same method to other annotation targets.
In case anyone is curious, here are my current additions to AnnotationTarget
:
/**
* Returns whether an annotation instance with given name is declared on this annotation target or any of its
* nested annotation targets.
*
* @param name name of the annotation type to look for, must not be {@code null}
* @return {@code true} if the annotation is present, {@code false} otherwise
* @since 3.0
* @see #annotation(DotName)
* @see #annotations()
*/
boolean hasAnnotation(DotName name);
/**
* Returns the annotation instance with given name declared on this annotation target or any of its nested
* annotation targets. The {@code target()} method of the returned annotation instance may be used to determine
* the exact location of the annotation instance.
* <p>
* In case an annotation with given name occurs more than once, the result of this method is not deterministic.
* For such situations, {@link #annotations(DotName)} is preferable.
*
* @param name name of the annotation type to look for, must not be {@code null}
* @return the annotation instance, or {@code null} if not found
* @since 3.0
* @see #annotations(DotName)
*/
AnnotationInstance annotation(DotName name);
/**
* Returns the annotation instances with given name declared on this annotation target and nested annotation targets.
* The {@code target()} method of the returned annotation instances may be used to determine the exact location
* of the respective annotation instance.
*
* @param name name of the annotation type, must not be {@code null}
* @return collection of annotation instances, never {@code null}
* @see #annotationsWithRepeatable(DotName, IndexView)
* @see #annotations()
* @since 3.0
*/
Collection<AnnotationInstance> annotations(DotName name);
/**
* Returns the annotation instances with given name declared on this annotation target and nested annotation targets.
* The {@code target()} method of the returned annotation instances may be used to determine the exact location
* of the respective annotation instance.
* <p>
* If the specified annotation is repeatable, the result also contains all values from the container annotation
* instance. In this case, the {@link AnnotationInstance#target()} returns the target of the container annotation
* instance.
*
* @param name name of the annotation type, must not be {@code null}
* @param index index used to obtain the annotation type, must not be {@code null}
* @return collection of annotation instances, never {@code null}
* @throws IllegalArgumentException if the index does not contain the annotation type or if {@code name} does not
* identify an annotation type
* @see #annotations()
* @since 3.0
*/
Collection<AnnotationInstance> annotationsWithRepeatable(DotName name, IndexView index);
/**
* Returns the annotation instances declared on this annotation target and nested annotation targets.
* The {@code target()} method of the returned annotation instances may be used to determine the exact location
* of the respective annotation instance.
*
* @return collection of annotation instances, never {@code null}
* @since 3.0
*/
Collection<AnnotationInstance> annotations();
The implementations of AnnotationTarget
, such as ClassInfo
, MethodInfo
or FieldInfo
, will have the same javadoc, modified to be more accurate (e.g. instead of "on this annotation target and nested annotation targets", the javadoc on MethodInfo
will say "on this this method, any of its parameters or any type within its signature") and sometimes include examples.
A second set of methods to only access annotations declared directly on the target and not on nested targets is yet to be added. I'm still not sure what would be the best naming :-)
from jandex.
One thing about the annotationsWithRepeatable(DotName, IndexView)
method that I'd like to hear opinions about. Currently, Jandex has 2 implementations that usually work the same, but are different when it comes to edge cases.
I identified 2 differences:
-
One implementation checks whether the passed
IndexView
isnull
right at the very beginning and throws an IAE if so. The other implementation doesn't check at all and assumes that it is nevernull
(a NPE may later be thrown). -
A somewhat more important difference occurs when the annotation (say
MyAnnotation
) in question is repeatable (say the container type is calledMyAnnotationList
) and the inspected element has both the annotation and the container annotation. That is actually perfectly legal, it looks like this:@MyAnnotation(1) @MyAnnotationList({ @MyAnnotation(2), @MyAnnotation(3) }) class MyClass { ... }
In this case, one implementation of
annotationsWithRepeatable
returns all 3 occurrences ofMyAnnotation
. The other implementation stops when it discovers thatMyAnnotation
is present and doesn't look for the possible container annotation (so it only returns 1).
My current plan is to unify all implementations of annotationsWithRepeatable
on these choices:
- Check
IndexView
fornull
eagerly, throw IAE ifnull
. - Always look for the container annotation, even if the "plain" annotation is present.
from jandex.
Done in #182.
from jandex.
Related Issues (20)
- `Not a class` error for generated constructor in nested class when using ECJ (and lombok) HOT 12
- NegativeArraySizeException while scanning groovy-3.0.13.jar HOT 1
- Jandex fails when indexing a Kotlin-generated class with certain type annotation HOT 2
- jandex-maven-plugin throws IllegalArgumentException: Comparison method violates its general contract HOT 8
- jandex does not build with Maven 3.9.0 HOT 2
- java.lang.ClassCastException HOT 12
- org.jboss.jandex.UnsupportedVersion: Version: 11 HOT 2
- Error when running on Java 21-ea HOT 11
- classloader.getResource(entryName) return null HOT 1
- Add good first issues and codeowners file HOT 1
- Indexing takes over 100x longer on big file HOT 15
- Migrating from Jandex maven plugin 1.1.1 to 3.1.3 fails to create intermediate directories when generating an index HOT 2
- Maven plugin error with 21-graalce HOT 3
- Indexing throws an NPE in some cases HOT 4
- Backport the fix for #310 to 2.4.x: Error with Java 21: `Cannot read the array length because "bytes" is null` HOT 2
- Comparison method violates its general contract! HOT 18
- Skip Plugin Execution on packaging type pom HOT 1
- CODEOWNERS contains errors HOT 1
- Add ClassInfo.method(String, List<Type>) HOT 2
- Published maven-metadata.xml delisted 3.1.6 and newer from version list HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from jandex.