Fix safety evaluation requiring lhs assignment to be safe unnecessari…

…ly (#2242) Fix safety evaluation requiring lhs of an assignment to be safe unnecessarily
palantir · May 2, 2022 · d041fbb · d041fbb
1 parent c190aec
commit d041fbb
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 7 deletions.
diff --git a/...-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/...-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java
@@ -61,15 +61,20 @@ public static Safety getSafety(Tree tree, VisitorState state) {
         // Check the symbol itself:
         Symbol treeSymbol = ASTHelpers.getSymbol(tree);
         Safety symbolSafety = getSafety(treeSymbol, state);
-        Type type = tree instanceof ExpressionTree
+        Type treeType = tree instanceof ExpressionTree
                 ? ASTHelpers.getResultType((ExpressionTree) tree)
                 : ASTHelpers.getType(tree);
-
-        if (type == null) {
-            return symbolSafety;
-        } else {
-            return Safety.mergeAssumingUnknownIsSame(symbolSafety, getSafety(type, state), getSafety(type.tsym, state));
-        }
+        Safety treeTypeSafety = treeType == null
+                ? Safety.UNKNOWN
+                : Safety.mergeAssumingUnknownIsSame(getSafety(treeType, state), getSafety(treeType.tsym, state));
+        Type symbolType = treeSymbol == null ? null : treeSymbol.type;
+        // If the type extracted from the symbol matches the type extracted from the tree, avoid duplicate work.
+        // However, in some cases type parameter information is excluded from one, but not the other.
+        Safety symbolTypeSafety = symbolType == null
+                        || (treeType != null && state.getTypes().isSameType(treeType, symbolType))
+                ? Safety.UNKNOWN
+                : Safety.mergeAssumingUnknownIsSame(getSafety(symbolType, state), getSafety(symbolType.tsym, state));
+        return Safety.mergeAssumingUnknownIsSame(symbolSafety, treeTypeSafety, symbolTypeSafety);
     }
 
     public static Safety getSafety(@Nullable Symbol symbol, VisitorState state) {
@@ -184,6 +189,10 @@ Safety getSafety(Type type, VisitorState state, @Nullable Set<String> dejaVu) {
                 }
                 Safety safety = Safety.SAFE;
                 List<Type> typeArguments = asSubtype.getTypeArguments();
+                if (typeArguments.isEmpty()) {
+                    // Type information is not available, not enough data to make a decision
+                    return null;
+                }
                 for (Type typeArgument : typeArguments) {
                     Safety safetyBasedOnType = SafetyAnnotations.getSafetyInternal(typeArgument, state, deJaVuToPass);
                     Safety safetyBasedOnSymbol = SafetyAnnotations.getSafety(typeArgument.tsym, state);

diff --git a/...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
@@ -1095,6 +1095,26 @@ public void testFieldAnnotationAssignment() {
                 .doTest();
     }
 
+    @Test
+    public void testThrowableFieldAssignment() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import java.util.*;",
+                        "class Test<OutputT> {",
+                        "  private volatile Set<Throwable> fieldValue = null;",
+                        "  static final class Other {",
+                        "  void f(Test obj, Set<Throwable> newValue) {",
+                        "    synchronized (obj) {",
+                        "      if (obj.fieldValue != newValue)",
+                        "      obj.fieldValue = newValue;",
+                        "    }",
+                        "  }",
+                        "  }",
+                        "}")
+                .doTest();
+    }
+
     @Test
     public void disagreeingSafetyAnnotations() {
         helper().addSourceLines(

diff --git a/changelog/@unreleased/pr-2242.v2.yml b/changelog/@unreleased/pr-2242.v2.yml
@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Fix safety evaluation requiring lhs of an assignment to be safe unnecessarily
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/2242