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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cushon
Copy link
Contributor

@cushon cushon commented Oct 8, 2024

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

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

…tions of different retentions

Reviewed-by: vromero
@cushon cushon marked this pull request as ready for review October 8, 2024 17:58
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 8, 2024

👋 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
Copy link

openjdk bot commented Oct 8, 2024

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

@openjdk openjdk bot changed the title Backport e36ce5f0341e8d0ec06cb12d0b2c0aa084401021 8336942: Improve test coverage for class loading elements with annotations of different retentions Oct 8, 2024
@openjdk
Copy link

openjdk bot commented Oct 8, 2024

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

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Oct 8, 2024
@mlbridge
Copy link

mlbridge bot commented Oct 8, 2024

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 5, 2024

@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
Copy link
Contributor Author

cushon commented Nov 5, 2024

Please keep this open for now

@openjdk
Copy link

openjdk bot commented Nov 7, 2024

⚠️ @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
Copy link
Contributor

fitzsim commented Nov 7, 2024

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
Copy link
Contributor Author

cushon commented Nov 9, 2024

@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
Copy link
Contributor

fitzsim commented Nov 9, 2024

@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
Copy link
Contributor Author

cushon commented Nov 9, 2024

@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
Copy link
Contributor Author

cushon commented Nov 9, 2024

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
Copy link
Contributor

fitzsim commented Nov 9, 2024 via email

@fitzsim
Copy link
Contributor

fitzsim commented Nov 11, 2024

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
Copy link
Contributor

fitzsim commented Nov 11, 2024

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 added a commit to cushon/jdk17u-dev that referenced this pull request Nov 11, 2024
cushon added a commit to cushon/jdk17u-dev that referenced this pull request Nov 11, 2024
@cushon
Copy link
Contributor Author

cushon commented Nov 11, 2024

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
Copy link
Contributor

fitzsim commented Nov 12, 2024

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.

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 11, 2024

@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
Copy link
Contributor Author

cushon commented Dec 11, 2024

Let's keep this open for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

3 participants