From d041fbb407b1d308585529b11518dbb30d4716fa Mon Sep 17 00:00:00 2001
From: Carter Kozak <ckozak@ckozak.net>
Date: Mon, 2 May 2022 10:22:07 -0400
Subject: [PATCH] Fix safety evaluation requiring lhs assignment to be safe
 unnecessarily (#2242)

Fix safety evaluation requiring lhs of an assignment to be safe unnecessarily
---
 .../errorprone/safety/SafetyAnnotations.java  | 23 +++++++++++++------
 .../IllegalSafeLoggingArgumentTest.java       | 20 ++++++++++++++++
 changelog/@unreleased/pr-2242.v2.yml          |  5 ++++
 3 files changed, 41 insertions(+), 7 deletions(-)
 create mode 100644 changelog/@unreleased/pr-2242.v2.yml

diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java
index eac6a160d..0be624704 100644
--- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java
+++ b/baseline-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/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
index 314b91f3e..a8e880b42 100644
--- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
+++ b/baseline-error-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
new file mode 100644
index 000000000..09b52c2ef
--- /dev/null
+++ 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