Commit daa5828
authored
Add positive test cases for annotations on generic method return types (#1704)
## Summary
#1201 reports that no error is issued when a method's return type
carries a nullability annotation on a
type variable that is instantiated from the receiver's type argument:
```java
@NullMarked
class Test {
private static class Inner<T extends @nullable Object> {
Inner<@nullable T> identity() { return this; }
}
Inner<Object> mThing = new Inner<Object>();
void foo() {
mThing = mThing.identity(); // should warn
}
}
```
Rather than assume the issue described current behavior, I ran the
example verbatim as a test first. On
current `master` NullAway **does** report the expected error:
```
warning: [NullAway] incompatible types: Inner<@nullable Object> cannot be converted to Inner<Object>
mThing = mThing.identity();
```
This was fixed by `938a8972` (#1371, "Handle annotations on type
variables in return and field types"),
which was filed against #1354 and resolved this issue as a side effect.
Confirmed by building both sides:
the repro reports nothing at `6dd3976f`, the commit immediately
preceding it, and is correctly flagged at
`938a8972`.
That matches the mechanism. #1371 added
`restoreExplicitNullabilityAnnotations` to the
`MethodInvocationTree` and `MemberSelectTree` branches of
`GenericsChecks.getTreeType`. That call has since
moved into the `getMethodTypeForInvocation` path but is still what makes
this case work: the receiver's
type is used to compute the method's type as a member of it via
`TypeSubstitutionUtils.memberType`,
substituting `T := Object` into the declared return type
`Inner<@nullable T>` to yield
`Inner<@nullable Object>`, and the explicit annotation is then restored
onto the result. The assignment
check then sees the mismatch against the declared `Inner<Object>`.
So no production change is needed here. While confirming that, though, I
found a gap in the test coverage,
which is what this PR closes.
`nonNullInReturnTypeArg` — the test #1371 added for method return types
— asserts only the **negative**
direction, that a restored `@NonNull` type argument is *accepted* where
a non-null type argument is
expected. The error-reporting direction is untested for method returns,
although `nonNullInFieldTypeArg`
covers it for field reads. That missing direction is exactly what #1201
reports, so it seems worth pinning
down.
This PR adds an `asNullable()` counterpart to the existing `asNonNull()`
and asserts both directions.
`MaybeNull<String> m2 = nonNull.asNullable();` is structurally the case
from the issue: the receiver
substitutes `T := String` into the declared return type
`MaybeNull<@nullable T>`, and the assignment is
flagged. No production code changes.
## Testing
- Extended `nonNullInReturnTypeArg` in `GenericsTests.java` with two
positive cases (argument passing and
assignment), keeping an unannotated negative case alongside them so the
test pins down both directions
and doesn't just assert that we over-report.
- Negative control on current `master`: guarding out the
`restoreExplicitNullabilityAnnotations` call that
#1371 added makes both new assertions fail. It also flips the
pre-existing negative case
`accept(nullable.asNonNull())` into a false positive, since without the
restore the receiver's type
argument is echoed back in place of the declared annotation.
- Confirmed the issue's own repro reports nothing at `6dd3976f` and
passes at `938a8972`.
- `./gradlew :nullaway:test` — 918 tests, 0 failures.
Resolves #1201
## AI usage disclosure
> I used Claude Code for this PR. I asked it to help me find a good next
contribution among the open
> `jspecify` issues; it ran the repro from #1201 and found the issue had
already been fixed on `master`.
> When I asked it to check my assumption that the behavior was already
well tested, it found that
> `nonNullInReturnTypeArg` covers only the negative direction. I decided
to close that gap with a test
> rather than simply comment on the issue, and directed it to identify
the exact commit that fixed the
> issue and to demonstrate that the new assertions fail without that
fix. It located #1371 via
> `git log -S`, installed a JDK 25 so the pre-fix commit would build,
and ran the repro at both
> `6dd3976f` and `938a8972`; it also guarded out the
`restoreExplicitNullabilityAnnotations` call on
> current `master` to confirm the new cases fail without it. I have read
and understood all the changes
> in this PR.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added support for converting `MaybeNull` values to a nullable generic
form.
* Nullable and non-null values can now be handled together while
preserving type-safety.
* **Tests**
* Expanded coverage for valid nullable conversions.
* Added verification that invalid conversions to non-null generic values
are rejected.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->1 parent 811383b commit daa5828
1 file changed
Lines changed: 9 additions & 1 deletion
Lines changed: 9 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3064 | 3064 | | |
3065 | 3065 | | |
3066 | 3066 | | |
| 3067 | + | |
| 3068 | + | |
| 3069 | + | |
3067 | 3070 | | |
3068 | 3071 | | |
3069 | | - | |
| 3072 | + | |
3070 | 3073 | | |
| 3074 | + | |
| 3075 | + | |
| 3076 | + | |
| 3077 | + | |
| 3078 | + | |
3071 | 3079 | | |
3072 | 3080 | | |
3073 | 3081 | | |
| |||
0 commit comments