-
Notifications
You must be signed in to change notification settings - Fork 1k
Enable NullAway static null-safety analysis for util module #10046
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
base: main
Are you sure you want to change the base?
Changes from all commits
6e0dcc8
aafb253
1c5c38f
2cccbb1
d446b70
3ee264f
968f49e
a45214e
8c884b3
7ea098b
3faa60c
a89a961
556f767
e139c95
29ab5e1
e9010a4
663be1f
fca589a
03b6cd9
04a98fd
0ff7f02
ce10ea2
07538d3
77be9e0
3259c91
97efaa6
9e69fc3
4fb6640
f0043a3
52fbc15
216158f
24b63ae
c54fe44
55fe3a2
7fe9235
ba09e84
7e951b8
822ae1b
b7a9122
ba1502e
27c61f0
bf86c2b
fdccaad
98d7b89
327cd32
0a49504
ee34253
b8bf18b
12e7836
6b4d169
6d73567
feda84d
7c95c22
6e0adbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,16 @@ compileJava { | |
| '-Alog4j.graalvm.groupId=' + project.group, | ||
| '-Alog4j.graalvm.artifactId=' + project.name | ||
| ] | ||
| options.errorprone { | ||
| check('NullAway', net.ltgt.gradle.errorprone.CheckSeverity.ERROR) | ||
| option('NullAway:AnnotatedPackages', 'org.hyperledger.besu.util') | ||
|
siladu marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
||
| tasks.named('compileTestJava', JavaCompile).configure { | ||
| options.errorprone { | ||
| check('NullAway', net.ltgt.gradle.errorprone.CheckSeverity.OFF) | ||
| } | ||
| } | ||
|
|
||
| dependencies { | ||
|
|
@@ -43,6 +53,8 @@ dependencies { | |
| annotationProcessor 'org.apache.logging.log4j:log4j-core' | ||
| annotationProcessor 'com.google.dagger:dagger-compiler' | ||
|
|
||
| errorprone 'com.uber.nullaway:nullaway:0.13.1' | ||
|
|
||
| implementation 'com.google.guava:guava' | ||
| implementation 'com.google.dagger:dagger' | ||
| implementation 'com.github.ben-manes.caffeine:caffeine' | ||
|
|
@@ -55,6 +67,8 @@ dependencies { | |
| implementation 'org.bouncycastle:bcpkix-jdk18on' | ||
| implementation 'org.xerial.snappy:snappy-java' | ||
|
|
||
| compileOnlyApi 'org.jspecify:jspecify' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: think I still prefer
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the review. I decided to keep compileOnlyApi for now, as it is currently used only in the util module. Adding compileOnly manually to every module could lead to omissions, so I felt this was the safer choice at this stage. Once all modules adopt it consistently, we can switch to a shared compileOnly configuration.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ve verified that updating the section you pointed out to |
||
|
|
||
| testImplementation 'org.mockito:mockito-junit-jupiter' | ||
| testImplementation 'org.assertj:assertj-core' | ||
| testImplementation 'org.junit.jupiter:junit-jupiter' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,10 @@ | |
| */ | ||
| public final class BesuVersionUtils { | ||
| private static final Logger LOG = LoggerFactory.getLogger(BesuVersionUtils.class); | ||
|
|
||
| /** Sentinel value used when the version or commit metadata is not available. */ | ||
| public static final String UNKNOWN = "UNKNOWN"; | ||
|
|
||
| private static final String CLIENT = "besu"; | ||
| private static final String VERSION; | ||
| private static final String OS = PlatformDetector.getOS(); | ||
|
|
@@ -66,16 +70,17 @@ public final class BesuVersionUtils { | |
| Optional.ofNullable(implVersion).orElse("NONE/null")); | ||
| } | ||
| } | ||
| COMMIT = commit; | ||
| VERSION = implVersion; | ||
| COMMIT = commit != null ? commit : UNKNOWN; | ||
| VERSION = implVersion != null ? implVersion : UNKNOWN; | ||
| } | ||
|
|
||
| private BesuVersionUtils() {} | ||
|
|
||
| /** | ||
| * Generate version-only Besu version | ||
| * | ||
| * @return Besu version in format such as "v23.1.0" or "v23.1.1-dev-ac23d311" | ||
| * @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() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check usage of this, should be able to remove some null checks e.g. in |
||
| return VERSION; | ||
|
|
@@ -117,7 +122,7 @@ public static String nodeName(final Optional<String> maybeIdentity) { | |
| /** | ||
| * Generate the commit hash for this besu version | ||
| * | ||
| * @return the commit hash for this besu version | ||
| * @return the commit hash for this besu version, or {@value #UNKNOWN} if not available | ||
| */ | ||
| public static String commit() { | ||
| return COMMIT; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| package org.hyperledger.besu.util; | ||
|
|
||
| import com.google.common.base.Throwables; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /** The Exception utils. */ | ||
| public class ExceptionUtils { | ||
|
|
@@ -25,9 +26,9 @@ private ExceptionUtils() {} | |
| * Returns the root cause of an exception | ||
| * | ||
| * @param throwable the throwable whose root cause we want to find | ||
| * @return The root cause | ||
| * @return The root cause, or {@code null} if the input is {@code null} | ||
| */ | ||
| public static Throwable rootCause(final Throwable throwable) { | ||
| public static @Nullable Throwable rootCause(final @Nullable Throwable throwable) { | ||
| return throwable != null ? Throwables.getRootCause(throwable) : null; | ||
|
siladu marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
28
to
33
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,10 +16,10 @@ | |
|
|
||
| import java.io.Closeable; | ||
| import java.io.DataOutputStream; | ||
| import java.io.File; | ||
| import java.io.FileNotFoundException; | ||
| import java.io.FileOutputStream; | ||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.function.BiFunction; | ||
|
|
||
|
|
@@ -51,10 +51,16 @@ public RollingFileWriter( | |
| currentSize = 0; | ||
| fileNumber = 0; | ||
| final Path firstOutputFile = filenameGenerator.apply(fileNumber, compressed); | ||
| final File parentDir = firstOutputFile.getParent().toFile(); | ||
| if (!parentDir.exists()) { | ||
| //noinspection ResultOfMethodCallIgnored | ||
| parentDir.mkdirs(); | ||
| final Path parentPath = firstOutputFile.getParent(); | ||
| if (parentPath != null) { | ||
| try { | ||
| Files.createDirectories(parentPath); | ||
| } catch (final IOException e) { | ||
| final FileNotFoundException fnfe = | ||
| new FileNotFoundException("Unable to create directory for rolling file: " + parentPath); | ||
| fnfe.initCause(e); | ||
| throw fnfe; | ||
|
Comment on lines
+58
to
+62
|
||
| } | ||
|
siladu marked this conversation as resolved.
siladu marked this conversation as resolved.
|
||
| } | ||
| out = new FileOutputStream(firstOutputFile.toFile()); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |||||||||
| package org.hyperledger.besu.util.log4j.plugin; | ||||||||||
|
|
||||||||||
| import java.util.Arrays; | ||||||||||
| import java.util.Objects; | ||||||||||
|
|
||||||||||
| import org.apache.logging.log4j.Level; | ||||||||||
| import org.apache.logging.log4j.Marker; | ||||||||||
|
|
@@ -25,6 +26,7 @@ | |||||||||
| import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory; | ||||||||||
| import org.apache.logging.log4j.core.filter.AbstractFilter; | ||||||||||
| import org.apache.logging.log4j.message.Message; | ||||||||||
| import org.jspecify.annotations.Nullable; | ||||||||||
|
|
||||||||||
| /** Matches a text in the stack trace */ | ||||||||||
| @Plugin( | ||||||||||
|
|
@@ -34,11 +36,11 @@ | |||||||||
| printObject = true) | ||||||||||
| public class StackTraceMatchFilter extends AbstractFilter { | ||||||||||
| private final String stackContains; | ||||||||||
| private final String messageEquals; | ||||||||||
| private final @Nullable String messageEquals; | ||||||||||
|
|
||||||||||
| private StackTraceMatchFilter( | ||||||||||
| final String stackContains, | ||||||||||
| final String messageEquals, | ||||||||||
| final @Nullable String messageEquals, | ||||||||||
| final Result onMatch, | ||||||||||
| final Result onMismatch) { | ||||||||||
| super(onMatch, onMismatch); | ||||||||||
|
|
@@ -71,9 +73,9 @@ public Result filter(final LogEvent event) { | |||||||||
| return filter(event.getThrown()); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private Result filter(final Throwable t) { | ||||||||||
| private Result filter(final @Nullable Throwable t) { | ||||||||||
| if (t != null) { | ||||||||||
| return (messageEquals == null || t.getMessage().equals(messageEquals)) | ||||||||||
| return (messageEquals == null || Objects.equals(t.getMessage(), messageEquals)) | ||||||||||
| && Arrays.stream(t.getStackTrace()) | ||||||||||
| .map(StackTraceElement::getClassName) | ||||||||||
| .anyMatch(cn -> cn.contains(stackContains)) | ||||||||||
|
siladu marked this conversation as resolved.
|
||||||||||
|
|
@@ -102,8 +104,8 @@ public static StackTraceMatchFilter.Builder newBuilder() { | |||||||||
| public static class Builder extends AbstractFilterBuilder<StackTraceMatchFilter.Builder> | ||||||||||
| implements org.apache.logging.log4j.core.util.Builder<StackTraceMatchFilter> { | ||||||||||
|
|
||||||||||
| @PluginBuilderAttribute private String stackContains = null; | ||||||||||
| @PluginBuilderAttribute private String messageEquals = null; | ||||||||||
| @PluginBuilderAttribute private @Nullable String stackContains; | ||||||||||
| @PluginBuilderAttribute private @Nullable String messageEquals; | ||||||||||
|
|
||||||||||
| /** Default constructor */ | ||||||||||
| public Builder() { | ||||||||||
|
|
@@ -116,7 +118,7 @@ public Builder() { | |||||||||
| * @param text the match string | ||||||||||
| * @return this builder | ||||||||||
| */ | ||||||||||
| public StackTraceMatchFilter.Builder setStackContains(final String text) { | ||||||||||
| public StackTraceMatchFilter.Builder setStackContains(final @Nullable String text) { | ||||||||||
| this.stackContains = text; | ||||||||||
|
Comment on lines
+121
to
122
|
||||||||||
| 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"); |
Copilot
AI
Apr 8, 2026
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.
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.
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.
Would be good to add
compileOnly 'org.jspecify:jspecify'in here similar to other modules' build.gradle e.g.
besu/evm/build.gradle
Line 57 in 15082ba