[vlsi]

This PR adds tests only. It changes no production code.

Why

A fix for #1727 has a failure mode that is easy to miss: while making the reported false positives go away, it can also stop reporting values that really are nullable. While prototyping, I hit exactly that: an explicitly @Nullable member and a library-modeled Map.get invoked through ? extends T both stopped producing diagnostics, and the existing suite stayed green.

As @msridhar notes in the issue, the fix needs NullAway to model the JSpecify NO_CHANGE nullness operator rather than treating expressions as either @Nullable or @NonNull. These tests pin what must not change while that work happens, so a later implementation PR has something to fail against.

What

The tests live in a new NullnessOperatorTests class. Sixteen of them record behavior that is already correct today:

• An explicitly @Nullable member reached through NBox<? extends T> is rejected at return, argument, and field-assignment sinks, and through NBox<T> as a control.
• The @Nullable result of Map.get is rejected through Map<String, ? extends T> and through Map<String, T> as a control.
• A value obtained through Box<? extends T> is rejected when dereferenced, directly or through a local.
• Passing a local to a T parameter is rejected once the local has been overwritten with null.
• Box<? extends @Nullable T> and a @NonNull T return target are both rejected.
• Box<T>, a nonnull-bounded T, a concrete wildcard bound, and a nested generic bound are all accepted.

Nine tests describe the defects and carry @Ignore("https://github.com/uber/NullAway/issues/1727"). Eight are false positives, where a value obtained through ? extends T is rejected at a sink that requires T:

• issue1727WildcardReturnFalsePositive
• issue1727WildcardLocalArgumentFalsePositive
• issue1727WildcardDirectArgumentFalsePositive
• issue1727WildcardFieldAssignmentFalsePositive
• issue1727IteratorFieldAssignmentFalsePositive
• issue1727AggregateFalsePositive
• issue1727LambdaReturnFalsePositive
• issue1727ArrayStoreFalsePositive

The ninth is a false negative, and it involves no wildcard:

• issue1727NullableBoundDereferenceFalseNegative — dereferencing a T parameter goes unreported while T extends @Nullable Object. NullAway does report the same dereference when the value arrives through Box<? extends T>, so the two halves of this class disagree with each other today.

The two field-assignment tests also cover the cascading initializer method does not guarantee @NonNull field warning: master reports both assignment diagnostics and the cascading initializer diagnostic in each test, and a fix should leave zero.

JUnit 4 does not fail when an ignored test starts passing, so that list doubles as the checklist for removing the annotations in a later PR.

Open question

Would this split work, so that A can merge on its own?

• A — classify the effective nullness operator of an expression and apply it at return, argument, field-assignment, and field-initializer sinks. Covers the issue body and the field example from the first comment, and leaves NullnessStore alone.
• B — track whether a local or parameter still holds a value compatible with its type variable, together with definite-initialization handling. Covers T value = box.get(); sink(value).
• C — extend the same model to lambda returns and array stores.

issue1727NullableBoundDereferenceFalseNegative sits outside all three, since it is about a dereference rather than a sink.

How to verify

./gradlew --quiet :nullaway:test --tests "com.uber.nullaway.jspecify.NullnessOperatorTests"

25 tests, 9 skipped, no failures.

./gradlew --quiet :nullaway:test --tests "com.uber.nullaway.jspecify.WildcardTests"

45 tests, no skips, no failures. This is the class the tests moved out of.

./gradlew --quiet :nullaway:test

996 tests, 20 skipped, no failures.

./gradlew --quiet :nullaway:buildWithNullAway

Compiles NullAway with NullAway.

Removing the nine @Ignore annotations produces exactly nine failures, one per method, so none of them passes by accident.

Summary by CodeRabbit

• Tests
  • Added comprehensive JSpecify-focused regression coverage for nullable members, wildcard captures, type variables, assignments, arguments, returns, dereferences, arrays, lambdas, iterators, and aggregates.
  • Added validation for accepted and rejected nullness scenarios, including nullable bounds and wildcard results.
  • Documented known issue #1727 cases as ignored tests pending resolution.

[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: 93f93ea6-4fd0-4d38-9d79-082ebd2b0399

📥 Commits

Reviewing files that changed from the base of the PR and between 02181e4 and 7fe65b5.

📒 Files selected for processing (1)

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

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

Walkthrough

Added a JSpecify-focused NullAway regression suite for the NO_CHANGE nullness operator. Enabled tests cover nullable members, modeled values, type variables, wildcard capture, assignments, arguments, returns, dereferences, arrays, lambdas, iterators, aggregates, and nullable bounds. Ignored tests document issue #1727 cases. A helper configures JSpecify mode with OnlyNullMarked=true.

Merge Risk: ⚪ Minimal · up to 7fe65

This PR adds nullness regression tests without changing production behavior, dependencies, or deployment configuration. No actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 3.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding tests for the JSpecify nullness operator.
Description check	✅ Passed	The description directly explains the test-only changes, covered JSpecify nullness behaviors, issue #1727 regressions, and verification results.

• Fix all pre-merge checks with AI

✨ 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.}

[msridhar]

Thanks for these tests! I think I would like them in a separate test file though. Maybe NoChangeTests, with Javadoc on the class explaining what is being tested?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.63%. Comparing base (f142e85) to head (33ffa7a).

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1767   +/-   ##
=========================================
  Coverage     87.63%   87.63%           
  Complexity     3276     3276           
=========================================
  Files           109      109           
  Lines         11043    11043           
  Branches       2235     2235           
=========================================
  Hits           9678     9678           
  Misses          641      641           
  Partials        724      724           

☔ 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.

[vlsi]

What does "NoChangeTests" means?
Did you mean JSpecify's NO_CHANGE?
I don't think all the tests relate to NO_CHANGE.

