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

Language model only returns runtime-retained annotations in build compatible extensions #554

Merged
merged 1 commit into from
Nov 17, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Nov 1, 2021

The language model API itself doesn't specify what annotations are
accessible. In the CDI context, only runtime-retained annotations
make sense, because that's the only way all implementation strategies
can be consistent. That's because:

  • purely runtime implementations, such as Weld, only have access
    to runtime-retained annotations;
  • bytecode-based implementations, such as ArC, only have access
    to runtime-retained and class-retained annotations, but don't
    have access to source-retained annotations;
  • annotation processing-based implementations, such as ODI, have
    access to all annotations.

An alternative would be to specify that the language model always
returns only runtime-retained annotations. That would be just fine
strictly from the CDI perspective, but unforeseen cases may exist
where the language model API would be useful, but nothing would
prevent providing access to class-retained or even source-retained
annotations. Hence, I believe it is better to make the restriction
on the CDI level, and not on the language model level.

…patible extensions

The language model API itself doesn't specify what annotations are
accessible. In the CDI context, only runtime-retained annotations
make sense, because that's the only way all implementation strategies
can be consistent. That's because:

- purely runtime implementations, such as Weld, only have access
  to runtime-retained annotations;
- bytecode-based implementations, such as ArC, only have access
  to runtime-retained and class-retained annotations, but don't
  have access to source-retained annotations;
- annotation processing-based implementations, such as ODI, have
  access to all annotations.

An alternative would be to specify that the language model _always_
returns only runtime-retained annotations. That would be just fine
strictly from the CDI perspective, but unforeseen cases may exist
where the language model API would be useful, but nothing would
prevent providing access to class-retained or even source-retained
annotations. Hence, I believe it is better to make the restriction
on the CDI level, and not on the language model level.
@Ladicek Ladicek requested a review from graemerocher November 1, 2021 12:07
@mkouba
Copy link
Contributor

mkouba commented Nov 1, 2021

FTR the support for RetentionPolicy.CLASS was added into Jandex only recently and that's the reason why ArC does not consider this retention policy at all.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 1, 2021

I think CDI is just built around runtime-retained annotations and I don't see a reason why (or even a way how) to change that.

It's true that Jandex ignores class-retained annotations right now (in version 2.x), and hence ArC does too, so my statement above is factually incorrect (until Jandex 3.0). But it's correct in principle :-)

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Makes sense, CDI doesn't really care about other than runtime-retained annotations anyway.

@graemerocher
Copy link
Contributor

Since we process the source those annotations are visible and we have methods in our metadata to differentiate whether an annotation is source or runtime. It would add overhead to filter out these annotations, I am not sure if there is any value if doing so. However I have no issue with the wording of only supporting runtime retention

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 2, 2021

My reasoning is that different CDI implementations should all present the same view of the world -- in this case, the same set of annotations.

This happened to me recently: I created an annotation, ran a test with it in the development version of Jandex and everything was fine. So I backported the test to Jandex 2.4, and it started failing for no obvious reason. Took me a little while to realize that the annotation didn't have a @Retention, so it was class-retained -- upcoming Jandex will return those just fine, while Jandex 2.4 ignores them.

I'd like to avoid similar situation between different CDI implementations.

@starksm64 starksm64 merged commit df41586 into jakartaee:master Nov 17, 2021
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 18, 2021

I'm not sure we reached consensus on this, so just want to say that I'm open to changes. While I'd obviously prefer to specify it the way this PR did, I could probably be persuaded to softening the requirement to something like:

In build compatible extensions, attempting to obtain annotations from {@link jakarta.enterprise.lang.model.AnnotationTarget}
with a retention policy different than the {@linkplain java.lang.annotation.RetentionPolicy#RUNTIME runtime} retention policy
leads to non-portable behavior.

@Ladicek Ladicek deleted the runtime-annotations branch November 18, 2021 11:36
@Ladicek Ladicek added Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal labels Nov 23, 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.

5 participants