From 19ed7cac775bba1131c3d26e3cd587dad802f9b9 Mon Sep 17 00:00:00 2001 From: Kurt Alfred Kluever Date: Tue, 25 Aug 2026 15:17:10 -0700 Subject: [PATCH] Handle negation in ObjectEqualsForPrimitives. When converting `!Objects.equals(a, b)` to use primitive equality, rewriting only the method call leaves an awkward `!(a == b)`. Inspect parent AST nodes across enclosing parentheses to detect logical negation (`!`) and replace the whole expression with `(a != b)`. PiperOrigin-RevId: 970828962 --- .../ObjectEqualsForPrimitives.java | 43 +++++++++++++++---- .../ObjectEqualsForPrimitivesTest.java | 25 ++++++----- 2 files changed, 46 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitives.java b/core/src/main/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitives.java index aff75c5aee6..9a988372f25 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitives.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitives.java @@ -25,13 +25,16 @@ import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; -import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; +import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Type; import javax.lang.model.type.TypeKind; @@ -41,7 +44,7 @@ * @author vlk@google.com (Volodymyr Kachurovskyi) */ @BugPattern( - summary = "Avoid unnecessary boxing by using plain == for primitive types.", + summary = "Avoid unnecessary boxing by using == (or !=) when comparing primitive types.", tags = StandardTags.PERFORMANCE, severity = WARNING) public class ObjectEqualsForPrimitives extends BugChecker implements MethodInvocationTreeMatcher { @@ -52,13 +55,13 @@ public class ObjectEqualsForPrimitives extends BugChecker implements MethodInvoc staticEqualsInvocation(), argument(0, isPrimitiveType()), argument(1, isPrimitiveType())); @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!MATCHER.matches(tree, state)) { + public Description matchMethodInvocation(MethodInvocationTree call, VisitorState state) { + if (!MATCHER.matches(call, state)) { return NO_MATCH; } - ExpressionTree expression1 = tree.getArguments().get(0); - ExpressionTree expression2 = tree.getArguments().get(1); + ExpressionTree expression1 = call.getArguments().get(0); + ExpressionTree expression2 = call.getArguments().get(1); if (isFloatingPoint(expression1) || isFloatingPoint(expression2)) { // Objects.equal(a_double, another_double) compares NaN as equal, but a_double == // another_double does not. @@ -71,9 +74,31 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState String arg1 = state.getSourceForNode(expression1); String arg2 = state.getSourceForNode(expression2); - // TODO: Rewrite to a != b if the original code has a negation (e.g. !Object.equals) - Fix fix = SuggestedFix.builder().replace(tree, "(" + arg1 + " == " + arg2 + ")").build(); - return describeMatch(tree, fix); + TreePath maybeNegation = state.getPath().getParentPath(); + + boolean isNegated = maybeNegation.getLeaf().getKind() == Kind.LOGICAL_COMPLEMENT; + String operator = isNegated ? "!=" : "=="; + + TreePath targetPath = isNegated ? maybeNegation : state.getPath(); + + String replacement = + String.format( + binaryOperatorReplacementRequiresParentheses(targetPath) ? "(%s %s %s)" : "%s %s %s", + arg1, + operator, + arg2); + Tree target = isNegated ? maybeNegation.getLeaf() : call; + return describeMatch(target, SuggestedFix.replace(target, replacement)); + } + + // TODO(kak): there's many other places we could remove the parentheses, but these 2 seem the most + // common and most important. + private static boolean binaryOperatorReplacementRequiresParentheses(TreePath path) { + return switch (path.getParentPath().getLeaf()) { + case ParenthesizedTree pt -> false; + case MethodInvocationTree mit -> !mit.getArguments().contains(path.getLeaf()); + default -> true; + }; } private static boolean isFloatingPoint(ExpressionTree expression) { diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitivesTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitivesTest.java index 205e35bcbb4..b9b7c9f8eb1 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitivesTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitivesTest.java @@ -171,15 +171,15 @@ private static boolean testLongs(long a, long b) { """ public class Test { private static boolean testBooleans(boolean a, boolean b) { - return !(a == b); + return (a != b); } private static boolean testInts(int a, int b) { - return !(a == b); + return (a != b); } private static boolean testLongs(long a, long b) { - return !(a == b); + return (a != b); } } """) @@ -204,17 +204,17 @@ private static boolean testInts(int a, int b) { } } """) - // TODO(kak): we should probably remove the extra parentheses + // TODO(kak): we _could_ remove the extra parentheses here if we really wanted .addOutputLines( "Test.java", """ public class Test { private static boolean testBooleans(boolean a, boolean b) { - return !((a == b)); + return !(a == b); } private static boolean testInts(int a, int b) { - return !(((a == b))); + return !((a == b)); } } """) @@ -275,7 +275,6 @@ private static int ternary(int a, int b) { } } """) - // TODO(kak): we should probably change !(a == b) to (a != b) .addOutputLines( "Test.java", """ @@ -285,7 +284,7 @@ private static String concat(int a, int b) { } private static String concatNegated(int a, int b) { - return "res: " + !(a == b); + return "res: " + (a != b); } private static Object cast(int a, int b) { @@ -301,8 +300,8 @@ private static boolean logical(int a, int b, boolean c) { } private static void ifCondition(int a, int b) { - if ((a == b)) {} - if (!(a == b)) {} + if (a == b) {} + if (a != b) {} } private static void statements(int a, int b) { @@ -310,12 +309,12 @@ private static void statements(int a, int b) { boolean y; y = (a == b); assert (a == b); - assert !(a == b); + assert (a != b); } private static void methodCall(int a, int b) { - consume((a == b)); - consume(!(a == b)); + consume(a == b); + consume(a != b); } private static void consume(boolean b) {}