[
https://issues.apache.org/jira/browse/GROOVY-12699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138703#comment-18138703
]
ASF GitHub Bot commented on GROOVY-12699:
-----------------------------------------
paulk-asert opened a new pull request, #3223:
URL: https://github.com/apache/groovy/pull/3223
…that <=> and < honour
> == 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
> Priority: Major
>
> 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)
