Improve verification of !null -> !null contracts#1441
Conversation
WalkthroughAdds AccessPathNullnessAnalysis.hasBottomAccessPathForContractDataflow(TreePath, Context) to query the contract dataflow store for access paths mapped to Nullness.BOTTOM. Refactors ContractCheckHandler to collect nested return expressions (ParenthesizedTree and ConditionalExpressionTree), avoid entering lambdas and local/anonymous classes during contract scanning, and use access-path analysis to skip contract-violation reports when infeasible branches are indicated by bottom mappings. Adds six unit tests in ContractsTests covering !null->!null contracts, multi-level cases, ternaries, void-return handling, and nested lambda/anonymous-class returns. Replaces an assertion with a Guava Verify check in ContractNullnessStoreInitializer. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractCheckHandler.java`:
- Around line 145-158: The code assumes returnTree.getExpression() is non-null
which crashes for bare "return;" in void methods; add a defensive null check
after computing ExpressionTree returnExpression (from
returnTree.getExpression()) and skip the nullness checks (e.g., return null or
continue the handler) when returnExpression == null to avoid constructing a
TreePath for a null expression; update use-sites including returnExpressionPath,
getNullnessForContractDataflow(..., returnState.context) and
hasBottomAccessPathForContractDataflow(..., returnState.context) to only run
when returnExpression is non-null.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nullaway/src/test/java/com/uber/nullaway/ContractsTests.java`:
- Around line 752-767: In testPositive1 replace the null-branch call that uses
the possibly-null parameter with a non-null constant to avoid unrelated nullness
diagnostics: update the branch in the method testPositive1 (which is annotated
`@Contract`("!null -> !null")) so it does not call Integer.parseInt(text) when
text == null but instead returns a non-null constant (e.g., use
Integer.parseInt("1") or return Integer.valueOf(1)) so the test focuses only on
the contract violation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1441 +/- ##
============================================
+ Coverage 88.37% 88.40% +0.02%
- Complexity 2702 2711 +9
============================================
Files 99 99
Lines 8965 9004 +39
Branches 1790 1799 +9
============================================
+ Hits 7923 7960 +37
- Misses 516 517 +1
- Partials 526 527 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| private static String getErrorMessageForViolatedContract( |
There was a problem hiding this comment.
This is drive-by refactoring to extract this method from the one above which was getting huge. Sorry for the unrelated change.
| // this should only be possible with an invalid @Contract on a void-returning method; | ||
| // report an error on that in the future? |
There was a problem hiding this comment.
I wonder if this is a responsibility of another EP checker that checks such contract validity?
There was a problem hiding this comment.
Yeah, fair enough, I agree this should probably be out of bounds for NullAway. Will update the comment
Fixes #1440
Fixes #1104
It turns out we can relatively easily handle these cases by looking for
BOTTOMin theNullnessStorebefore the return expression (which means the path is infeasible). I do wonder if there are other places in our analyses we should be checking forBOTTOM, but we want to be careful since some users may expect issues to be reported in unreachable code. But for contract verification, it seems essential.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.