Add library model for ObjectUtils.firstNonNull

[dbwiddis]

Summary

org.apache.commons.lang3.ObjectUtils.firstNonNull returns the first non-null value passed to it, or
null when every value is null. Today NullAway has no model for it, so — since ObjectUtils is
unannotated — the return is optimistically treated as non-null and code like this is silently accepted:

ObjectUtils.firstNonNull(a, b).length();  // NPE when both are null
ObjectUtils.firstNonNull().toString();    // guaranteed NPE

As #612 notes, the semantics are "@Contract("!null -> !null") for each element of its varargs." That
can't be written as a @Contract today: ContractUtils.getAntecedent requires the antecedent to have
exactly one entry per call-site argument, and a varargs method has a different arity at each call site.

Approach

This adds a library model kind, allParamsNullImpliesNullReturn, for methods whose return is null only
when all of their arguments are null. It is the dual of the existing nullImpliesNullParameters —
that one is "any listed argument null ⇒ nullable return", this one is "every argument null ⇒ nullable
return" — and it keys on MethodRef alone rather than on fixed parameter indices, which is what makes it
applicable to varargs. It is a default method on LibraryModels so existing implementors are unaffected.

The one subtlety worth flagging for review is in onDataflowVisitMethodInvocation. By the time the
dataflow hook runs, the CFG has already desugared a varargs call: node.getArguments() returns a single
ArrayCreationNode, and AccessPathNullnessPropagation.visitArrayCreation correctly reports it as
non-null, because a freshly created array is never null. Reading the arguments directly therefore
concludes "some argument is non-null" at every call site. The implementation instead inspects that node's
initializers. This also means firstNonNull(new String[] {"x", a}) is handled correctly, which is
covered by a test rather than assumed.

When an existing array is passed in the varargs position, its contents are not visible at this level, so
those positions are skipped and the return is conservatively treated as nullable.

The model is consumed in two places: the dataflow hook above, and onOverrideMayBeNullExpr, the
non-dataflow path answering whether an expression can be null at all. Without the second, the first is
overruled.

Known tradeoff

firstNonNull is often used the way SQL's COALESCE is, relying on an invariant the analysis cannot see —
for example two fields where a constructor guarantees exactly one is non-null. That guarantee is a
relational fact between two access paths, and NullAway tracks nullness for each access path
independently, so such calls will now be reported. Objects.requireNonNull at the point of use resolves
it (verified), as does a suppression. I've documented this on the new interface method so it's discoverable
next to the model definition.

This is the same shape of tradeoff as modeling Map.remove as @Nullable in #1623, which shipped with a
release note about newly-reported warnings — but it is a real behavior change and I'd rather have it
called out explicitly than found in review. Happy to drop or gate the model if you'd rather not take it.

ObjectUtils.defaultIfNull(T, T) has identical semantics and would be a one-line addition to the same set,
but it is an even purer instance of the COALESCE idiom, so I left it out deliberately rather than
doubling the affected surface in one PR.

Testing

Two tests in FrameworkTests.java, alongside the existing commons-lang3 Validate tests.

