jqno#540 BigDecimal equality using compareTo

docs/_errormessages/bigdecimal-equality.md

@@ -0,0 +1,99 @@
+---
+title: "BigDecimal equality"
+---
+Be aware that `BigDecimal` fields with values such as `1`, `1.0`, `1.00`, ... are not considered equal.
+
+The `Comparable` interface strongly recommends but does not require that implementations consider two objects equal using
+`compareTo` whenever they are equal using `equals` and vice versa. `BigDecimal` is a class where this is not applied.
+
+{% highlight java %}
+BigDecimal one = new BigDecimal("1");
+BigDecimal alsoOne = new BigDecimal("1.0");
+
+// prints true - 1 is the same as 1.0
+System.out.println(one.compareTo(alsoOne) == 0);
+// prints false - 1 is not the same as 1.0
+System.out.println(one.equals(alsoOne));
+{% endhighlight %}
+
+Ways to resolve this error
+---
+If values like `1` and `1.0` need not be considered equal then this check can be disabled by suppressing `Warning.BIGDECIMAL_EQUALITY`.
+
+{% highlight java %}
+EqualsVerifier.forClass(Foo.class)
+    .suppress(Warning.BIGDECIMAL_EQUALITY)
+    .verify();
+{% endhighlight %}
+
+If values like `1` and `1.0` *should* be considered equal then some options are:
+1. **Do not use `BigDecimal` as fields.** But unfortunately I cannot recommend well-known generally accepted drop-in alternative.
+
+    The argument for this: it is not great having to complicate classes with the options below. A valid `equals`and
+    `hashCode` is already easy enough to get wrong (option 2) and it is easy to forget and hard to validate `BigDecimal`
+    fields have been normalised (option 3). More special handling in `equals` and `hashCode` moves against standardisation,
+    such as making things easier e.g. `Objects.equals`, `Objects.hashCode`, or eliminating the need to think about it e.g.
+    Lombok's `@EqualsAndHashCode` or Java 16 (preview since 14) `record` classes: it is a shame having to add boilerplate
+    for every [`@EqualsAndHashCode` annotated class](https://stackoverflow.com/questions/36625347/how-to-make-lomboks-equalsandhashcode-work-with-bigdecimal)
+    or every [`record` class](https://stackoverflow.com/questions/68690126/java-16-records-bigdecimal-equals-hashcode)
+    that has a `BigDecimal` field.
+
+2. **Implement `equals`  to use `compareTo` for `BigDecimal` fields** and ensure values of those fields that are equal using
+    `compareTo` will produce the same hashcode.
+
+    EqualsVerifier checks this by default which causes the *BigDecimal equality* error messages.
+
+    It wouldn't be unwise to use utility methods as this is not as simple as a call to Java's `Objects.equals` and `Objects.hashcode`.
+    The logic for correct equality is:
+
+    {% highlight java %}
+    // true if bdField and other.bdField are
+    // either both null or are equal using compareTo
+    boolean comparablyEqual = (bdField == null && other.bdField == null)
+        || (bdField != null && other.bdField != null && bdField.compareTo(other.bdField) == 0);
+    {% endhighlight %}
+
+    A consistent hashcode needs a way to normalise the value that it represents. A simple normalisation is:
+
+    {% highlight java %}
+    // Remove trailing zeros from the unscaled value of the
+    // BigDecimal to yield a consistently scaled instance
+    int consistentHashcode = Objects.hashCode(bdField.stripTrailingZeros());
+    {% endhighlight %}
+
+3. **Normalise the `BigDecimal` fields** during your class's construction (and setters if it is mutable) such that there
+    is only one way it represents the same value for these fields. This means standard `equals` and `hashCode` can be used.
+    For example, your class ensures all variants of `1` such as `1.0`, `1.00`, etc are converted to `1` in its constructor.
+
+    A simple normalisation is:
+
+    {% highlight java %}
+    class Foo {
+        ...
+        Foo(BigDecimal bdField) {
+            // Now if bdField is final it will only have instances
+            // that are equal when compareTo is equal.
+            // If mutable then setters will need to do the same.
+            this.bdField = bdField.stripTrailingZeros();
+        }
+    }
+    {% endhighlight %}
+
+    Unfortunately it is difficult to confirm this has been done. This check will then want disabling by suppressing `Warning.BIGDECIMAL_EQUALITY`
+    and will not catch regressions.
+
+If performance is important then you will need to consider the costs of using `BigDecimal` and of where and how normalisation
+is achieved. Option 2 performs the work when objects are stored in a `HashSet` or used as keys in a `HashMap`. Option 3
+performs work on creation of each object but is then cheaper to hash. There may be better normalisations than `stripTrailingZeros`.
+`BigDecimal` already has a cost traded in return for its accuracy.
+
+Why does this happen for BigDecimal?
+---
+`BigDecimal` can have multiple representations of the same value. It uses an *unscaled value* and a *scale* (both integers).
+For example, the value of 1 can be represented as unscaled value 1 with scale of 0 (scale is the number of places after
+the decimal point when 0 or greater) or as unscaled value 10 with scale of 1 resolving to 1.0. Its `equals` and `hashCode`
+methods use both of these attributes in their calculation rather than the resolved value.
+
+There is more information on `compareTo` and `equals` in the `Comparable` Javadoc and Effective Java's chapter on implementing `Comparable`.
+
+There is more information on `BigDecimal` in its Javadoc (and its representation can be seen by printing `unscaledValue()` and `scale()`).

docs/_manual/12-legacy-systems.md

@@ -17,6 +17,7 @@ For these situations, there are several types of warnings that you can suppress:
 * `Warning.STRICT_HASHCODE`: disables the check that all fields used in `equals` must also be used in `hashCode`.
 * `Warning.STRICT_INHERITANCE`: disables the check that classes or their `equals` methods be final, or that inheritance is properly accounted for. Read more about this topic on the [page about inheritance](/equalsverifier/manual/inheritance).
 * `Warning.TRANSIENT_FIELDS`: disables the check that transient fields do not participate in `equals`. This applies both to Java's `transient` keyword, which applies to serialization, and to JPA's `@Transient` annotation, which applies to, well, JPA.
+* `Warning.BIGDECIMAL_EQUALITY`: disables the check that equality of `BigDecimal` fields is implemented using `compareTo` rather than `equals`. Read more about this topic on the [page about BigDecimal equality](/equalsverifier/errormessages/bigdecimal-equality).
 
 Of course, once you have sufficient test coverage, you _will_ come back and fix these issues, right? 😉
 

docs/_pages/errormessages.md

@@ -19,13 +19,14 @@ Before you continue, please check your class's `toString` method. Since EqualsVe
 This is not a complete list. I'll add to it as needed, so if you need help with an error message, please [file an issue](https://github.com/jqno/equalsverifier/issues) or let me know on the [discussion group](https://groups.google.com/forum/?fromgroups#!forum/equalsverifier), and I'll add an explanation as soon as possible.
 
 * [Abstract delegation](/equalsverifier/errormessages/abstract-delegation)
+* [BigDecimal equality](/equalsverifier/errormessages/bigdecimal-equality)
 * [ClassCastException: java.lang.Object cannot be cast to …](/equalsverifier/errormessages/classcastexception)
 * [Coverage is not 100%](/equalsverifier/errormessages/coverage-is-not-100-percent)
 * [Double: equals doesn't use Double.compare for field foo](/equalsverifier/errormessages/double-equals-doesnt-use-doublecompare-for-field-foo)
 * [Float: equals doesn't use Float.compare for field foo](/equalsverifier/errormessages/float-equals-doesnt-use-floatcompare-for-field-foo)
 * [Mutability: equals depends on mutable field](/equalsverifier/errormessages/mutability-equals-depends-on-mutable-field)
 * [NoClassDefFoundError](/equalsverifier/errormessages/noclassdeffounderror)
-* [Non-nullity: equals/hashCode/toString throws NullPointerExcpetion](/equalsverifier/errormessages/non-nullity-equals-hashcode-tostring-throws-nullpointerexception)
+* [Non-nullity: equals/hashCode/toString throws NullPointerException](/equalsverifier/errormessages/non-nullity-equals-hashcode-tostring-throws-nullpointerexception)
 * [Precondition: two objects are equal to each other](/equalsverifier/errormessages/precondition-two-objects-are-equal-to-each-other)
 * [Recursive datastructure](/equalsverifier/errormessages/recursive-datastructure)
 * [Redefined superclass: object should not equal superclass instance](/equalsverifier/errormessages/redefined-superclass-object-should-not-equal-superclass-instance)

src/main/java/nl/jqno/equalsverifier/Warning.java

@@ -166,5 +166,20 @@ public enum Warning {
      * <p>If measures are taken that this will never happen, this warning can be suppressed to
      * disable {@link EqualsVerifier}'s transience test.
      */
-    TRANSIENT_FIELDS
+    TRANSIENT_FIELDS,
+
+    /**
+     * Disables the check that equality of {@code BigDecimal} fields is implemented using
+     * {@code compareTo} rather than {@code equals}.
+     *
+     * <p>{@code BigDecimal} objects that are equal using {@code compareTo} are not necessarily
+     * equal using {@code equals}, for example the values of {@literal 1} and {@literal 1.0}.
+     * For variants of the same value to be considered equal, classes with {@code BigDecimal}
+     * fields should use {@code compareTo} to check equality of non-null {@code BigDecimal}
+     * references and produce the same hashcode for all equal variants.
+     *
+     * <p>{@code EqualsVerifier} checks for this by default but it can be disabled by suppressing
+     * this warning.
+     */
+    BIGDECIMAL_EQUALITY
 }

src/main/java/nl/jqno/equalsverifier/internal/checkers/FieldsChecker.java

@@ -23,6 +23,7 @@ public class FieldsChecker<T> implements Checker {
     private final SymmetryFieldCheck<T> symmetryFieldCheck;
     private final TransientFieldsCheck<T> transientFieldsCheck;
     private final TransitivityFieldCheck<T> transitivityFieldCheck;
+    private final BigDecimalFieldCheck<T> bigDecimalFieldCheck;
 
     public FieldsChecker(Configuration<T> config) {
         this.config = config;
@@ -48,6 +49,8 @@ public FieldsChecker(Configuration<T> config) {
         this.symmetryFieldCheck = new SymmetryFieldCheck<>(prefabValues, typeTag);
         this.transientFieldsCheck = new TransientFieldsCheck<>(config);
         this.transitivityFieldCheck = new TransitivityFieldCheck<>(prefabValues, typeTag);
+        this.bigDecimalFieldCheck =
+            new BigDecimalFieldCheck<>(config.getCachedHashCodeInitializer());
     }
 
     @Override
@@ -80,6 +83,10 @@ public void check() {
                 skippingSignificantFieldCheck
             );
         }
+
+        if (!config.getWarningsToSuppress().contains(Warning.BIGDECIMAL_EQUALITY)) {
+            inspector.check(bigDecimalFieldCheck);
+        }
     }
 
     private boolean ignoreMutability(Class<?> type) {

src/main/java/nl/jqno/equalsverifier/internal/checkers/fieldchecks/BigDecimalFieldCheck.java

@@ -0,0 +1,90 @@
+package nl.jqno.equalsverifier.internal.checkers.fieldchecks;
+
+import static nl.jqno.equalsverifier.internal.util.Assert.assertEquals;
+
+import java.lang.reflect.Field;
+import java.math.BigDecimal;
+import java.math.RoundingMode;
+import nl.jqno.equalsverifier.Warning;
+import nl.jqno.equalsverifier.internal.reflection.FieldAccessor;
+import nl.jqno.equalsverifier.internal.reflection.ObjectAccessor;
+import nl.jqno.equalsverifier.internal.util.CachedHashCodeInitializer;
+import nl.jqno.equalsverifier.internal.util.Formatter;
+
+public class BigDecimalFieldCheck<T> implements FieldCheck<T> {
+
+    public static final String ERROR_DOC_TITLE = "BigDecimal equality";
+
+    private final CachedHashCodeInitializer<T> cachedHashCodeInitializer;
+
+    public BigDecimalFieldCheck(CachedHashCodeInitializer<T> cachedHashCodeInitializer) {
+        this.cachedHashCodeInitializer = cachedHashCodeInitializer;
+    }
+
+    @Override
+    public void execute(
+        ObjectAccessor<T> referenceAccessor,
+        ObjectAccessor<T> copyAccessor,
+        FieldAccessor fieldAccessor
+    ) {
+        if (BigDecimal.class.equals(fieldAccessor.getFieldType())) {
+            Field field = fieldAccessor.getField();
+            BigDecimal referenceField = (BigDecimal) referenceAccessor.getField(field);
+            BigDecimal changedField = referenceField.setScale(
+                referenceField.scale() + 1,
+                RoundingMode.UNNECESSARY
+            );
+            ObjectAccessor<T> changed = copyAccessor.withFieldSetTo(field, changedField);
+
+            T left = referenceAccessor.get();
+            T right = changed.get();
+
+            checkEquals(field, referenceField, changedField, left, right);
+            checkHashCode(field, referenceField, changedField, left, right);
+        }
+    }
+
+    private void checkEquals(
+        Field field,
+        BigDecimal referenceField,
+        BigDecimal changedField,
+        T left,
+        T right
+    ) {
+        Formatter f = Formatter.of(
+            ERROR_DOC_TITLE +
+            ": object does not equal a copy of itself" +
+            " where BigDecimal field %% has a value that is equal using compareTo: %% compared to %%" +
+            "\nIf these values should be considered equal then use compareTo rather than equals for this field." +
+            "\nIf these values should not be considered equal, suppress Warning.%% to disable this check.",
+            field.getName(),
+            referenceField,
+            changedField,
+            Warning.BIGDECIMAL_EQUALITY
+        );
+        assertEquals(f, left, right);
+    }
+
+    private void checkHashCode(
+        Field field,
+        BigDecimal referenceField,
+        BigDecimal changedField,
+        T left,
+        T right
+    ) {
+        Formatter f = Formatter.of(
+            ERROR_DOC_TITLE +
+            ": hashCode of object does not equal hashCode of a copy of itself" +
+            " where BigDecimal field %% has a value that is equal using compareTo: %% compared to %%" +
+            "\nIf these values should be considered equal then make sure to derive the same constituent hashCode from this field." +
+            "\nIf these values should not be considered equal, suppress Warning.%% to disable this check.",
+            field.getName(),
+            referenceField,
+            changedField,
+            Warning.BIGDECIMAL_EQUALITY
+        );
+        int leftHashCode = cachedHashCodeInitializer.getInitializedHashCode(left);
+        int rightHashCode = cachedHashCodeInitializer.getInitializedHashCode(right);
+        assertEquals(f, leftHashCode, rightHashCode);
+    }
+}