Enable StatementSwitchToExpressionSwitch check and fix all warnings#1413
Enable StatementSwitchToExpressionSwitch check and fix all warnings#1413
Conversation
WalkthroughRefactors many traditional switch statements into Java 14+ switch expressions with arrow syntax across nullaway and jar-infer modules, adds a LOCAL_VARIABLE case in symbol location mapping, replaces a string-based RECORD kind check with ElementKind.RECORD, and re-enables the StatementSwitchToExpressionSwitch Error Prone check in build.gradle. No public/exported signatures were changed. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nullaway/src/main/java/com/uber/nullaway/fixserialization/XMLUtil.java (1)
170-187: Error message doesn’t match supported types inDefaultXMLValueProviderThe switch supports
"Integer","Boolean", and"String, but the exception text says “only Double|Boolean|String accepted,” which is misleading.Consider aligning the message with the actual cases (or adding
Doublesupport if that was intended). For example:Proposed message-only fix
- + ", only Double|Boolean|String accepted."); + + ", only Integer|Boolean|String accepted.");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
build.gradlejar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.javanullaway/src/main/java/com/uber/nullaway/ErrorBuilder.javanullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/NullabilityUtil.javanullaway/src/main/java/com/uber/nullaway/Nullness.javanullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.javanullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.javanullaway/src/main/java/com/uber/nullaway/fixserialization/XMLUtil.javanullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationAdapter.javanullaway/src/main/java/com/uber/nullaway/fixserialization/location/SymbolLocation.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
💤 Files with no reviewable changes (1)
- build.gradle
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
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.
Learnt from: msridhar
Repo: uber/NullAway PR: 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-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
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/generics/GenericsChecks.javanullaway/src/main/java/com/uber/nullaway/NullabilityUtil.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/Nullness.javanullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.javanullaway/src/main/java/com/uber/nullaway/NullAway.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 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/main/java/com/uber/nullaway/NullabilityUtil.javanullaway/src/main/java/com/uber/nullaway/Nullness.javanullaway/src/main/java/com/uber/nullaway/NullAway.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/ErrorBuilder.javanullaway/src/main/java/com/uber/nullaway/Nullness.java
📚 Learning: 2025-09-03T19:40:11.118Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1276
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java:235-241
Timestamp: 2025-09-03T19:40:11.118Z
Learning: In JDK 21+, ListBuffer can be passed directly to TypeMetadata.Annotations constructor without needing conversion to List via toList() method. The APIs were updated to make this compatible.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/Nullness.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
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:
nullaway/src/main/java/com/uber/nullaway/NullAway.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build spring-framework with snapshot
🔇 Additional comments (17)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
215-222: Switch expression fortypeArgumentsis tight and correctUsing
switch (tree.getKind())with explicitMETHOD_INVOCATION/NEW_CLASSbranches and a failing default matches the method’s contract and keeps bad call sites visible at runtime. This refactor is behavior‑preserving and clearer than the previous branching.jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java (1)
674-680: Wildcard switch expression is clear and equivalentThe new switch expression for
sourceLevelWildcardTypecleanly preserves the*/+/-handling with an explicit failing default. No behavioral concerns.nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java (1)
188-196:serializeSymbolswitch expression keeps existing behaviorThe enhanced switch cleanly captures the previous branching: fields/parameters by simple name, methods/constructors via the adapter, everything else via
flatName(). The sharedMethodSymbolcast for METHOD and CONSTRUCTOR is appropriate.nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (1)
168-193: Worklist propagation switch is safely refactoredThe arrow-style switch on
st.nullnesspreserves the NONNULL/NULLABLE propagation logic and turns the previously implicit “should be unreachable” UNKNOWN path into an explicit failure. Given the worklist is only seeded for non‑UNKNOWN states, this is a sound and slightly safer refactor.nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (2)
383-421:targetTypeMatchesarrow switch + RECORD support look correctThe enhanced switch keeps the existing behavior for parameters, locals, fields, methods, etc., and explicitly groups
CLASS,ENUM, andRECORDas “no top-level type-use annotations,” returningfalse. The AssertionError default ensures any unexpectedElementKindis caught early. This is a clean, behavior‑preserving modernization with improved RECORD support.
534-540:nullnessToBoolswitch matches documented semanticsMapping
NULLandNULLABLEtotrueandBOTTOM/NONNULLtofalsein a switch expression exactly matches the method’s contract. The default AssertionError is a good guard against futureNullnessenum extensions.nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (1)
739-756:visitFieldAccessNullnessHint handling is preserved and clearerThe switch expression on
handler.onDataflowVisitFieldAccess(...)preserves the HINT_NULLABLE / FORCE_NONNULL / UNKNOWN mapping, with UNKNOWN still delegating tomayBeNullFieldFromType. The default case throws, which is appropriate as a safeguard ifNullnessHintever gains new values.nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationAdapter.java (1)
81-95: Version-to-adapter switch expression is straightforwardThe new switch expression keeps the same mapping (1→v1 adapter, 3→v3 adapter, 2 and others throwing targeted RuntimeExceptions) and uses
LATEST_VERSIONin the error text. This is a clean modernization with unchanged behavior.nullaway/src/main/java/com/uber/nullaway/NullAway.java (4)
349-354: LGTM: Clean switch expression conversion.The conversion from traditional switch to switch expression is straightforward and preserves the original logic correctly.
656-680: LGTM: Correct switch expression conversion with block bodies.The conversion properly uses blocks for each case that contains multiple statements, maintaining the original logic.
2457-2503: LGTM: Switch expression conversion for entity collection.The conversion correctly uses arrow syntax with blocks for each case, and the default case properly throws an exception for unexpected tree kinds.
2577-2688: LGTM: Switch expression conversion with extensive case grouping.The conversion consolidates a large number of expression kinds that all return
false(clearly non-nullable) into a single arrow case (lines 2582-2632). This is a significant readability improvement. The second switch expression (lines 2638-2688) correctly handles specific expression types that require nullness analysis.nullaway/src/main/java/com/uber/nullaway/Nullness.java (2)
135-142: LGTM: Clean switch expression conversion.The conversion to a switch expression with explicit returns for each
Nullnessvalue is straightforward and correct.
26-26: LGTM: Improved type checking using ElementKind enum.The change from string-based comparison (
symbol.owner.toString().equals("RECORD")) to enum-based comparison (symbol.owner.getKind().equals(ElementKind.RECORD)) is more type-safe and robust. This is a good improvement beyond just the switch expression conversion.Also applies to: 259-259
nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java (2)
194-223: LGTM: Switch expression with appropriate case grouping.The conversion groups message types with identical handling (DEREFERENCE_NULLABLE, RETURN_NULLABLE, etc.) into a single arrow case, improving readability. Individual cases for specific message types are handled correctly.
332-349: LGTM: Switch expression conversion with varied return logic.The conversion correctly handles different return logic for each case:
NULL_LITERALalways returnsfalse,IDENTIFIERhas conditional logic for parameters, and the default returnstrue.nullaway/src/main/java/com/uber/nullaway/fixserialization/location/SymbolLocation.java (1)
58-78: LGTM: Switch expression conversion with functional enhancement.Beyond the switch expression conversion, this change adds support for
LOCAL_VARIABLEsymbols (line 75), which was previously missing. Note that the comment (lines 62-74) indicates that local variables declared inside lambda expressions are still not fully handled and will require future changes toLocalVariableLocation.This is a functional enhancement, not just a syntax refactor, but it appropriately handles a previously unhandled case.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.