Validate that safety var annotaitons and type annotations agree

palantir · Apr 2, 2022 · 03fbc55 · 03fbc55
1 parent 9adca85
commit 03fbc55
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 1 deletion.
diff --git a/...rror-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java b/...rror-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();
+    }
 }
diff --git a/...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
@@ -891,6 +891,29 @@ 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 testSafeArgOfUnsafe_recommendsUnsafeArgOf() {
         refactoringHelper()