Start cleanup of AnnotationTarget #507

graemerocher · 2021-07-27T15:26:06Z

Initial progress on cleanup of annotation API. I think we should re-consider the annotation member value coercion APIs as they add a little bit overhead as you have to wrap ever value in AnnotationMemberValue (renamed from AnnotationAttributeValue in this PR)

In Micronaut these are resolved through fine grained methods like intValue https://github.com/micronaut-projects/micronaut-core/blob/09a46c9a54a8704ae7cc20572ed08520e8c006ed/core/src/main/java/io/micronaut/core/annotation/AnnotationValueResolver.java#L27

There is also a AnnotationValueBuilder which is simpler and more fluent than the Annotations interface that is proposed. See https://github.com/micronaut-projects/micronaut-core/blob/09a46c9a54a8704ae7cc20572ed08520e8c006ed/core/src/main/java/io/micronaut/core/annotation/AnnotationValueBuilder.java#L32

Either way it is technically possible in Micronaut it just seems this API could be a better IMO

graemerocher · 2021-07-27T15:33:38Z

I also have 2 additional questions in https://github.com/eclipse-ee4j/cdi/pull/507/files#diff-4f4cd81c5bc64d64445b4cbd22353033233726967c802e968f7cf4a6f6301f4cR26 and https://github.com/eclipse-ee4j/cdi/pull/507/files#diff-5c2d70d5496d5fd13910ffa79f5983ae652fb671b22bf25c69fd0f2ebb72b86aR25-R26

Ladicek · 2021-07-28T10:22:15Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationInfo.java

    /**
     * Target of this annotation.
     * That is, the declaration, the type parameter or the type use on which this annotation is present.
     * TODO what if this annotation is a nested annotation?
     * TODO what if this annotation doesn't have a known target (e.g. qualifier of a synthetic bean)?
+     * TODO Do we need this? Retrieving the target from an annotation value is not supported in Micronaut


I think we can probably remove this.

In Quarkus, the equivalent is used quite often, because we often start by simply looking up all annotations of some type, and then filter based on their target etc. But at the moment, I can't think of a way that would enable something like this in CDI Lite. I think we only allow top-down access, never bottom-up.

Ok I will remove it in the PR

Ladicek · 2021-07-28T10:27:03Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationInfo.java


    /**
     * Fully qualified name of this annotation.
     * Equivalent to {@code declaration().name()}.
     *
-     * @return fully qualified name of this annotation
+     * @return fully qualified name of this annotation, never {@code null}


Off topic, since this was introduced earlier and you didn't change it, but I want to bring this up anyway.

I recently learned that the JLS makes an important distinction between fully qualified name and binary name. They are identical for top-level types, but not for nested types.

For example:

package com.example; public class MyClass { public @interface MyAnnotation { } }

Fully qualified name of the MyAnnotation type is com.example.MyClass.MyAnnotation, while binary name is com.example.MyClass$MyAnnotation.

I think we should probably settle on binary names, and I started doing that here: https://github.com/eclipse-ee4j/cdi/pull/506/files#diff-a8c5a19048a48d991137c3ecb9f78f593b33a264b81a41bd5fa543cf9ac7ae73

WDYT about that?

+1 for binary names

Ladicek · 2021-07-28T10:29:30Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationInfo.java

-     * @return value of this annotation's attribute with given {@code name}
+     * @param name attribute name, never {@code null}
+     * @return value of this annotation's attribute with given {@code name} or {@code null} if it doesn't exist.
+     * @throws java.lang.IllegalArgumentException if the argument is {@code null}


WDYT about the distintion between NullPointerException and IllegalArgumentException for null arguments? I personally like IllegalArgumentException more, but the convention seems to be (and Effective Java advises the same) to use NPE.

On the other hand, if the rest of CDI uses IAE, then I'd say we follow suit.

Perhaps NullPointerException is fine, consistency is the key. I have tended to use IAE but then again methods like Objects.requireNonNull throw NPE

Ladicek · 2021-07-28T10:32:13Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationInfo.java


+    /**
+     * Returns whether this annotation has a value defined using the {@link #MEMBER_VALUE} member.


Suggested change

* Returns whether this annotation has a value defined using the {@link #MEMBER_VALUE} member.

* Returns whether this annotation has the {@link #MEMBER_VALUE} member.

Reads better to me?

Ladicek · 2021-07-28T10:32:54Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationMember.java

@@ -1,8 +1,8 @@
 package jakarta.enterprise.lang.model;

 // TODO "attribute" is a colloquial expression, perhaps use something closer to the JLS? AnnotationMember?


+1 to AnnotationMember, we can remove this TODO comment.

Ladicek · 2021-07-28T10:33:13Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationMemberValue.java

@@ -5,7 +5,7 @@
 import java.util.List;

 // TODO "attribute" is a colloquial expression, perhaps use something closer to the JLS? AnnotationMember?


+1 to AnnotationMemberValue, we can remove this TODO comment.

Ladicek · 2021-07-28T10:35:21Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationTarget.java

 */
 public interface AnnotationTarget {
    // TODO specify equals/hashCode (for the entire .lang.model hierarchy)
    // TODO settle on using Optional everywhere, or allowing null everywhere; it's a mix right now
+    // TODO what about declared vs not declared annotations?


If we remove AnnotationInfo.target, as you propose, will there still be a difference?

It relates to inheritance. For example if an annotation has @Inherited and is defined on the superclass it should appear in getAnnotation(..) methods but not getDeclaredAnnotation(..) methods. There appears to be no distinction in this API?

Ah gotcha. I originally intended to only return [directly] declared annotations and let users traverse the hierarchy if they also need inherited annotations, but we can certainly revisit that.

The Annotated.getAnnotation[s] methods from Portable Extensions always return all annotations, including inherited, and don't have a way to only return directly declared annotations. We could move in that direction for sure. (Implementation in Quarkus will become a little more complex, but it's perfectly possible.)