apacheObjectUtilsFirstNonNull covers the semantics: all values nullable (reported), last value non-null
(silent), first value non-null (silent — so the check isn't accidentally position-dependent), non-null known
only from dataflow via an enclosing null check (silent), no arguments at all (reported), result null-checked
before use (silent), result never dereferenced (silent), and a boxed result unboxed to int (reported as an
unboxing error).

apacheObjectUtilsFirstNonNullVarargsForms covers the call shapes: an existing array passed in the varargs
position (reported, contents not visible), an array created at the call site with all-nullable elements
(reported) and with a non-null element (silent), and values that are themselves of array type so that T is
String[], both all-nullable (reported) and with one non-null (silent).

objectUtilsFirstNonNull in JSpecifyLibraryModelsTests.java pins the same behavior down in JSpecify
mode, since library modeling of varargs differs there (#1481, #1485). Behavior is identical in both modes.

• Negative control: deleting the single methodRef entry makes both tests fail, so every assertion is
  load-bearing and the signature string is confirmed correct. That matters here because model lookup is
  string equality against MethodSymbol.toString() with no validation — a typo silently matches nothing.
• ./gradlew :nullaway:test — 920 tests, 0 failures.
• ./gradlew :test-library-models:test — passing; run because the LibraryModels interface changed.

No CHANGELOG entry, as that file has no unreleased section; happy to add one wherever you'd prefer.

Fixes #612

AI usage disclosure

I recognize this is a low-priority issue. I chose it deliberately as an educational exercise, to learn
more about the codebase — so please weigh it accordingly, and don't feel obliged to take it if the
tradeoff above isn't one you want.

I used Claude Code for this PR. I picked the issue and directed the approach throughout. It walked me
through the library-model pipeline — the LibraryModels interface, DefaultLibraryModels,
CombinedLibraryModels, OptimizedLibraryModels, and the two consumer sites — and wrote the
registration boilerplate once I understood what each layer did. I made the design calls: framing the
model's semantics in terms of arguments that are possibly null rather than known-null, accepting the
COALESCE-style false positive as preferable to silently missing real NPEs, and asking for that
limitation to be documented on the interface method. Its first implementation compiled and was silently
wrong, reading the desugared array rather than the values; it found the cause by instrumenting the
handler. I then pushed back on whether the test cases were exhaustive, particularly for varargs whose
values are themselves arrays. That review took the tests from five assertions to thirteen and caught a
behavior — the handling of an array created at the call site — that was about to be described in this PR
without any test covering it. I also asked whether #1485 had any bearing here, which is what prompted the
JSpecify-mode test. I have read and understood all the changes in this PR.

Summary by CodeRabbit

• New Features

  • Improved nullness analysis for methods that return a nullable value only when all arguments are null.
  • Added support for zero-argument and varargs calls, including explicit varargs arrays.
  • Added handling for Apache Commons Lang ObjectUtils.firstNonNull.

• Bug Fixes

  • Nullability warnings now more accurately identify unsafe dereferences and unboxing when no non-null fallback is available.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7f6d1d5d-19b0-479c-8693-c8b7ee50ec0d

📥 Commits

Reviewing files that changed from the base of the PR and between 2599b0c and be24353.

📒 Files selected for processing (1)

• nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java

---

Walkthrough

Adds an allParamsNullImpliesNullReturn() library-model contract. Library-model implementations aggregate and index these models. Nullability analysis handles ordinary arguments and desugared varargs arrays, while existing varargs arrays remain conservatively nullable. Apache Commons Lang ObjectUtils.firstNonNull(T...) uses the model. Tests cover standard, varargs, and JSpecify invocation forms.

Possibly related PRs

• uber/NullAway#1518: Modifies LibraryModelsHandler varargs nullability handling.
• uber/NullAway#1677: Adds related library-model semantics through LibraryModels and LibraryModelsHandler.

Suggested reviewers: msridhar, yuxincs

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes implement support for ObjectUtils.firstNonNull with correct varargs nullability semantics and comprehensive tests required by issue #612.
Out of Scope Changes check	✅ Passed	All changes support the requested library model, its integration, and related test coverage; no unrelated code changes are evident.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the main change: adding a library model for ObjectUtils.firstNonNull.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cpovirk]

I'm not on the NullAway team, but: I'd like to think that JSpecify mode would at least eventually handle this "naturally" if we could have a library model of the form:

@NullMarked
public static <T extends @Nullable Object> T firstNonNull(T... values)

I don't know whether that would work now or anytime soon, but does it seem right in principle?

[msridhar]

@cpovirk I think what is wanted here is that for a call like this:

Object x = ObjectUtils.firstNonNull(null, null, new Object());

NullAway should learn that x cannot be null. I think the type argument inferred for the above call with the signature you proposed would be @Nullable Object for T, which is not quite what we want here; I may have misunderstood.

[cpovirk]

Oh, you're right, sorry.

[dbwiddis]

@cpovirk I think what is wanted here is that for a call like this:

Object x = ObjectUtils.firstNonNull(null, null, new Object());

NullAway should learn that x cannot be null.

Precisely: in the expected use case, you try a bunch of things and default to something non-null. Normally the guaranteed non-null is at the end of the list (because having it any earlier makes everything past it dead code).

What I've written will work fine in this case, or also in the case where everything is nullable in which case it will infer nullability of x.

There is an edge case, however, when we know that one of N values is non null but not which one. But there's no way of really determining given firstNonNull(x, y) if both x and y can be nullable but code assumes one is not:

Consider this class I wrote in my day job:

• this constructor sets source to null but getResponse to a non-null value.
• this constructor sets source to a non-null value but getResponse is null.
• this getter uses null as a sentinel to return the non-null getResponse or create it from source.

Sure, ultimately it's @Nullable at the end of the day from an exception handling standpoint but other than that exception this is essentially the same use case as firstNonNull().

Should that default to nullable in the simpler case (without the exceptional return)? I think so; one can always suppress the warning.

[msridhar]

Hi @dbwiddis one thing I'm confused on here. I don't see that ObjectUtils has any nullability annotations at this point, or that it is @NullMarked. So by default I don't think code calling the ObjectUtils.firstNonNull method is going to see any warnings about the return being @Nullable right now. Is this PR also introducing the modeling of firstNonNull as possibly returning @Nullable? I'm pretty sure this must be true, just confirming.

[codecov]

Codecov Report

❌ Patch coverage is 82.85714% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.81%. Comparing base (6bba0e5) to head (5f2829e).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...m/uber/nullaway/handlers/LibraryModelsHandler.java	82.35%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1705      +/-   ##
============================================
- Coverage     87.83%   87.81%   -0.02%     
- Complexity     3184     3192       +8     
============================================
  Files           109      109              
  Lines         10815    10850      +35     
  Branches       2186     2199      +13     
============================================
+ Hits           9499     9528      +29     
- Misses          622      625       +3     
- Partials        694      697       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dbwiddis]

