Remove useless null checks (CodeQL java/useless-null-check)#236
Open
vharseko wants to merge 3 commits into
Open
Remove useless null checks (CodeQL java/useless-null-check)#236vharseko wants to merge 3 commits into
vharseko wants to merge 3 commits into
Conversation
Seven null checks tested a value that provably cannot be null at that point, so
the guarded branch (or the null arm of a ternary) was dead code. Simplify each:
- Persistit.crash: _journalManager and _bufferPoolTable are final fields
initialised inline to new instances, so they are never null. Drop the
always-true "!= null" guards and run the bodies unconditionally.
- Cookie.hashCode/toString: the port field is a final, inline-initialised
ArrayList that is never reassigned, so it is never null. Use
port.hashCode() directly and append it unconditionally.
- Info.toString: the two "ip != null ? ip.getHostAddress() : ipString"
ternaries sit inside branches already guarded by "ip != null", so they
always take the non-null arm; replace them with ip.getHostAddress().
- OSGiFrameworkService.loadConfiguration: the check on the freshly created
BufferedReader was useless (a constructor never returns null), and it left
the real failure mode - a missing /launcher.json on the classpath -
unhandled: getResourceAsStream returns null and new InputStreamReader(null)
then throws NullPointerException. Null-check the resource stream before
wrapping it, so the intended IllegalArgumentException is thrown instead.
Behaviour is unchanged for every existing input; the OSGi case additionally
reports the missing-resource error cleanly rather than as an NPE.
vharseko
added
bug
codeql
maximthomas
approved these changes
Jul 8, 2026
The missing-volume alert asserted after startup is posted by
JournalManager.writeForCopy when the journal copier copies a page back to a
now-deleted volume. For the non-transactional pages this test writes, that copy
runs on the background JOURNAL_COPIER thread, so the assertion immediately after
new Persistit(_config) races it: on a slow/loaded runner no alert has been
posted yet and getHistory(MISSING_VOLUME_CATEGORY) returns null ("Startup with
missing volumes should have generated alerts", seen on macos-latest JDK 26).
Force the copy-back with copyBackPages() before the assertion so the alert is
generated deterministically. ignoreMissingVolumes is still false at that point,
so the missing-volume pages are retained and the later
ignoreMissingVolumes-behavior check is unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves all seven open
java/useless-null-checkCodeQL alerts. Each site testeda value that provably cannot be null at that point, so the guarded branch (or the
null arm of a ternary) was dead.
Persistit.java_journalManager,_bufferPoolTablefinalfields initialised inline tonew …()Cookie.javaportfinal List<Integer> = new ArrayList<>(), never reassignedInfo.javaipip != nullOSGiFrameworkService.javainputnew BufferedReader(...)Notes
Persistit.crash— the two!= nullguards are always true; the bodies nowrun unconditionally.
Cookie—hashCode()usesport.hashCode()directly andtoString()appends
portunconditionally (it was already appended on every call, since theguard could never be false).
Info.toString— theip != null ? ip.getHostAddress() : ipStringternaries are inside
if (ip != null && …)branches, so they always take thenon-null arm. The other, unguarded ternaries further down are left untouched.
OSGiFrameworkService— this one hid a real bug. The check was on theBufferedReader, which a constructor never returns as null, so the intendedfailure mode (a missing
/launcher.jsonon the classpath) was never caught:getResourceAsStreamreturnsnullandnew InputStreamReader(null)thenthrows
NullPointerException. The null check now guards the resource streambefore it is wrapped, so the intended
IllegalArgumentExceptionis thrown.Behaviour is unchanged for every existing input; the OSGi case additionally
reports the missing-resource error cleanly instead of as an NPE.
Testing
mvn -o -pl commons/geo,persistit/core,commons/http-framework/core,commons/launcher/launcher compile— each module builds (BUILD SUCCESS).