-
Notifications
You must be signed in to change notification settings - Fork 296
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
Refactor PreservedAnnotationTreeVisitor #955
Refactor PreservedAnnotationTreeVisitor #955
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #955 +/- ##
============================================
- Coverage 86.11% 86.06% -0.06%
+ Complexity 2030 2027 -3
============================================
Files 81 81
Lines 6662 6679 +17
Branches 1291 1287 -4
============================================
+ Hits 5737 5748 +11
- Misses 512 518 +6
Partials 413 413 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Particularly since all existing tests pass after the refactor.
@@ -156,6 +160,32 @@ private static MethodHandle createHandle() { | |||
} | |||
} | |||
|
|||
private static MethodHandle createGetMetadataHandle() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remind me, all of these methods with reflection trickery are to avoid having a compile-time dep on JDK21, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of (internal) javac APIs changed between JDK 17 and 21 related to these TypeMetadata
objects. On JDK 21+ we use reflection to operate on them since I expect that is used less for compiling than JDK 17 and before. Unfortunately I think we can only drop this reflection approach once we drop support for building on JDK 17. This is hacky, but the alternative that doesn't rely on internal APIs would require building up our own type data structures from scratch, like Checker Framework, which would be a lot more code.
We extract some code from
visitParameterizedType
into a propervisitAnnotatedType
method. This will be useful for an upcoming PR that handlesAnnotatedTypeTree
s in new array expressions. The change introduces some complexity due to API changes in JDK 21, which we work around through further use ofMethodHandle
s.