I don't see that ObjectUtils has any nullability annotations at this point, or that it is @NullMarked. So by default I don't think code calling the ObjectUtils.firstNonNull method is going to see any warnings about the return being @Nullable right now. Is this PR also introducing the modeling of firstNonNull as possibly returning @Nullable? I'm pretty sure this must be true, just confirming.

Great question and this bears a bit of context on how I got to this PR.

I've been picking through open issues looking for small ones to learn the codebase on. I used AI to help scan for tractable ones and this surfaced early, but I passed on it at first because it needed new code rather than reusing an existing pattern. After a few easier ones I came back to it as a challenge, knowing it was marked low priority and might not go anywhere.

To your question: yes. ObjectUtils has no annotations, so today NullAway assumes the return is non-null and nobody sees a warning. This PR is what makes it @Nullable in the first place.

However, the behavior itself isn't in question: commons-lang3's own javadoc says firstNonNull returns null when every value is null. This is documented nullability if not annotated.

The catch is that a model applies to everyone at once, and here it creates new warnings on builds that are clean today. I actually encountered this very decision today when implementing JSpecify annotations on OSHI. One module implemented an unannotated external dependency's interface and I had to decide whether to introduce warnings where they did not previously exist in a widely adopted dependency. I decided not to "narrow" the contract in that case. I get it.

So I'm actually not eager to merge this for this specific upstream method. I'm wondering if is there is value in keeping the new model type but not registering firstNonNull with it? Even if commons-lang3 annotated this method tomorrow, the most it could say is "the return is @nullable", which would make every call site warn. I was hoping to at least improve on that and identify a case where it could pass (if one of the arguments was demonstrably non null).

I'll admit I focused on how to do this and never asked whether it should be done. Anyway, I've taken my best shot at the how; the whether is yours to call, and if the answer is that it isn't worth the churn, that's worth writing down on the issue so the next person doesn't retrace my steps. Either way this was a genuinely useful way to learn the codebase.

[msridhar]

Thanks so much for the context, @dbwiddis; really appreciate all the fixes and tests you've contributed thus far!

Here are some thoughts on this PR. First, we're pretty careful these days about adding features that lead to new warnings outside of JSpecify mode, as that causes disruptions for many users. JSpecify mode is more explicitly "under development" and is the future of NullAway so I'm more ok with new warnings there. So, at the least, I'd prefer that whatever we do here not impact non-JSpecify-mode users.

I'm still a bit unsure about the original motivation behind #612; @lazaroclapp do you remember by any chance? If I had to guess, I'd say maybe we ran into an NPE issue where a user passed all @Nullable arguments to firstNonNull and then didn't null check the result. It's a valid hole in our checking, and one we should address, but I'm not sure how to prioritize.

Speaking of priorities, I think our biggest priority right now is to get more JSpecify mode support debugged and shipped. A great deal of new support is currently gated under the JSpecifyExperimental flag. Enabling this flag causes a lot of new errors to be reported in existing projects, mostly due to the JSpecify JDK models. I'm looking through reports from our integration test subjects and trying to find remaining NullAway bugs to fix. I will be more disciplined about creating issues on the things I'm finding, and on the triage work, so that others can pick up some of those tasks if desired.

I think ideally how we'd address this one is something like the following:

1. Add library models to give firstNonNull the type signature that @cpovirk suggested above. That's the best type we can give it within the JSpecify type system.
2. Separately have a way to model that if any argument is @NonNull, then the result is @NonNull.

Right now the implementation mixes these two things, and I think it'd be good to separate them.

Regarding 1, we have a way to designate a class as @NullMarked in a library model, but not yet a way to make an individual method @NullMarked in an otherwise @NullUnmarked class. We could add a model for all of org.apache.commons.lang3.ObjectUtils to deal with this. Or maybe we could submit a PR upstream with annotations, if they'd be open to that? But if we don't want to do either of those, we'd have to add a way to make individual methods @NullMarked.

So, bottom line, I'd like to get this in, assuming we can change the implementation strategy a bit, and also make it impact only those with JSpecify mode enabled. It's probably not the highest priority at the moment, but if we want to just finish it up that's fine with me.

[dbwiddis]

Here are some thoughts on this PR. First, we're pretty careful these days about adding features that lead to new warnings outside of JSpecify mode, as that causes disruptions for many users.

Speaking of priorities, I think our biggest priority right now is to get more JSpecify mode support debugged and shipped.

Noted! JSpecify is what brought me here to begin with and I'm 100% on board with this and I actually did use the jspecify tag to filter when working on other contributions. I also see you actively working many of those so I've been digging through older issues to try to find the "less important" ones.... this particular issue was outside that umbrella but interesting from an educational lens.

So, bottom line, I'd like to get this in, assuming we can change the implementation strategy a bit, and also make it impact only those with JSpecify mode enabled. It's probably not the highest priority at the moment, but if we want to just finish it up that's fine with me.

Fair. I'm going to flip this PR into draft mode for now as I think the revised scope you mention is currently above my repo comprehension. Anyone else is free to take it on!