Support java.util.Objects.toString() #1283
Conversation
|
|
WalkthroughAdds a library model entry for java.util.Objects.toString(Object, String) indicating the return may be null if the second parameter is null. Introduces a test validating that a non-null default is safe and a null default triggers a diagnostic on dereference. Changes
Sequence Diagram(s)sequenceDiagram
participant DevCode as Developer Code
participant Compiler as Java Compiler
participant NullAway as NullAway Analyzer
participant LibModels as Library Models
participant Dataflow as Dataflow Engine
DevCode->>Compiler: Compile code using Objects.toString(obj, default)
Compiler->>NullAway: AST + symbols
NullAway->>LibModels: Query model for Objects.toString(Object, String)
activate LibModels
Note right of LibModels: New rule: return nullable if<br/>arg[1] (default) is null
LibModels-->>NullAway: Nullability constraints
deactivate LibModels
NullAway->>Dataflow: Run with constraints
alt default is non-null
Dataflow-->>NullAway: Return inferred non-null
NullAway-->>DevCode: No diagnostic on dereference
else default is null
Dataflow-->>NullAway: Return may be null
NullAway-->>DevCode: Report potential null dereference
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate 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. 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: 0
🧹 Nitpick comments (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
815-816: Behavior note: possible false positives when obj != null but defaultString == nullEncoding Objects.toString(Object, String) under NULL_IMPLIES_NULL_PARAMETERS on index 1 makes the return HINT_NULLABLE whenever the second arg may be null, even if the first arg is known non-null (e.g., inside if (o != null) { Objects.toString(o, null) }). That’s conservative and fixes #1282, but it can flag safe code.
If you want to avoid these FPs, consider a targeted disjunctive check for this method in onDataflowVisitMethodInvocation: FORCE_NONNULL when arg0 || arg1 is NONNULL; HINT_NULLABLE only when both may be null. Example minimal tweak:
@@ public NullnessHint onDataflowVisitMethodInvocation( - if (!nullImpliesNullIndexes.isEmpty()) { + if (!nullImpliesNullIndexes.isEmpty()) { + // Special-case: java.util.Objects.toString(Object, String) + if (isObjectsToString2Param(callee)) { + boolean objNonNull = inputs.valueOfSubNode(node.getArgument(0)).equals(NONNULL); + boolean defNonNull = inputs.valueOfSubNode(node.getArgument(1)).equals(NONNULL); + return (objNonNull || defNonNull) ? NullnessHint.FORCE_NONNULL : NullnessHint.HINT_NULLABLE; + } boolean anyNull = false; ...And add (outside this hunk) a small helper:
private static boolean isObjectsToString2Param(Symbol.MethodSymbol callee) { if (!callee.name.contentEquals("toString")) return false; Symbol.ClassSymbol owner = callee.enclClass(); if (owner == null || !"java.util.Objects".equals(owner.getQualifiedName().toString())) return false; if (callee.getParameters().size() != 2) return false; String p0 = callee.getParameters().get(0).type.toString(); String p1 = callee.getParameters().get(1).type.toString(); return "java.lang.Object".equals(p0) && "java.lang.String".equals(p1); }If the conservative model is intentional, all good—just calling out the trade-off.
nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java (1)
1157-1175: LGTM: test covers null-default path and non-null default pathThis validates the intended behavior and will catch regressions for #1282. As a follow-up (optional), consider adding a case inside an o != null branch with Objects.toString(o, null) to document current conservative behavior or to assert non-warning if you adopt the disjunctive modeling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java(1 hunks)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1283 +/- ##
=========================================
Coverage 88.42% 88.42%
Complexity 2451 2451
=========================================
Files 92 92
Lines 8087 8088 +1
Branches 1608 1608
=========================================
+ Hits 7151 7152 +1
Misses 472 472
Partials 464 464 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Add support for java.util.Objects.toString(). Closes #1282
When passing
nullas a nullDefault String, the Object is not guaranteed nonNull.Summary by CodeRabbit
Bug Fixes
Tests