Prevent UnnecessaryParentheses false-positiving on compile-time-const…

…ant String expressions. RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=207260298
google · Aug 6, 2018 · 3f9df16 · 3f9df16
1 parent 26325be
commit 3f9df16
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 5 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
@@ -86,6 +86,7 @@
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.comp.Enter;
 import com.sun.tools.javac.comp.Resolve;
+import com.sun.tools.javac.parser.Tokens.TokenKind;
 import com.sun.tools.javac.tree.JCTree;
 import com.sun.tools.javac.tree.JCTree.JCAnnotatedType;
 import com.sun.tools.javac.tree.JCTree.JCAnnotation;
@@ -274,7 +275,7 @@ public static MethodSymbol getSymbol(MemberReferenceTree tree) {
   }
 
   /* Checks whether an expression requires parentheses. */
-  public static boolean requiresParentheses(ExpressionTree expression) {
+  public static boolean requiresParentheses(ExpressionTree expression, VisitorState state) {
     switch (expression.getKind()) {
       case IDENTIFIER:
       case MEMBER_SELECT:
@@ -286,7 +287,16 @@ public static boolean requiresParentheses(ExpressionTree expression) {
         return false;
       default: // continue below
     }
-    if (expression instanceof LiteralTree || expression instanceof UnaryTree) {
+    if (expression instanceof LiteralTree) {
+      if (!isSameType(getType(expression), state.getSymtab().stringType, state)) {
+        return false;
+      }
+      // TODO(b/112139121): work around for javac's too-early constant string folding
+      return ErrorProneTokens.getTokens(state.getSourceForNode(expression), state.context)
+          .stream()
+          .anyMatch(t -> t.kind() == TokenKind.PLUS);
+    }
+    if (expression instanceof UnaryTree) {
       return false;
     }
     return true;

diff --git a/.../src/main/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilon.java b/.../src/main/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilon.java
@@ -257,7 +257,7 @@ private String castArgumentIfNecessary(ExpressionTree tree, VisitorState state)
           return suffixed.get();
         }
       }
-      if (ASTHelpers.requiresParentheses(tree)) {
+      if (ASTHelpers.requiresParentheses(tree, state)) {
         return String.format("(%s) (%s)", typeName, source);
       }
       return String.format("(%s) %s", typeName, source);

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MathRoundIntLong.java b/core/src/main/java/com/google/errorprone/bugpatterns/MathRoundIntLong.java
@@ -74,7 +74,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
 
   private Description removeMathRoundCall(MethodInvocationTree tree, VisitorState state) {
     if (ROUND_CALLS_WITH_INT_ARG.matches(tree, state)) {
-      if (ASTHelpers.requiresParentheses(Iterables.getOnlyElement(tree.getArguments()))) {
+      if (ASTHelpers.requiresParentheses(Iterables.getOnlyElement(tree.getArguments()), state)) {
         return buildDescription(tree)
             .addFix(
                 SuggestedFix.builder()

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryParentheses.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryParentheses.java
@@ -47,7 +47,7 @@ public Description matchParenthesized(ParenthesizedTree tree, VisitorState state
     if (state.getPath().getParentPath().getLeaf() instanceof StatementTree) {
       return NO_MATCH;
     }
-    if (ASTHelpers.requiresParentheses(expression)) {
+    if (ASTHelpers.requiresParentheses(expression, state)) {
       return NO_MATCH;
     }
     return describeMatch(

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryParenthesesTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryParenthesesTest.java
@@ -17,6 +17,7 @@
 package com.google.errorprone.bugpatterns;
 
 import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.CompilationTestHelper;
 import java.io.IOException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -27,6 +28,8 @@
 public class UnnecessaryParenthesesTest {
   private final BugCheckerRefactoringTestHelper testHelper =
       BugCheckerRefactoringTestHelper.newInstance(new UnnecessaryParentheses(), getClass());
+  private final CompilationTestHelper helper =
+      CompilationTestHelper.newInstance(UnnecessaryParentheses.class, getClass());
 
   @Test
   public void test() throws IOException {
@@ -85,4 +88,24 @@ public void anonymousClass() throws IOException {
             "}")
         .doTest();
   }
+
+  @Test
+  public void binaryTrees() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  int e() {",
+            "    // BUG: Diagnostic contains:",
+            "    return (\"b\").hashCode();",
+            "  }",
+            "  int f() {",
+            "    return (\"a\" + \"b\").hashCode();",
+            "  }",
+            "  int g() {",
+            "    return (1 + 2) & 3;",
+            "  }",
+            "}")
+        .doTest();
+  }
 }