chore: Upgrading and fixing nullaway on JDK 25 #520
Conversation
* upgrading nullaway and error-prone on default profile * fixing nullaway detection not working Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
Summary of ChangesHello @ehsavoie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request performs essential maintenance by upgrading the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request successfully upgrades the Error Prone and NullAway dependencies and refactors the compiler arguments. However, there is a critical issue with the logic for applying the -XDaddTypeAnnotationsToSymbol=true compiler flag. It is currently enabled by default and disabled for older JDKs, which is the reverse of the required behavior. This will cause the build to fail on JDK 22 and newer, including the target JDK 25. My review comment provides details on how to correct this.
| <arg>${error-prone.flag}</arg> | ||
| <arg>-Xplugin:ErrorProne -XepDisableAllChecks -Xep:NullAway:ERROR -XepOpt:NullAway:OnlyNullMarked=true -XepOpt:NullAway:JSpecifyMode=true -XepExcludedPaths:.*/src/test/.* -XepDisableWarningsInGeneratedCode</arg> |
There was a problem hiding this comment.
While separating compiler arguments into distinct <arg> tags is a good practice, this change highlights a critical misconfiguration of the -XDaddTypeAnnotationsToSymbol=true flag, which is passed via the ${error-prone.flag} property.
According to the NullAway documentation, this flag is:
- Required for JDK versions before 22.
- Removed from
javacin JDK 22 and later versions.
The current pom.xml logic is inverted:
- The flag is enabled by default (defined on line 71), which will cause builds to fail on JDK 22+ (including the target JDK 25 for this PR).
- The flag is disabled for older JDKs via the
old_jdk_supportprofile (on line 542).
To resolve this, the logic must be reversed: the flag should be empty by default and set only within the old_jdk_support profile. This will require changes on lines 71 and 542.
Additionally, the property definition on line 71 currently includes a trailing space. When correcting the logic, this space should be removed from the property value to prevent passing an invalid argument to the compiler.
There was a problem hiding this comment.
You are wrong gemini: the flag works properly and the version is 21.0.8 except for Oracle JDK. second the railing space is trimmed down by maven
* upgrading nullaway and error-prone on default profile * fixing nullaway detection not working Fixes a2aproject#519 🦕 Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
Fixes #519 🦕