fix(#5845): strip parentheses before matching in 6 bug checks to eliminate false positives/negatives

core/src/main/java/com/google/errorprone/bugpatterns/AssignmentExpression.java

@@ -33,6 +33,7 @@
 import com.sun.source.tree.ParenthesizedTree;
 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.VarSymbol;
 
 /** A BugPattern; see the summary. */
@@ -55,18 +56,23 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) {
     if (parent instanceof LambdaExpressionTree) {
       return Description.NO_MATCH;
     }
+    // Walk through any enclosing parentheses to find the effective parent context.
+    TreePath effectiveParentPath = state.getPath().getParentPath();
+    while (effectiveParentPath.getLeaf() instanceof ParenthesizedTree) {
+      effectiveParentPath = effectiveParentPath.getParentPath();
+    }
+    Tree effectiveParent = effectiveParentPath.getLeaf();
     // Exempt the C-ism of (foo = getFoo()) != null, etc.
-    if (parent instanceof ParenthesizedTree
-        && state.getPath().getParentPath().getParentPath().getLeaf() instanceof BinaryTree) {
+    if (effectiveParent instanceof BinaryTree) {
       return Description.NO_MATCH;
     }
     // Detect duplicate assignments: a = a = foo() so that we can generate a fix.
-    if (isDuplicateAssignment(tree, parent)) {
+    if (isDuplicateAssignment(tree, effectiveParent)) {
       return describeMatch(
           tree, SuggestedFix.replace(tree, state.getSourceForNode(tree.getExpression())));
     }
     // If we got here it's something like x = y = 0, which is odd but not disallowed.
-    if (parent instanceof AssignmentTree) {
+    if (effectiveParent instanceof AssignmentTree) {
       return Description.NO_MATCH;
     }
     return describeMatch(tree);

core/src/main/java/com/google/errorprone/bugpatterns/DuplicateBranches.java

@@ -20,6 +20,7 @@
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
+import static com.google.errorprone.util.ASTHelpers.stripParentheses;
 import static java.util.stream.Collectors.joining;
 
 import com.google.errorprone.BugPattern;
@@ -31,6 +32,7 @@
 import com.google.errorprone.util.ErrorProneTokens;
 import com.sun.source.tree.BlockTree;
 import com.sun.source.tree.ConditionalExpressionTree;
+import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.IfTree;
 import com.sun.source.tree.Tree;
 
@@ -66,7 +68,9 @@ private Description match(Tree tree, Tree thenTree, Tree elseTree, VisitorState
     // do exactly what we want here, which is to compare the syntax including of identifiers and
     // not their underlying symbols, and it would require a lot of case work to implement for all
     // AST nodes.
-    if (!thenTree.toString().equals(elseTree.toString())) {
+    Tree strippedThen = thenTree instanceof ExpressionTree et ? stripParentheses(et) : thenTree;
+    Tree strippedElse = elseTree instanceof ExpressionTree et ? stripParentheses(et) : elseTree;
+    if (!strippedThen.toString().equals(strippedElse.toString())) {
       return NO_MATCH;
     }
     int start = getStartPosition(elseTree);

core/src/main/java/com/google/errorprone/bugpatterns/InlineTrivialConstant.java

@@ -20,6 +20,7 @@
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.isSameType;
+import static com.google.errorprone.util.ASTHelpers.stripParentheses;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ListMultimap;
@@ -119,7 +120,7 @@ private static Optional<String> isTrivialConstant(
     if (!isSameType(sym.asType(), state.getSymtab().stringType, state)) {
       return Optional.empty();
     }
-    ExpressionTree initializer = tree.getInitializer();
+    ExpressionTree initializer = stripParentheses(tree.getInitializer());
     if (initializer.getKind().equals(Tree.Kind.STRING_LITERAL)) {
       String value = (String) ((LiteralTree) initializer).getValue();
       if (Objects.equals(value, "")) {

core/src/main/java/com/google/errorprone/bugpatterns/LoopConditionChecker.java

@@ -187,7 +187,7 @@ public Void visitCompoundAssignment(CompoundAssignmentTree tree, Void unused) {
     }
 
     private void check(ExpressionTree expression) {
-      Symbol sym = ASTHelpers.getSymbol(expression);
+      Symbol sym = ASTHelpers.getSymbol(ASTHelpers.stripParentheses(expression));
       modified |= variables.contains(sym);
     }
   }

core/src/main/java/com/google/errorprone/bugpatterns/NonAtomicVolatileUpdate.java

@@ -28,6 +28,7 @@
 import static com.google.errorprone.matchers.Matchers.toType;
 
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.util.ASTHelpers;
 import com.google.errorprone.BugPattern.StandardTags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.AssignmentTreeMatcher;
@@ -55,23 +56,23 @@ public class NonAtomicVolatileUpdate extends BugChecker
   /** Extracts the expression from a UnaryTree and applies a matcher to it. */
   private static Matcher<UnaryTree> expressionFromUnaryTree(Matcher<ExpressionTree> exprMatcher) {
     return (UnaryTree tree, VisitorState state) -> {
-      return exprMatcher.matches(tree.getExpression(), state);
+      return exprMatcher.matches(ASTHelpers.stripParentheses(tree.getExpression()), state);
     };
   }
 
   /** Extracts the variable from a CompoundAssignmentTree and applies a matcher to it. */
   private static Matcher<CompoundAssignmentTree> variableFromCompoundAssignmentTree(
       Matcher<ExpressionTree> exprMatcher) {
     return (CompoundAssignmentTree tree, VisitorState state) -> {
-      return exprMatcher.matches(tree.getVariable(), state);
+      return exprMatcher.matches(ASTHelpers.stripParentheses(tree.getVariable()), state);
     };
   }
 
   /** Extracts the variable from an AssignmentTree and applies a matcher to it. */
   private static Matcher<AssignmentTree> variableFromAssignmentTree(
       Matcher<ExpressionTree> exprMatcher) {
     return (AssignmentTree tree, VisitorState state) -> {
-      return exprMatcher.matches(tree.getVariable(), state);
+      return exprMatcher.matches(ASTHelpers.stripParentheses(tree.getVariable()), state);
     };
   }
 

core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java

@@ -23,6 +23,7 @@
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.stripParentheses;
 import static com.google.errorprone.util.SourceVersion.supportsPatternMatchingInstanceof;
 import static com.google.errorprone.util.TargetType.targetType;
 import static java.lang.Boolean.TRUE;
@@ -278,7 +279,7 @@ private ImmutableSet<TreePath> findAllCasts(
         new TreePathScanner<Void, Void>() {
           @Override
           public Void visitTypeCast(TypeCastTree node, Void unused) {
-            var castee = constantExpressions.constantExpression(node.getExpression(), state);
+            var castee = constantExpressions.constantExpression(stripParentheses(node.getExpression()), state);
             if (castee.isPresent()
                 && castee.get().equals(symbol)
                 && state.getTypes().isSameType(getType(node.getType()), targetType)) {

core/src/test/java/com/google/errorprone/bugpatterns/AssignmentExpressionTest.java

@@ -130,6 +130,157 @@ void test() {
         .doTest();
   }
 
+  @Test
+  public void multipleAssignments_parenthesizedInner_noFinding() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                Object a;
+                Object b;
+                a = (b = new Object());
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void comparedToNull_innerParenthesized_noFinding() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                Object a;
+                Object b;
+                if ((a = (b = new Object())) != null) {
+                  return;
+                }
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void initializerWithParenthesizedAssignment_finding() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                int j;
+                // BUG: Diagnostic contains:
+                int i = (j = 0);
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void deeplyChainedAssignments_noFinding() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                Object a, b, c;
+                a = (b = (c = new Object()));
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void deeplyChainedInitializer_innerFinding() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                int k;
+                int j;
+                // BUG: Diagnostic contains:
+                int i = (j = (k = 0));
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void duplicateAssignment_throughParens() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                // BUG: Diagnostic contains:
+                Object a = (a = new Object());
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            class Test {
+              void test() {
+                // BUG: Diagnostic contains:
+                Object a = (new Object());
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void doubleParenthesizedBinaryContext_noFinding() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              Object a;
+              Object b;
+
+              void test() {
+                if (((a = b)) != null) {
+                  return;
+                }
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void returnedParenthesized_finding() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              Object field;
+
+              Object test() {
+                // BUG: Diagnostic contains:
+                return (field = new Object());
+              }
+            }
+            """)
+        .doTest();
+  }
+
   @Test
   public void returnedValue() {
     helper