Repository: smithy-lang/smithy
Pull request #1965 (2023-09-01 20:49:57)

Improve NumberNode
This change now makes the BigDecimal get created eagerly, and accounts
for NaN and Infinity. The previous implementation would have failed
in these cases. NodeValidationVisitor now uses the computed BigDecimal
rather than create a one-off each time. It also doesn't recreate the
already available BigDecimcal of the range trait.

This change deprecates isNaturalNumber since that method was poorly
named. A natural number is an integer >= 1, but we were including 0
(which is likely why some callers assumed it asserts >= 0).

This also adds an isNegative method since multiple callers need to
assert that a NumberNode contains a non-negative value.


Patch:
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
index a52349cb5..dd4572510 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
  *
  * Licensed under the Apache License, Version 2.0 (the "License").
  * You may not use this file except in compliance with the License.
@@ -31,17 +31,45 @@ import software.amazon.smithy.model.SourceLocation;
  */
 public final class NumberNode extends Node {
 
-    private final Number value;
+    private final BigDecimal value;
+    private final Number originalValue;
     private final String stringCache;
-    private volatile BigDecimal equality;
+    private boolean isNaN;
+    private boolean isPositiveInfinity;
+    private boolean isNegativeInfinity;
 
     public NumberNode(Number value, SourceLocation sourceLocation) {
         super(sourceLocation);
-        this.value = Objects.requireNonNull(value);
+        originalValue = value;
         stringCache = value.toString();
+        this.value = toBigDecimal(originalValue);
+    }
 
+    private BigDecimal toBigDecimal(Number value) {
         if (value instanceof BigDecimal) {
-            equality = (BigDecimal) value;
+            return (BigDecimal) value;
+        } else if (value instanceof Integer || value instanceof Long || value instanceof Short
+                   || value instanceof Byte) {
+            return BigDecimal.valueOf(value.longValue());
+        } else if (value instanceof Float || value instanceof Double) {
+            double d = value.doubleValue();
+            if (Double.isNaN(d)) {
+                isNaN = true;
+                return null;
+            } else if (Double.isInfinite(d)) {
+                if (stringCache.startsWith("-")) {
+                    isNegativeInfinity = true;
+                } else {
+                    isPositiveInfinity = true;
+                }
+                return null;
+            } else {
+                return BigDecimal.valueOf(d);
+            }
+        } else if (value instanceof BigInteger) {
+            return new BigDecimal((BigInteger) value);
+        } else {
+            return new BigDecimal(stringCache);
         }
     }
 
@@ -51,28 +79,61 @@ public final class NumberNode extends Node {
      * @return Returns a number.
      */
     public Number getValue() {
-        return value;
+        return originalValue;
     }
 
     /**
-     * Returns true if the node contains a natural number.
+     * Gets the number value as a BigDecimal if possible.
      *
-     * @return Returns true if the node contains a natural number.
+     * <p>NaN and infinite numbers will return an empty Optional.
+     *
+     * @return Returns the BigDecimal value of the wrapped number.
      */
+    public Optional<BigDecimal> asBigDecimal() {
+        return Optional.ofNullable(value);
+    }
+
+    @Deprecated
     public boolean isNaturalNumber() {
         return !isFloatingPointNumber();
     }
 
+    /**
+     * Check the value is negative, including negative infinity.
+     *
+     * <p>Any number >= 0, +Infinity, and NaN return false.
+     *
+     * @return Return true if negative.
+     */
+    public boolean isNegative() {
+        return isNegativeInfinity || (value != null && value.compareTo(BigDecimal.ZERO) < 0);
+    }
+
     /**
      * Returns true if the node contains a floating point number.
      *
      * @return Returns true if the node contains a floating point number.
      */
     public boolean isFloatingPointNumber() {
-        return value instanceof Float
-                || value instanceof Double
-                || value instanceof BigDecimal
-                || stringCache.contains(".");
+        return value == null || value.scale() > 0 || toString().contains(".");
+    }
+
+    /**
+     * Returns true if the number is a floating point NaN.
+     *
+     * @return Return true if NaN.
+     */
+    public boolean isNaN() {
+        return isNaN;
+    }
+
+    /**
+     * Returns true if the number is infinite.
+     *
+     * @return Return true if infinite.
+     */
+    public boolean isInfinite() {
+        return isPositiveInfinity || isNegativeInfinity;
     }
 
     @Override
@@ -117,19 +178,7 @@ public final class NumberNode extends Node {
      * @return Returns true if set to zero.
      */
     public boolean isZero() {
-        // Do a cheap test based on the serialized value of the number first.
-        // This test covers byte, short, integer, and long.
-        if (toString().equals("0") || toString().equals("0.0")) {
-            return true;
-        } else if (value instanceof BigDecimal) {
-            return value.equals(BigDecimal.ZERO);
-        } else if (value instanceof BigInteger) {
-            return value.equals(BigInteger.ZERO);
-        } else if (value instanceof Float) {
-            return value.floatValue() == 0f;
-        } else {
-            return value.doubleValue() == 0d;
-        }
+        return value != null && value.compareTo(BigDecimal.ZERO) == 0;
     }
 
     @Override
@@ -139,42 +188,12 @@ public final class NumberNode extends Node {
         } else if (other == this) {
             return true;
         } else {
-            NumberNode otherNode = (NumberNode) other;
-
-            // This only works if both values are the same type.
-            if (value.equals(otherNode.value)) {
-                return true;
-            }
-
-            // Attempt a cheap check based on the string cache.
-            if (stringCache.equals(otherNode.stringCache)) {
-                return true;
-            }
-
-            // Convert both to BigDecimal and compare equality.
-            return getEquality().equals(otherNode.getEquality());
-        }
-    }
-
-    private BigDecimal getEquality() {
-        BigDecimal e = equality;
-
-        if (e == null) {
-            if (value instanceof Integer) {
-                e = BigDecimal.valueOf(value.intValue());
-            } else if (value instanceof Short) {
-                e = BigDecimal.valueOf(value.shortValue());
-            } else if (value instanceof Byte) {
-                e = BigDecimal.valueOf(value.byteValue());
-            } else if (value instanceof Long) {
-                e = BigDecimal.valueOf(value.longValue());
-            } else {
-                e = new BigDecimal(stringCache);
-            }
-            equality = e;
+            NumberNode o = (NumberNode) other;
+            return isNaN == o.isNaN
+                   && isPositiveInfinity == o.isPositiveInfinity
+                   && isNegativeInfinity == o.isNegativeInfinity
+                   && Objects.equals(value, o.value);
         }
-
-        return e;
     }
 
     @Override
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java
index 330b377f7..a3e74ebd2 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java
@@ -218,7 +218,7 @@ public final class NodeValidationVisitor implements ShapeVisitor<List<Validation
     private List<ValidationEvent> validateNaturalNumber(Shape shape, Long min, Long max) {
         return value.asNumberNode()
                 .map(number -> {
-                    if (!number.isNaturalNumber()) {
+                    if (number.isFloatingPointNumber()) {
                         return ListUtils.of(event(String.format(
                                 "%s shapes must not have floating point values, but found `%s` provided for `%s`",
                                 shape.getType(), number.getValue(), shape.getId())));


Test patch:
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
index 910860574..b146cc1a9 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
@@ -17,6 +17,7 @@ package software.amazon.smithy.model.node;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -199,6 +200,11 @@ public class NumberNodeTest {
             public double doubleValue() {
                 return 0;
             }
+
+            @Override
+            public String toString() {
+                return "0.000";
+            }
         }, true);
         cases.put(new Number() {
             @Override
@@ -283,4 +289,14 @@ public class NumberNodeTest {
 
         assertThat(left, equalTo(right));
     }
+
+    @Test
+    public void detectsNegativeValues() {
+        assertThat(NumberNode.from(Double.NEGATIVE_INFINITY).isNegative(), is(true));
+        assertThat(NumberNode.from(Double.POSITIVE_INFINITY).isNegative(), is(false));
+        assertThat(NumberNode.from(Double.NaN).isNegative(), is(false));
+        assertThat(NumberNode.from(0).isNegative(), is(false));
+        assertThat(NumberNode.from(1).isNegative(), is(false));
+        assertThat(NumberNode.from(-1).isNegative(), is(true));
+    }
 }