diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java index ca59147de..074ed93d4 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java @@ -22,7 +22,7 @@ public enum Safety implements AbstractValue { UNKNOWN() { @Override public Safety leastUpperBound(Safety other) { - return other == SAFE ? this : other; + return other == SAFE ? this : nullToUnknown(other); } @Override @@ -53,18 +53,18 @@ public Safety leastUpperBound(Safety other) { public boolean allowsValueWith(Safety valueSafety) { // We allow safe data to be provided to an unsafe annotated parameter because that's safe, however // we should separately flag and prompt migration of such UnsafeArgs to SafeArg. - return valueSafety != Safety.DO_NOT_LOG; + return nullToUnknown(valueSafety) != Safety.DO_NOT_LOG; } }, SAFE() { @Override public Safety leastUpperBound(Safety other) { - return other; + return nullToUnknown(other); } @Override public boolean allowsValueWith(Safety valueSafety) { - return valueSafety == Safety.UNKNOWN || valueSafety == Safety.SAFE; + return nullToUnknown(valueSafety) == Safety.UNKNOWN || valueSafety == Safety.SAFE; } }; @@ -79,7 +79,9 @@ public boolean allowsAll() { * no confidence, preferring the other type if data is available. * For example, casting from {@link Object} to a known-safe type should result in {@link Safety#SAFE}. */ - public static Safety mergeAssumingUnknownIsSame(Safety one, Safety two) { + public static Safety mergeAssumingUnknownIsSame(Safety first, Safety second) { + Safety one = nullToUnknown(first); + Safety two = nullToUnknown(second); if (one == UNKNOWN) { return two; } @@ -91,6 +93,10 @@ public static Safety mergeAssumingUnknownIsSame(Safety one, Safety two) { public static Safety mergeAssumingUnknownIsSame(Safety one, Safety two, Safety three) { Safety result = mergeAssumingUnknownIsSame(one, two); - return mergeAssumingUnknownIsSame(result, three); + return mergeAssumingUnknownIsSame(result, nullToUnknown(three)); + } + + static Safety nullToUnknown(Safety input) { + return input == null ? Safety.UNKNOWN : input; } } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java index a5ab716ce..b4ff166f7 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java @@ -32,7 +32,7 @@ public final class SafetyAnalysis { public static Safety of(VisitorState state) { SafetyPropagationTransfer propagation = instance(state.context); try (ClearVisitorState ignored = propagation.setVisitorState(state)) { - return DataFlow.expressionDataflow(state.getPath(), state.context, propagation); + return Safety.nullToUnknown(DataFlow.expressionDataflow(state.getPath(), state.context, propagation)); } } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 738fbb3e8..376e53cc6 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -330,14 +330,14 @@ private TransferResult> literal( private TransferResult> unary( UnaryOperationNode node, TransferInput> input) { - Safety safety = input.getValueOfSubNode(node.getOperand()); + Safety safety = getValueOfSubNode(input, node.getOperand()); return noStoreChanges(safety, input); } private TransferResult> binary( BinaryOperationNode node, TransferInput> input) { - Safety safety = input.getValueOfSubNode(node.getLeftOperand()) - .leastUpperBound(input.getValueOfSubNode(node.getRightOperand())); + Safety safety = getValueOfSubNode(input, node.getLeftOperand()) + .leastUpperBound(getValueOfSubNode(input, node.getRightOperand())); return noStoreChanges(safety, input); } @@ -509,8 +509,8 @@ public TransferResult> visitBitwiseXor( @Override public TransferResult> visitStringConcatenateAssignment( StringConcatenateAssignmentNode node, TransferInput> input) { - Safety safety = input.getValueOfSubNode(node.getLeftOperand()) - .leastUpperBound(input.getValueOfSubNode(node.getRightOperand())); + Safety safety = getValueOfSubNode(input, node.getLeftOperand()) + .leastUpperBound(getValueOfSubNode(input, node.getRightOperand())); ReadableUpdates updates = new ReadableUpdates(); updates.trySet(node.getLeftOperand(), safety); return updateRegularStore(safety, input, updates); @@ -573,8 +573,8 @@ public TransferResult> visitConditionalNot( @Override public TransferResult> visitTernaryExpression( TernaryExpressionNode node, TransferInput> input) { - Safety safety = input.getValueOfSubNode(node.getThenOperand()) - .leastUpperBound(input.getValueOfSubNode(node.getElseOperand())); + Safety safety = getValueOfSubNode(input, node.getThenOperand()) + .leastUpperBound(getValueOfSubNode(input, node.getElseOperand())); return noStoreChanges(safety, input); } @@ -588,7 +588,7 @@ public TransferResult> visitSwitchExpressionNode public TransferResult> visitAssignment( AssignmentNode node, TransferInput> input) { ReadableUpdates updates = new ReadableUpdates(); - Safety expressionSafety = input.getValueOfSubNode(node.getExpression()); + Safety expressionSafety = getValueOfSubNode(input, node.getExpression()); Safety targetSymbolSafety = SafetyAnnotations.getSafety( ASTHelpers.getSymbol(node.getTarget().getTree()), state); Safety safety = Safety.mergeAssumingUnknownIsSame(expressionSafety, targetSymbolSafety); @@ -597,7 +597,7 @@ public TransferResult> visitAssignment( updates.trySet(target, safety); } else if (target instanceof ArrayAccessNode) { Node arrayNode = ((ArrayAccessNode) target).getArray(); - Safety arrayNodeSafety = input.getValueOfSubNode(arrayNode); + Safety arrayNodeSafety = getValueOfSubNode(input, arrayNode); safety = arrayNodeSafety == null ? safety : arrayNodeSafety.leastUpperBound(safety); updates.trySet(arrayNode, safety); } else if (target instanceof FieldAccessNode) { @@ -769,7 +769,7 @@ public TransferResult> visitSuper( public TransferResult> visitReturn( ReturnNode node, TransferInput> input) { Node result = node.getResult(); - Safety safety = result == null ? Safety.SAFE : input.getValueOfSubNode(result); + Safety safety = result == null ? Safety.SAFE : getValueOfSubNode(input, result); return noStoreChanges(safety, input); } @@ -782,7 +782,7 @@ public TransferResult> visitLambdaResultExpressi @Override public TransferResult> visitStringConversion( StringConversionNode node, TransferInput> input) { - Safety safety = input.getValueOfSubNode(node.getOperand()); + Safety safety = getValueOfSubNode(input, node.getOperand()); return noStoreChanges(safety, input); } @@ -808,7 +808,7 @@ public TransferResult> visitTypeCast( /** Handles type changes (widen, narrow, and cast). */ private TransferResult> handleTypeConversion( Tree newType, Node original, TransferInput> input) { - Safety valueSafety = input.getValueOfSubNode(original); + Safety valueSafety = getValueOfSubNode(input, original); Type targetType = ASTHelpers.getType(newType); Safety narrowTargetSafety = targetType == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(targetType.tsym, state); @@ -860,17 +860,17 @@ private Safety getKnownMethodSafety( if (THROWABLE_GET_MESSAGE.matches(node.getTree(), state)) { // Auth failures are sometimes annotated '@DoNotLog', which getMessage should inherit. return Safety.mergeAssumingUnknownIsSame( - Safety.UNSAFE, input.getValueOfSubNode(node.getTarget().getReceiver())); + Safety.UNSAFE, getValueOfSubNode(input, node.getTarget().getReceiver())); } else if (RETURNS_SAFETY_COMBINATION_OF_ARGS.matches(node.getTree(), state)) { Safety safety = Safety.SAFE; for (Node argument : node.getArguments()) { - safety = safety.leastUpperBound(input.getValueOfSubNode(argument)); + safety = safety.leastUpperBound(getValueOfSubNode(input, argument)); } return safety; } else if (RETURNS_SAFETY_OF_RECEIVER.matches(node.getTree(), state)) { - return input.getValueOfSubNode(node.getTarget().getReceiver()); + return getValueOfSubNode(input, node.getTarget().getReceiver()); } else if (RETURNS_SAFETY_OF_FIRST_ARG.matches(node.getTree(), state)) { - return input.getValueOfSubNode(node.getArguments().get(0)); + return getValueOfSubNode(input, node.getArguments().get(0)); } return Safety.UNKNOWN; } @@ -884,7 +884,7 @@ private Safety getMethodSymbolSafety( SafetyAnnotations.getSafety(methodSymbol, state), resultTypeSafety); // non-annotated toString inherits type-level safety. if (methodSafety == Safety.UNKNOWN && TO_STRING.matches(node.getTree(), state)) { - return input.getValueOfSubNode(node.getTarget().getReceiver()); + return getValueOfSubNode(input, node.getTarget().getReceiver()); } return methodSafety; } @@ -909,7 +909,7 @@ public TransferResult> visitArrayCreation( ArrayCreationNode node, TransferInput> input) { Safety safety = Safety.SAFE; for (Node item : node.getInitializers()) { - safety = safety.leastUpperBound(input.getValueOfSubNode(item)); + safety = safety.leastUpperBound(getValueOfSubNode(input, item)); } return noStoreChanges(safety, input); } @@ -955,4 +955,13 @@ public TransferResult> visitClassDeclaration( ClassDeclarationNode node, TransferInput> input) { return unknown(input); } + + /** + * Equivalent to {@link TransferInput#getValueOfSubNode(Node)}, + * but returning {@link Safety#UNKNOWN} rather than null. + */ + private static Safety getValueOfSubNode(TransferInput> input, Node node) { + Safety maybeSafety = input.getValueOfSubNode(node); + return Safety.nullToUnknown(maybeSafety); + } } diff --git a/changelog/@unreleased/pr-2169.v2.yml b/changelog/@unreleased/pr-2169.v2.yml new file mode 100644 index 000000000..fe26f757f --- /dev/null +++ b/changelog/@unreleased/pr-2169.v2.yml @@ -0,0 +1,5 @@ +type: fix +fix: + description: Avoid nulls in safety analysis + links: + - https://github.com/palantir/gradle-baseline/pull/2169