8254023: A module declaration is not allowed to be a target of an annotation that lacks an @Target meta-annotation

[lgxbslgx]

Hi all,

The Java Language Specifications(JLS), from 8 to 13 included, state it as below:

If an annotation of type java.lang.annotation.Target is not present on the
declaration of an annotation type T , then T is applicable in all declaration contexts
except type parameter declarations, and in no type contexts.
These contexts are the syntactic locations where annotations were allowed in Java SE 7.

The JLS 14 and 15 state it as below:

If an annotation of type java.lang.annotation.Target is not present on the
declaration of an annotation type T , then T is applicable in all nine declaration
contexts and in all 16 type contexts.

This patch adds names.MODULE, names.TYPE_PARAMETER, names.TYPE_USE to com.sun.tools.javac.comp.Check.dfltTargetMeta to fix this issue.
Thank you for taking the time to review.

Best Regards.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8254023: A module declaration is not allowed to be a target of an annotation that lacks an @target meta-annotation

Download

$ git fetch https://git.openjdk.java.net/jdk pull/622/head:pull/622
$ git checkout pull/622

[bridgekeeper]

Hi @lgxbslgx, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user lgxbslgx" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[lgxbslgx]

/signed

[bridgekeeper]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[openjdk]

@lgxbslgx The following label will be automatically applied to this pull request:

• compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 03: Full (149855f)
• 02: Full - Incremental (d7777f4)
• 01: Full - Incremental (5ff3e25)
• 00: Full (8bb0c5d)

[jbf]

Hello,

Thank you for the contribution.

As you can see from the @target javadoc ( https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/lang/annotation/Target.html ) there is some ambiguity here. I have rased a separate discussion on compiler-dev, depending on the outcome of that discussion this patch will need to be revised.

[lgxbslgx]

Thanks for focusing on this patch. I will continue to follow the discussion and revise my patch based on the outcome of the discussion.

[bridgekeeper]

@lgxbslgx This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[lgxbslgx]

Short term (Java 16) it makes sense to fix https://bugs.openjdk.java.net/browse/JDK-8254023 given that both competing specs agree for this case, it is a small fix, and externally found.

For Java 17 we should align both specs and the reference implementation one way or another.

@jbf According to your comments, I revise my code to only fix JDK-8254023.
The rest of the work, ambiguity of the specifications, will be clarified and fixed in Java 17.
Thank you for taking the time to review.

[vicente-romero-oracle]

the change looks good but I'm surprised that no other tests are failing and need changes, in particular tests like: test/langtools/tools/javac/annotations/repeatingAnnotations/combo/TargetAnnoCombo.java

[lgxbslgx]

@vicente-romero-oracle It is difficult to identify which test should be changed, because all the tests passed. As you said, the TargetAnnoCombo.java maybe need to be changed. I want to revise it now. Should I do it?

[vicente-romero-oracle]

@vicente-romero-oracle It is difficult to identify which test should be changed, because all the tests passed. As you said, the TargetAnnoCombo.java maybe need to be changed. I want to revise it now. Should I do it?

sure please, I think that you should not only check that that test passes but also modify it, explicitly adding cases for the MODULE target. As a general comment this change is simple but the implications are not. I think the patch needs to add more tests. We also need to test that annotation processors can see annotations applied to modules, etc. We also need tests that check that target-less annotations, applied to modules are actually represented as such in the corresponding class file.

[jbf]

Hi,

I don't think TargetAnnoCombo should be updated, at least not for this fix. It is an old hard to understand test that predates modules.

It would be good to do further testing as Vicente writes. I suggest:

1. A test that the module info compiles, you already have this.
2. A test that the class file contains the bytes for the annotation might make sense. As Jon has pointed out in different review threads there is the toolbox, perhaps that could be of use? There are some examples of toolbox tests in jdk/test/langtools/tools/javac/modules though I don't have a good example of one that verifies the contents of the class file. Perhaps @vicente-romero-oracle can point to an example?
3. There should already be tests that annotation processing can pick up annotations on module declarations, it looks like jdk/test/langtools/tools/javac/modules/AnnotationProcessing.java contains some of this. See if you can add a case with an annotation without @target.

[vicente-romero-oracle]

Hi,

I don't think TargetAnnoCombo should be updated, at least not for this fix. It is an old hard to understand test that predates modules.

It would be good to do further testing as Vicente writes. I suggest:

1. A test that the module info compiles, you already have this.

2. A test that the class file contains the bytes for the annotation might make sense. As Jon has pointed out in different review threads there is the toolbox, perhaps that could be of use? There are some examples of toolbox tests in  `jdk/test/langtools/tools/javac/modules` though I don't have a good example of one that verifies the contents of the class file. Perhaps @vicente-romero-oracle can point to an example?

it is probably an overkill but method: testAnnos in test: test/langtools/tools/javac/records/RecordCompilationTests.java is an example applied to records that could be adapted to what we want here

[openjdk]

@lgxbslgx this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8254023
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[lgxbslgx]

I added two test cases. Thank you for taking the time to review.

[jbf]

Hi @lgxbslgx

This looks good to me. Since this is targeted to Java 16 we need to close this PR and open a new PR towards the JDK16 project and finish the review there, add a link back to this PR would also be neat. Can you do that?

cheers
/Joel

[openjdk]

@jbf Unknown command joel - for a list of valid commands use /help.

[lgxbslgx]

@jbf I open a new PR in JDK 16. Please see openjdk/jdk16#34

[lgxbslgx]

The later work will be completed in JDK 16.