Ok yes I see, seems that the core CDI API makes no distinction so we probably don't need to in this API

Ladicek · 2021-07-28T10:38:16Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationTarget.java

 */
 public interface AnnotationTarget {
    // TODO specify equals/hashCode (for the entire .lang.model hierarchy)
    // TODO settle on using Optional everywhere, or allowing null everywhere; it's a mix right now
+    // TODO what about declared vs not declared annotations?
+    // TODO I am wondering why we are deviating from javax.lang.model names here like AnnotatedConstruct, Element etc.


To be honest, I find the javax.lang.model hierarchy quite counter-intuitive (e.g. using VariableElement for both fields and parameters). The Portable Extensions types are much nicer, and I'd be in favor of moving towards similar names -- except that makes for rather poor coding experience.

Ladicek · 2021-07-28T11:44:25Z

I added AnnotationMemberValue to allow abstracting over all value types. Sometimes, you don't need to know what type the annotation member is, you just need to transfer it somewhere. If you think that's too much (performance/cognitive) overhead, we can certainly rethink that -- but I'd prefer not to use Object :-)

When it comes to a builder API for constructing annotations, I totally agree the Annotations type is a provisional solution and we need something better. There are several options we could use. For example, I was thinking about something like this:

ClassConfig clazz = ...;
clazz.addAnnotation(MyAnnotation.class, (builder) -> {
    builder.value(someValue); // shortcut for .member("value", someValue); arrayValue or annotationValue for non-scalar members
    builder.member("name", myStringValue);
    builder.member("something", someOtherValue);
    builder.arrayMember("someArray", (valueBuilder) -> {
        valueBuilder.of(1);
        valueBuilder.of(2);
        valueBuilder.of(3);
    });
    builder.annotationMember("nestedAnnotation", SomeOtherAnnotation.class, (builder2) -> {
        builder2.member("x", x);
        builder2.member("y", y);
    });
    builder.arrayMember("nestedAnnotationArray", (valueBuilder) -> {
        valueBuilder.ofAnnotation(YetAnotherAnnotation.class, (builder2) -> { ... });
        valueBuilder.ofAnnotation(YetAnotherAnnotation.class, (builder2) -> { ... });
    });
});

This allows keeping everything in the spec as interfaces (adding implementation of anything directly to the spec API should be a last resort kind of thing IMHO). At the same time, I also feel it's quite unusual.

Another option would be a more traditional builder. Something like:

ClassConfig clazz = ...;
clazz.addAnnotation(AnnotationBuilder.of(MyAnnotation.class)
    .value(someValue), // shortcut for .member("value", someValue); arrayValue or annotationValue for non-scalar members
    .member("name", myStringValue), // for scalar values
    .arrayMember("someArray").add(1).add(2).add(3).build(),
    .annotationMember("nestedAnnotation", AnnotationBuilder.of(SomeOtherAnnotation.class)
        .member("x", x)
        .member("y", y)
        .build()),
    .arrayMember("nestedAnnotationArray")
        .add(AnnotationBuilder.of(YetAnotherAnnotation.class).add(...).add(...).build())
        .add(AnnotationBuilder.of(YetAnotherAnnotation.class).add(...).add(...).build())
        .build()
    .build()
);

This is a bit more readable and more common, but requires the CDI spec to ship some code. We could probably make AnnotationBuilder a service provider and use ServiceLoader to get actual implementation.

That would actually probably be best...

graemerocher · 2021-07-28T13:34:27Z

The former approach is almost identical to what we have in Micronaut:

https://github.com/micronaut-projects/micronaut-core/blob/790a0ecf877f2623ae85e40fe24c5127d987a153/test-suite/src/main/java/example/micronaut/inject/visitor/AnnotatingVisitor.java#L35

The main different is we don't have a specific method for arrays and just use varargs and for nested annotations we have a specific type, so it could look like:

ClassConfig clazz = ...;
clazz.addAnnotation(MyAnnotation.class, (builder) -> {
    builder.value(someValue);
    builder.member("name", myStringValue);
    builder.member("something", someOtherValue);
    builder.member("someArray", 1,2,3);
    builder.member("nestedAnnotation", AnnotationValue.builder(NestedAnnotation.cass).value("someValue"));
});

Note as well Micronaut's API allows you to specify the retention policy if you only want to use the annotation for build time metadata.

I am not that bothered about AnnotationMember as it is not unreasonable so will leave it for now.

Ladicek · 2021-07-28T14:02:39Z

After realizing that if we remove AnnotationInfo.target, we can easily make AnnotationBuilder.build() return an AnnotationInfo, I pretty much started liking the 2nd approach :-) Here's a sketch of the API: https://github.com/Ladicek/cdi-spec/commits/annotation-builder

