8371960: Missing null check in AnnotatedType annotation accessor methods

[liach]

AnnotatedElement methods specify they throw NPE if they take any null argument: isAnnotationPresent, get(Declared)Annotation, get(Declared)AnnnotationsByType. However, AnnotatedType has implementation quality issues that it does not throw NPE for isAnnotationPresent and get(Declared)Annotation.

This patch fixes this oversight by adding explicit null checks. For get(Declared)AnnotationsByType, even though it did throw exceptions, it was in the middle of parsing, so throwing it in the beginning is better.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Change requires CSR request JDK-8371961 to be approved
• Commit message must refer to an issue

Issues

• JDK-8371960: Missing null check in AnnotatedType annotation accessor methods (Bug - P4)
• JDK-8371961: Missing null check in AnnotatedType annotation accessor methods (CSR)

Reviewers

• Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28341/head:pull/28341
$ git checkout pull/28341

Update a local copy of the PR:
$ git checkout pull/28341
$ git pull https://git.openjdk.org/jdk.git pull/28341/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28341

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28341.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back liach! 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]

@liach 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:

8371960: Missing null check in AnnotatedType annotation accessor methods

Reviewed-by: alanb

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 23 new commits pushed to the master branch:

• 695a4ab: 8371966: RISC-V: Incorrect pointer dereference in TemplateInterpreterGenerator::generate_native_entry
• 46b5e58: 8371697: test/jdk/java/nio/file/FileStore/Basic.java fails after 8360887 on linux
• 6968216: 8293484: AArch64: TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on CPU with SHA512 feature support
• ... and 20 more: https://git.openjdk.org/jdk/compare/7d35a283cf2497565d230e3d5426f563f7e5870d...master

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 master branch, type /integrate in a new comment.

[openjdk]

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

• core-libs

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

• 00: Full (9616bbef)

[AlanBateman]

I'm trying to judge the behavior/compatibility impact. Are there cases where NPE will be thrown for cases that is doesn't throw today? I get it that the parameter checking doesn't check it and instead the NPE comes from deep in the implementation, just want to understand if there is any impact of the change or not.

[liach]

There are impact to the 3 methods:

• isAnnotationPresent: previously returned false, now throws
• getAnnotation: previously returned null, now throws
• getDeclaredAnnotation: previously returned null, now throws

For these two methods:

• getAnnnotationsByType
• getDeclaredAnnotationsByType

They currently already throw NullPointerException from within AnnotationSupport.getIndirectlyPresent. The only action that happens before is a Map.get call, so I don't think the lifted null check would hide any existing linkage error or other exceptions.

I think the main impact of this change is that AnnotatedType is the only way to retrieve type annotations, so the accessors might see more frequent usage compared to those on other AnnotatedElements.

[AlanBateman]

I think the main impact of this change is that AnnotatedType is the only way to retrieve type annotations, so the accessors might see more frequent usage compared to those on other AnnotatedElements.

Yes, it seems this will really only be noticed if getAnnotatedReturnType/getAnnotatedParameterTypes/etc. methods are used and something has a bug that results in invoking AnnotatedType methods with null. Seems low risk.

[AlanBateman]

Good sanity test on the method source.

[liach]

Thanks for the reviews!

/integrate

[openjdk]

Going to push as commit b6d83ed.
Since your change was applied there have been 37 commits pushed to the master branch:

• 43040f3: 8372012: java/nio/file/attribute/BasicFileAttributeView/SetTimesNanos.java should check ability to create links
• dcba014: 8371967: Add Visual Studio 2026 to build toolchain for Windows
• 2e68b79: 8364991: Incorrect not-exhaustive error
• ... and 34 more: https://git.openjdk.org/jdk/compare/7d35a283cf2497565d230e3d5426f563f7e5870d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@liach Pushed as commit b6d83ed.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.