8225377: type annotations are not visible to javac plugins across compilation boundaries #16407
Conversation
…pilation boundaries
|
👋 Welcome back cushon! A progress list of the required criteria for merging this PR into |
Webrevs
|
| @@ -2261,6 +2261,7 @@ public void run() { | |||
| currentClassFile = classFile; | |||
| List<Attribute.TypeCompound> newList = deproxyTypeCompoundList(proxies); | |||
| sym.setTypeAttributes(newList.prependList(sym.getRawTypeAttributes())); | |||
| TypeAnnotationMapper.addTypeAnnotationsToSymbol(sym, newList); | |||
There was a problem hiding this comment.
I wonder if the new TypeAnnotationsMapper should actually live inside of ClassReader
There was a problem hiding this comment.
I have tentatively gone ahead and moved it into ClassReader
|
Thanks for the review!
Moving it into ClassReader is fine with me. I initially thought it might end up requiring more logic than it did. I left it in a separate class because it's still a medium amount of logic and is fairly self-contained, but if you're preference is to move it into ClassReader I'm happy to proceed with that. Just for my understanding, if it was a separate class would the preferred style be to register it in |
if it was a separate instance I would prefer ClassReader to create an instance of it even it it is not registered in Context, but registering it in Context would be the "standard" approach I think |
Sounds good, thanks. I'm happy to implement whichever of these options you prefer. Do you prefer having the logic in ClassReader, or a separate class that ClassReader creates an instance of? |
I'm OK with the last version, thanks |
|
@cushon This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 74 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Receiver types are not otherwise initialized for classes, see also https://bugs.openjdk.org/browse/JDK-8319196
Thanks for the review! I did some more validation of the change on Google's codebase, and everything looks good on that side. I made one more small update for receiver parameters and added an additional test case. Receiver parameters are not currently filled in for classes loaded from the classpath. (That is potentially an issue even without type annotations being present, I filed JDK-8319196.) I updated the logic here to fill in the receiver parameter type if diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java
index f86a95e501c..d15d5ca2526 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java
@@ -2333,8 +2333,9 @@ public Void visitMethodSymbol(Symbol.MethodSymbol s, Void unused) {
}
mt.thrown = thrown.toList();
mt.restype = addTypeAnnotations(mt.restype, TargetType.METHOD_RETURN);
- if (mt.recvtype != null) {
- mt.recvtype = addTypeAnnotations(mt.recvtype, TargetType.METHOD_RECEIVER);
+ if (attributes.stream().anyMatch(a -> a.position.type == TargetType.METHOD_RECEIVER)) {
+ Assert.checkNull(mt.recvtype);
+ mt.recvtype = addTypeAnnotations(s.owner.type, TargetType.METHOD_RECEIVER);
}
return null;
}
diff --git a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
index af63575052c..18b41b4a80c 100644
--- a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
+++ b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
@@ -573,4 +573,10 @@ class Inner90<@TA(90) T> {
@Test(posn=4, annoType = TB.class, expect = "100")
class Inner100<T extends Inner100<@TB(100) T>> {
}
+
+ // receiver parameters
+ class Inner110 {
+ @Test(posn=2, annoType = TA.class, expect = "110")
+ void f(@TA(110) Inner110 this) {}
+ }
} |
This reverts commit 7071226.
On second thought, I would like to defer that, I have switched back to the last version that was reviewed. The receiver parameter issue seems like a pre-existing problem that is doesn't just affect type annotations, I have proposed a separate fix for JDK-8319196. |
yep better to defer the receiver thing |
|
/integrate |
|
Going to push as commit de6667c.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Hello,
Please consider this fix for JDK-8225377: type annotations are not visible to javac plugins across compilation boundaries.
To provide some background context and motivation for the bug fix, consider an example like:
If
@Nullableis a@Target(METHOD)annotation, an annotation processor can retrieve the annotation by from theExecutableElementforfby callinggetAnnotationMirrors(). This is true regardless of whetherBis being compiled from source in the current compilation, or loaded from a class file.If
@Nullableis a@Target(TYPE_USE)annotation, an annotation processor should be able to retrieve the annotation by locating theExecutableElementforf, and callinggetReturnType().getAnnotationMirrors(). This works today ifBis compiled from source, but (due to JDK-8225377) not ifBis loaded from a class file.This is a hurdle to migrating from existing declaration annotations to
@Target(TYPE_USE)annotations for use-cases like@Nullable. For example: a dependency injection framework might use an annotation processor to generate code, and that code would want to know if a formal parameter type acceptednullvalues, or if a method returnednullvalues. That works today with declaration annotation definitions of@Nullable, and this fix will allow it work withTYPE_USEdefinitions of@Nullablein the future.javac already reads type annotation information from
Runtime{Visible,Invisible}TypeAnnotationsattributes in class files, andClassReaderregisters a completer that callsSymbol#setTypeAttributesto add the type annotations to the symbol's metadata. This change adds logic to associate those annotations with the corresponding type contained in the symbol.Some notes on the approach:
There are some similarities between the logic being added and existing logic in
TypeAnnotations, butTypeAnnotationsoperates on the AST, and is solving different problems. In the AST annotations are already adjacent to the type they are annotating, and in bytecode they have been separated out and the implementation has to interpret the annotation target and type path information to match the annotations back up with their types.For initial test coverage, I have enabled test coverage for annotation processing of type annotations to also run the annotation processor on the pre-compiled class. There may be other type annotation tests that could be updated to exercise this change. Overall I think updating the existing extensive test coverage for type annotations avoids duplicating test inputs, and helps ensure both the type annotations for symbols compiled from source and loaded from bytecode match, and that they continue to match when there are changes in the future.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16407/head:pull/16407$ git checkout pull/16407Update a local copy of the PR:
$ git checkout pull/16407$ git pull https://git.openjdk.org/jdk.git pull/16407/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16407View PR using the GUI difftool:
$ git pr show -t 16407Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16407.diff
Webrev
Link to Webrev Comment