Enable NullAway static null-safety analysis for util module#10046
Enable NullAway static null-safety analysis for util module#10046Apisapple wants to merge 54 commits intobesu-eth:mainfrom
Conversation
- Replace javax.annotation.Nullable and javax.annotation.CheckForNull with org.jspecify.annotations.Nullable across 18 Java files - Update platform constraint to org.jspecify:jspecify:1.0.0 - Replace compileOnly com.google.code.findbugs:jsr305 with compileOnly org.jspecify:jspecify in affected modules Signed-off-by: Mykim <38449976+Apisapple@users.noreply.github.com>
Migrate JSR305 nullness annotations to JSpecify
Signed-off-by: mykim <kimminyong2034@gmail.com>
- Add NullAway 0.12.4 to util errorprone dependencies - Annotate nullable fields/returns in 6 util classes with @nullable - Fix nullable dereferences: RollingFileWriter, StackTraceMatchFilter, PlatformDetector - Enable NullAway:ERROR for util main compile, OFF for tests - Add optional CI job (run-nullaway label) for gradual monitoring - All util compilation passes with NullAway ERROR by default Fixes: - MemoryBoundCache: mark getIfPresent() return as @nullable - ExceptionUtils: annotate rootCause() for nullable input/output - RollingFileWriter: guard Path.getParent() null dereference - PlatformDetector: make static fields @nullable, add fallback to UNKNOWN - BesuVersionUtils: mark VERSION/COMMIT fields as @nullable - StackTraceMatchFilter: fix nullable message comparison, builder fields Signed-off-by: mykim <kimminyong2034@gmail.com>
…besu into feature/nullaway-util
…ilter Signed-off-by: mykim <kimminyong2034@gmail.com>
… null Replace null return with UNKNOWN to satisfy NullAway @nonnull contract. Signed-off-by: mykim <kimminyong2034@gmail.com>
Feature/nullaway util
- Return null to account for existing code that expects and handles null values. Signed-off-by: mykim <kimminyong2034@gmail.com>
- Remove the unnecessary -PenableNullAway flag Signed-off-by: mykim <kimminyong2034@gmail.com>
- Update the Javadoc to reflect the actual behavior of the getGlibc function. Signed-off-by: mykim <kimminyong2034@gmail.com>
Signed-off-by: mykim <kimminyong2034@gmail.com>
Signed-off-by: mykim <kimminyong2034@gmail.com>
Signed-off-by: mykim <kimminyong2034@gmail.com>
Feature/fix nullaway util
Adjust indentation of GRADLEW_UNIT_TEST_ARGS in .github/workflows/pre-review.yml under unitTests.env to align with surrounding keys. Signed-off-by: mykim <kimminyong2034@gmail.com>
|
I’ll revisit this PR after fully addressing the review feedback. |
Add org.jspecify:jspecify as a compileOnly dependency to util/build.gradle. This brings JSpecify annotations into the module for static nullness/type-checking without introducing a runtime dependency. Signed-off-by: mykim <kimminyong2034@gmail.com>
Update acceptance tests to call BesuVersionUtils.shortVersion() directly instead of using orElse("unknown"). In StackTraceMatchFilter, mark the Throwable parameter as @nullable and simplify toString() to return stackContains directly. These changes clarify nullability and streamline version usage/representation.
Signed-off-by: mykim <kimminyong2034@gmail.com>
Delete verification-metadata entries for com.uber.nullaway:nullaway:0.12.4 and org.checkerframework:dataflow-nullaway:3.48.0 (their artifact SHA entries were removed). These versions are superseded in the file by nullaway:0.13.1 and dataflow-nullaway:3.53.0, so the stale metadata was cleaned up. Signed-off-by: mykim <kimminyong2034@gmail.com>
Feature/nullaway util Signed-off-by: mykim <kimminyong2034@gmail.com>
| public void versionStringIsEthstatsFriendly() { | ||
| assertThat(BesuVersionUtils.version()) | ||
| .matches("[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|null)/[^/]+/[^/]+"); | ||
| .matches("[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|UNKNOWN)/[^/]+/[^/]+"); |
There was a problem hiding this comment.
These tests hardcode the sentinel string UNKNOWN. Since BesuVersionUtils.UNKNOWN is now a public constant, consider referencing it (e.g., via string concatenation in the regex) to prevent tests drifting if the sentinel value changes.
| public void noIdentityNodeNameIsEthstatsFriendly() { | ||
| assertThat(BesuVersionUtils.nodeName(Optional.empty())) | ||
| .matches("[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|null)/[^/]+/[^/]+"); | ||
| .matches("[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|UNKNOWN)/[^/]+/[^/]+"); |
There was a problem hiding this comment.
These tests hardcode the sentinel string UNKNOWN. Since BesuVersionUtils.UNKNOWN is now a public constant, consider referencing it (e.g., via string concatenation in the regex) to prevent tests drifting if the sentinel value changes.
| public void userIdentityNodeNameIsEthstatsFriendly() { | ||
| assertThat(BesuVersionUtils.nodeName(Optional.of("TestUserIdentity"))) | ||
| .matches("[^/]+/[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|null)/[^/]+/[^/]+"); | ||
| .matches("[^/]+/[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|UNKNOWN)/[^/]+/[^/]+"); |
There was a problem hiding this comment.
These tests hardcode the sentinel string UNKNOWN. Since BesuVersionUtils.UNKNOWN is now a public constant, consider referencing it (e.g., via string concatenation in the regex) to prevent tests drifting if the sentinel value changes.
| * @param throwable the throwable whose root cause we want to find | ||
| * @return The root cause | ||
| */ | ||
| public static Throwable rootCause(final Throwable throwable) { | ||
| public static @Nullable Throwable rootCause(final @Nullable Throwable throwable) { | ||
| return throwable != null ? Throwables.getRootCause(throwable) : null; | ||
| } |
There was a problem hiding this comment.
The Javadoc no longer matches the method contract: the parameter and return value are now nullable. Update the @param and @return docs to reflect that null is accepted and that the method may return null when passed null.
| final Pattern pattern = Pattern.compile("[-+]?[\\d]*\\.?[\\d]+"); | ||
| final Matcher matcher = pattern.matcher(rawGlibcVersion); |
There was a problem hiding this comment.
This method compiles a regex Pattern on each call. Since the pattern is constant, make it a private static final Pattern to avoid repeated compilation overhead and simplify the method.
| public static String getOS() { | ||
| if (_os == null) { | ||
| detect(); | ||
| } | ||
| return _os; | ||
| return _os == null ? UNKNOWN : _os; | ||
| } |
There was a problem hiding this comment.
The lazy initialization of mutable static fields is not thread-safe (no synchronization/volatile). With the new UNKNOWN fallback, a thread can observe _os as null during/after another thread’s detect() and return UNKNOWN even though detection will eventually set a real value. Consider making the cached fields volatile and/or synchronizing detect() + reads (or using an initialization-on-demand holder) to ensure safe publication and consistent results under concurrency.
Replace hardcoded "UNKNOWN" literal in regex assertions with BesuVersionUtils.UNKNOWN constant in three unit tests (versionStringIsEthstatsFriendly, noIdentityNodeNameIsEthstatsFriendly, userIdentityNodeNameIsEthstatsFriendly) in BesuVersionUtilsTest. This keeps the tests consistent with the source constant and avoids duplicating the literal value; no behavioral change. Signed-off-by: mykim <kimminyong2034@gmail.com>
Remove the local BESU_VERSION_UNKNOWN constant and use BesuVersionUtils.UNKNOWN instead. Simplify getRuntimeVersionString() to return BesuVersionUtils.shortVersion() directly, construct VersionMetadata with BesuVersionUtils.UNKNOWN on FileNotFoundException, and compare metadata versions against BesuVersionUtils.UNKNOWN. Centralizes the unknown-version sentinel in BesuVersionUtils. Signed-off-by: mykim <kimminyong2034@gmail.com>
Reformat the long regex in userIdentityNodeNameIsEthstatsFriendly test to improve readability by splitting the string across lines. This is a purely formatting change in util/src/test/java/org/hyperledger/besu/util/BesuVersionUtilsTest.java and does not alter test behavior. Signed-off-by: mykim <kimminyong2034@gmail.com>
Feature/nullaway util
util/build.gradle
Outdated
| implementation 'org.bouncycastle:bcpkix-jdk18on' | ||
| implementation 'org.xerial.snappy:snappy-java' | ||
|
|
||
| compileOnly 'org.jspecify:jspecify' |
There was a problem hiding this comment.
org.jspecify.annotations.Nullable is now part of util’s public API surface (annotations appear in public method signatures/fields). Using compileOnly means downstream modules/consumers may not have the annotation type available on their compile classpath, which can break static analysis tooling and (depending on build tooling) compilation when reading annotated signatures. Prefer compileOnlyApi 'org.jspecify:jspecify' (with the java-library plugin) so consumers get the annotation on their compile classpath without adding a runtime dependency.
| compileOnly 'org.jspecify:jspecify' | |
| compileOnlyApi 'org.jspecify:jspecify' |
| public StackTraceMatchFilter.Builder setStackContains(final @Nullable String text) { | ||
| this.stackContains = text; |
There was a problem hiding this comment.
The builder setter setStackContains(@Nullable String) allows explicitly passing null, but build() then unconditionally fails via requireNonNull. This makes the API easier to misuse and shifts validation to a later point. Consider making the setter parameter non-null (and annotating it accordingly), or validate immediately in setStackContains (e.g., requireNonNull(text, ...)) so failures are earlier and closer to the source of the misuse.
| public StackTraceMatchFilter.Builder setStackContains(final @Nullable String text) { | |
| this.stackContains = text; | |
| public StackTraceMatchFilter.Builder setStackContains(final String text) { | |
| this.stackContains = Objects.requireNonNull(text, "stackContains must be provided"); |
| final String nonNullStackContains = | ||
| Objects.requireNonNull(stackContains, "stackContains must be provided"); |
There was a problem hiding this comment.
The builder setter setStackContains(@Nullable String) allows explicitly passing null, but build() then unconditionally fails via requireNonNull. This makes the API easier to misuse and shifts validation to a later point. Consider making the setter parameter non-null (and annotating it accordingly), or validate immediately in setStackContains (e.g., requireNonNull(text, ...)) so failures are earlier and closer to the source of the misuse.
| } catch (final IOException e) { | ||
| final FileNotFoundException fnfe = | ||
| new FileNotFoundException("Unable to create directory for rolling file: " + parentPath); | ||
| fnfe.initCause(e); | ||
| throw fnfe; |
There was a problem hiding this comment.
Wrapping a directory-creation IOException as a FileNotFoundException is a bit misleading (the directory may be unreadable, permission denied, invalid path, etc., not “file not found”). If the constructor/method signature allows, prefer throwing IOException directly (or UncheckedIOException if you can’t). If you must keep FileNotFoundException, consider using a message that clearly indicates this is a directory creation failure and ensure callers can still distinguish it (e.g., via a dedicated exception type or documented cause inspection).
| public void versionStringIsEthstatsFriendly() { | ||
| assertThat(BesuVersionUtils.version()) | ||
| .matches("[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|null)/[^/]+/[^/]+"); | ||
| .matches("[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|" + BesuVersionUtils.UNKNOWN + ")/[^/]+/[^/]+"); |
There was a problem hiding this comment.
These tests interpolate BesuVersionUtils.UNKNOWN directly into a regex. Today it’s safe ("UNKNOWN"), but if the sentinel ever changes to contain regex metacharacters, these patterns will become brittle. Wrapping the interpolated token with regex quoting (e.g., Pattern.quote(...)) would make the tests resilient to future sentinel changes.
| public void noIdentityNodeNameIsEthstatsFriendly() { | ||
| assertThat(BesuVersionUtils.nodeName(Optional.empty())) | ||
| .matches("[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|null)/[^/]+/[^/]+"); | ||
| .matches("[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|" + BesuVersionUtils.UNKNOWN + ")/[^/]+/[^/]+"); |
There was a problem hiding this comment.
These tests interpolate BesuVersionUtils.UNKNOWN directly into a regex. Today it’s safe ("UNKNOWN"), but if the sentinel ever changes to contain regex metacharacters, these patterns will become brittle. Wrapping the interpolated token with regex quoting (e.g., Pattern.quote(...)) would make the tests resilient to future sentinel changes.
| .matches( | ||
| "[^/]+/[^/]+/v(\\d+\\.\\d+\\.\\d+[^/]*|" + BesuVersionUtils.UNKNOWN + ")/[^/]+/[^/]+"); |
There was a problem hiding this comment.
These tests interpolate BesuVersionUtils.UNKNOWN directly into a regex. Today it’s safe ("UNKNOWN"), but if the sentinel ever changes to contain regex metacharacters, these patterns will become brittle. Wrapping the interpolated token with regex quoting (e.g., Pattern.quote(...)) would make the tests resilient to future sentinel changes.
Replace compileOnly with compileOnlyApi for org.jspecify:jspecify in util/build.gradle so jspecify annotations are exposed on the compile classpath to consumers of this module. This ensures downstream modules compiling against this artifact can see the jspecify types without packaging the dependency. Signed-off-by: mykim <kimminyong2034@gmail.com>
Use Pattern.quote(BesuVersionUtils.UNKNOWN) in regex assertions to ensure the UNKNOWN token is matched literally and not treated as a regex. Added import java.util.regex.Pattern and updated three assertions in BesuVersionUtilsTest (versionStringIsEthstatsFriendly, noIdentityNodeNameIsEthstatsFriendly, userIdentityNodeNameIsEthstatsFriendly) to avoid accidental regex interpretation and potential test flakiness. Signed-off-by: mykim <kimminyong2034@gmail.com>
Apply Spotless formatting Signed-off-by: mykim <kimminyong2034@gmail.com>
Feature/nullaway util
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
util/src/main/java/org/hyperledger/besu/util/BesuVersionUtils.java:87
- The Javadoc for
shortVersion()claims it returns strings like\"v23.1.0\", but (based on the ethstats format/tests using/vas a prefix)shortVersion()is likely intended to return the version without a leadingv. Please update the examples/wording to match the actual format (e.g.,\"23.1.0\"/\"23.1.1-dev-ac23d311\"), while keeping theUNKNOWNpart.
/**
* Generate version-only Besu version
*
* @return Besu version in format such as "v23.1.0" or "v23.1.1-dev-ac23d311", or {@value
* #UNKNOWN} if not available
*/
public static String shortVersion() {
return VERSION;
}
|
|
||
| private static String normalizeGLibcVersion(final String rawGlibcVersion) { | ||
| final Pattern pattern = Pattern.compile("[-+]?[0-9]*\\.?[0-9]+"); | ||
| final Pattern pattern = Pattern.compile("[-+]?[\\d]*\\.?[\\d]+"); |
There was a problem hiding this comment.
The regex change from [0-9] to [\\d] is both redundant (a character class around \\d isn’t needed) and subtly changes matching semantics (Unicode digits vs ASCII digits). If the intent is to parse glibc versions, consider reverting to [0-9] (original behavior) or using \\d without square brackets if Unicode digits are explicitly desired.
| final Pattern pattern = Pattern.compile("[-+]?[\\d]*\\.?[\\d]+"); | |
| final Pattern pattern = Pattern.compile("[-+]?[0-9]*\\.?[0-9]+"); |
|
@siladu I've incorporated all the feedback you provided. |
PR description
This PR enables NullAway static null-safety analysis for the util module as a pilot and fixes all violations surfaced by the check.
Summary of changes
Developer setup
Validation
Fixed Issue(s)
fixes #10004
Thanks for sending a pull request! Have you done the following?
Locally, you can run these tests to catch failures early: