refactor: declare org.testng.internal.annotations null-marked - #3382
Merged
Conversation
Thirty files in testng-core, three in testng-core-api -- the first package whose minority half is more than a single file, and the first one upstream of packages already marked and merged: org.testng.internal.invokers, org.testng.internal.objects, org.testng.annotations and org.testng.internal.objects.pojo all read it. Marking a leaf could only produce errors in its own files; this one could produce them in code already shipped. The package-info goes in testng-core-api, which testng-core depends on, and the per-module control confirms both halves are covered: a throwaway null-returning method is clean in both modules before the file and fails after it, at DisabledRetryAnalyzer.java:14 and, separately, at BaseAnnotation.java:9. Forty-nine errors -- one in testng-core-api, forty-eight in testng-core. 91 @nullable, two Objects.requireNonNull and nine restructurings answer them. The shape of the package is a tag hierarchy -- BaseAnnotation, then TestOrConfiguration and the leaves TestAnnotation, FactoryAnnotation, DataProviderAnnotation, ListenersAnnotation -- with one producer, JDK15TagFactory, which constructs a tag and immediately fills every field from the Java annotation it mirrors. That producer decides how the "field not initialized" errors are answered. Where the interface already published a @nullable getter the field takes the annotation; where it published a non-null one the field takes the default of the annotation member it mirrors, because that is what the tag factory assigns a line later and what every reader already treats as absent: "" for the data provider name and the two dataProvider strings, an empty list for indices, DisableDataProviderRetries for retryUsing, an empty array for @listeners, and DisabledRetryAnalyzer for retryAnalyzer -- the value BaseTestMethod.setRetryAnalyzerClass already substitutes for null. Annotating those instead would have published nullity on six getters whose every caller dereferences them. IAnnotationFinder is the other half. Its javadoc has said "or null if none found" since it was written, and JDK15AnnotationFinder returns a literal null in two of the six overloads, so the implementation had to be @nullable -- and NullAway then rejected it against the non-null interface. Those six returns are demanded, not documentation: NullAway is silent on type *arguments*, not on a @nullable return that happens to be a type variable. AnnotationHelper's five delegates and both findConfiguration overloads follow from the same source. Downstream, ten files in the four marked packages read this package and two needed anything. ClassBasedParallelWorker.isSequential already tested its ITestAnnotation for null; #3380 had to read that guard as residue because findAnnotation was non-null, and it is now demanded. ConfigInvoker:308 passes the result of AnnotationHelper.findConfiguration to handleConfigurationSkip, which #3381 tightened to non-null; requireNonNull there records that a ConfigurationMethod only exists because TestNGMethodFinder:130 found a configuration annotation on that very method, through the same lookup. That settles the second deferred finding of #3381 -- handleConfigurationFailure keeps `null != annotation` on one path and Objects.requireNonNull on the other. Annotating the return makes the asymmetry compiler-visible instead of a reading, and it does not reproduce: that parameter is null only when the throw happened before the assignment, and the statements that precede it can only produce an NPE (requireNonNull on the instance) or a TestNGException (the annotation finder), neither of which passes isSkipExceptionAndSkip -- the sole gate to the requireNonNull path. The remaining guard is unreachable rather than wrong, so no issue is opened. DisabledRetryAnalyzer needs nothing, which is worth saying rather than leaving as a zero: it is public surface despite the package -- Test#retryAnalyzer() names it -- but its one method overrides IRetryAnalyzer.retry(ITestResult), which is unannotated, and NullAway never widens an implementation against an unannotated supertype. Of the other two files in the minority, IDataProvidable needed the getDataProviderClass pair, which its own subinterface ITestAnnotation had already declared @nullable and therefore contradicted, and IAnnotationFinder needed the six returns plus the one class its own overload passes null for. findOptionalValues stays as it is: its javadoc describes null *elements*, which NullAway does not check. Three of the nine restructurings are not field defaults. JDK15AnnotationFinder returns early when findAnnotationInSuperClasses finds nothing, which is what the private overload it calls did on its first line, and keeps Pair's constructor non-null. JDK15TagFactory asserts the method it was handed when building a data provider tag -- @dataProvider is only ever looked up on a method, never on a class. ListenersAnnotation starts from an empty array instead of null. Every annotation is classified by deletion and recompilation, one at a time: 87 of the 91 bring back a named error. The four that do not stay, because dropping them would leave a contract contradicting itself. Two are interface parameters, which NullAway never checks an implementation against: IAnnotationFinder's @nullable class, whose matching parameter in JDK15AnnotationFinder is demanded by the literal null its own overload passes, and IDataProvidable.setDataProviderClass, which ITestAnnotation already declares @nullable and whose two implementations are demanded. The other two are JDK models NullAway reads optimistically: IgnoreListener walks up package names with Package.getPackage, and the private findAnnotation in JDK15AnnotationFinder takes the result of Method.getAnnotation. Both test their parameter on the first line of the body, and both are null on every lookup that finds nothing. One trap worth recording: a plain javac error anywhere in the module -- here a generic array creation -- stops the NullAway pass entirely, so the compiler reports zero NullAway errors on code full of them. A clean reading means nothing until the compile itself is clean.
|
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 (19)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change adds JSpecify nullability contracts across annotation APIs and implementations. It also adds explicit annotation defaults, an early null return in annotation lookup, nullable tag creation inputs, and configuration-annotation validation. ChangesAnnotation nullability and defaults
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds nullness annotations with localized call-site adjustments, and the reported full build succeeds without an actionable correctness concern. No merge-blocking risk remains beyond normal checks. 🚥 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 |
3 tasks
krmahadevan
approved these changes
Aug 17, 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.
Continues the JSpecify/NullAway stack. Base is
master— #3380 and #3381 are merged, so this onestands alone.
One package:
org.testng.internal.annotations— thirty main files intestng-coreand three(
DisabledRetryAnalyzer,IAnnotationFinder,IDataProvidable) intestng-core-api. Nosub-packages. Two things change at once compared with #3381: the minority half is more than a single
file for the first time, and this is the first package upstream of packages that are already
marked and merged.
org.testng.internal.invokers(#3381),org.testng.internal.objects,org.testng.annotations(#3374, #3375) andorg.testng.internal.objects.pojoall read it. Markinga leaf could only produce errors in its own files; this one could produce them in code already
shipped.
The coverage was proved, not assumed
testng-coredeclaresapi(projects.testngCoreApi), so thepackage-info.javagoes intestng-core-apiand the mark travels downstream. That is what needed proving: put it on the wrongside and the thirty-file majority compiles unchecked, which looks exactly like success.
Per the procedure in
AGENTS.md, the negative control runs once per module traversed. A throwawaywent into one file of each module. Before the
package-info.javaboth compile clean, zero NullAway.After it both fail with
[NullAway] returning @Nullable expression from method with @NonNull return type:testng-core-apiDisabledRetryAnalyzer.java:14testng-coreBaseAnnotation.java:9Both reverted. The
testng-corerow is the one that matters, and it is red, so the counts belowmean something.
What the check reported
testng-core-api, 48 intestng-core)@NullableObjects.requireNonNullEvery annotation is classified by deletion and recompilation — one at a time, each stripped and
the modules recompiled. 87 bring back a named error.
The tag hierarchy decides the field errors
The shape is
BaseAnnotation, thenTestOrConfigurationand the leavesTestAnnotation,FactoryAnnotation,DataProviderAnnotation,ListenersAnnotation, with one producer:JDK15TagFactoryconstructs a tag and immediately fills every field from the Java annotation itmirrors. That producer decides how the "field not initialized" errors are answered.
Where the interface already published a
@Nullablegetter the field takes the annotation. Where itpublished a non-null one the field takes the default of the annotation member it mirrors, because
that is what the tag factory assigns a line later and what every reader already treats as absent:
""for the data provider name and the twodataProviderstrings, an empty list forindices,DisableDataProviderRetriesforretryUsing, an empty array for@Listeners, andDisabledRetryAnalyzerforretryAnalyzer— the valueBaseTestMethod.setRetryAnalyzerClassalready substitutes for null. Annotating those instead would have published nullity on six getters
whose every caller dereferences them.
IAnnotationFinderis the other half. Its javadoc has said "or null if none found" since it waswritten, and
JDK15AnnotationFinderreturns a literal null in two of the six overloads, so theimplementation had to be
@Nullable— and NullAway then rejected it against the non-null interface.Those six returns are demanded, not documentation: NullAway is silent on type arguments, not on
a
@Nullablereturn that happens to be a type variable.AnnotationHelper's five delegates andboth
findConfigurationoverloads follow from the same source.The new variable: errors in code already merged
Ten files across the four marked packages read this one. Two needed anything:
ClassBasedParallelWorker.isSequentialalready tested itsITestAnnotationfor null. refactor: declare four cross-module packages null-marked #3380 had toread that guard as residue because
findAnnotationwas non-null; it is now demanded.ConfigInvoker:308passes the result ofAnnotationHelper.findConfigurationtohandleConfigurationSkip, which refactor: declare org.testng.internal.invokers null-marked #3381 tightened to non-null.requireNonNullthere records that aConfigurationMethodonly exists becauseTestNGMethodFinder:130found a configuration annotationon that very method, through the same lookup.
The second deferred finding of #3381 is settled
handleConfigurationFailurekeepsnull != annotationon one path andObjects.requireNonNullonthe other. Annotating the return makes that asymmetry compiler-visible instead of a reading, and
it does not reproduce: the parameter is null only when the throw happened before the assignment,
and the statements that precede it can only produce an NPE (
requireNonNullon the instance) or aTestNGException(the annotation finder), neither of which passesisSkipExceptionAndSkip— thesole gate to the
requireNonNullpath. The remaining guard is unreachable rather than wrong, so noissue is opened.
The minority half
DisabledRetryAnalyzerneeds nothing, which is worth saying rather than leaving as a zero in atable: it is public surface despite the package —
Test#retryAnalyzer()names it — but its onemethod overrides
IRetryAnalyzer.retry(ITestResult), which is unannotated, and NullAway neverwidens an implementation against an unannotated supertype. Same situation as
RetryAnalyzerCountin#3380.
IDataProvidableneeded thegetDataProviderClasspair, which its own subinterfaceITestAnnotationhad already declared
@Nullableand therefore contradicted.IAnnotationFinderneeded the sixreturns plus the one class its own overload passes null for.
findOptionalValuesstays as it is:its javadoc describes null elements, which NullAway does not check.
The four that stay without a demand
Dropping any of them would leave a contract contradicting itself.
IAnnotationFinder's@Nullable Class<?>JDK15AnnotationFinderis demanded by the literal null its own overload passesIDataProvidable.setDataProviderClassITestAnnotationalready declares it@Nullableand both implementations are demandedIgnoreListener.findAnnotation(Package)Package.getPackageis read optimistically by the JDK modelfindAnnotationinJDK15AnnotationFinderMethod.getAnnotationis read optimisticallyThe last two test their parameter on the first line of the body and are null on every lookup that
finds nothing.
Restructured rather than annotated
Six of the nine are the field defaults above. The other three:
JDK15AnnotationFinderreturns earlywhen
findAnnotationInSuperClassesfinds nothing, which is what the private overload it calls did onits first line, and keeps
Pair's constructor non-null;JDK15TagFactoryasserts the method it washanded when building a data provider tag —
@DataProvideris@Target(METHOD), so it is neverlooked up on a class;
ListenersAnnotationstarts from a shared empty array instead of null.Verification
./gradlew buildon the rebased branch: BUILD SUCCESSFUL,16854 completed, 0 failed, 12 skipped— the twelve are the pre-existing environment-dependent skips (heap-dump support and alistener sample). Every count in this description comes from a full recompile
(
--rerun-tasks), never an incremental one.One trap worth recording for the next lot: a plain javac error anywhere in the module — here a
generic array creation — stops the NullAway pass entirely, so the compiler reports zero NullAway
errors on code full of them. A clean reading means nothing until the compile itself is clean.
No behaviour change.
Summary by CodeRabbit