Insert parens if the thing being substituted in may require them at the replacement site.

PiperOrigin-RevId: 733759318

Author: graememorgan

check_api/src/main/java/com/google/errorprone/util/OperatorPrecedence.java

@@ -17,15 +17,17 @@
 package com.google.errorprone.util;
 
 import com.sun.source.tree.Tree;
+import java.util.Optional;
 
 /**
  * The precedence for an operator kind in the {@link com.sun.source.tree} API.
  *
  * <p>As documented at: http://docs.oracle.com/javase/tutorial/java/nutsandbolts/operators.html
  */
 public enum OperatorPrecedence {
-  POSTFIX(13),
-  UNARY(12),
+  POSTFIX(14),
+  UNARY(13),
+  CAST(12),
   MULTIPLICATIVE(11),
   ADDITIVE(10),
   SHIFT(9),
@@ -50,20 +52,26 @@ public boolean isHigher(OperatorPrecedence other) {
   }
 
   public static OperatorPrecedence from(Tree.Kind kind) {
+    return optionallyFrom(kind)
+        .orElseThrow(() -> new IllegalArgumentException("Unexpected operator kind: " + kind));
+  }
+
+  public static Optional<OperatorPrecedence> optionallyFrom(Tree.Kind kind) {
     return switch (kind) {
-      case POSTFIX_DECREMENT, POSTFIX_INCREMENT -> OperatorPrecedence.POSTFIX;
-      case PREFIX_DECREMENT, PREFIX_INCREMENT -> OperatorPrecedence.UNARY;
-      case MULTIPLY, DIVIDE, REMAINDER -> OperatorPrecedence.MULTIPLICATIVE;
-      case PLUS, MINUS -> OperatorPrecedence.ADDITIVE;
-      case RIGHT_SHIFT, UNSIGNED_RIGHT_SHIFT, LEFT_SHIFT -> OperatorPrecedence.SHIFT;
+      case POSTFIX_DECREMENT, POSTFIX_INCREMENT -> Optional.of(OperatorPrecedence.POSTFIX);
+      case PREFIX_DECREMENT, PREFIX_INCREMENT -> Optional.of(OperatorPrecedence.UNARY);
+      case TYPE_CAST -> Optional.of(OperatorPrecedence.CAST);
+      case MULTIPLY, DIVIDE, REMAINDER -> Optional.of(OperatorPrecedence.MULTIPLICATIVE);
+      case PLUS, MINUS -> Optional.of(OperatorPrecedence.ADDITIVE);
+      case RIGHT_SHIFT, UNSIGNED_RIGHT_SHIFT, LEFT_SHIFT -> Optional.of(OperatorPrecedence.SHIFT);
       case LESS_THAN, LESS_THAN_EQUAL, GREATER_THAN, GREATER_THAN_EQUAL, INSTANCE_OF ->
-          OperatorPrecedence.RELATIONAL;
-      case EQUAL_TO, NOT_EQUAL_TO -> OperatorPrecedence.EQUALITY;
-      case AND -> OperatorPrecedence.AND;
-      case XOR -> OperatorPrecedence.XOR;
-      case OR -> OperatorPrecedence.OR;
-      case CONDITIONAL_AND -> OperatorPrecedence.CONDITIONAL_AND;
-      case CONDITIONAL_OR -> OperatorPrecedence.CONDITIONAL_OR;
+          Optional.of(OperatorPrecedence.RELATIONAL);
+      case EQUAL_TO, NOT_EQUAL_TO -> Optional.of(OperatorPrecedence.EQUALITY);
+      case AND -> Optional.of(OperatorPrecedence.AND);
+      case XOR -> Optional.of(OperatorPrecedence.XOR);
+      case OR -> Optional.of(OperatorPrecedence.OR);
+      case CONDITIONAL_AND -> Optional.of(OperatorPrecedence.CONDITIONAL_AND);
+      case CONDITIONAL_OR -> Optional.of(OperatorPrecedence.CONDITIONAL_OR);
       case ASSIGNMENT,
           MULTIPLY_ASSIGNMENT,
           DIVIDE_ASSIGNMENT,
@@ -76,8 +84,8 @@ public static OperatorPrecedence from(Tree.Kind kind) {
           OR_ASSIGNMENT,
           RIGHT_SHIFT_ASSIGNMENT,
           UNSIGNED_RIGHT_SHIFT_ASSIGNMENT ->
-          OperatorPrecedence.ASSIGNMENT;
-      default -> throw new IllegalArgumentException("Unexpected operator kind: " + kind);
+          Optional.of(OperatorPrecedence.ASSIGNMENT);
+      default -> Optional.empty();
     };
   }
 }

core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java

@@ -29,6 +29,7 @@
 import static com.google.errorprone.util.ASTHelpers.stringContainsComments;
 import static com.google.errorprone.util.MoreAnnotations.getValue;
 import static com.google.errorprone.util.SideEffectAnalysis.hasSideEffect;
+import static java.lang.String.format;
 import static java.util.stream.Collectors.joining;
 
 import com.google.auto.value.AutoValue;
@@ -49,13 +50,19 @@
 import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.util.MoreAnnotations;
+import com.google.errorprone.util.OperatorPrecedence;
+import com.sun.source.tree.AssignmentTree;
 import com.sun.source.tree.ExpressionStatementTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.MemberReferenceTree;
+import com.sun.source.tree.MemberSelectTree;
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.NewClassTree;
 import com.sun.source.tree.Tree;
+import com.sun.source.tree.UnaryTree;
+import com.sun.source.tree.VariableTree;
+import com.sun.source.util.TreePath;
 import com.sun.source.util.TreeScanner;
 import com.sun.tools.javac.code.Attribute;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
@@ -65,6 +72,7 @@
 import com.sun.tools.javac.tree.JCTree;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.function.BiConsumer;
 import java.util.regex.Matcher;
@@ -384,7 +392,12 @@ public int replaceTree(JCTree oldtree, JCTree newtree) {
                 })
             .applyReplacements(replacementFixes.build());
 
