refactor: declare org.testng.collections, org.testng.cli and org.testng.cli.jcommander null-marked - #3365
Merged
krmahadevan merged 3 commits intoAug 16, 2026
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
This was referenced Aug 15, 2026
krmahadevan
approved these changes
Aug 16, 2026
Second package through the NullAway wiring, and the first outside testng-core-api. org.testng.collections qualifies for the same reason org.testng.log4testng did: @NullMarked applies to a package, so a package that appears in more than one module cannot be marked one half at a time. This one lives only in testng-collections -- unlike org.testng.util, whose Strings sits here while RetryAnalyzerCount and TimeUtils carry the same package in testng-core. Only one annotation is demanded by the check: removeAll hands back Map.remove, which NullAway models as @nullable. No caller in the main sources reads that result, so the null is documented rather than designed away. toString dereferenced m_objects.get(i) directly, which NullAway rejects even though the keys come from keySet(). It now goes through get(K), the class's own accessor; since every key is already present, computeIfAbsent inserts nothing and the output is unchanged. The five remaining annotations are contract, not compiler pressure -- each one sits on a parameter whose method body *is* a null test: CollectionUtils.hasElements, both overloads, called from ConfigurationGroupMethods with the result of a Map.get; Objects.ToStringHelper.add, both overloads, and the private s() they share, called from TestResult.toString. Dropping those five compiles green; keeping them stops the null tests from reading as dead code.
org.testng.cli lives only in testng-cli, so the package can be marked without dragging a half of itself in from another module. Twenty-three of the twenty-four annotations are demanded by the check: CliOptions holds one field per command line option, and an option that was not passed leaves its field null. NullAway refuses a @nonnull field with no initializer, which is the useful outcome here -- the class now states which options are absent instead of leaving CliConfigurer's null tests (cli.verbose != null, cli.threadCount != null, and a dozen more) as the only record of it. Fields that do carry a default -- suiteFiles, commandLineMethods, useDefaultListeners, xmlPathInJar, spiListenersToSkip, suiteThreadPoolSize, randomizeSuites, alwaysRunListeners, the Boolean toggles -- stay bare. The twenty-fourth is contract: AbstractCliRunner.run already tests null != listener, and overriding ITestNGCliRunner.run with a widened parameter is what NullAway expects of an unannotated supertype. Dropping it compiles green. CliConfigurer needs nothing. Everything it hands to TestNG crosses into unmarked code, which NullAway reads optimistically.
Third and last package of the batch, and the one that pays for the first two: marking org.testng.cli.jcommander is what makes the cli boundary report anything. JCommanderOptions carries the same twenty-three fields as CliOptions, null for the same reason -- JCommander only assigns an option that was passed -- and NullAway demands the same annotation for each. The twenty-fourth annotation is the finding of the batch. JCommanderCliRunner.parse wraps a ParameterException as new CliParseException(ex.getMessage(), ex) and Throwable.getMessage() is modelled @nullable, so the constructor that became marked in the previous commit rejected the call. Widening that overload is the truthful fix: it exists to carry another exception's message. Its single-argument sibling is left non-null; every caller has a real message to give, and the check does not ask for more. Converter.m_files was the one field where the annotation would have been a lie -- @parameter(required = true) means JCommander never leaves it unset. It is seeded with an empty list instead. That is invisible because JCommander reuses a main parameter list it finds in place (clearing it before the first value) and decides `required` from the parameter description rather than from the field. Both halves are now characterized in ConverterMainParameterTest, so a JCommander upgrade that changed either would say so.
krmahadevan
force-pushed
the
juherr/null-marked-collections-and-cli
branch
from
August 16, 2026 03:26
e6bf205 to
68c87a9
Compare
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.
Stacked on #3367 — review only the three commits above
refactor(log4testng): declare the package null-marked.#3367 wires NullAway and takes one package through it. That first package sits in
testng-core-api, so nothing there exercises the mechanism across a module boundary. This takes the next three, chosen for one property: each lives in exactly one module.That constraint is not cosmetic.
@NullMarkedapplies to a package, and a package split across modules gets its unmarked half marked anyway through the compile classpath. It is whyorg.testng.utilstays out even thoughStringssits intestng-collections:RetryAnalyzerCountandTimeUtilscarry the same package intestng-core.The annotation rule
An
@Nullableis written only when one of two things is true, and the reason was checked one annotation at a time by deleting it and recompiling:Everything else stays bare. Nothing on
Lists,Sets,Maps,ListMultiMap,SetMultiMap,CliConfigurer,JCommanderCliRunner, nor on any field that carries a default.org.testng.collectionsorg.testng.cliorg.testng.cli.jcommanderForty-six of the fifty-four are one field per command line option. An option that was not passed leaves its field null — that is the mechanism
CliConfigurerreads with a dozen!= nulltests, and it was nowhere stated until now.Commit 1 —
org.testng.collectionsMultiMap.removeAllhands backMap.remove, which NullAway models@Nullable;MultiMapTest.removeAllOnAnAbsentKeyReturnsNullhad already pinned that null, so the annotation just says out loud what the test asserts.toStringdereferencedm_objects.get(i)and is rejected even though the keys come fromkeySet(). It now goes throughget(K), the class's own accessor: every key is already present, socomputeIfAbsentinserts nothing andtoStringRendersOneIndentedLinePerKeyis unchanged.The five contract annotations are the parameters of
CollectionUtils.hasElements(both overloads, called fromConfigurationGroupMethodswith the result of aMap.get) and ofObjects.ToStringHelper.add(both overloads plus the privates()they share, called fromTestResult.toString).Commit 2 —
org.testng.cliCliOptions: 23 fields with no initializer, each null when its option is absent. The one contract annotation isAbstractCliRunner.run, whose body already testsnull != listener.Commit 3 —
org.testng.cli.jcommanderThis is where the boundary reports something.
JCommanderCliRunner.parsewraps aParameterExceptionasnew CliParseException(ex.getMessage(), ex), andThrowable.getMessage()is modelled@Nullable, so the constructor marked in commit 2 rejected the call. That overload exists to carry another exception's message, so widening it is the truthful fix. Its single-argument sibling stays non-null — every caller has a real message, and the check does not ask for more.Converter.m_filesis the one field where an annotation would have been a lie:@Parameter(required = true)means JCommander never leaves it unset. It is seeded with an empty list instead. Verified against jcommander 2.0 bytecode, and now characterized inConverterMainParameterTest:JCommander.initMainParameterValuereuses a main parameter list it finds in place, clearing it before the first value;JCommander.validateOptionsdecidesrequiredfromParameterDescription.isAssigned(), not from the field being null.Verification
./gradlew buildgreen: 15948 tests, 0 failures,testng-test-osgiincluded.The check was proved live rather than inert on each of the three packages: a throwaway
return null;in a marked class of each made the compile fail with[NullAway], and was removed. The six contract annotations were each deleted and recompiled green — none is there to satisfy the compiler.Noticed, not fixed
Two pre-existing defects in
org.testng.collections, filed as #3366 because fixing either changestoString()output:ToStringHelper.omitNulls()andomitEmptyStrings()are both no-ops — the value is stringified before the filters see it, soValueHolder.isNull()andisEmptyString()are always false.TestResult.toString()asks for both, and so printsoutput={null}.MultiMap.toString()iterateskeySet(), which copies into aHashSetand so loses the ordering of a sorted multimap.No
CHANGES.txtentry: no behaviour changes, and #3367 added none.