Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove Annotations and add AnnotationBuilder #508

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jul 29, 2021

The Annotations API was a provisional solution for creating
annotation instances. This commit replaces it with a simpler,
more traditional builder API: AnnotationBuilder. Considering
that AnnotationInfo.target() was dropped, the builder can
simply produce AnnotationInfo instances, which allows
simplification of other APIs that expect users to pass
annotation instances (annotation transformation and synthetic
component registration).

Additionally, this commit tries to establish a convention
for documenting null handling:

  • if the user is forbidden to pass null, we say "must not
    be null" and we don't have to document that a NPE will be
    thrown;
  • if the API guarantees that null will never be returned,
    we simply say "never null".

@Ladicek Ladicek requested a review from graemerocher July 29, 2021 12:01
@Ladicek Ladicek force-pushed the annotation-builder branch 2 times, most recently from 0f1c89f to e17bb2f Compare July 29, 2021 12:23
api/src/main/java/jakarta/enterprise/inject/spi/CDI.java Outdated Show resolved Hide resolved
*
* @return A {@link jakarta.enterprise.lang.model.declarations.ClassInfo} representing the type of an enum value.
* @return a {@link ClassInfo} representing the enum type
* @throws IllegalStateException if this annotation member value is not an enum value
*/
ClassInfo<?> asEnumClass();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if this is necessary since in Micronaut we don't actually store the enum type and only the constant value so it would be complicate matters, given the user can already lookup types via the Types impl ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed you dropped this method and I thought it's probably a mistake, so I added it back. What I didn't notice is that you added a parameter to <E extends Enum<E>> E asEnum(Class<E> enumType) :-) IMHO, that should just be <E extends Enum<E>> E asEnum(). The enum type is available on the declaration of the annotation member, isn't it? As in, you should always be able to find it even if you don't remember it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like as of today we just store enum values as strings. See https://github.com/micronaut-projects/micronaut-core/blob/a949a27c69ff00fe50fa751adf0501540aee79b2/inject-java/src/main/java/io/micronaut/annotation/processing/JavaAnnotationMetadataBuilder.java#L637-L641

I guess we would have to rework that code to store the actual enum values, although I am not sure how possible that is, we would I guess need to store the underlying VariableElement from the annotation processing API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright that sounds weird. I don't know how you [intend to] implement this, but if your implementation of AnnotationMemberValue retains a reference to whatever you use to implement AnnotationInfo, you should be able to lookup the declaration of the annotation member and from there, lookup the enum type. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah probably there is a way, will try work it out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I thought asEnum might be problematic, I originally had a TODO for that method: // TODO should this be present?

Does adding the Class parameter help with the classpath issues? If so, I'm fine with leaving it there (and indeed it also help with type inference).

OK, I'll submit another PR to change AnnotationInfo.members() to return Map<String, SomeAnnotationMemberValueType_ProbablyAnnotationMember> and remove AnnotationMember as it currently stands. I've have to figure out one other thing now, but I should get to it later today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes having the Class parameter is better as the API user knows they have to have the class before they can the enum. That is why I added it in the other PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, the API user is the extension author. That is typically not the same person as the extension user.

I'd rather remove asEnum completely than having it sometimes failing with classloading errors, to be honest :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, my point was if the extension author has to supply a Class parameter, they know that they have to configure their classpath to make sure the Class they are requesting is on the same classpath as the extension.

FooEnum e = info.asEnum(FooEnum.class);

In the above case FooEnum will not be referencable if it is not on the classpath of the extension and the extension author would get a compilation error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if you're fine with it, then I am :-) Let's keep asEnum as it is now, then. I'll remove the TODO comment.

@Ladicek Ladicek force-pushed the annotation-builder branch 3 times, most recently from 47f5c76 to 6acd8f3 Compare July 29, 2021 14:18
The `Annotations` API was a provisional solution for creating
annotation instances. This commit replaces it with a simpler,
more traditional builder API: `AnnotationBuilder`. Considering
that `AnnotationInfo.target()` was dropped, the builder can
simply produce `AnnotationInfo` instances, which allows
simplification of other APIs that expect users to pass
annotation instances (annotation transformation and synthetic
component registration).

Additionally, this commit tries to establish a convention
for documenting `null` handling:

- if the user is forbidden to pass `null`, we say "must not
  be null" and we don't have to document that a NPE will be
  thrown;
- if the API guarantees that `null` will never be returned,
  we simply say "never null".
@Ladicek Ladicek force-pushed the annotation-builder branch from 6acd8f3 to 5b4891d Compare July 30, 2021 07:25
@Ladicek Ladicek merged commit ed2963a into jakartaee:master Jul 30, 2021
@Ladicek Ladicek deleted the annotation-builder branch July 30, 2021 07:29
@Ladicek Ladicek added Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal labels Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants