Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle JDK 21 case operands in type refinement #928

Merged
merged 5 commits into from
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -206,4 +206,42 @@ public void testSwitchExprNullCase() {
"}")
.doTest();
}

@Test
public void testSwitchExprNullCaseDataflow() {
defaultCompilationHelper
.addSourceLines(
"SwitchNullCase.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"class SwitchNullCase {",
" public enum NullableEnum {",
" A,",
" B,",
" }",
" static Object testPositive1(@Nullable NullableEnum nullableEnum) {",
" return switch (nullableEnum) {",
" case A -> new Object();",
" // BUG: Diagnostic contains: dereferenced expression nullableEnum is @Nullable",
" case null -> nullableEnum.hashCode();",
" default -> nullableEnum.toString();",
" };",
" }",
" static Object testPositive2(@Nullable NullableEnum nullableEnum) {",
" return switch (nullableEnum) {",
" case A -> new Object();",
" // BUG: Diagnostic contains: dereferenced expression nullableEnum is @Nullable",
" case null, default -> nullableEnum.toString();",
" };",
" }",
" static Object testNegative(@Nullable NullableEnum nullableEnum) {",
" return switch (nullableEnum) {",
" case A, B -> nullableEnum.hashCode();",
" case null -> new Object();",
" default -> nullableEnum.toString();",
" };",
" }",
"}")
.doTest();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -405,54 +405,43 @@
@Override
public TransferResult<Nullness, NullnessStore> visitEqualTo(
EqualToNode equalToNode, TransferInput<Nullness, NullnessStore> input) {
ReadableUpdates thenUpdates = new ReadableUpdates();
ReadableUpdates elseUpdates = new ReadableUpdates();
handleEqualityComparison(
true,
equalToNode.getLeftOperand(),
equalToNode.getRightOperand(),
values(input),
thenUpdates,
elseUpdates);
ResultingStore thenStore = updateStore(input.getThenStore(), thenUpdates);
ResultingStore elseStore = updateStore(input.getElseStore(), elseUpdates);
return conditionalResult(
thenStore.store, elseStore.store, thenStore.storeChanged || elseStore.storeChanged);
return handleEqualityComparison(
input, equalToNode.getLeftOperand(), equalToNode.getRightOperand(), true);
}

@Override
public TransferResult<Nullness, NullnessStore> visitNotEqual(
NotEqualNode notEqualNode, TransferInput<Nullness, NullnessStore> input) {
ReadableUpdates thenUpdates = new ReadableUpdates();
ReadableUpdates elseUpdates = new ReadableUpdates();
handleEqualityComparison(
false,
notEqualNode.getLeftOperand(),
notEqualNode.getRightOperand(),
values(input),
thenUpdates,
elseUpdates);
ResultingStore thenStore = updateStore(input.getThenStore(), thenUpdates);
ResultingStore elseStore = updateStore(input.getElseStore(), elseUpdates);
return conditionalResult(
thenStore.store, elseStore.store, thenStore.storeChanged || elseStore.storeChanged);
return handleEqualityComparison(
input, notEqualNode.getLeftOperand(), notEqualNode.getRightOperand(), false);
}

private void handleEqualityComparison(
boolean equalTo,
Node leftNode,
Node rightNode,
SubNodeValues inputs,
Updates thenUpdates,
Updates elseUpdates) {
Nullness leftVal = inputs.valueOfSubNode(leftNode);
Nullness rightVal = inputs.valueOfSubNode(rightNode);
/**
* Handle nullability refinements from an equality comparison.
*
* @param input transfer input for the operation
* @param leftOperand left operand of the comparison
* @param rightOperand right operand of the comparison
* @param equalTo if {@code true}, the comparison is an equality comparison, otherwise it is a
* dis-equality ({@code !=}) comparison
* @return a TransferResult reflecting any updates from the comparison
*/
private TransferResult<Nullness, NullnessStore> handleEqualityComparison(
TransferInput<Nullness, NullnessStore> input,
Node leftOperand,
Node rightOperand,
boolean equalTo) {
ReadableUpdates thenUpdates = new ReadableUpdates();
ReadableUpdates elseUpdates = new ReadableUpdates();
SubNodeValues inputs = values(input);
Nullness leftVal = inputs.valueOfSubNode(leftOperand);
Nullness rightVal = inputs.valueOfSubNode(rightOperand);
Nullness equalBranchValue = leftVal.greatestLowerBound(rightVal);
Updates equalBranchUpdates = equalTo ? thenUpdates : elseUpdates;
Updates notEqualBranchUpdates = equalTo ? elseUpdates : thenUpdates;

Node realLeftNode = unwrapAssignExpr(leftNode);
Node realRightNode = unwrapAssignExpr(rightNode);
Node realLeftNode = unwrapAssignExpr(leftOperand);
Node realRightNode = unwrapAssignExpr(rightOperand);

AccessPath leftAP = AccessPath.getAccessPathForNode(realLeftNode, state, apContext);
if (leftAP != null) {
Expand All @@ -467,6 +456,10 @@
notEqualBranchUpdates.set(
rightAP, rightVal.greatestLowerBound(leftVal.deducedValueWhenNotEqual()));
}
ResultingStore thenStore = updateStore(input.getThenStore(), thenUpdates);
ResultingStore elseStore = updateStore(input.getElseStore(), elseUpdates);
return conditionalResult(
thenStore.store, elseStore.store, thenStore.storeChanged || elseStore.storeChanged);
}

@Override
Expand Down Expand Up @@ -932,7 +925,18 @@
@Override
public TransferResult<Nullness, NullnessStore> visitCase(
CaseNode caseNode, TransferInput<Nullness, NullnessStore> input) {
return noStoreChanges(NULLABLE, input);
List<Node> caseOperands = caseNode.getCaseOperands();
if (caseOperands.isEmpty()) {
return noStoreChanges(NULLABLE, input);

Check warning on line 930 in nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java#L930

Added line #L930 was not covered by tests
} else {
// `null` can only appear on its own as a case operand, or together with the default case
// (i.e., `case null, default:`). So, it is safe to only look at the first case operand, and
// update the stores based on that. We treat the case operation as an equality comparison
// between the switch expression and the case operand.
Node switchOperand = caseNode.getSwitchOperand().getExpression();
Node caseOperand = caseOperands.get(0);
return handleEqualityComparison(input, switchOperand, caseOperand, true);
}
}

@Override
Expand Down