-    fixBuilder.replace(replacementStart, replacementEnd, fixedReplacement);
+    fixBuilder.replace(
+        replacementStart,
+        replacementEnd,
+        inliningRequiresParentheses(state.getPath(), replacementExpression)
+            ? format("(%s)", fixedReplacement)
+            : fixedReplacement);
 
     return maybeCheckFixCompiles(tree, state, fixBuilder.build(), api);
   }
@@ -397,6 +410,73 @@ private static List<? extends ExpressionTree> getArguments(Tree tree) {
     };
   }
 
+  /**
+   * Checks whether an expression requires parentheses when substituting in.
+   *
+   * <p>{@code treePath} is the original path including the old tree at the tip; {@code replacement}
+   * is the proposed replacement tree.
+   *
+   * <p>This was originally from {@link com.google.errorprone.util.ASTHelpers#requiresParentheses}
+   * but is heavily specialised for this use case.
+   */
+  private static boolean inliningRequiresParentheses(
+      TreePath treePath, ExpressionTree replacement) {
+    var originalExpression = treePath.getLeaf();
+    var parent = treePath.getParentPath().getLeaf();
+
+    Optional<OperatorPrecedence> replacementPrecedence =
+        OperatorPrecedence.optionallyFrom(replacement.getKind());
+    Optional<OperatorPrecedence> parentPrecedence =
+        OperatorPrecedence.optionallyFrom(parent.getKind());
+    if (replacementPrecedence.isPresent() && parentPrecedence.isPresent()) {
+      return parentPrecedence.get().isHigher(replacementPrecedence.get());
+    }
+
+    // There are some locations, based on the parent path, where we never want to parenthesise.
+    // This list is likely not exhaustive.
+    switch (parent.getKind()) {
+      case RETURN, EXPRESSION_STATEMENT -> {
+        return false;
+      }
+      case VARIABLE -> {
+        if (Objects.equals(((VariableTree) parent).getInitializer(), originalExpression)) {
+          return false;
+        }
+      }
+      case ASSIGNMENT -> {
+        if (((AssignmentTree) parent).getExpression().equals(originalExpression)) {
+          return false;
+        }
+      }
+      case METHOD_INVOCATION, NEW_CLASS -> {
+        if (getArguments(parent).contains(originalExpression)) {
+          return false;
+        }
+      }
+      default -> {
+        // continue below
+      }
+    }
+    switch (replacement.getKind()) {
+      case IDENTIFIER,
+          MEMBER_SELECT,
+          METHOD_INVOCATION,
+          ARRAY_ACCESS,
+          PARENTHESIZED,
+          NEW_CLASS,
+          MEMBER_REFERENCE -> {
+        return false;
+      }
+      default -> {
+        // continue below
+      }
+    }
+    if (replacement instanceof UnaryTree) {
+      return parent instanceof MemberSelectTree;
+    }
+    return true;
+  }
+
   private Description maybeCheckFixCompiles(
       ExpressionTree tree, VisitorState state, SuggestedFix fix, Api api) {
     if (checkFixCompiles && fix.getImportsToAdd().isEmpty()) {
@@ -489,7 +569,7 @@ final String message() {
 
     /** Returns {@code FullyQualifiedClassName#methodName}. */
     final String methodId() {
-      return String.format("%s#%s", className(), methodName());
+      return format("%s#%s", className(), methodName());
     }
 
     /**
@@ -498,7 +578,7 @@ final String methodId() {
      */
     final String shortName() {
       String humanReadableClassName = className().replaceFirst(packageName() + ".", "");
-      return String.format("`%s.%s()`", humanReadableClassName, methodName());
+      return format("`%s.%s()`", humanReadableClassName, methodName());
     }
 
     /** Returns the simple class name (e.g., {@code ClassName}). */

core/src/test/java/com/google/errorprone/bugpatterns/inlineme/InlinerTest.java

@@ -1226,7 +1226,7 @@ public void doTest() {
             public final class Caller {
               public void doTest() {
                 Client client = new Client();
-                int x = 1 + 2 * 3;
+                int x = (1 + 2) * 3;
                 int y = 1 + 2 + 3;
               }
             }
@@ -1877,13 +1877,12 @@ void test(String x) {
               }
             }
             """)
-        // Broken! The inlined code requires parens.
         .addOutputLines(
             "Test.java",
             """
             class Test {
               void test(String x) {
-                String abn = x + "b".repeat(10);
+                String abn = (x + "b").repeat(10);
               }
             }
             """)