issue-1250 pattern matching for instanceof in switch case#1259
issue-1250 pattern matching for instanceof in switch case#1259msridhar merged 18 commits intouber:masterfrom
Conversation
WalkthroughAdds type-pattern binding handling to switch-case analysis in AccessPathNullnessPropagation: when a case label binds a pattern variable, the code resolves the bound LocalVariableNode and marks it NONNULL in the matching (then) branch. Also adds two JDK17 tests exercising pattern-match and switch behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DF as DataflowAnalyzer
participant CN as CaseNode
participant CT as CaseTree
participant VN as LocalVariableNode
participant Store as Dataflow Store
DF->>CN: visitCase(caseNode)
DF->>CT: inspect case label for type-pattern binding
alt type-pattern binding present
CT-->>DF: binding identifier
DF->>VN: resolve binding -> LocalVariableNode
DF->>Store: create then-store (VN = NONNULL)
DF->>Store: create else-store (unchanged)
DF-->>CN: return conditional result with then/else stores
else no type-pattern binding
DF->>CN: fallback to existing operand/equality handling
DF-->>CN: return previous-style result
end
note right of Store: Only the matching (then) branch gets NONNULL fact
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
📜 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)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-14T18:50:06.159ZApplied to files:
📚 Learning: 2025-08-29T18:41:43.566ZApplied to files:
🔇 Additional comments (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 (
|
msridhar
left a comment
There was a problem hiding this comment.
Looks promising! A few comments.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1)
2576-2595: Indent test source lines for readabilityPlease indent the code inside these tests. This was requested previously.
Also applies to: 2606-2623
🧹 Nitpick comments (2)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1)
2591-2591: Exercise the bound pattern variable in the case bodyReturn
e.getMessage()to assert the pattern-bound var is treated NONNULL in the matching branch.Apply:
- "case BusinessExceptionUnauthenticated e -> \"\"; ", + "case BusinessExceptionUnauthenticated e -> e.getMessage();",nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (1)
1010-1011: Javadoc nit: remove stray paragraph markerThe isolated
<p>adds no content.Apply:
- * <p>
📜 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/dataflow/AccessPathNullnessPropagation.java(3 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-28T04:54:20.922Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.922Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java
📚 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/GenericsTests.java
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java (1)
AccessPath(78-797)
🔇 Additional comments (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (1)
33-38: Imports LGTMNew imports are appropriate for pattern-case handling and used below.
Also applies to: 137-137
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java (1)
364-369: Remove stray semicolon after method for consistency.Harmless, but inconsistent with surrounding style.
- }; + }
📜 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 (1)
jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1259 +/- ##
============================================
- Coverage 88.16% 88.12% -0.05%
- Complexity 2422 2426 +4
============================================
Files 92 92
Lines 8010 8033 +23
Branches 1594 1600 +6
============================================
+ Hits 7062 7079 +17
- Misses 492 494 +2
- Partials 456 460 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
left a comment
There was a problem hiding this comment.
Looking better! A few more comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (2)
976-987: Mark scrutinee non-null in matching type-pattern
In visitCase (nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java), afterthenUpdates.set(localVariableNode, NONNULL);add:@@ –982,6 +982,8 // Pattern variable is non-null in the matching branch thenUpdates.set(localVariableNode, NONNULL); + // Scrutinee is non-null when the type-pattern matches + setNonnullIfAnalyzeable(thenUpdates, switchOperand);Add a SwitchTests.java case that dereferences the switch expression inside a matching pattern branch to verify this behavior.
1015-1034: Guard against null from BindingPatternUtils and drop brittle kind-name checks.Rely on
BindingPatternUtils.getVariable(label); if it returns null, skip. This removes the string-kind dependency and avoids potential NPEs.- for (Tree label : labels) { - String kindName = label.getKind().name(); - if (kindName.equals("BINDING_PATTERN") || kindName.equals("PATTERN_CASE_LABEL")) { - VariableTree varTree = TreeUtilsAfterJava11.BindingPatternUtils.getVariable(label); - for (Node operand : caseNode.getCaseOperands()) { - Symbol operandSymbol = ASTHelpers.getSymbol(operand.getTree()); - Symbol varSymbol = ASTHelpers.getSymbol(varTree); - if (operand instanceof LocalVariableNode) { - if (operandSymbol != null && operandSymbol.equals(varSymbol)) { - return (LocalVariableNode) operand; - } - } - } - throw new RuntimeException("Unexpectedly did not find the pattern variable"); - } - } + for (Tree label : labels) { + VariableTree varTree = TreeUtilsAfterJava11.BindingPatternUtils.getVariable(label); + if (varTree == null) { + continue; + } + Symbol varSymbol = ASTHelpers.getSymbol(varTree); + for (Node operand : caseNode.getCaseOperands()) { + if (operand instanceof LocalVariableNode) { + Symbol operandSymbol = ASTHelpers.getSymbol(operand.getTree()); + if (operandSymbol != null && operandSymbol.equals(varSymbol)) { + return (LocalVariableNode) operand; + } + } + } + throw new RuntimeException( + "Unexpectedly did not find the pattern variable for label: " + + state.getSourceForNode(label)); + }
🧹 Nitpick comments (2)
jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java (1)
364-369: Stray semicolon after method end.Harmless but noisy; consider removing for clarity.
- }; + }nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (1)
989-997: Minor: avoid computing caseOperand when unused.Move
caseOperandacquisition into the non-pattern path.- Node caseOperand = caseOperands.get(0); LocalVariableNode localVariableNode = getVariableNodeForTypePattern(caseNode); if (localVariableNode != null) { ... } else { + Node caseOperand = caseOperands.get(0); AccessPath switchOperandAccessPath =
📜 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)
jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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:
jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java
📚 Learning: 2025-08-29T18:41:43.566Z
Learnt from: msridhar
PR: uber/NullAway#1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.566Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java
📚 Learning: 2025-08-28T04:54:20.922Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.922Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java (1)
AccessPath(78-797)
🔇 Additional comments (5)
jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java (3)
318-321: Good use of @NullMarked to force analysis.This ensures NullAway analyzes these helper classes without needing a com.uber package.
337-339: Dereferencing the bound variable is great.
e.toString()proves NONNULL for the pattern variable in the matching branch.
395-399: Covers else-branch propagation.This case verifies that subsequent labels (default) keep prior nullness facts for the scrutinee.
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (2)
33-38: Imports for pattern-case support look correct.Also applies to: 137-137
984-987: Else-branch store propagation matches objective.No unintended nullness changes leak into following cases.
msridhar
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix!
Fixes #1250
The goal is to make visitCase aware of pattern variables. When it encounters a case with a pattern (like case BusinessExceptionUnauthenticated e), it should:
causing the bug right now - the information is being corrupted for the following default case.
Summary by CodeRabbit