Let me think what could be the better class name or class names.

[msridhar]

Ah, I thought all the tests related to JSpecify's NO_CHANGE functionality. That's what I meant. Maybe move those only those tests to a new NoChangeTests class then?

[vlsi]

Will do, and the class comment is a good call — it forces the set to state what it guards.

On the name: I would lean toward NullnessOperatorTests over NoChangeTests. The set pins all three operators rather than only NO_CHANGE: issue1727UnionNullWildcardReturnIsRejected covers UNION_NULL and issue1727NonNullTargetReturnIsRejected covers MINUS_NULL. And "no change" is hard to find later — someone who has not read this thread would not guess that a class by that name holds the wildcard-to-type-variable rules, whereas "nullness operator" is the heading the spec uses and the phrase they would search for. If you would rather keep your wording, NoChangeOperatorTests reads as a term where NoChangeTests does not. Your call either way.

What moves

All 24 tests, including the seven that guard explicitly @Nullable members and library models.

Those seven are tempting to leave behind, since they sit next to annotationRestoredFromUpperBoundToCapturedWildcard and are about capture rather than about NO_CHANGE. I would still move them: they are the lower bound an implementation must not cross, and that is not hypothetical. The prototype that removed the false positives also stopped reporting @Nullable R get() and Map.get through ? extends T, with the rest of the suite still green. Keeping the bound and the goal in one file is what makes the set useful.

WildcardTests goes back to 1420 lines. The new class extends NullAwayTestsBase and repeats the four-line makeHelper(), same as the other classes in this package.

Two tests get a cross-reference on the way out, so they do not read as copies of what stays behind: issue1727WildcardResultDereferenceIsRejected overlaps wildcardCaptureReturnWithTypeVariableUpperBound (the type variable is declared on the method here, on the class there), and issue1727WildcardLocalDereferenceIsRejected overlaps wildcardCaptureLocals (? extends T here, ? extends @Nullable Object there).

I would also drop the issue1727 prefix from the 16 green tests once the class name carries that context, and name them after the property instead: nullableMemberThroughWildcardIsRejected, wildcardResultIsNotDereferenceable. The eight ignored ones keep the prefix, since their names double as the checklist for removing the @Ignore annotations.

Class comment

/**
 * Fails when a value whose type is a type variable, or a wildcard captured against one, changes how
 * it is treated at an assignment sink or at a dereference.
 *
 * <p>Such a value carries the JSpecify {@code NO_CHANGE} nullness operator. It satisfies the same
 * type variable at a return, an argument, or a field assignment, and it stays unsafe to dereference
 * while the type variable has a {@code @Nullable} upper bound. An explicit {@code @Nullable},
 * declared on the member or supplied by a library model, overrides that and is reported at every
 * sink.
 *
 * <p>The green tests pin the second half of that rule, which a fix for the first half can break
 * without failing anything else. The tests annotated {@code @Ignore} describe the false positives
 * from issue #1727, where a value obtained through {@code ? extends T} is rejected at a sink that
 * requires {@code T}. A fix turns those green and leaves the rest of this class alone.
 *
 * @see <a href="https://jspecify.dev/docs/spec/#nullness-operator">JSpecify: nullness operator</a>
 */

Two things I would keep out of this PR

Three tests already in WildcardTests look like they belong elsewhere, independent of #1727: weirdErrorMessageReducedFromSpring and identicalLookingWildcardNestedInArrayErrorMessage assert diagnostic wording and would fit GenericInferenceErrorReportingTests, and unboundWildcardTypeVarUnmarked is about @NullUnmarked rather than wildcards. Happy to move them in a follow-up if you agree.

Splitting the rest of WildcardTests looks premature to me. It is third by size behind GenericsTests (3238 lines) and GenericMethodTests (2170), and a split would conflict with every open wildcard branch. If it is worth doing later, the natural seams are containment between wildcard types (13 tests), capture and the nullness of the captured result (7), and wildcards under inference in lambdas and method references (13, several of which would fit GenericMethodLambdaOrMethodRefArgTests).

[msridhar]

Sounds great to me! I like the name NullnessOperatorTests a lot.

Could you also include this test as ignored, to reinforce that the issues go beyond wildcards:

@NullMarked
class Test {
  static class Foo<T extends @Nullable Object> {
      int test(T t) {
         return t.hashCode();  // there should be a warning here!
      }
  }
}

(from #1727 (comment))

[vlsi]

Done, and thanks for the extra case — it is a good one, because it makes the two halves of the class disagree with each other. NullAway reports box.get().hashCode() for Box<? extends T>, and reports nothing for t.hashCode() on a T parameter, with the same nullable upper bound behind both. I confirmed the same silence for a local of type T and for a static method, so it is the type variable rather than the nesting.

It is in as issue1727NullableBoundDereferenceFalseNegative, which brings the ignored set to nine: eight false positives and one false negative.

Also in this push:

• NullnessOperatorTests, with the class comment.
• The sixteen passing tests renamed after the property each establishes, now that the class name carries the issue context. The nine ignored ones keep the prefix.
• Two of the moved tests gained a {@link} to the WildcardTests test they overlap, so they do not read as copies.
• WildcardTests is back to 1415 lines.

I retitled the PR, since "wildcard" no longer covers the set.

Verification: NullnessOperatorTests runs 25 tests with 9 skipped, WildcardTests runs 45 with none skipped, the full :nullaway:test runs 996 with 20 skipped and no failures, and :nullaway:buildWithNullAway passes. Removing the nine @Ignore annotations produces exactly nine failures, one per method.

The A/B/C question in the description is still open whenever you get to it. The new test sits outside all three, since it is about a dereference rather than about a sink.