[
https://issues.apache.org/jira/browse/GROOVY-12699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138766#comment-18138766
]
ASF GitHub Bot commented on GROOVY-12699:
-----------------------------------------
codecov-commenter commented on PR #3223:
URL: https://github.com/apache/groovy/pull/3223#issuecomment-6658777923
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3223?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 74.1763%. Comparing base
([`38b012e`](https://app.codecov.io/gh/apache/groovy/commit/38b012ed693b6d791a7efde760056e4086b8b0fc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`87c9a5f`](https://app.codecov.io/gh/apache/groovy/commit/87c9a5f84fce8a4b19e833eb9880bfbd4df46fb2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/3223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3223 +/- ##
==================================================
+ Coverage 74.1656% 74.1763% +0.0107%
- Complexity 43752 43764 +12
==================================================
Files 1692 1692
Lines 149591 149599 +8
Branches 28382 28386 +4
==================================================
+ Hits 110945 110967 +22
+ Misses 28435 28428 -7
+ Partials 10211 10204 -7
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3223?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...untime/typehandling/DefaultTypeTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3223?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2Ftypehandling%2FDefaultTypeTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvdHlwZWhhbmRsaW5nL0RlZmF1bHRUeXBlVHJhbnNmb3JtYXRpb24uamF2YQ==)
| `64.8855% <100.0000%> (ø)` | |
... and [17 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3223/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> == 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)
