Fix JSpecify false negative when override narrows method type variable bound - #1682
Fix JSpecify false negative when override narrows method type variable bound#1682arimu1 wants to merge 5 commits into
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughOverride checks compare nullability on corresponding method type-variable upper bounds. The checks resolve substituted bounds, skip unannotated methods and mismatched type-variable counts, and report Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 2400-2423: Add Javadoc to
reportMismatchedMethodTypeVariableBoundError describing that it reports an error
when overriding and overridden method type variables have mismatched nullability
upper bounds, and document errorTree, overridingTv, overridingNullable,
overriddenMethod, overriddenNullable, and state.
- Around line 2350-2351: Update
checkMethodTypeVariableUpperBoundNullnessForOverriding and its call from
checkTypeParameterNullnessForMethodOverriding to accept the overridden method
type after member-type substitution in the overriding class context. Read
type-variable upper bounds from this contextual type instead of
overriddenMethod.getTypeParameters(), preserving correct nullable/non-null
instantiation behavior and JDK suppression handling. Add regressions covering
both nullable and non-null enclosing-class type-variable instantiations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 909e93f4-3d4b-4773-bd3f-a2f2fe8a8d7b
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Beyond the comment below, if you run ./gradlew :nullaway:buildWithNullAway you'll see several new warnings, and also integration tests are failing. I think we may need to special-case overrides of methods from @NullUnmarked code; if you could take a look that'd be great
d3497a0 to
baebf51
Compare
|
@msridhar Thanks for the review and the Fixed on the branch tip
Local verification (JDK 21): |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 2386-2389: Update
checkMethodTypeVariableUpperBoundNullnessForOverriding so differing
type-parameter counts do not bypass validation for erasure-compatible overrides
such as a concrete Object parameter overriding a generic T parameter. Validate
the erased parameter and return nullness contracts before any unsupported-count
exit, while preserving the existing compiler-handled behavior for genuinely
incompatible overrides. Add a regression covering `@Override` void bar(Object arg)
{ arg.hashCode(); } with a nullable generic call path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: acc6975b-f749-46c6-bbea-a8d6e3dfeb6b
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1682 +/- ##
============================================
- Coverage 87.82% 87.82% -0.01%
- Complexity 3181 3193 +12
============================================
Files 109 109
Lines 10809 10855 +46
Branches 2185 2196 +11
============================================
+ Hits 9493 9533 +40
- Misses 622 625 +3
- Partials 694 697 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks for the revisions! I have some more feedback
| substitutedMethodTypeVarUpperBoundIsNullable( | ||
| overriddenTypeVar, overriddenMethod, i, state); | ||
| if (overridingNullable != overriddenNullable) { | ||
| Tree errorTree = i < typeParameterTrees.size() ? typeParameterTrees.get(i) : tree; |
There was a problem hiding this comment.
Why do we need this i < typeParameterTrees.size() check here?
There was a problem hiding this comment.
Agreed — unnecessary. In a valid MethodTree, getTypeParameters() matches the overriding method type-parameter count. Dropped the guard and always use typeParameterTrees.get(i).
| if (methodType instanceof Type.ForAll forAll) { | ||
| return forAll.tvars; | ||
| } | ||
| return com.sun.tools.javac.util.List.nil(); |
There was a problem hiding this comment.
Is this actually reachable? This method is only used to get the type variables of the overridden method. I guess it might be reached if the overriding method introduces a type variable?
In any case, assuming this is reachable, then rather than using this method please explicitly check for no type variables in the caller and bail out there
There was a problem hiding this comment.
Yes — mainly the non-generic overridden case (not a Type.ForAll). Per your suggestion I removed getMethodTypeVariables and now bail in the caller with if (!(overriddenMethodType instanceof Type.ForAll forAll)) return; before reading forAll.tvars.
| if (handler.onOverrideMethodTypeVariableUpperBound(overriddenMethod, typeVarIndex, state)) { | ||
| return true; | ||
| } | ||
| if (!(substitutedTypeVar instanceof Type.TypeVar typeVar)) { |
There was a problem hiding this comment.
Just make the declared type of the parameter Type.TypeVar rather than having this bailout
There was a problem hiding this comment.
Done — parameter is now Type.TypeVar, with an explicit cast from forAll.tvars at the call site.
| // javac member-type substitution can drop type-use annotations on method type-variable | ||
| // bounds. If the original bound was a concrete type with an explicit @Nullable, honor that | ||
| // declaration. Do not consult original bounds that are still type variables — those must be | ||
| // resolved via substitution (or the free type-var path above). | ||
| List<Symbol.TypeVariableSymbol> originalTypeParams = overriddenMethod.getTypeParameters(); | ||
| if (typeVarIndex >= 0 && typeVarIndex < originalTypeParams.size()) { | ||
| Type originalBound = | ||
| (Type) ((TypeVariable) originalTypeParams.get(typeVarIndex).asType()).getUpperBound(); | ||
| if (originalBound.getKind() != TypeKind.TYPEVAR | ||
| && Nullness.hasNullableAnnotation( | ||
| originalBound.getAnnotationMirrors().stream(), config)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't understand when this code would apply. Can you please explain it?
There was a problem hiding this comment.
This path covers cases where member-type substitution (asMemberOf) leaves a concrete upper bound (e.g. Object) but drops the type-use @Nullable that was on the original method type-variable declaration.
Example that still needs it (covered by overridePreservesNullableMethodTypeVariableBound):
interface Foo {
<T extends @Nullable Object> void bar(T arg);
}
class Baz implements Foo {
public <T extends @Nullable Object> void bar(T arg) {}
}After substitution the overridden bound can appear as plain Object with no annotation mirrors. Without reading the original declaration’s @Nullable we would treat the overridden bound as non-null and false-positive on a matching @Nullable override.
We intentionally skip original bounds that are still type variables — those must go through substitution / the free type-var path above (the enclosing-class X cases).
|
@msridhar Thanks for the follow-up review — addressed on tip
|
There was a problem hiding this comment.
Thanks a lot! One more minor comment.
Also, you'll see that several of our integration tests, like for junit, fail with this change. Can you eyeball the new errors and check that they look like valid issues? You can see our CI config if you want to re-run the checks locally.
| // false-positive on a matching @Nullable override. Skip original bounds that are still type | ||
| // variables — those must be resolved via substitution (or the free type-var path above). | ||
| List<Symbol.TypeVariableSymbol> originalTypeParams = overriddenMethod.getTypeParameters(); | ||
| if (typeVarIndex >= 0 && typeVarIndex < originalTypeParams.size()) { |
There was a problem hiding this comment.
Is it possible for typeVarIndex to be out of bounds in a valid override? I don't think so?
| // Member-type substitution (asMemberOf) can strip type-use @Nullable from a concrete method | ||
| // type-variable bound while leaving the bound type itself (e.g. Object). Example that needs | ||
| // this fallback: | ||
| // interface Foo { <T extends @Nullable Object> void bar(T arg); } | ||
| // class Baz implements Foo { public <T extends @Nullable Object> void bar(T arg) {} } | ||
| // After substitution the bound may look like plain Object with no annotation mirrors; without | ||
| // consulting the original declaration we would treat the overridden bound as non-null and | ||
| // false-positive on a matching @Nullable override. Skip original bounds that are still type | ||
| // variables — those must be resolved via substitution (or the free type-var path above). |
…e bound In JSpecify mode, compare upper-bound nullability of corresponding method type variables between an overriding method and the method it overrides. Narrowing `<T extends @nullable Object>` to `<T>` (or the reverse) is unsound because callers can still instantiate the type variable via the overridden signature. Fixes uber#1512
…thods Read overridden method type-variable upper bounds from the method type after member-type substitution in the overriding class, so bounds that reference enclosing-class type variables compare correctly after instantiation (e.g. <T extends X> on Foo<@nullable Object>). Skip the check when the overridden method is from @NullUnmarked / unannotated code to avoid false positives from unmarked bounds. Add regressions for nullable and non-null enclosing-class instantiations, narrowing after substitution, and NullUnmarked overrides.
- Bail out in the caller when overridden type is not ForAll - Drop defensive typeParameterTrees size guard - Take Type.TypeVar in substituted bound helper - Document when original-declaration @nullable fallback applies
Valid overrides always have matching type-parameter counts; the caller already bails when counts differ, so typeVarIndex is always in range.
4e0871c to
585a1fb
Compare
|
Thanks for the follow-up!
Rebase: Rebased onto latest Integration test analysis: Re-ran the CI integration jobs locally (
No false positives from Tests (JDK 21):
Tip: |
3 participants
Summary
Fixes #1512.
In JSpecify mode, NullAway did not compare method type-variable upper-bound nullability between an overriding method and the method it overrides. That allowed unsound overrides such as:
Callers can still invoke the method via the super type with a
@Nullabletype argument (e.g.f.<@Nullable String>bar(null)), so treating the override's parameter as non-null is incorrect.This change, in
GenericsChecks.checkTypeParameterNullnessForMethodOverriding, compares upper-bound nullability of corresponding method type variables (usingGenericsUtils.upperBoundIsNullable) and reportsWRONG_OVERRIDE_PARAM_GENERICwhen they differ—whether the override narrows@Nullable→ non-null or widens non-null →@Nullable.Tests
overrideNarrowsNullableMethodTypeVariableBound— issue JSpecify: False negative when overriding narrows@Nullabletype variable bound #1512 repro (param position)overrideWidensNonNullMethodTypeVariableBound— reverse mismatchoverridePreservesNullableMethodTypeVariableBound/overridePreservesNonNullMethodTypeVariableBound— matching bounds remain legaloverrideNarrowsNullableMethodTypeVariableBoundOnReturn— return-only type variable(JDK 21)
AI disclosure
I used AI tools (Grok) to help draft the fix and tests. I reviewed all changes, ran the tests above, and understand the code.
@Nullabletype variable bound #1512Summary by CodeRabbit
Bug Fixes
Tests