-
Notifications
You must be signed in to change notification settings - Fork 331
Support java.util.Objects.toString()#1283
Conversation
Add support for java.util.Objects.toString(). Closes #1282
When passing null as a nullDefault String, the Object is not guaranteed nonNull.
Summary by CodeRabbit
-
Bug Fixes
- Improved nullability analysis for java.util.Objects.toString(Object, String). The return value is now considered nullable when the default string argument may be null, preventing unsafe dereferences. Calls with a non-null default string remain treated as non-null.
-
Tests
- Added a test validating the updated behavior for Objects.toString with both non-null and null default string arguments.
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 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 effort2 (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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
msridhar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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:
|