Fix arg concatenation bug in JMH #1357
Conversation
WalkthroughUpdates jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java to centralize Error Prone options in a new Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-14T18:50:06.159ZApplied to files:
📚 Learning: 2025-08-28T04:54:20.953ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1357 +/- ##
=========================================
Coverage 88.39% 88.39%
Complexity 2594 2594
=========================================
Files 98 98
Lines 8705 8705
Branches 1732 1732
=========================================
Hits 7695 7695
Misses 505 505
Partials 505 505 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java (1)
182-195: Excellent! Now I have confirmed the issue and identified a flaw in the proposed fix. Let me generate the corrected review comment:
JavaCompiler options must be passed as individual array elements, not bundled with spaces
The current code bundles multiple javac flags into a single string
allErrorProneArgsand adds it as one element to theoptionsarray. According to the JavaCompiler API, each flag must be a separate array element; space-separated flags in a single string will not be parsed correctly by javac.The proposed fix in the diff is incomplete—it still bundles
extraErrorProneArgswith spaces intonullawayAnnotatedPackagesArg, creating the same parsing problem. The correct approach is to add each flag individually:- String allErrorProneArgs = - String.format( - "-Xplugin:ErrorProne -XepDisableAllChecks -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages=%s %s", - annotatedPackages, String.join(" ", extraErrorProneArgs)); options.addAll( Arrays.asList( "-processorpath", processorPath, "-d", outputDir.toAbsolutePath().toString(), "-XDcompilePolicy=simple", "--should-stop=ifError=FLOW", - allErrorProneArgs)); + "-Xplugin:ErrorProne", + "-XepDisableAllChecks", + "-Xep:NullAway:ERROR", + "-XepOpt:NullAway:AnnotatedPackages=" + annotatedPackages)); + options.addAll(extraErrorProneArgs);This keeps each option tokenized separately while fixing the missing space issue between
annotatedPackagesandextraErrorProneArgs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on macos-latest
| // for JSpecify mode | ||
| options.add("-XDaddTypeAnnotationsToSymbol=true"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify/verify JDK expectations for -XDaddTypeAnnotationsToSymbol=true
Always adding -XDaddTypeAnnotationsToSymbol=true aligns with the goal of supporting JSpecify mode and, per prior learnings, failing fast on unsupported JDKs rather than silently skipping behavior. Based on learnings, this is desirable for catching regressions in CI, but it does assume all JMH runs for this harness use a javac that understands this -XD flag.
Consider either:
- Documenting that the JMH benchmarks require a JDK where this flag is supported, or
- If this helper is also used in non-JSpecify benchmarks, making the comment more explicit (e.g., that the option is benign outside JSpecify but required for JSpecify runs).
Functionality is likely correct; this is mainly about explicitness and version expectations.
🤖 Prompt for AI Agents
In jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java around lines
210-211, clarify the JDK expectations for adding
"-XDaddTypeAnnotationsToSymbol=true": update the inline comment to explicitly
state that this option is required for JSpecify-mode benchmarks and must be
supported by the javac used for JMH runs (or that running on an older javac will
fail fast), or alternatively gate adding the option behind a boolean
parameter/flag used only for JSpecify runs and add Javadoc/README guidance that
JMH benchmarks requiring JSpecify need a JDK whose javac accepts this -XD flag.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java (1)
210-211: Acknowledge the existing review comment about JDK expectations.The addition of
-XDaddTypeAnnotationsToSymbol=truefor JSpecify mode has been previously reviewed. As noted in the past review comment, consider documenting the JDK version requirements for this flag or making the inline comment more explicit about when this option is required and supported.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: benchmarking
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (1)
jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java (1)
194-194: LGTM: Consolidated Error Prone arguments passed correctly.Using the single
allErrorProneArgsstring ensures all Error Prone plugin arguments are passed together as required by the-Xplugin:ErrorPronemechanism.
| String allErrorProneArgs = | ||
| String.format( | ||
| "-Xplugin:ErrorProne -XepDisableAllChecks -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages=%s %s", | ||
| annotatedPackages, String.join(" ", extraErrorProneArgs)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The concatenation fix correctly addresses the missing space bug.
The use of String.format with space-separated %s placeholders ensures Error Prone arguments are properly spaced, resolving the issue described in the PR.
One minor edge case: when extraErrorProneArgs is empty, the format produces a trailing space (e.g., "...AnnotatedPackages=com.uber "). While benign for command-line parsing, you could avoid it with:
- String allErrorProneArgs =
- String.format(
- "-Xplugin:ErrorProne -XepDisableAllChecks -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages=%s %s",
- annotatedPackages, String.join(" ", extraErrorProneArgs));
+ String allErrorProneArgs =
+ String.format(
+ "-Xplugin:ErrorProne -XepDisableAllChecks -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages=%s%s",
+ annotatedPackages,
+ extraErrorProneArgs.isEmpty() ? "" : " " + String.join(" ", extraErrorProneArgs));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String allErrorProneArgs = | |
| String.format( | |
| "-Xplugin:ErrorProne -XepDisableAllChecks -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages=%s %s", | |
| annotatedPackages, String.join(" ", extraErrorProneArgs)); | |
| String allErrorProneArgs = | |
| String.format( | |
| "-Xplugin:ErrorProne -XepDisableAllChecks -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages=%s%s", | |
| annotatedPackages, | |
| extraErrorProneArgs.isEmpty() ? "" : " " + String.join(" ", extraErrorProneArgs)); |
🤖 Prompt for AI Agents
In jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java around lines 182
to 185, the formatted string can produce a trailing space when
extraErrorProneArgs is empty; change the construction so you only append a
leading space plus joined extraErrorProneArgs when extraErrorProneArgs is
non-empty (or trim the final string) so the resulting Error Prone argument
string never has an unnecessary trailing space.
|
Main Branch: With This PR: |
Before we were missing a space character so the Error Prone args were not getting passed properly. Do some related minor cleanup.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.