Require safety propagation based on getters (#2218)

Require log-safety annotation propagation based detected getters and superclasses/superinterfaces

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

@@ -0,0 +1,115 @@
+/*
+ * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.baseline.errorprone;
+
+import com.google.auto.service.AutoService;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.Matchers;
+import com.google.errorprone.util.ASTHelpers;
+import com.palantir.baseline.errorprone.safety.Safety;
+import com.palantir.baseline.errorprone.safety.SafetyAnnotations;
+import com.sun.source.tree.AnnotationTree;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.Tree;
+import com.sun.tools.javac.code.Symbol.ClassSymbol;
+import javax.lang.model.element.Modifier;
+
+@AutoService(BugChecker.class)
+@BugPattern(
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
+        // This will be promoted after an initial rollout period
+        severity = BugPattern.SeverityLevel.SUGGESTION,
+        summary = "Safe logging annotations should be propagated to encapsulating elements to allow static analysis "
+                + "tooling to work with as much information as possible. This check can be auto-fixed using "
+                + "`./gradlew classes testClasses -PerrorProneApply=SafeLoggingPropagation`")
+public final class SafeLoggingPropagation extends BugChecker implements BugChecker.ClassTreeMatcher {
+
+    private static final Matcher<Tree> SAFETY_ANNOTATION_MATCHER = Matchers.anyOf(
+            Matchers.isSameType(SafetyAnnotations.SAFE),
+            Matchers.isSameType(SafetyAnnotations.UNSAFE),
+            Matchers.isSameType(SafetyAnnotations.DO_NOT_LOG));
+
+    private static final Matcher<MethodTree> GETTER_METHOD_MATCHER = Matchers.allOf(
+            Matchers.not(Matchers.hasModifier(Modifier.STATIC)),
+            Matchers.not(Matchers.methodReturns(Matchers.isVoidType())),
+            Matchers.methodHasNoParameters());
+
+    @Override
+    public Description matchClass(ClassTree classTree, VisitorState state) {
+        ClassSymbol classSymbol = ASTHelpers.getSymbol(classTree);
+        Safety existingClassSafety = SafetyAnnotations.getSafety(classSymbol, state);
+        Safety safety = SafetyAnnotations.getSafety(classTree.getExtendsClause(), state);
+        for (Tree implemented : classTree.getImplementsClause()) {
+            safety = safety.leastUpperBound(SafetyAnnotations.getSafety(implemented, state));
+        }
+        for (Tree member : classTree.getMembers()) {
+            if (member instanceof MethodTree) {
+                MethodTree methodMember = (MethodTree) member;
+                if (GETTER_METHOD_MATCHER.matches(methodMember, state)) {
+                    safety = safety.leastUpperBound(SafetyAnnotations.getSafety(methodMember.getReturnType(), state));
+                }
+            }
+        }
+        return handleSafety(classTree, state, existingClassSafety, safety);
+    }
+
+    private Description handleSafety(
+            ClassTree classTree, VisitorState state, Safety existingSafety, Safety computedSafety) {
+        if (existingSafety != Safety.UNKNOWN && existingSafety.allowsValueWith(computedSafety)) {
+            // Do not suggest promotion, this check is not exhaustive.
+            return Description.NO_MATCH;
+        }
+        switch (computedSafety) {
+            case UNKNOWN:
+                // Nothing to do
+                return Description.NO_MATCH;
+            case SAFE:
+                // Do not suggest promotion to safe, this check is not exhaustive.
+                return Description.NO_MATCH;
+            case DO_NOT_LOG:
+                return annotate(classTree, state, SafetyAnnotations.DO_NOT_LOG);
+            case UNSAFE:
+                return annotate(classTree, state, SafetyAnnotations.UNSAFE);
+        }
+        return Description.NO_MATCH;
+    }
+
+    private Description annotate(ClassTree classTree, VisitorState state, String annotationName) {
+        // Don't cause churn in test-code.
+        if (TestCheckUtils.isTestCode(state)) {
+            return Description.NO_MATCH;
+        }
+        SuggestedFix.Builder fix = SuggestedFix.builder();
+        String qualifiedAnnotation = SuggestedFixes.qualifyType(state, fix, annotationName);
+        for (AnnotationTree annotationTree : classTree.getModifiers().getAnnotations()) {
+            Tree annotationType = annotationTree.getAnnotationType();
+            if (SAFETY_ANNOTATION_MATCHER.matches(annotationType, state)) {
+                fix.replace(annotationTree, "");
+            }
+        }
+        fix.prefixWith(classTree, String.format("@%s ", qualifiedAnnotation));
+        return buildDescription(classTree).addFix(fix.build()).build();
+    }
+}

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

@@ -33,6 +33,9 @@ private TestCheckUtils() {
 
     /** Note that this is a relatively expensive check and should be executed after simpler validation. */
     static boolean isTestCode(VisitorState state) {
+        if (state.errorProneOptions().isTestOnlyTarget()) {
+            return true;
+        }
         TreePath path = state.getPath();
         for (Tree ancestor : path) {
             if (ancestor instanceof ClassTree && hasTestCases.matches((ClassTree) ancestor, state)) {

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

@@ -41,9 +41,9 @@
 import javax.lang.model.element.TypeElement;
 
 public final class SafetyAnnotations {
-    private static final String SAFE = "com.palantir.logsafe.Safe";
-    private static final String UNSAFE = "com.palantir.logsafe.Unsafe";
-    private static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog";
+    public static final String SAFE = "com.palantir.logsafe.Safe";
+    public static final String UNSAFE = "com.palantir.logsafe.Unsafe";
+    public static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog";
 
     private static final TypeArgumentHandlers SAFETY_IS_COMBINATION_OF_TYPE_ARGUMENTS = new TypeArgumentHandlers(
             new TypeArgumentHandler(Iterable.class),

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

@@ -0,0 +1,197 @@
+/*
+ * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.baseline.errorprone;
+
+import org.junit.jupiter.api.Test;
+
+class SafeLoggingPropagationTest {
+
+    @Test
+    void testAddsAnnotation_dnlType() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "interface Test {",
+                        "  BearerToken token();",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "@DoNotLog",
+                        "interface Test {",
+                        "  BearerToken token();",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    void testMixedSafety() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "interface Test {",
+                        "  @Safe String one();",
+                        "  @Unsafe String two();",
+                        "  String three();",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "@Unsafe",
+                        "interface Test {",
+                        "  @Safe String one();",
+                        "  @Unsafe String two();",
+                        "  String three();",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    void testAddsAnnotation_dnlReturnValue() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "interface Test {",
+                        "  @DoNotLog",
+                        "  String token();",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "@DoNotLog",
+                        "interface Test {",
+                        "  @DoNotLog",
+                        "  String token();",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    void testReplacesAnnotation_dnlReturnValue() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "@Unsafe",
+                        "interface Test {",
+                        "  @DoNotLog",
+                        "  String token();",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "@DoNotLog",
+                        "interface Test {",
+                        "  @DoNotLog",
+                        "  String token();",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    void testDoesNotReplaceStrictAnnotation() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.tokens.auth.*;",
+                        "import com.palantir.logsafe.*;",
+                        "@DoNotLog",
+                        "interface Test {",
+                        "  @Unsafe",
+                        "  String token();",
+                        "}")
+                .expectUnchanged()
+                .doTest();
+    }
+
+    @Test
+    void testDoesNotAddSafeAnnotation() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "interface Test {",
+                        "  @Safe",
+                        "  String token();",
+                        "}")
+                .expectUnchanged()
+                .doTest();
+    }
+
+    @Test
+    void testIgnoresStaticMethods() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "interface Test {",
+                        "  @Safe",
+                        "  static String token() { return \"\"; }",
+                        "}")
+                .expectUnchanged()
+                .doTest();
+    }
+
+    @Test
+    void testIgnoresVoidMethods() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "interface Test {",
+                        "  @DoNotLog",
+                        "  void token();",
+                        "}")
+                .expectUnchanged()
+                .doTest();
+    }
+
+    @Test
+    void testIgnoresMethodsWithParameters() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "interface Test {",
+                        "  @DoNotLog",
+                        "  String token(int i);",
+                        "}")
+                .expectUnchanged()
+                .doTest();
+    }
+
+    @Test
+    void testIgnoresThrowable() {
+        // exceptions are unsafe-by-default, it's unnecessary to annotate every exception as unsafe.
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "class MyException extends RuntimeException {",
+                        "  @Override public String getMessage() {",
+                        "     return super.getMessage();",
+                        "  }",
+                        "}")
+                .expectUnchanged()
+                .doTest();
+    }
+
+    private RefactoringValidator fix() {
+        return RefactoringValidator.of(SafeLoggingPropagation.class, getClass());
+    }
+}

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

@@ -0,0 +1,6 @@
+type: improvement
+improvement:
+  description: '`SafeLoggingPropagation` error-prone check propages log-safety annotations
+    from getters and superclasses/superinterfaces to the type level'
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/2218