Don't rewrite equal to == for floating-point values.

`Objects.equal(a_double, another_double)` and `a_double == another_double` have different semantics (different result when both compared values are NaN). This is especially relevant when implementing `equals` methods. Fixes #4392 Fixes #4394 COPYBARA_INTEGRATE_REVIEW=#4394 from findepi:findepi/double-compare 603b9c6 PiperOrigin-RevId: 634395242
google · May 16, 2024 · 45ae00b · 45ae00b
1 parent 619f434
commit 45ae00b
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 2 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
@@ -29,7 +29,11 @@
 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.tools.javac.code.Type;
+import javax.lang.model.type.TypeKind;
 
 /**
  * Check for usage of {@code Objects.equal} on primitive types.
@@ -53,11 +57,30 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
       return NO_MATCH;
     }
 
-    String arg1 = state.getSourceForNode(tree.getArguments().get(0));
-    String arg2 = state.getSourceForNode(tree.getArguments().get(1));
+    ExpressionTree expression1 = tree.getArguments().get(0);
+    ExpressionTree expression2 = tree.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.
+      // We could replace this with Double.doubleToLongBits(a_double) ==
+      // Double.doubleToLongBits(another_double)
+      // to avoid boxing, but that can be considered as less readable.
+      return NO_MATCH;
+    }
+
+    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);
   }
+
+  private static boolean isFloatingPoint(ExpressionTree expression) {
+    Type type = ASTHelpers.getType(expression);
+    if (type == null) {
+      return false;
+    }
+    return type.getKind() == TypeKind.DOUBLE || type.getKind() == TypeKind.FLOAT;
+  }
 }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitivesTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ObjectEqualsForPrimitivesTest.java
@@ -179,4 +179,64 @@ public void intAndLong() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void doubles() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            "import java.util.Objects;",
+            "public class Test {",
+            "  private static boolean testDoubles(double a, double b) {",
+            "    return Objects.equals(a, b);",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void doubleAndFloat() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            "import java.util.Objects;",
+            "public class Test {",
+            "  private static boolean testDoubles(double a, float b) {",
+            "    return Objects.equals(a, b);",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void doubleAndLong() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            "import java.util.Objects;",
+            "public class Test {",
+            "  private static boolean testDoubles(double a, long b) {",
+            "    return Objects.equals(a, b);",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void floatAndLong() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            "import java.util.Objects;",
+            "public class Test {",
+            "  private static boolean testDoubles(float a, long b) {",
+            "    return Objects.equals(a, b);",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
 }