In OperatorPrecedence, suggest parens for e.g. a && b ? c : d

PiperOrigin-RevId: 566362803
google · Sep 19, 2023 · 1d5121e · 1d5121e
1 parent c9726da
commit 1d5121e
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 1 deletion.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/OperatorPrecedence.java b/core/src/main/java/com/google/errorprone/bugpatterns/OperatorPrecedence.java
@@ -19,20 +19,25 @@
 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.getType;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.BugPattern.StandardTags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.ConditionalExpressionTreeMatcher;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.BinaryTree;
+import com.sun.source.tree.ConditionalExpressionTree;
+import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.ParenthesizedTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.Tree.Kind;
+import com.sun.tools.javac.code.TypeTag;
 import com.sun.tools.javac.tree.JCTree.JCBinary;
 import com.sun.tools.javac.tree.TreeInfo;
 
@@ -41,7 +46,8 @@
     summary = "Use grouping parenthesis to make the operator precedence explicit",
     severity = WARNING,
     tags = StandardTags.STYLE)
-public class OperatorPrecedence extends BugChecker implements BinaryTreeMatcher {
+public class OperatorPrecedence extends BugChecker
+    implements BinaryTreeMatcher, ConditionalExpressionTreeMatcher {
 
   private static final ImmutableSet<Kind> CONDITIONAL =
       Sets.immutableEnumSet(Kind.AND, Kind.OR, Kind.XOR, Kind.CONDITIONAL_AND, Kind.CONDITIONAL_OR);
@@ -71,6 +77,22 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {
     return createAppropriateFix(tree, state);
   }
 
+  @Override
+  public Description matchConditionalExpression(
+      ConditionalExpressionTree tree, VisitorState state) {
+    ExpressionTree condition = tree.getCondition();
+    if (!(condition instanceof BinaryTree)) {
+      return NO_MATCH;
+    }
+    if (!CONDITIONAL.contains(condition.getKind())) {
+      return NO_MATCH;
+    }
+    if (!state.getTypes().unboxedTypeOrType(getType(tree)).hasTag(TypeTag.BOOLEAN)) {
+      return NO_MATCH;
+    }
+    return basicFix((BinaryTree) condition);
+  }
+
   private static boolean isConfusing(Kind thisKind, Kind parentKind) {
     if (CONDITIONAL.contains(thisKind) && CONDITIONAL.contains(parentKind)) {
       return true;

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/OperatorPrecedenceTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/OperatorPrecedenceTest.java
@@ -191,4 +191,42 @@ public void aLotOfParenthesis() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void conditionalBoolean() {
+    helper
+        .addInputLines(
+            "Test.java", //
+            "class Test {",
+            " void f(boolean a, boolean b, boolean c, boolean d) {",
+            "   boolean g = a || b ? c : d;",
+            "   g = a && b ? c : d;",
+            "   g = a == b ? c : d;",
+            "  }",
+            "}")
+        .addOutputLines(
+            "Test.java", //
+            "class Test {",
+            " void f(boolean a, boolean b, boolean c, boolean d) {",
+            "   boolean g = (a || b) ? c : d;",
+            "   g = (a && b) ? c : d;",
+            "   g = a == b ? c : d;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void conditionOtherType() {
+    helper
+        .addInputLines(
+            "Test.java", //
+            "class Test {",
+            " void f(boolean a, boolean b, String c, String d) {",
+            "   String g = a || b ? c : d;",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
 }