8336942: Improve test coverage for class loading elements with annotations of different retentions

[cushon]

This change improves annotation processing test coverage, see JDK-8336942. I resolved a trivial merge conflict with JDK-8323684, which added the nameToAnnotation map. The modified test passes.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• JDK-8336942 needs maintainer approval

Issue

• JDK-8336942: Improve test coverage for class loading elements with annotations of different retentions (Enhancement - P4)

Reviewers

• Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/2955/head:pull/2955
$ git checkout pull/2955

Update a local copy of the PR:
$ git checkout pull/2955
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/2955/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2955

View PR using the GUI difftool:
$ git pr show -t 2955

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/2955.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back cushon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 01: Full - Incremental (8d083450)
• 00: Full (6592b5ab)

[bridgekeeper]

@cushon 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!

[cushon]

Please keep this open for now

[openjdk]

⚠️ @cushon This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[fitzsim]

I am not a Reviewer; I maintain java-17-openjdk in RHEL so I try to review rfr-labelled pull requests for jdk17u-dev, time-permitting.

This looks like a good change to have in 17. I tested it on Fedora 41 x86_64.

While trying this out, I noticed a potential oddity regarding BasicAnnoTests.java: the first of the two Test annotations does not seem to take effect. For example, I would expect that changing:

@Test(posn=0, annoType=TA.class, expect="1")

to:

@Test(posn=0, annoType=TA.class, expect="33")

would cause a failure, but it does not. Am I missing something?

I am testing with jtreg 857ed6167418cb4ebe2844fd536461a1649bdced running on jdk-17.0.13+11.

[cushon]

@fitzsim I tried with jtreg 857ed6167418cb4ebe2844fd536461a1649bdced and 17.0.13+11 and I can reproduce a test failure with that change. Could you double-check you're still unable to reproduce that failure?

diff --git a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
index d4540107022..e22c3d132b9 100644
--- a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
+++ b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
@@ -423,7 +423,7 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
     //             vary position of one annotated
     //       the three above with the corner case of the ambiguos decl + type anno added

-    @Test(posn=0, annoType=TA.class, expect="1")
+    @Test(posn=0, annoType=TA.class, expect="33")
     public @TA(1) int f1;

     @Test(posn=0, annoType=TA.class, expect="11")

make test TEST="jtreg:test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java"
...
./src/jdk17u-dev/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java:427: error: Unexpected value: 1, expected: 33
    public @TA(1) int f1;
                      ^
1 error

[fitzsim]

@cushon I should have included a patch for context; I was modifying the first of the two Test annotations added by this pull request. Can you try this patch, on top of this pull request?

diff --git a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
index d4540107022..06bd32bcfbb 100644
--- a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
+++ b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
@@ -535,7 +535,7 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
     @Test(posn=1, annoType=TA.class, expect="23")
     public Set<@TA(23) ? super Object> f9;
 
-    @Test(posn=0, annoType=TA.class, expect="1")
+    @Test(posn=0, annoType=TA.class, expect="33")
     @Test(posn=0, annoType=TD.class, expect="2")
     public @TA(1) @TD(2) int f10;
 

I get:

make test TEST="jtreg:test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java"
...
Passed: tools/javac/processing/model/type/BasicAnnoTests.java
Test results: passed: 1
...
TEST SUCCESS

but I would expect a failure.

[cushon]

@fitzsim thanks for clarifying, and for the catch! I think that may be a long-standing bug in the test harness, it's grouping the annotations by position and if there are multiple annotations with the same position it only ends up looking at one of them.

I can reproduce the expected failure with your change and something like the following.

I think this issue with the test should be fixed at head, I will follow up on that.

The motivation for originally adding the test in JDK-8336942 that's being backported here was that this example would cause a crash if the implementation failed to handle two annotations with different retentions at the same location, so there's some value in this even without the fix to the test harness.

