Suppress CastToNonNull warnings for @NullUnmarked method calls#1258
Suppress CastToNonNull warnings for @NullUnmarked method calls#1258msridhar merged 3 commits intouber:masterfrom
CastToNonNull warnings for @NullUnmarked method calls#1258Conversation
WalkthroughAdds a private helper to detect direct calls to unannotated methods and uses it to suppress CastToNonNull warnings for such calls. Removes one CastToNonNull diagnostic expectation in a Thrift test and adds multiple jspecify tests (duplicated blocks) covering @NullUnmarked package/class/method and related cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Compiler as Compilation
participant NA as NullAway
participant Check as checkCastToNonNullTakesNullable
participant Helper as isCallToUnmarkedMethod
participant CA as CodeAnnotationInfo
Compiler->>NA: analyze cast-to-NonNull call
NA->>Check: checkCastToNonNullTakesNullable(actual)
Check->>Helper: isCallToUnmarkedMethod(actual)
Helper->>CA: isSymbolUnannotated(methodSymbol)?
CA-->>Helper: true/false
Helper-->>Check: result
alt direct unannotated call
Check-->>NA: suppress CastToNonNull warning
else
Check-->>NA: proceed with existing checks (may report warning)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
Don't warn when passing return values from @NullUnmarked methods to castToNonNull/assumeNonNull methods. This supports incremental null safety migrations by allowing developers to use assumeNonNull on unmarked code without spurious warnings.
60a57e2 to
88e0c31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (2)
324-346: Broaden detection: strip parens/casts and use base symbol for invocation lookupThe helper does the job for direct calls, but two small tweaks will make it robust and consistent with the rest of this class:
- Strip surrounding parentheses and casts before the instanceof check.
- Use getSymbolForMethodInvocation(...) (baseSymbol) instead of ASTHelpers.getSymbol(...) to correctly handle static imports and JDK 18’s implicit Object methods.
Also consider clarifying “direct” in the method name or Javadoc.
Apply:
- private boolean isCallToUnmarkedMethod(ExpressionTree expr) { - if (!(expr instanceof MethodInvocationTree)) { + private boolean isCallToUnmarkedMethod(ExpressionTree expr) { + expr = stripParensAndCasts(expr); + if (!(expr instanceof MethodInvocationTree)) { return false; } - MethodInvocationTree methodInvoke = (MethodInvocationTree) expr; - Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvoke); + MethodInvocationTree methodInvoke = (MethodInvocationTree) expr; + Symbol.MethodSymbol methodSymbol = getSymbolForMethodInvocation(methodInvoke); if (methodSymbol == null) { return false; } return codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config, handler); }
2082-2101: Minor perf: check isCallToUnmarkedMethod before mayBeNullExprWhen the argument is a call to @NullUnmarked code, we can skip the (potentially) more expensive dataflow in mayBeNullExpr. Flip the checks for a tiny win in hot code paths.
- if (!isInitializer && !mayBeNullExpr(state, actual) && !isCallToUnmarkedMethod(actual)) { + if (!isInitializer && !isCallToUnmarkedMethod(actual) && !mayBeNullExpr(state, actual)) { String message = "passing known @NonNull parameter '" + state.getSourceForNode(actual) + "' to CastToNonNullMethod (" + qualifiedName + ") at position " + castToNonNullPosition + ". This method argument should only take values that NullAway considers @Nullable " + "at the invocation site, but which are known not to be null at runtime."; return errorBuilder.createErrorDescription( new ErrorMessage(MessageTypes.CAST_TO_NONNULL_ARG_NONNULL, message), // The Tree passed as suggestTree is the expression being cast // to avoid recomputing the arg index: actual, buildDescription(tree), state, null); }nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java (3)
1288-1326: Good coverage for package-level @NullUnmarked; add a “non-immediate wrapping” negative caseThese assertions validate the intended suppression when castToNonNull immediately wraps a return from unmarked code. Add a companion test to ensure we still warn if the unmarked value is first stored in a local (i.e., not immediately wrapped).
Suggested addition:
+ @Test + public void castToNonNullWithNullUnmarkedPackage_localVariableStillWarns() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:CastToNonNullMethod=com.uber.nullaway.testdata.Util.castToNonNull")) + .addSourceLines( + "package-info.java", + "@NullUnmarked package com.example.thirdparty;", + "import org.jspecify.annotations.NullUnmarked;") + .addSourceLines( + "ThirdPartyUtils.java", + "package com.example.thirdparty;", + "public class ThirdPartyUtils {", + " public static String getStringValue() {", + " return \"some value\";", + " }", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "import com.example.thirdparty.ThirdPartyUtils;", + "public class Test {", + " public static void test() {", + " String s = ThirdPartyUtils.getStringValue();", + " // Now not an immediate wrapper around the unmarked call; should warn:", + " // BUG: Diagnostic contains: passing known @NonNull parameter", + " String val = castToNonNull(s);", + " }", + "}") + .doTest(); + }
1330-1359: Also test parentheses/casts around the unmarked callThe production code now strips parens/casts (or will, with the suggested refactor). Add a test to lock this in so we don’t regress.
+ @Test + public void castToNonNullWithNullUnmarkedClass_withParensAndCast() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:CastToNonNullMethod=com.uber.nullaway.testdata.Util.castToNonNull")) + .addSourceLines( + "ThirdPartyUtils.java", + "package com.uber;", + "import org.jspecify.annotations.NullUnmarked;", + "@NullUnmarked", + "public class ThirdPartyUtils {", + " public static String getStringValue() {", + " return \"x\";", + " }", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "public class Test {", + " public static void test() {", + " String a = castToNonNull((ThirdPartyUtils.getStringValue()));", + " String b = castToNonNull((String) (ThirdPartyUtils.getStringValue()));", + " System.out.println(a + b);", + " }", + "}") + .doTest(); + }
1361-1392: Add a chained-call negative case to ensure we only suppress immediate wrappersTo prevent over-suppression, add a test where the unmarked return is first transformed by a marked method (e.g., String.trim()) before the cast. This should still warn.
+ @Test + public void castToNonNullWithNullUnmarkedMethod_chainedCallShouldWarn() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:CastToNonNullMethod=com.uber.nullaway.testdata.Util.castToNonNull")) + .addSourceLines( + "ThirdPartyUtils.java", + "package com.uber;", + "import org.jspecify.annotations.NullUnmarked;", + "public class ThirdPartyUtils {", + " @NullUnmarked", + " public static String getStringValue() {", + " return \"x\";", + " }", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "public class Test {", + " public static void test() {", + " // BUG: Diagnostic contains: passing known @NonNull parameter", + " String s = castToNonNull(ThirdPartyUtils.getStringValue().trim());", + " }", + "}") + .doTest(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/NullAway.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/ThriftTests.java(0 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java(1 hunks)
💤 Files with no reviewable changes (1)
- nullaway/src/test/java/com/uber/nullaway/ThriftTests.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1258 +/- ##
============================================
- Coverage 88.19% 88.17% -0.02%
- Complexity 2424 2427 +3
============================================
Files 92 92
Lines 8002 8010 +8
Branches 1593 1595 +2
============================================
+ Hits 7057 7063 +6
- Misses 491 492 +1
- Partials 454 455 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
left a comment
There was a problem hiding this comment.
Thanks a lot @raccoonback!! Overall this looks great. I think the nitpick comments from @coderabbitai in this comment are worth addressing, except for the last comment asking for a test case involving trim() (I don't get that one). Could you look at those and then I will approve this?
|
@msridhar I will check it! |
|
@msridhar |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java (3)
1397-1427: Great: covers ParenthesizedTree and TypeCastTree wrappers. Consider a negative case for conditionals.You’re already guarding against trivial wrappers. One more high-signal negative would ensure we don’t suppress on non-trivial wrappers like conditionals (argument expression is not “a call” anymore):
Add a test like the following (new method), expecting a diagnostic on the conditional:
@Test public void castToNonNullWithConditionalFromNullUnmarkedDoesWarn() { makeTestHelperWithArgs( Arrays.asList( "-d", temporaryFolder.getRoot().getAbsolutePath(), "-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:CastToNonNullMethod=com.uber.nullaway.testdata.Util.castToNonNull")) .addSourceLines( "ThirdPartyUtils.java", "package com.uber;", "import org.jspecify.annotations.NullUnmarked;", "@NullUnmarked", "public class ThirdPartyUtils {", " public static String a() { return \"a\"; }", " public static String b() { return \"b\"; }", "}") .addSourceLines( "Test.java", "package com.uber;", "import static com.uber.nullaway.testdata.Util.castToNonNull;", "public class Test {", " public static void test(boolean flag) {", " // BUG: Diagnostic contains: CastToNonNullMethod", " String s = castToNonNull(flag ? ThirdPartyUtils.a() : ThirdPartyUtils.b());", " }", "}") .doTest(); }
1287-1460: Add parallel coverage for assumeNonNull and an explicit-@Nullable-in-unmarked negative.The PR objective mentions both assumeNonNull and castToNonNull. These tests only target castToNonNull. To guard against regressions and clarify semantics, please add:
- Parallel tests for assumeNonNull (same five scenarios). Example for the package-level case:
@Test public void assumeNonNullWithNullUnmarkedPackage() { makeTestHelperWithArgs( Arrays.asList( "-d", temporaryFolder.getRoot().getAbsolutePath(), "-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:AssumeNonNullMethod=com.uber.nullaway.testdata.Util.assumeNonNull")) .addSourceLines( "package-info.java", "@NullUnmarked package com.example.thirdparty;", "import org.jspecify.annotations.NullUnmarked;") .addSourceLines( "ThirdPartyUtils.java", "package com.example.thirdparty;", "public class ThirdPartyUtils {", " public static String getStringValue() { return \"v\"; }", "}") .addSourceLines( "Test.java", "package com.uber;", "import static com.uber.nullaway.testdata.Util.assumeNonNull;", "import com.example.thirdparty.ThirdPartyUtils;", "public class Test {", " public static void test() {", " String val = assumeNonNull(ThirdPartyUtils.getStringValue());", " System.out.println(val);", " }", "}") .doTest(); }
- A negative to ensure we still warn when the unmarked method explicitly returns @nullable (i.e., we do not suppress in that case). Example:
@Test public void castToNonNullWithExplicitNullableFromUnmarkedWarns() { makeTestHelperWithArgs( Arrays.asList( "-d", temporaryFolder.getRoot().getAbsolutePath(), "-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:CastToNonNullMethod=com.uber.nullaway.testdata.Util.castToNonNull")) .addSourceLines( "ThirdPartyUtils.java", "package com.uber;", "import org.jspecify.annotations.NullUnmarked;", "import org.jspecify.annotations.Nullable;", "@NullUnmarked", "public class ThirdPartyUtils {", " public static @Nullable String maybeNull() { return null; }", "}") .addSourceLines( "Test.java", "package com.uber;", "import static com.uber.nullaway.testdata.Util.castToNonNull;", "public class Test {", " public static void test() {", " // BUG: Diagnostic contains: passing @Nullable parameter", " String s = castToNonNull(ThirdPartyUtils.maybeNull());", " }", "}") .doTest(); }These additions tighten the spec coverage without inflating complexity.
If you’d like, I can push a follow-up commit adding these tests in the same style as the current ones.
1328-1362: Strengthen CastToNonNull warning assertionThe test in
NullMarkednessTests.java(lines 1328–1362) currently checks only for the substring// BUG: Diagnostic contains: passing known @NonNull parameterbut the real message built in
NullAway.java(around line 2085) is:"passing known @NonNull parameter '" + state.getSourceForNode(actual) + "' to CastToNonNullMethod (" + qualifiedName + ") at position "To reduce brittleness without weakening the check, add a second
containsassertion for the configured method name. For example:@@ Test.java (in NullMarkednessTests.java) - // BUG: Diagnostic contains: passing known @NonNull parameter + // BUG: Diagnostic contains: passing known @NonNull parameter + // BUG: Diagnostic contains: Util.castToNonNullIf you’d rather match the fully-qualified name:
+ // BUG: Diagnostic contains: com.uber.nullaway.testdata.Util.castToNonNullThis ensures the test still fails if the message wording shifts, while confirming we’re warning on the right method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nullaway/src/main/java/com/uber/nullaway/NullAway.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java
🔇 Additional comments (3)
nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java (3)
1287-1326: Good positive coverage for direct wrapping of @NullUnmarked package returns.This validates the intended suppression path for immediate castToNonNull on values returned from @NullUnmarked code. Scenarios with both definitely-non-null and maybe-null flows are exercised. Looks good.
1364-1395: Class-level @NullUnmarked: direct wrapper suppression behaves as intended.This complements the package-level case and ensures the helper recognizes class-level marking. No issues spotted.
1429-1460: Method-level @NullUnmarked: direct wrapper suppression covered.This completes the trio (package/class/method) and aligns with the stated behavior. Good.
msridhar
left a comment
There was a problem hiding this comment.
Looks great! Thanks again @raccoonback!
Description
Currently, NullAway emits warnings when developers pass the return values of
@NullUnmarkedmethods toassumeNonNullorcastToNonNull.However, in incremental null-safety migration scenarios, these warnings are unnecessary and may create noise for developers.
This change ensures that no warnings are reported in such cases, allowing developers to use
assumeNonNullandcastToNonNullfreely with unmarked code.Key Changes
Related issue
Summary by CodeRabbit
Bug Fixes
Tests