refactor: declare org.testng.annotations and org.testng.internal.protocols null-marked - #3370
Merged
krmahadevan merged 2 commits intoAug 16, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change adds JSpecify nullability annotations and package-level ChangesAPI contracts and protocol defaults
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR adds package nullness declarations while preserving the existing non-null empty-value behavior expected by consumers. No actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
krmahadevan
force-pushed
the
juherr/null-marked-annotations-and-protocols
branch
from
August 16, 2026 03:27
e8489d5 to
2e0d81c
Compare
Base automatically changed from
juherr/null-marked-collections-and-cli
to
master
August 16, 2026 03:27
org.testng.annotations lives only in testng-core-api, so it can be marked
without dragging an unmarked half in from another module through
package-info.class on the compile classpath.
Nothing in the package is demanded by the check. Twenty-two of the thirty-four
files are annotation types and one is an enum, none of which carry a method
body; the only bodies are IConfigurationAnnotation.isBeforeGroups and
isAfterGroups, which return a boolean. The compile is green the moment
package-info.java lands.
The six annotations are all contract, and they sit on the interfaces because
that is the only place the nullness is stated at all. Their implementations
live in org.testng.internal.annotations, which spans testng-core-api and
testng-core and so cannot be marked one half at a time; NullAway does not check
it, and no already marked package references these types. Annotating here
breaks nothing today and becomes binding the day that package is marked, which
is why each one had to be true rather than decorative:
ITestOrConfiguration.getDescription and setDescription --
JDK15TagFactory.findInherited returns null and createTestTag feeds it
straight in, which is what TestNGMethod.getDescription and
IgnoreListener.onBeforeClass already test for.
ITestAnnotation.getDataProviderClass and setDataProviderClass -- same
findInherited path; TestAnnotation leaves the field null and Parameters
guards it four times over.
IFactoryAnnotation.getIndices -- FactoryAnnotation never initialises the
field, and FactoryMethod tests indices == null twice.
IFactoryAnnotation.setLazy -- FactoryAnnotation.setLazy *is* the null test:
m_lazy = lazy == null ? Lazy.UNSET : lazy. The getter stays non-null.
Dropping all six compiles green, so none is there to satisfy the compiler.
Everything else stays bare, including the getters whose field starts out null
but is always assigned before anyone can read it: getRetryAnalyzerClass,
IDataProviderAnnotation.getIndices, getName, getDataProvider, retryUsing, and
every array getter, which either initialises to {} or takes its value from a
Java annotation member that cannot be null.
No behaviour changes -- annotations only.
org.testng.internal.protocols lives only in testng-core-api, the last package of that module to sit in exactly one place. It is also the last one small enough to take in a single pass. The check demands one thing and it is not an annotation. Input.Builder declares five reference fields with no initialiser and a constructor that assigns nothing, so NullAway refuses all five at once. Annotating them would be a lie: PackageUtils.findClassesInPackage is the only caller in the repository, it calls all six setters, and no test builds an Input. They are seeded with empty values instead, the way Converter.m_files was. The one difference is what build() yields when a setter is skipped -- a NullPointerException out of Collections.unmodifiableList before, empty values now. No caller takes that path. Nothing else in the package reports. File.listFiles and Method.invoke are not in NullAway's JDK models, so the existing dirfiles == null test in findClassesInDirPackage stands unremarked and BundledResourceProcessor keeps dereferencing the reflected URL; Utils.log and the java.net calls cross into unmarked code, which NullAway reads optimistically. No behaviour changes on any reachable path -- annotations only.
krmahadevan
force-pushed
the
juherr/null-marked-annotations-and-protocols
branch
from
August 16, 2026 03:27
2e0d81c to
3b4a8cb
Compare
krmahadevan
approved these changes
Aug 16, 2026
This was referenced Aug 16, 2026
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 #3365 — review only the two commits above
refactor(jcommander): declare the package null-marked.These are the last two packages of
testng-core-apithat live in exactly one module, which is theconstraint the whole stack is built on:
@NullMarkedapplies to a package, and a package splitacross modules gets its unmarked half marked anyway through
package-info.classon the compileclasspath. Between them they are the 41 files left in that module that could be taken without
dragging in a second one.
The annotation rule
An
@Nullableis written only when one of two things is true, and the reason was checked bydeleting it and recompiling:
Everything else stays bare. Restructuring wins over annotating when it costs nothing.
org.testng.annotationsorg.testng.internal.protocolsZero required is not the check going quiet. Between them these two packages contain three method
bodies —
IConfigurationAnnotation.isBeforeGroups,isAfterGroups, both returningboolean, andthe
Processorhierarchy — and the one thing NullAway does demand is answered without anannotation. See the negative control below.
Commit 1 —
org.testng.annotationsThirty-four files: 22 annotation types, 11 interfaces, one enum. Annotation types and enums carry
no method body, so
package-info.javalands green on its own.The six annotations are all contract, and they go on the interfaces because that is the only place
the nullness is stated at all. The implementations live in
org.testng.internal.annotations, whichspans
testng-core-apiandtestng-coreand so cannot be marked one half at a time. NullAway doesnot check it, and no already marked package references these types — so annotating here cannot
break anything today, and becomes binding the day that package is marked. Which is exactly why each
one had to be true rather than decorative:
ITestOrConfiguration.getDescription/setDescriptionJDK15TagFactory.findInheritedreturnsnullandcreateTestTagfeeds it straight in — the null thatTestNGMethod.getDescriptionandIgnoreListener.onBeforeClassalready test forITestAnnotation.getDataProviderClass/setDataProviderClassfindInheritedpath;TestAnnotationleaves the fieldnullandParametersguards it four timesIFactoryAnnotation.getIndicesFactoryAnnotationnever initialises the field;FactoryMethodtestsindices == nulltwiceIFactoryAnnotation.setLazyFactoryAnnotation.setLazyis the null test:m_lazy = lazy == null ? Lazy.UNSET : lazy. The getter stays non-nullDropping all six compiles green, so none of them is there to satisfy the compiler.
What stays bare is the more interesting half. Every getter whose field starts out
nullbut isalways assigned before anyone can read it keeps its non-null contract:
getRetryAnalyzerClass,IDataProviderAnnotation.getIndices(unlike itsIFactoryAnnotationnamesake — that asymmetry is real,
createDataProviderTagsets it andcreateFactoryTagis theone that can be overridden to null),
getName,getDataProvider,retryUsing,getSuiteName,getTestName,getExpectedExceptionsMessageRegExp,IFactoryAnnotation.getLazy, and every arraygetter — those either initialise to
{}or take their value from a Java annotation member, whichcannot be null.
Commit 2 —
org.testng.internal.protocolsSeven files, and the check demands one thing that is not an annotation:
Input.Builderdeclares five reference fields with no initialiser and a constructor that assignsnothing. Annotating them would be a lie, the same lie
Converter.m_fileswould have been in #3365:PackageUtils.findClassesInPackageis the only caller in the repository, it calls all six setters,and no test builds an
Input. They are seeded with empty values instead — five initialisers, zeroannotations, and none of the cascade that
@Nullableon those fields would have pushed intoInput's constructor and on into all four processors.The one difference is what
build()yields when a setter is skipped: aNullPointerExceptionoutof
Collections.unmodifiableListbefore, empty values now. No caller takes that path.Nothing else in the package reports, and three near-misses are worth recording because they look
like they should.
File.listFilesandMethod.invokeare not in NullAway 0.13.8's JDK models, sothe existing
dirfiles == nulltest infindClassesInDirPackagestands unremarked andBundledResourceProcessorkeeps dereferencing the reflectedURL.Utils.logand thejava.netcalls cross into unmarked code, which NullAway reads optimistically.
NullAway:JSpecifyModeis off,so
List<String>type arguments are not checked either.Verification
./gradlew buildgreen: 15948 tests, 0 failures, 12 skipped,testng-test-osgiincluded.The check was proved live rather than inert on each package separately. A throwaway
default String probe() { return null; }onIAnnotation, and a throwawayreturn null;inNoOpProcessor.process, each failed the compile:Both were removed. The six contract annotations were deleted together and recompiled green.
Noticed, not fixed
FactoryAnnotation.getDataProviderDynamicClass()returnsnullfor every@Factory, whileIDataProvidabledeclares it non-null andTestAnnotationholds"". Filed as #3369. It does notNPE today only because
Parametersshort-circuits ondataProviderClass == null, whichcreateFactoryTaghappens to guarantee — a guarantee anIAnnotationTransformercan remove. Thefix is one initialiser, but it is a behaviour change, so it does not belong here.
That is also why
IDataProvidableitself is untouched: it sits inorg.testng.internal.annotations,the split package, and marking it is a later step.
No
CHANGES.txtentry: no behaviour changes on any reachable path.Summary by CodeRabbit