diff --git a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java b/test/langtools/tools/javac/processing/m
odel/type/BasicAnnoTests.java
index d4540107022..718b4aae95a 100644
--- a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
+++ b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
@@ -139,7 +139,7 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
      */
     class TestTypeScanner extends TypeScanner<Void, Void> {
         Element elem;
-        NavigableMap<Integer, AnnotationMirror> toBeFound;
+        NavigableMap<Integer, List<AnnotationMirror>> toBeFound;
         int count = 0;
         Set<TypeMirror> seen = new HashSet<>();

@@ -147,10 +147,10 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
             super(types);
             this.elem = elem;

-            NavigableMap<Integer, AnnotationMirror> testByPos = new TreeMap<>();
+            NavigableMap<Integer, List<AnnotationMirror>> testByPos = new TreeMap<>();
             for (AnnotationMirror test : tests) {
                 for (int pos : getPosn(test)) {
-                    testByPos.put(pos, test);
+                    testByPos.computeIfAbsent(pos, ArrayList::new).add(test);
                 }
             }
             this.toBeFound = testByPos;
@@ -172,17 +172,18 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
                         out.println("scan " + count + ": " + t);
                     if (toBeFound.size() > 0) {
                         if (toBeFound.firstKey().equals(count)) {
-                            AnnotationMirror test = toBeFound.pollFirstEntry().getValue();
-                            String annoType = getAnnoType(test);
-                            AnnotationMirror anno = getAnnotation(t, annoType);
-                            if (anno == null) {
-                                error(elem, "annotation not found on " + count + ": " + t);
-                            } else {
-                                String v = getValue(anno, "value").toString();
-                                if (v.equals(getExpect(test))) {
-                                    out.println("found " + anno + " as expected");
+                            for (AnnotationMirror test : toBeFound.pollFirstEntry().getValue()) {
+                                String annoType = getAnnoType(test);
+                                AnnotationMirror anno = getAnnotation(t, annoType);
+                                if (anno == null) {
+                                    error(elem, "annotation not found on " + count + ": " + t);
                                 } else {
-                                    error(elem, "Unexpected value: " + v + ", expected: " + getExpect(test));
+                                    String v = getValue(anno, "value").toString();
+                                    if (v.equals(getExpect(test))) {
+                                        out.println("found " + anno + " as expected");
+                                    } else {
+                                        error(elem, "Unexpected value: " + v + ", expected: " + getExpect(test));
+                                    }
                                 }
                             }
                         } else if (count > toBeFound.firstKey()) {

[cushon]

I think this issue with the test should be fixed at head, I will follow up on that.

I filed https://bugs.openjdk.org/browse/JDK-8343882 for this

[fitzsim]

Liam Miller-Cushon ***@***.***> writes:

I think this issue with the test should be fixed at head, I will follow up on that. I filed https://bugs.openjdk.org/browse/JDK-8343882 for this

Looks good, thank you. On Monday I will experiment on `jdk17u-dev` with your preliminary fix for the test harness.

[fitzsim]

I can reproduce the expected failure with your change and something like the following.

Confirmed on my setup:

/source/jdk17u-dev/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java:541: error: Unexpected value: 1, expected: 33
    public @TA(1) @TD(2) int f10;
                             ^
1 error

[fitzsim]

The motivation for originally adding the test in JDK-8336942 that's being backported here was that this example would cause a crash if the implementation failed to handle two annotations with different retentions at the same location, so there's some value in this even without the fix to the test harness.

Independent of the harness fix, I would ideally like to be able replicate the crash you mention. I have not yet found a way to make the f10 lines you added produce a failure. Can you advise how I would do that?

So far, I tried the new test on, and it succeeded on, 959665633eae0923d8acce533834940d1797bffd, the commit before your backport of the fix for JDK-8225377, 3a2bf8e570d5e329f22b4eaf5e90fef8cd14f8b2. The f10 lines also passed on 3a2bf8e570d5e329f22b4eaf5e90fef8cd14f8b2. Maybe there is some other fix I need to revert?

[cushon]

Independent of the harness fix, I would ideally like to be able replicate the crash you mention. I have not yet found a way to make the f10 lines you added produce a failure. Can you advise how I would do that?

So far, I tried the new test on, and it succeeded on, 959665633eae0923d8acce533834940d1797bffd, the commit before your backport of the fix for JDK-8225377, 3a2bf8e570d5e329f22b4eaf5e90fef8cd14f8b2. The f10 lines also passed on 3a2bf8e570d5e329f22b4eaf5e90fef8cd14f8b2. Maybe there is some other fix I need to revert?

Sorry for not providing more of this context up front. The crash was an interaction with the backport for JDK-8225377, which was previously backported to JDK 17 and backed out. The motivation for backporting this test change was to have coverage in-place if the backport ends up getting re-done (https://bugs.openjdk.org/browse/JDK-8341779). (It isn't certain if that backport will be re-done, if it is there will be additional discussion and a CSR first.)

I created a draft PR with the changes to reproduce the crash here: #3038

make test TEST="jtreg:test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java"
...
java.lang.AssertionError
        at jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:155)
        at jdk.compiler/com.sun.tools.javac.util.Assert.check(Assert.java:46)
        at jdk.compiler/com.sun.tools.javac.code.TypeMetadata$Annotations.combine(TypeMetadata.java:194)
        at jdk.compiler/com.sun.tools.javac.code.TypeMetadata$Annotations.combine(TypeMetadata.java:174)
        at jdk.compiler/com.sun.tools.javac.code.TypeMetadata.combine(TypeMetadata.java:90)
        at jdk.compiler/com.sun.tools.javac.code.Type.annotatedType(Type.java:403)
        at jdk.compiler/com.sun.tools.javac.jvm.ClassReader$TypeAnnotationTypeMapping.reannotate(ClassReader.java:2476)

The changes in the draft PR are

• Re-do the backport of JDK-8225377 (https://bugs.openjdk.org/browse/JDK-8341779)
• Update the backport for some changes in javac's TypeMetadata API
• This PR
• Removing the logic that was updated for the TypeMetadata, which reproduces the crash

[fitzsim]

Thanks, that does clarify motivation. I do not see harm in merging this pull request in the meantime, even if the backport does not ultimately happen. From what I can tell (though I am not an expert in this area, and I did not step through the code) the new test lines may increase coverage of the existing ClassReader implementation, since they introduce an as yet unrepresented annotated type declaration pattern. And even if they are superfluous, they are harmless because they do not cause a test failure on the current 17 code.