Align runtime numeric handlings with IEEE 754 by fixing NaN and negative zero comparisons

PiperOrigin-RevId: 1008426293

Author: l46kok

common/src/main/java/dev/cel/common/internal/BUILD.bazel

@@ -88,7 +88,6 @@ java_library(
     tags = [
     ],
     deps = [
-        "//:auto_value",
         "//common/annotations",
         "@maven//:com_google_errorprone_error_prone_annotations",
         "@maven//:com_google_guava_guava",

common/src/main/java/dev/cel/common/internal/ComparisonFunctions.java

@@ -30,32 +30,17 @@
 public final class ComparisonFunctions {
 
   private static final UnsignedLong UINT_TO_INT_MAX = UnsignedLong.valueOf(Long.MAX_VALUE);
-  private static final double DOUBLE_TO_INT_MAX = (double) Long.MAX_VALUE;
-  private static final double DOUBLE_TO_INT_MIN = (double) Long.MIN_VALUE;
-  private static final double DOUBLE_TO_UINT_MAX = UnsignedLong.MAX_VALUE.doubleValue();
 
   public static int compareDoubleInt(double d, long l) {
-    if (d < DOUBLE_TO_INT_MIN) {
-      return -1;
-    }
-    if (d > DOUBLE_TO_INT_MAX) {
-      return 1;
-    }
-    return Double.compare(d, (double) l);
+    return compareDouble(d, (double) l);
   }
 
   public static int compareIntDouble(long l, double d) {
     return -compareDoubleInt(d, l);
   }
 
   public static int compareDoubleUint(double d, UnsignedLong ul) {
-    if (d < 0.0) {
-      return -1;
-    }
-    if (d > DOUBLE_TO_UINT_MAX) {
-      return 1;
-    }
-    return Double.compare(d, ul.doubleValue());
+    return compareDouble(d, ul.doubleValue());
   }
 
   public static int compareUintDouble(UnsignedLong ul, double d) {
@@ -73,54 +58,29 @@ public static int compareUintInt(UnsignedLong ul, long l) {
     return -compareIntUint(l, ul);
   }
 
+  /** Returns true if {@code value} is one of CEL's canonical runtime numeric types. */
+  public static boolean isNumeric(Object value) {
+    return value instanceof Long || value instanceof Double || value instanceof UnsignedLong;
+  }
+
   /**
    * Compare two numeric values of any type (double, int, uint) for equality.
    *
-   * <p>Floating point values are follow IEEE 754 standard for NaN comparisons.
+   * <p>Floating point values follow IEEE 754 standard for NaN and negative zero comparisons.
    */
   public static boolean numericEquals(Number x, Number y) {
-    if (x instanceof Double) {
-      if (y instanceof Double) {
-        return !(Double.isNaN((Double) x) || Double.isNaN((Double) y))
-            && x.doubleValue() == y.doubleValue();
-      }
-      if (y instanceof Long) {
-        return compareDoubleInt((Double) x, (Long) y) == 0;
-      }
-      if (y instanceof UnsignedLong) {
-        return compareDoubleUint((Double) x, (UnsignedLong) y) == 0;
-      }
-    }
-    if (x instanceof Long) {
-      if (y instanceof Long) {
-        return x.equals(y);
-      }
-      if (y instanceof Double) {
-        return compareIntDouble((Long) x, (Double) y) == 0;
-      }
-      if (y instanceof UnsignedLong) {
-        return compareIntUint((Long) x, (UnsignedLong) y) == 0;
-      }
-    }
-    if (x instanceof UnsignedLong) {
-      if (y instanceof UnsignedLong) {
-        return x.equals(y);
-      }
-      if (y instanceof Double) {
-        return compareUintDouble((UnsignedLong) x, (Double) y) == 0;
-      }
-      if (y instanceof Long) {
-        return compareUintInt((UnsignedLong) x, (Long) y) == 0;
-      }
-    }
-    return false;
+    // Only x needs a NaN check: if y is NaN and x is not, numericCompare returns -1 != 0.
+    return isNumeric(x)
+        && isNumeric(y)
+        && !(x instanceof Double && ((Double) x).isNaN())
+        && numericCompare(x, y) == 0;
   }
 
   /** Compare two numeric values of any type (double, int, uint). */
   public static int numericCompare(Number x, Number y) {
     if (x instanceof Double) {
       if (y instanceof Double) {
-        return ((Double) x).compareTo((Double) y);
+        return compareDouble((Double) x, (Double) y);
       }
       if (y instanceof Long) {
         return compareDoubleInt((Double) x, (Long) y);
@@ -155,5 +115,10 @@ public static int numericCompare(Number x, Number y) {
         "Unsupported argument types: " + x.getClass() + ", " + y.getClass());
   }
 
+  private static int compareDouble(double a, double b) {
+    // Treat -0.0 and 0.0 as equal via ==; Double.compare orders NaN after all non-NaN numbers.
+    return a == b ? 0 : Double.compare(a, b);
+  }
+
   private ComparisonFunctions() {}
 }

common/src/main/java/dev/cel/common/internal/ProtoEquality.java

@@ -16,6 +16,8 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
 import com.google.errorprone.annotations.CheckReturnValue;
 import com.google.errorprone.annotations.Immutable;
 import com.google.protobuf.Any;
@@ -26,6 +28,7 @@
 import dev.cel.common.annotations.Internal;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 
 /**
@@ -35,9 +38,11 @@
  * <p>These are the key differences between CEL's proto equality and Java's proto equality:
  *
  * <ul>
- *   <li/>NaN is not equal to itself.
- *   <li/>Any values are unpacked before comparison.
- *   <li/>If two Any values cannot be unpacked, they are compared by bytes.
+ *   <li>NaN is not equal to itself.
+ *   <li>Positive and negative zero (+0.0 and -0.0) compare as equal when both fields are present
+ *       (under proto3 implicit presence, -0.0 is serialized as present while +0.0 is unset).
+ *   <li>Any values are unpacked before comparison.
+ *   <li>If two Any values cannot be unpacked, they are compared by bytes.
  * </ul>
  *
  * <p>CEL Library Internals. Do Not Use.
@@ -56,9 +61,6 @@ public ProtoEquality(DynamicProto dynamicProto) {
   }
 
   public boolean equals(Message message1, Message message2) {
-    if (message1 == message2) {
-      return true;
-    }
     if (!message1.getDescriptorForType().equals(message2.getDescriptorForType())) {
       return false;
     }
@@ -124,22 +126,50 @@ public boolean equals(Message message1, Message message2) {
     return true;
   }
 
+  /** Returns a hash code consistent with {@link #equals(Message, Message)}. */
+  public int hashCode(Message message) {
+    if (isAny(message)) {
+      // Constant hash: unpacked Any equality ignores wire bytes, while fallback ignores type_url.
+      return ANY_FULL_NAME.hashCode();
+    }
+    return Objects.hash(
+        message.getDescriptorForType(),
+        message.getUnknownFields(),
+        Maps.transformEntries(message.getAllFields(), this::hashFieldEntry));
+  }
+
+  private int hashFieldEntry(FieldDescriptor field, Object value) {
+    if (field.isMapField()) {
+      ProtoMap protoMap = protoMap((List<?>) value);
+      return Maps.transformValues(protoMap.map(), v -> hashFieldValue(protoMap.valueField(), v))
+          .hashCode();
+    }
+    if (field.isRepeated()) {
+      return Lists.transform((List<?>) value, elem -> hashFieldValue(field, elem)).hashCode();
+    }
+    return hashFieldValue(field, value);
+  }
+
+  private int hashFieldValue(FieldDescriptor field, Object value) {
+    switch (field.getJavaType()) {
+      case MESSAGE:
+        return hashCode((Message) value);
+      case DOUBLE:
+      case FLOAT:
+        // Adding 0.0d normalizes -0.0d to +0.0d per IEEE 754 so equal zeros hash alike.
+        return Double.hashCode(((Number) value).doubleValue() + 0.0d);
+      default:
+        return value.hashCode();
+    }
+  }
+
   private boolean equalsFieldValues(FieldDescriptor field, Object value1, Object value2) {
     switch (field.getJavaType()) {
       case MESSAGE:
         return equals((Message) value1, (Message) value2);
       case DOUBLE:
-        // fallthrough
       case FLOAT:
-        Number num1 = (Number) value1;
-        Number num2 = (Number) value2;
-        boolean num1IsNaN = Double.isNaN(num1.doubleValue());
-        boolean num2IsNaN = Double.isNaN(num2.doubleValue());
-        // handle the NaN case
-        if (num1IsNaN && num2IsNaN) {
-          return false;
-        }
-        return num1.equals(num2);
+        return ((Number) value1).doubleValue() == ((Number) value2).doubleValue();
       default:
         return value1.equals(value2);
     }

common/src/test/java/dev/cel/common/internal/ComparisonFunctionsTest.java

@@ -17,8 +17,10 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.primitives.UnsignedLong;
+import com.google.testing.junit.testparameterinjector.TestParameter;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import com.google.testing.junit.testparameterinjector.TestParameters;
+import java.math.BigDecimal;
 import java.math.BigInteger;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -28,20 +30,24 @@ public final class ComparisonFunctionsTest {
 
   @Test
   @TestParameters("{x: 1.0, y: 1, expect: 0}")
+  @TestParameters("{x: -0.0, y: 0, expect: 0}")
   @TestParameters("{x: -2.1, y: -2, expect: -1}")
   @TestParameters("{x: 2.1, y: -3, expect: 1}")
   @TestParameters("{x: -9223372036854778809.0, y: 1, expect: -1}")
   @TestParameters("{x: 9223372036854778809.0, y: 1, expect: 1}")
+  @TestParameters("{x: NaN, y: 0, expect: 1}")
   public void compareDoubleInt(double x, long y, int expect) {
     assertThat(ComparisonFunctions.compareDoubleInt(x, y)).isEqualTo(expect);
     assertThat(ComparisonFunctions.compareIntDouble(y, x)).isEqualTo(-1 * expect);
   }
 
   @Test
   @TestParameters("{x: 1.0, y: 1, expect: 0}")
+  @TestParameters("{x: -0.0, y: 0, expect: 0}")
   @TestParameters("{x: -2.1, y: 0, expect: -1}")
   @TestParameters("{x: 2.1, y: 1, expect: 1}")
   @TestParameters("{x: 18446744073709653620.1, y: 1, expect: 1}")
+  @TestParameters("{x: NaN, y: 0, expect: 1}")
   public void compareDoubleUint(double x, long y, int expect) {
     UnsignedLong uy = UnsignedLong.valueOf(y);
     assertThat(ComparisonFunctions.compareDoubleUint(x, uy)).isEqualTo(expect);
@@ -93,4 +99,49 @@ public void numericCompareUintInt(long x, long y, int expect) {
     assertThat(ComparisonFunctions.numericCompare(ux, y)).isEqualTo(expect);
     assertThat(ComparisonFunctions.numericCompare(y, ux)).isEqualTo(-1 * expect);
   }
+
+  @Test
+  @TestParameters("{x: -0.0, y: 0.0, expect: 0}")
+  @TestParameters("{x: 1.0, y: 2.0, expect: -1}")
+  @TestParameters("{x: NaN, y: 1.0, expect: 1}")
+  @TestParameters("{x: NaN, y: NaN, expect: 0}")
+  public void numericCompareDoubleDouble(double x, double y, int expect) {
+    assertThat(ComparisonFunctions.numericCompare(x, y)).isEqualTo(expect);
+    assertThat(ComparisonFunctions.numericCompare(y, x)).isEqualTo(-1 * expect);
+  }
+
+  @SuppressWarnings("ImmutableEnumChecker") // Test only
+  private enum NumericEqualsTestCase {
+    NEGATIVE_ZERO_AND_POSITIVE_ZERO(-0.0, 0.0, true),
+    NEGATIVE_ZERO_AND_LONG_ZERO(-0.0, 0L, true),
+    NEGATIVE_ZERO_AND_ULONG_ZERO(-0.0, UnsignedLong.ZERO, true),
+    DOUBLE_ONE_AND_LONG_ONE(1.0, 1L, true),
+    DOUBLE_ONE_AND_ULONG_ONE(1.0, UnsignedLong.ONE, true),
+    DISTINCT_DOUBLES(1.0, 2.0, false),
+    DISTINCT_DOUBLE_AND_LONG(1.0, 2L, false),
+    NAN_AND_NAN(Double.NaN, Double.NaN, false),
+    NAN_AND_LONG_ZERO(Double.NaN, 0L, false),
+    NAN_AND_ULONG_ZERO(Double.NaN, UnsignedLong.ZERO, false),
+    SHORT_AND_LONG((short) 1, 1L, false),
+    SHORT_AND_NAN((short) 1, Double.NaN, false),
+    BIG_DECIMAL_AND_DOUBLE(BigDecimal.ONE, 1.0, false);
+
+    private final Number x;
+    private final Number y;
+    private final boolean expected;
+
+    NumericEqualsTestCase(Number x, Number y, boolean expected) {
+      this.x = x;
+      this.y = y;
+      this.expected = expected;
+    }
+  }
+
+  @Test
+  public void numericEquals(@TestParameter NumericEqualsTestCase testCase) {
+    assertThat(ComparisonFunctions.numericEquals(testCase.x, testCase.y))
+        .isEqualTo(testCase.expected);
+    assertThat(ComparisonFunctions.numericEquals(testCase.y, testCase.x))
+        .isEqualTo(testCase.expected);
+  }
 }