Validate that safety var annotations and type annotations agree (#2161)

Validate that safety var annotations and type annotations agree

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java

@@ -36,6 +36,8 @@
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.ReturnTree;
 import com.sun.source.tree.StatementTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
 import com.sun.source.util.TreePath;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import com.sun.tools.javac.code.Symbol.VarSymbol;
@@ -61,7 +63,9 @@ public final class IllegalSafeLoggingArgument extends BugChecker
         implements BugChecker.MethodInvocationTreeMatcher,
                 BugChecker.ReturnTreeMatcher,
                 BugChecker.AssignmentTreeMatcher,
-                BugChecker.CompoundAssignmentTreeMatcher {
+                BugChecker.CompoundAssignmentTreeMatcher,
+                BugChecker.MethodTreeMatcher,
+                BugChecker.VariableTreeMatcher {
 
     private static final String UNSAFE_ARG = "com.palantir.logsafe.UnsafeArg";
     private static final Matcher<ExpressionTree> SAFE_ARG_OF_METHOD_MATCHER = MethodMatchers.staticMethod()
@@ -174,4 +178,42 @@ private Description handleAssignment(
                         assignmentValue, variableDeclaredSafety))
                 .build();
     }
+
+    @Override
+    public Description matchMethod(MethodTree tree, VisitorState state) {
+        Tree returnType = tree.getReturnType();
+        if (returnType == null) {
+            return Description.NO_MATCH;
+        }
+        Safety methodSafetyAnnotation = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(tree), state);
+        if (methodSafetyAnnotation.allowsAll()) {
+            return Description.NO_MATCH;
+        }
+        Safety returnTypeSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(returnType), state);
+        if (methodSafetyAnnotation.allowsValueWith(returnTypeSafety)) {
+            return Description.NO_MATCH;
+        }
+        return buildDescription(returnType)
+                .setMessage(String.format(
+                        "Dangerous return type: type is '%s' but the method is annotated '%s'.",
+                        returnTypeSafety, methodSafetyAnnotation))
+                .build();
+    }
+
+    @Override
+    public Description matchVariable(VariableTree tree, VisitorState state) {
+        Safety parameterSafetyAnnotation = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(tree), state);
+        if (parameterSafetyAnnotation.allowsAll()) {
+            return Description.NO_MATCH;
+        }
+        Safety variableTypeSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(tree.getType()), state);
+        if (parameterSafetyAnnotation.allowsValueWith(variableTypeSafety)) {
+            return Description.NO_MATCH;
+        }
+        return buildDescription(tree)
+                .setMessage(String.format(
+                        "Dangerous variable: type is '%s' but the variable is annotated '%s'.",
+                        variableTypeSafety, parameterSafetyAnnotation))
+                .build();
+    }
 }

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java

@@ -52,6 +52,10 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
+import java.util.OptionalDouble;
+import java.util.OptionalInt;
+import java.util.OptionalLong;
 import java.util.Set;
 import javax.annotation.Nullable;
 import javax.lang.model.element.VariableElement;
@@ -173,9 +177,30 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction<
                     .namedAnyOf("of", "copyOf"),
             MethodMatchers.staticMethod().onClass(Arrays.class.getName()).named("asList"));
 
+    private static final Matcher<ExpressionTree> OPTIONAL_FACTORIES = Matchers.anyOf(
+            MethodMatchers.staticMethod().onClass(Optional.class.getName()).namedAnyOf("of", "ofNullable"),
+            MethodMatchers.staticMethod()
+                    .onClassAny(
+                            OptionalInt.class.getName(), OptionalLong.class.getName(), OptionalDouble.class.getName())
+                    .named("of"));
+
     // These methods do not take the receiver (generally a static class) into account, only the inputs.
     private static final Matcher<ExpressionTree> RETURNS_SAFETY_COMBINATION_OF_ARGS =
-            Matchers.anyOf(STRING_FORMAT, OBJECTS_TO_STRING, IMMUTABLE_COLLECTION_FACTORY);
+            Matchers.anyOf(STRING_FORMAT, OBJECTS_TO_STRING, IMMUTABLE_COLLECTION_FACTORY, OPTIONAL_FACTORIES);
+
+    private static final Matcher<ExpressionTree> OPTIONAL_ACCESSORS = Matchers.anyOf(
+            MethodMatchers.instanceMethod()
+                    .onDescendantOf(Optional.class.getName())
+                    .namedAnyOf("filter", "get", "orElseThrow", "stream"),
+            MethodMatchers.instanceMethod()
+                    .onDescendantOf(OptionalInt.class.getName())
+                    .namedAnyOf("getAsInt", "orElseThrow"),
+            MethodMatchers.instanceMethod()
+                    .onDescendantOf(OptionalLong.class.getName())
+                    .namedAnyOf("getAsLong", "orElseThrow"),
+            MethodMatchers.instanceMethod()
+                    .onDescendantOf(OptionalDouble.class.getName())
+                    .namedAnyOf("getAsDouble", "orElseThrow"));
 
     // Returns the safety of the receiver, e.g. myString.getBytes() returns the safety of myString.
     private static final Matcher<ExpressionTree> RETURNS_SAFETY_OF_RECEIVER = Matchers.anyOf(
@@ -190,7 +215,8 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction<
                     .namedAnyOf("toArray", "stream", "parallelStream"),
             MethodMatchers.instanceMethod()
                     .onDescendantOf(Iterable.class.getName())
-                    .namedAnyOf("toArray", "iterator", "spliterator"));
+                    .namedAnyOf("toArray", "iterator", "spliterator"),
+            OPTIONAL_ACCESSORS);
 
     private static final Matcher<ExpressionTree> RETURNS_SAFETY_OF_FIRST_ARG = Matchers.anyOf(
             MethodMatchers.staticMethod().onClass(Objects.class.getName()).named("requireNonNull"),

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java

@@ -891,6 +891,67 @@ public void testFieldAnnotationAssignment() {
                 .doTest();
     }
 
+    @Test
+    public void disagreeingSafetyAnnotations() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "class Test {",
+                        "  @DoNotLog static class DoNotLogClass {}",
+                        "  @Safe static class SafeClass {}",
+                        "  @Unsafe static class UnsafeClass {}",
+                        "  // BUG: Diagnostic contains: Dangerous return type: type is 'DO_NOT_LOG' "
+                                + "but the method is annotated 'UNSAFE'.",
+                        "  @Unsafe DoNotLogClass f(",
+                        "    @Safe SafeClass superSafe,",
+                        "    // BUG: Diagnostic contains: Dangerous variable: type is 'UNSAFE' "
+                                + "but the variable is annotated 'SAFE'.",
+                        "    @Safe UnsafeClass oops",
+                        "    ) {",
+                        "    return null;",
+                        "  }",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    public void testOptionalUnwrapping() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import java.util.*;",
+                        "class Test {",
+                        "  void f(@Unsafe Optional<String> p) {",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    fun(p);",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    fun(p.get());",
+                        "  }",
+                        "  void fun(@Safe Object in) {}",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    public void testOptionalUnsafeType() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import java.util.*;",
+                        "class Test {",
+                        "  @Unsafe static class UnsafeClass {}",
+                        "  void f(Optional<UnsafeClass> p) {",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    fun(p.get());",
+                        "  }",
+                        "  void fun(@Safe Object in) {}",
+                        "}")
+                .doTest();
+    }
+
     @Test
     public void testSafeArgOfUnsafe_recommendsUnsafeArgOf() {
         refactoringHelper()

changelog/@unreleased/pr-2161.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Validate that safety var annotations and type annotations agree
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/2161