graemerocher · 2021-07-28T14:06:11Z

Looks like a good improvement some feedback:

member instead of attribute as agreed here
Do we need the array* methods? Do varargs not suffice?

Ladicek · 2021-07-28T14:12:10Z

Sure, I used member in documentation, didn't change the type names yet. As for arrays... yea good point, a varargs variant should probably suffice. I wanted to add some addAll methods to the ArrayBuilder, but forgot about it. And now thinking about it... yea, we should just settle on varargs (which are also arrays) and be done with it. I'll amend the branch in a bit.

Ladicek · 2021-07-28T14:35:16Z

Done on the same branch. I also removed the Annotations type and added default implementations of the value methods, delegating to member.

I'm quite satisfied with how the AnnotationBuilder looks. Maybe let's get this PR in and then I'll rebase that branch and submit another PR?

graemerocher · 2021-07-28T14:48:05Z

@Ladicek sounds good let me just push a few more changes I had been working on and then mark it for review

graemerocher · 2021-07-28T15:30:33Z

Ok this is ready for review

Ladicek · 2021-07-29T07:30:23Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationInfo.java

+ *
+ * @param <T>  The annotation type.
+ */
+public interface AnnotationInfo<T extends Annotation> {


I'm not sure if this needs to be parameterized, but let's discuss that later.

(As for ClassInfo, I'd pretty much love to remove the type parameter -- I originally added it to be able to express a query, but we don't allow "injecting" ClassInfo anymore and we want ClassConfig to not inherit from ClassInfo, so that reason no longer applies.)

Ladicek · 2021-07-29T07:33:35Z

api/src/main/java/jakarta/enterprise/lang/model/AnnotationMemberValue.java

+     *
+     * @return The {@link jakarta.enterprise.lang.model.types.Type} of the value.
+     */
+    Type asType(); // can be a VoidType, PrimitiveType or ClassType


I originally forgot it can also be an array, but I can amend that in my follow-up PR.

graemerocher added 2 commits July 27, 2021 17:24

Start cleanup of AnnotationTarget

d5642c5

rename

9d8b916

graemerocher requested a review from Ladicek July 27, 2021 15:34

Ladicek reviewed Jul 28, 2021

View reviewed changes

more cleanup

0e921f7

graemerocher marked this pull request as ready for review July 28, 2021 15:29

graemerocher requested a review from Ladicek July 28, 2021 15:29

Ladicek reviewed Jul 29, 2021

View reviewed changes

Ladicek approved these changes Jul 29, 2021

View reviewed changes

Ladicek merged commit 63749f2 into master Jul 29, 2021

Ladicek deleted the annotation-api-cleanup branch July 29, 2021 07:35

Ladicek added Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal labels Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start cleanup of AnnotationTarget #507

Start cleanup of AnnotationTarget #507

graemerocher commented Jul 27, 2021 •

edited

Loading

graemerocher commented Jul 27, 2021 •

edited

Loading

Ladicek Jul 28, 2021

graemerocher Jul 28, 2021

Ladicek Jul 28, 2021 •

edited

Loading

graemerocher Jul 28, 2021

Ladicek Jul 28, 2021

graemerocher Jul 28, 2021

Ladicek Jul 28, 2021

Ladicek Jul 28, 2021

Ladicek Jul 28, 2021

Ladicek Jul 28, 2021

graemerocher Jul 28, 2021

Ladicek Jul 28, 2021 •

edited

Loading

graemerocher Jul 29, 2021

Ladicek Jul 28, 2021

Ladicek commented Jul 28, 2021

graemerocher commented Jul 28, 2021

Ladicek commented Jul 28, 2021

graemerocher commented Jul 28, 2021

Ladicek commented Jul 28, 2021

Ladicek commented Jul 28, 2021 •

edited

Loading

graemerocher commented Jul 28, 2021

graemerocher commented Jul 28, 2021

Ladicek Jul 29, 2021

Ladicek Jul 29, 2021


		/**
		* Returns whether this annotation has a value defined using the {@link #MEMBER_VALUE} member.

	* Returns whether this annotation has a value defined using the {@link #MEMBER_VALUE} member.
	* Returns whether this annotation has the {@link #MEMBER_VALUE} member.

		@@ -1,8 +1,8 @@
		package jakarta.enterprise.lang.model;

		// TODO "attribute" is a colloquial expression, perhaps use something closer to the JLS? AnnotationMember?

		@@ -5,7 +5,7 @@
		import java.util.List;

		// TODO "attribute" is a colloquial expression, perhaps use something closer to the JLS? AnnotationMember?

Start cleanup of AnnotationTarget #507

Start cleanup of AnnotationTarget #507

Conversation

graemerocher commented Jul 27, 2021 • edited Loading

graemerocher commented Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Jul 28, 2021

graemerocher commented Jul 28, 2021

Ladicek commented Jul 28, 2021

graemerocher commented Jul 28, 2021

Ladicek commented Jul 28, 2021

Ladicek commented Jul 28, 2021 • edited Loading

graemerocher commented Jul 28, 2021

graemerocher commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graemerocher commented Jul 27, 2021 •

edited

Loading

graemerocher commented Jul 27, 2021 •

edited

Loading

Ladicek Jul 28, 2021 •

edited

Loading

Ladicek Jul 28, 2021 •

edited

Loading

Ladicek commented Jul 28, 2021 •

edited

Loading