Avoid nulls in safety analysis (#2169)

Avoid nulls in safety analysis
palantir · Apr 1, 2022 · 135a3c1 · 135a3c1
1 parent 34eb494
commit 135a3c1
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 25 deletions.
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
@@ -22,7 +22,7 @@ public enum Safety implements AbstractValue<Safety> {
     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/...ine-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java b/...ine-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/...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java
@@ -330,14 +330,14 @@ private TransferResult<Safety, AccessPathStore<Safety>> literal(
 
     private TransferResult<Safety, AccessPathStore<Safety>> unary(
             UnaryOperationNode node, TransferInput<Safety, AccessPathStore<Safety>> input) {
-        Safety safety = input.getValueOfSubNode(node.getOperand());
+        Safety safety = getValueOfSubNode(input, node.getOperand());
         return noStoreChanges(safety, input);
     }
 
     private TransferResult<Safety, AccessPathStore<Safety>> binary(
             BinaryOperationNode node, TransferInput<Safety, AccessPathStore<Safety>> 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<Safety, AccessPathStore<Safety>> visitBitwiseXor(
     @Override
     public TransferResult<Safety, AccessPathStore<Safety>> visitStringConcatenateAssignment(
             StringConcatenateAssignmentNode node, TransferInput<Safety, AccessPathStore<Safety>> 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<Safety, AccessPathStore<Safety>> visitConditionalNot(
     @Override
     public TransferResult<Safety, AccessPathStore<Safety>> visitTernaryExpression(
             TernaryExpressionNode node, TransferInput<Safety, AccessPathStore<Safety>> 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<Safety, AccessPathStore<Safety>> visitSwitchExpressionNode
     public TransferResult<Safety, AccessPathStore<Safety>> visitAssignment(
             AssignmentNode node, TransferInput<Safety, AccessPathStore<Safety>> 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<Safety, AccessPathStore<Safety>> 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<Safety, AccessPathStore<Safety>> visitSuper(
     public TransferResult<Safety, AccessPathStore<Safety>> visitReturn(
             ReturnNode node, TransferInput<Safety, AccessPathStore<Safety>> 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<Safety, AccessPathStore<Safety>> visitLambdaResultExpressi
     @Override
     public TransferResult<Safety, AccessPathStore<Safety>> visitStringConversion(
             StringConversionNode node, TransferInput<Safety, AccessPathStore<Safety>> input) {
-        Safety safety = input.getValueOfSubNode(node.getOperand());
+        Safety safety = getValueOfSubNode(input, node.getOperand());
         return noStoreChanges(safety, input);
     }
 
@@ -808,7 +808,7 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitTypeCast(
     /** Handles type changes (widen, narrow, and cast). */
     private TransferResult<Safety, AccessPathStore<Safety>> handleTypeConversion(
             Tree newType, Node original, TransferInput<Safety, AccessPathStore<Safety>> 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<Safety, AccessPathStore<Safety>> visitArrayCreation(
             ArrayCreationNode node, TransferInput<Safety, AccessPathStore<Safety>> 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<Safety, AccessPathStore<Safety>> visitClassDeclaration(
             ClassDeclarationNode node, TransferInput<Safety, AccessPathStore<Safety>> input) {
         return unknown(input);
     }
+
+    /**
+     * Equivalent to {@link TransferInput#getValueOfSubNode(Node)},
+     * but returning {@link Safety#UNKNOWN} rather than null.
+     */
+    private static Safety getValueOfSubNode(TransferInput<Safety, AccessPathStore<Safety>> 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
@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: Avoid nulls in safety analysis
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/2169