Paul King created GROOVY-12699:
----------------------------------
Summary: == on a non-Comparable Number ignores numeric equality
that <=> and < honour
Key: GROOVY-12699
URL: https://issues.apache.org/jira/browse/GROOVY-12699
Project: Groovy
Issue Type: Bug
Reporter: Paul King
h3. Description
{{compareToWithEqualityCheck}} accepts a {{Number}} on the left even when it
isn't {{Comparable}}, but {{compareEqual}} doesn't. So for a {{Number}}
subclass that doesn't implement {{Comparable}}, {{<=>}}, {{<}} and friends
compare numerically (via {{NumberMath}}), while {{==}} falls through to
{{equals()}}:
{code:groovy}
class Frac extends Number { // a Number, but not Comparable
final BigInteger n, d
Frac(long n, long d) { this.n = n; this.d = d }
int intValue() { (n / d) as int }
long longValue() { (n / d) as long }
float floatValue() { doubleValue() as float }
double doubleValue() { n.doubleValue() / d.doubleValue() }
String toString() { "$n/$d" }
}
def f = new Frac(1, 4)
assert (f <=> 0.25) == 0 // numerically equal
assert !(f > 0.25) && !(f < 0.25)
assert !(f == 0.25) // but not ==
assert !(f == 0.25d)
assert ![f].contains(0.25)
{code}
A real-world case is JSR 385's reference implementation, indriya, whose
quantity values are often a {{tech.units.indriya.function.RationalNumber}}
({{extends Number}}, not {{Comparable}}):
{code:groovy}
@Grab('tech.units:indriya:2.2.4')
import tech.units.indriya.function.RationalNumber
import tech.units.indriya.quantity.Quantities
import static tech.units.indriya.unit.Units.METRE
import static javax.measure.MetricPrefix.CENTI
def v = Quantities.getQuantity(1, CENTI(METRE)).to(METRE).value
assert v instanceof RationalNumber
assert (v <=> 0.01) == 0
assert !(v == 0.01) // surprising
assert !(RationalNumber.of(4, 1) == 4) // even a whole-number value
{code}
The same happens under {{@CompileStatic}}: both {{==}} and {{<=>}} compile to
the same runtime helpers ({{ScriptBytecodeAdapter.compareEqual}} /
{{ScriptBytecodeAdapter.compareTo}}, a primitive operand boxed first), whose
same-class JDK fast path doesn't apply here, so they reach
{{DefaultTypeTransformation}} exactly as dynamic code does:
{code:groovy}
@groovy.transform.CompileStatic
class CS {
static boolean eq(Number f) { f == 0.25G } // false for new Frac(1, 4)
static boolean eqD(Number f) { f == 0.25d } // false
static int cmp(Frac f) { f <=> 0.25G } // 0
static boolean rev(Frac f) { 0.25G == f } // true
}
{code}
Observed on 6.0.0, dynamic and {{@CompileStatic}} alike (and still true in
master).
h3. Cause
{{DefaultTypeTransformation.compareToWithEqualityCheck}} opens with
{code:java}
if (left instanceof Comparable || left instanceof Number) {
if (left instanceof Number) {
if (right instanceof Character || right instanceof Number) {
return DefaultGroovyMethods.compareTo((Number) left,
castToNumber(right));
{code}
so {{compareTo}} (used by {{<=>}}, {{<}}, ...) reaches {{NumberMath.compareTo}}
for any {{Number}}, and {{NumberMath.getMath}} deliberately maps an unknown
{{Number}} to {{BigDecimalMath}} ("also for custom Number implementations").
But {{compareEqual}} only routes through that method under
{code:java}
if (left instanceof Comparable) {
return compareToWithEqualityCheck(left, right, true) == 0;
}
{code}
with no matching {{Number}} clause, so a non-Comparable {{Number}} falls
through to {{left.equals(right)}}.
h3. Suggested fix
Route a {{Number}}-vs-{{Number}} comparison numerically in {{compareEqual}} too:
{code:java}
if (left instanceof Comparable || (left instanceof Number && right instanceof
Number)) {
return compareToWithEqualityCheck(left, right, true) == 0;
}
{code}
Requiring both sides to be numbers matters: widening to {{left instanceof
Number}} alone would send e.g. {{frac == 'hello'}} into the GROOVY-7954 {{right
instanceof Comparable}} branch, whose {{invokeMethod(left, "compareTo",
right)}} throws {{MissingMethodException}} for a non-Comparable left (only
{{ClassCastException}} is caught there). With both numbers, the {{Number}}
branch always returns before that.
h3. Compatibility
A non-Comparable {{Number}} whose {{equals()}} intentionally differs from
numeric equality would see {{==}} change. That seems unlikely to be relied on
given {{<=>}} already reports the numeric answer, but it may merit a
breaking-change note. Note also that the numeric route converts an unknown
{{Number}} via {{new BigDecimal(n.toString())}}, falling back to
{{doubleValue()}}, which is what {{<=>}} already does today.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
