Propagate nullness facts for CastToNonNullMethod - #1630
Conversation
Walkthrough
Possibly related PRs
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: 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/handlers/LibraryModelsHandler.java`:
- Around line 406-408: Update setUnconditionalArgumentNullness to retrieve
config.getCastToNonNullMethod() first and use short-circuit checks before
constructing the qualifiedName string. Only concatenate
ASTHelpers.enclosingClass(callee) with callee.getSimpleName() when the
configured cast-to-non-null method is present and the one-parameter condition
can match, preserving the existing isCliCastToNonNull behavior.
🪄 Autofix (Beta)
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
Run ID: 826ea860-36b5-4be7-aa26-9fffb322e12c
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/test/java/com/uber/nullaway/CoreTests.java
d1297f1 to
fcfb69f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
406-408: 🧹 Nitpick | 🔵 TrivialAvoid unconditional string allocations on the hot path.
Because
setUnconditionalArgumentNullnessis called on every method invocation, computing thequalifiedNamevia string concatenation unconditionally creates unnecessary allocation overhead during compilation.You can avoid this overhead for the vast majority of cases (especially when the CLI property isn't configured) by extracting
config.getCastToNonNullMethod()and leveraging short-circuit evaluation.⚡ Proposed performance optimization
- String qualifiedName = ASTHelpers.enclosingClass(callee) + "." + callee.getSimpleName(); - boolean isCliCastToNonNull = - qualifiedName.equals(config.getCastToNonNullMethod()) && callee.getParameters().size() == 1; + String cliCastToNonNull = config.getCastToNonNullMethod(); + boolean isCliCastToNonNull = + cliCastToNonNull != null + && callee.getParameters().size() == 1 + && cliCastToNonNull.equals( + ASTHelpers.enclosingClass(callee) + "." + callee.getSimpleName());🤖 Prompt for 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. In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java` around lines 406 - 408, Update setUnconditionalArgumentNullness to retrieve config.getCastToNonNullMethod() first and use short-circuit checks before constructing the qualified method name. Only concatenate ASTHelpers.enclosingClass(callee) with callee.getSimpleName() when the configured cast-to-non-null method is present and the parameter-count condition can still match, preserving the existing isCliCastToNonNull behavior.
🤖 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.
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 406-408: Update setUnconditionalArgumentNullness to retrieve
config.getCastToNonNullMethod() first and use short-circuit checks before
constructing the qualified method name. Only concatenate
ASTHelpers.enclosingClass(callee) with callee.getSimpleName() when the
configured cast-to-non-null method is present and the parameter-count condition
can still match, preserving the existing isCliCastToNonNull behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93acfbfd-6b5d-44f1-b58c-cd534a1992bf
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/test/java/com/uber/nullaway/CoreTests.java
|
@Shankar-v27 please see our new policy on AI usage https://github.com/uber/NullAway/blob/master/CONTRIBUTING.md#ai-usage Does this PR adhere to the policy? Please update the description if needed. |
I have updated the PR description to include the AI usage disclosure and to reflect the final implementation. Could you please take another look when you have a chance? Thank you! |
|
Hi @msridhar, I updated the PR description to comply with the new AI usage policy and verified the implementation and tests. When you have some time, could you please take another look? I'd appreciate any further feedback. Thank you! |
|
I think the general approach here is reasonable. But, I'm still not sure it's worth the additional complexity to add this feature. Also, we have some higher-priority features / issues to deal with right now. So, I'm going to hold off on fully reviewing and merging this. Sorry for the delay @Shankar-v27 |
2 participants
Fixes #1628.
Summary
Objects.requireNonNull(...)propagates non-null facts during dataflow analysis because it is guaranteed to throw when passednull. Arbitrary methods configured via-XepOpt:NullAway:CastToNonNullMethoddo not necessarily have that guarantee, so propagating non-null facts by default would be unsound.This PR introduces a new opt-in configuration option:
-XepOpt:NullAway:CastToNonNullMethodFailsOnNull=trueWhen enabled, configured
CastToNonNullMethods are treated as fail-fast during dataflow analysis and propagate non-null facts similarly toObjects.requireNonNull(...). The default behavior remains unchanged, preserving backward compatibility and soundness.Changes
CastToNonNullMethodFailsOnNullconfiguration option.CastToNonNullMethods only when the option is enabled.failIfNullParametersand the defaultCastToNonNullMethodbehavior.false,true,Objects.requireNonNull(...)behavior.Validation
:nullaway:test.spotlessCheck.AI Usage
AI assistance was used during the development of this contribution to:
I personally implemented, reviewed, and validated all code changes. I inspected the relevant source files, made the implementation decisions, ran the project's test suite locally, and verified the final behavior. I understand the implementation and can explain the reasoning behind each change.
Summary by CodeRabbit
castToNonNull(...)is treated as throwing on null inputs.false, and enabledtruecases, plus verification thatObjects.requireNonNull(...)behavior remains unchanged.