refactor: declare org.testng.internal null-marked - #3395
Closed
juherr wants to merge 11 commits into
Closed
Conversation
Marking org.testng.internal reaches four modules, and the packages that were already marked answer for what they receive from it. XmlPackage no longer hands a null package name to PackageUtils: an unnamed <package> tag is now reported the way an unreadable one already was, instead of raising a NullPointerException from inside findClassesInPackage. The other three are checker-visibility fixes with no behaviour change: defaultIfStringEmpty inlines its predicate because a nullness test hidden behind a call does not refine the argument on the other branch, XmlWeaver tests the class directly instead of through a boolean local, and getSkipCausedBy reads the method and the context into locals it already had in hand.
…ations
BaseTestMethod.m_instance was declared non-null while its own getInstanceId()
answered through ofNullable(...).orElse(null) and getFactoryParameterInfo()
tested it. Saying so resolves the constructors of TestNGMethod,
ConfigurationMethod and FactoryMethod in one move, and carries through to
getInstanceId() and IInstanceIdentity.
Three latent NullPointerExceptions surfaced while writing the contracts down:
- TestNGMethod.clone() wrapped getTestClass() in a NoOpTestClass, which
dereferences it on the spot. A method cloned before setTestClass has run threw
there; it now propagates the absence, which is what ConfigurationMethod.clone()
already did.
- TestNGMethodFinder wrote null into m_beforeGroups/m_afterGroups, whose
declaration says {}. Every configuration method that is not a group one carried
a null array past MethodGroupsHelper, which iterates it. The default is used
instead, and the guards that anticipated the null in XmlMethodSelector and
ConfigurationMethod become residue.
- MethodInstance.SORT_BY_INDEX compared two <test> names without either being
guaranteed to have one.
BaseTestMethod.getTestClass() stays nullable: setTestClass is called late in the
lifecycle by code outside TestNG, so findMethodParameters answers an absent test
class with the suite and <test> parameters - what XmlTestUtils computes anyway
when no <class> tag matches.
The setters of the nullable getters are widened with them. Kotlin only
synthesises a mutable property when both halves agree; leaving setTestClass,
setMissingGroup, setDescription and setXmlTest non-null would turn four
properties into read-only ones for every Kotlin caller.
Parameters carried the largest share of the diagnostics, behind seven causes rather than twenty-six sites. MethodParameters.context and .testResult are absent on the constructor injection path, which SimpleObjectDispenser reaches before any test context exists. The callees that store the null answer for it - ReflectionRecipes.inject already tested the context internally, MethodMatcherContext was handed a literal null, and CreationAttributes held it in an already nullable field. The one that dereferences it, invokeDataProvider, is given a context asserted at the boundary instead. ConstructorOrMethod gains requireConstructor(), the twin of requireMethod(): the three sites that reach for the constructor have already established the wrapper holds one, and widening IAnnotationFinder.findOptionalValues or ReflectionRecipes.getConstructorParameters to say otherwise would loosen contracts that third parties implement. Two facts had been recorded in a second variable and were lost on the way: the retry analyzer's existence lived in shouldRetry, and the data provider iterator's in thrownException. Both are now read from the value itself. The retry analyzer also gets a message: the dispenser can answer null, and the NullPointerException now names what could not be created. The remaining sites are private helpers whose callers already tested their result, and a nullness test that NullAway cannot follow through a boolean local.
…and graphs The rest of the package: class discovery, method collection, the dependency graphs and the invokers. Two facts had to stop travelling through a field. Graph.m_independentNodes was built lazily and read back off the field afterwards, so any call in between invalidated what the initialiser had just established; initializeIndependentNodes now hands the map back. ConfigurationGroupMethods.m_afterGroupsMap had the same shape, read from two lambdas that ran after the assignment. MultiMap says what it is: a HashMap-backed multimap. A method that carries no instance has no instance id, and both the instance dependency graph and the per-instance workers already grouped those methods under a null key. Keying them anywhere else would change how they are partitioned. The rest are local: private helpers whose callers already tested their result, lookups whose key came from the very map being read, an AtomicReference<Boolean> holding a fact that is a boolean, and the reflective members that are a method because their call site already established it is not a constructor. Where the value must exist for the caller to work at all, it is asserted at the boundary rather than carried further: TestNGClassFinder and ParameterHandler dispense instances through an object factory, so they say so once instead of threading the absence down to SimpleObjectDispenser.
Its next() was already declared to answer null; the implements clause said the opposite. A type-use annotation does not change type identity for javac, so the clause can now say the same thing without touching Parameters, ParameterHolder or the published IDataProviderInterceptor, and NullAway reports nothing new. The class comment names the three consumers that read the null, so the checks in MethodRunner and FactoryMethod do not read as dead code. Restructuring the producer to skip rather than answer null is deliberately left alone: hasNext() is handed to setMoreInvocationChecker and is called outside the iteration by ConfigurationGroupMethods and TestNgMethodUtils, so pre-fetching would pull rows from a possibly Stream-backed CloseableIterator during configuration scheduling, and MethodRunner needs the nulls to keep its reported parameter index aligned. ClonedMethod.setId and TestResult.setTestName are widened alongside their already nullable getters, so Kotlin keeps synthesising a mutable property for both.
…not justify Deleting each @nullable one at a time and recompiling showed three that nothing asks for: initBeforeAfterGroups is only ever handed the arrays a @BeforeGroups or @AfterGroups annotation reports, and two local variables whose nullness the checker infers on its own.
The four modules that host org.testng.internal compile clean under the check. Coverage was proven per module with a throwaway probe, as AGENTS.md requires: before this file each of the four compiled clean, after it each failed with "[NullAway] returning @nullable expression from method with @nonnull return type" at the probe's own line, with no other javac error in the log.
getConstructorOrMethod() answered null while the class held the java.lang.reflect.Method it was built from, so the wrapper is now built once in the constructor and handed back. The null was never a contract. All 59 call sites of ITestNGMethod.getConstructorOrMethod() in main dereference the result on the spot, and ClonedMethod's own toString() reads getDeclaringClass() off it, so printing a ClonedMethod threw a NullPointerException every time. Keeping the member non-null is also what lets ITestNGMethod.getConstructorOrMethod() stay non-null when org.testng is marked in turn - every other member of that interface this batch touches has to widen. Dropping this commit leaves the branch green: the annotation it replaces is the @nullable landed earlier in the branch.
FactoryMethod unwrapped an IParameterInfo by hand where IParameterInfo.embeddedInstance already does it, and four other sites call it. XmlTestUtils.findMethodParameters answers an absent class name with the suite and <test> parameters on its own, so BaseTestMethod states the rule by handing the null down rather than reproducing the result. TestNGMethodFinder builds its group arrays empty from the start instead of creating a null and undoing it 67 lines later, ConfigurationGroupMethods caches its group map through an accessor the way Graph does rather than threading it through two private methods, and Parameters drops an initial value that is never read along with the comment that had to explain it.
|
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 |
Member
Author
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
1 participant
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.
Adds
testng-core-api/src/main/java/org/testng/internal/package-info.javaand resolves the 111NullAway diagnostics it opens. #3393 wrote the nullness contracts into the signatures of
org.testng.internaland deliberately left the mark out; this is the half that turns them fromdocumentation into assertions.
Coverage
org.testng.internalspans four modules, and one module'spackage-info.classreaches another onlythrough a compile dependency. A throwaway
private static Object nullAwayProbe() { return null; }went into one file of each, as
AGENTS.mdrequires:TestNGDeadLockException[NullAway] returning @Nullable expression from method with @NonNull return typeat:12InstanceInfo:25XmlTestUtils:31YamlSchema:484grep ' error: ' LOG | grep -v '[NullAway]'was empty in both runs, so no ordinary javac errorsilenced the pass in either direction.
Counters
@NullableaddedrequireNonNulladdedClonedMethod, in its own!commit)CHANGES.txtEvery
@Nullablein the diff was deleted on its own and the four modules recompiled from scratch —98 runs. A verdict of (E) was accepted only when the returning error named the exact member or the
exact argument: of the 83, 57 are reported in the declaring file and 26 at the call site that passes
the value, which is where a widened parameter is necessarily demanded. Three annotations that
nothing asked for were deleted rather than justified (
initBeforeAfterGroups's parameter and twolocal variables); they are the
drop the annotations the classification pass did not justifycommit.Ripple into packages that are already marked
org.testng.internal.reflect,org.testng.internal.objects.pojo,org.testng.internal.annotations,org.testng.internal.invokers,org.testng.collectionsandorg.testng.xmlread these types, sowhat they receive had to be answered in the same pass. The rule applied: widen the callee where its
implementation already stores or tolerates the null, assert at the boundary where the callee
dereferences it.
ReflectionRecipes.inject(both overloads —nativelyInjectalready testedcontext != null),MethodMatcherContext(handed a literalnull),CreationAttributes(its field and getter werealready
@Nullable),MethodInvocationHelper.invokeDataProvider'sfedInstance,MultiMap's key.TestNGClassFinderandParameterHandlerdispense instances through an object factory,so they say so once rather than threading the absence down to
SimpleObjectDispenser;MethodParameters.requireContext()for the data-provider path, where every real caller supplies acontext.
Fixes that came out of the pass
Five null dereferences that were reachable before this branch, each with a
CHANGES.txtentry:TestNGMethod.clone()wrappedgetTestClass()in aNoOpTestClass, which dereferences it onthe spot. A method cloned before
setTestClasshas run threw there.ConfigurationMethod.clone()already propagated the absence; now both do.
TestNGMethodFinderwrotenullintom_beforeGroups/m_afterGroups, whose declaration says{}. Every configuration method that is not a group one carried a null array pastMethodGroupsHelper, which iterates it without testing.ClonedMethod.toString()readgetDeclaringClass()off its owngetConstructorOrMethod(),which answered
null— so printing aClonedMethodalways threw.MethodInstance.SORT_BY_INDEXcompared two<test>names without either being guaranteed tohave one.
XmlPackagehanded a null package name toPackageUtils.findClassesInPackage, which reads itsfirst character. A
<package>tag with nonameattribute is now reported the way an unreadableone already was.
Annotations the checker did not demand
Five, each kept for a stated reason:
MultiMap.containsKey,remove,removeAll,putAll—putandgetare demanded (amethod with no instance has no instance id, and both the instance dependency graph and the
per-instance workers already grouped those methods under a null key). A map that accepts a null key
through
putbut cannot be asked about it throughcontainsKeywould be incoherent.FilteredParameters implements Iterator<Object @Nullable []>—next()was already declared toanswer null and the implements clause said the opposite. A type-use annotation does not change type
identity for javac, so this touches neither
ParameterHoldernor the publishedIDataProviderInterceptor, and NullAway reports nothing either way.The six Kotlin ones are the setters of already-nullable getters —
BaseTestMethod.setTestClass/setMissingGroup/setDescription/setXmlTest,ClonedMethod.setId,TestResult.setTestName. Kotlin only synthesises a mutable property when both halves agree; withoutthem six properties become read-only for every Kotlin caller. Each was proven load-bearing by
deleting it and recompiling a throwaway Kotlin file that assigns all six.
That Kotlin evidence is only worth anything because the control failed as it should: assigning
ClassHelper.forName(...)to a non-nullClass<*>givesInitializer type mismatch: expected 'Class<*>', actual 'Class<*>?'. Kotlin does read theannotations.
What this pre-commits
Marking
org.testngnext will have to widen these, because the implementations annotated herealready answer null:
ITestNGMethod.getTestClass,getInstance,getMissingGroup,getRetryAnalyzer,getDescription,getXmlTest,getDataProviderMethod(the only one whosejavadoc already documents it) and the deprecated
getFactoryMethodParamsInfo; and onITestResult,getMethod,getName,getThrowable.getConstructorOrMethodis the one member saved from thatlist, by the
!commit.The largest downstream cost to budget for is
MethodInstance:42-55, which dereferenceso1.getMethod().getTestClass()and starts reporting the momentgetTestClass()is annotated.Left alone on purpose
FilteredParametersstill answersnullfor a filtered-out row. Restructuring the producer toskip instead was considered and rejected:
hasNext()is handed tosetMoreInvocationCheckerand iscalled outside the iteration by
ConfigurationGroupMethodsandTestNgMethodUtils, so pre-fetchingwould pull rows from a possibly
Stream-backedCloseableIteratorduring configurationscheduling — and
MethodRunnercounts the nulls to keep the reported parameter index aligned with-invocationnumbers. No test covers that alignment.ConfigurationMethod:377and:392still testm_beforeGroups != null, andXmlMethodSelector:112-114still wrapsgetBeforeGroups()inOptional.ofNullable. Both are now unreachable in-tree, butITestNGMethodis not marked yet, so athird-party implementation can still return null. They belong to the
org.testngbatch. The guardsalready noted in
PropertyUtilsandClassHelper.forNameare untouched.MultiMapwas widened rather than given a shared non-null token. ANO_INSTANCEsentinel inIInstanceIdentity.getInstanceId(Object)would keep the collection's key non-null and preserve thegrouping, at the cost of changing that helper's contract and the six sites that test its result for
null — four of them outside this diff. Worth doing, but not inside a nullness PR.
ITestNGMethod.getConstructorOrMethod()'s 59 call sites were checked: not one tests the result,which is what makes the
!commit safe.ConfigurationGroupMethods' latch protocol was raised in review previously and set aside: a racepredating this batch, no test, outside the scope of a nullness PR.
Verification
The four modules compile clean under a forced run of the guard set at the real severity, with the
scaffolding used to collect the diagnostics reverted (
git status --porcelain build-logicempty):Dropping the
!commit leaves the branch green — verified by rebasing it out and building:BUILD SUCCESSFUL,16938 completed, 0 failed, 12 skipped. It replaces a@Nullablelanded earlierin the branch, which is what makes that possible.