[
https://issues.apache.org/jira/browse/GROOVY-12439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18125102#comment-18125102
]
ASF GitHub Bot commented on GROOVY-12439:
-----------------------------------------
codecov-commenter commented on PR #2958:
URL: https://github.com/apache/groovy/pull/2958#issuecomment-6063170769
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2958?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 71.6110%. Comparing base
([`23e5e69`](https://app.codecov.io/gh/apache/groovy/commit/23e5e69dc72f31643457a40c8ec79bed19c30608?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`8cc704d`](https://app.codecov.io/gh/apache/groovy/commit/8cc704d31d58a060f68b309dac1011d085b1f84b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2958?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2958 +/- ##
==================================================
+ Coverage 71.6089% 71.6110% +0.0021%
- Complexity 38061 38069 +8
==================================================
Files 1588 1588
Lines 137078 137081 +3
Branches 25507 25508 +1
==================================================
+ Hits 98160 98165 +5
- Misses 30011 30012 +1
+ Partials 8907 8904 -3
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2958?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...a/org/codehaus/groovy/runtime/MetaClassHelper.java](https://app.codecov.io/gh/apache/groovy/pull/2958?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FMetaClassHelper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvTWV0YUNsYXNzSGVscGVyLmphdmE=)
| `36.0934% <100.0000%> (-0.0177%)` | :arrow_down: |
... and [7 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2958/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>
> Regression: BigDecimal silently narrowed to float in overload selection since
> 4.0.4
> -----------------------------------------------------------------------------------
>
> Key: GROOVY-12439
> URL: https://issues.apache.org/jira/browse/GROOVY-12439
> Project: Groovy
> Issue Type: Bug
> Components: groovy-runtime
> Affects Versions: 4.0.4, 5.1.3, 6.0.0
> Environment: Temurin JDK 17, 21, 25 and 27 on Linux x64 — behaviour
> is identical on all four, so this is not JDK-specific. Plain dynamic Groovy:
> no @CompileStatic or @TypeChecked, no metaprogramming, no third-party
> libraries.
> Reporter: Stanley Brubaker
> Assignee: Paul King
> Priority: Critical
>
> A varargs call that selected a {{double}} overload up to 4.0.3 now silently
> selects a narrowing {{float}} one, keeping about 7 significant digits instead
> of 16 -- or, when exactly one integral argument accompanies the
> {{BigDecimal}}, fails with {{GroovyRuntimeException}} instead. Which of the
> two you get depends only on how many integral arguments are present. Code
> that worked on 4.0.3 therefore either loses precision without warning or dies
> at runtime.
> The root cause is that in 4.0.4 a {{BigDecimal}} argument became applicable
> to a {{float}} *varargs* parameter; scalar {{float}} parameters already
> accepted it. From 5.0.0 a tie between scalar overloads also resolves to
> {{float}}. So where both {{float}} and {{double}} overloads exist, the
> outcome now depends only on argument count and call shape: {{double}}, an
> ambiguity error, or a silently narrowing {{float}}.
> h2. Reproducer
> {code:java}
> public class Sel {
> public static String m(float... x) { return "float..."; }
> public static String m(double... x) { return "double..."; }
> public static String m2(float a, float b) { return "float(2)"; }
> public static String m2(double a, double b) { return "double(2)"; }
> }
> {code}
> {code:groovy}
> import static Sel.*
> z = 1
> d = 5.0 // untyped decimal literal, therefore BigDecimal
> println m(d)
> println m(z, d)
> println m(5, z, d)
> println m2(z, d)
> {code}
> {code}
> javac --release 17 Sel.java // --release 17 so pre-4.0.33 ASM can read
> the class
> java -cp groovy-<version>.jar:. groovy.ui.GroovyMain repro.groovy
> {code}
> ||call||3.0.25||4.0.3||4.0.4||5.0.0 - 6.0.0||
> |{{m(d)}}|double...|double...|double...|double...|
> |{{m(z, d)}}|double...|double...|*AMBIGUOUS*|*AMBIGUOUS*|
> |{{m(5, z, d)}}|double...|double...|*float...*|*float...*|
> |{{m2(z, d)}}|AMBIGUOUS|AMBIGUOUS|AMBIGUOUS|*float(2)*|
> Expected {{double}} in all three failing cases. They regress differently:
> ||call||behaviour||
> |{{m(d)}}|{{double...}} on every version 3.0.25 - 6.0.0. Unchanged, included
> as a control.|
> |{{m(z, d)}}|{{double...}} up to 4.0.3, ambiguity error from 4.0.4. One
> integral argument gives a 10/10 tie.|
> |{{m(5, z, d)}}|{{double...}} up to 4.0.3, silently narrowing {{float...}}
> from 4.0.4. Two integral arguments outvote the {{BigDecimal}}: 15 against 17.|
> |{{m2(z, d)}}|ambiguity error 3.0.25 - 4.0.33, then silently resolves to the
> narrowing {{float(2)}} from 5.0.0.|
> h2. Cause
> Two independent changes, each bisected to a single release.
> *4.0.4 -- BigDecimal became applicable to float varargs.*
> {{ParameterTypes.isValidVargsMethod}} swapped its applicability predicate in
> ef7026317f ("GROOVY-8045: support coercion for variadic parameter"):
> {code:java}
> // 4.0.3
> if (MetaClassHelper.isAssignableFrom(clazz, arguments[i])) continue;
> // 4.0.4
> if (!componentType.isAssignableFrom(argumentTypes[i])) { return false; }
> {code}
> {{MetaClassHelper.isAssignableFrom}} and {{CachedClass.isAssignableFrom}}
> disagree about {{BigDecimal}} -> {{float}}. With a class exposing *only*
> {{float...}}, so no competing candidate and no distance comparison involved,
> 4.0.3 raises {{MissingMethodException}} for {{f(BigDecimal)}} while 4.0.4
> accepts it. So {{float...}} was previously not a candidate at all and
> {{double...}} won by being the only applicable overload.
> Note this predicate is on the varargs path only, so {{BigDecimal}} ->
> {{float}} was already applicable for *scalar* parameters before 4.0.4. That
> is why {{m2(z, d)}} was ambiguous as far back as 3.0.25 and why only the
> varargs row changed in 4.0.4.
> Once both are applicable, {{PRIMITIVE_DISTANCE_TABLE}} decides.
> {{getPrimitiveDistance(from, to)}} returns {{TABLE\[toIndex\]\[fromIndex\]}}
> and is called as {{getPrimitiveDistance(parameterClass, argument)}}, so the
> lookup is {{TABLE\[argument\]\[parameter\]}}:
> ||argument||-> float||-> double||
> |{{int}} / {{Integer}}|5|7|
> |{{BigDecimal}}|5|3|
> For {{(int, BigDecimal)}} the totals are 5+5=10 for {{float}} and 7+3=10 for
> {{double}}: the integral argument's two-point lean toward {{float}} exactly
> cancels the {{BigDecimal}}'s two-point lean toward {{double}}, producing the
> tie reported as ambiguous.
> *5.0.0 -- the scalar tie started resolving to float.* This coincides with the
> GROOVY-11258 redundant-match pruning added to
> {{MetaClassImpl.doChooseMostSpecificParams}} (absent in 4.0.33, present in
> 5.0.0). It removes a tied match whenever another match's parameter types can
> be passed to it ({{parametersAreCompatible(innerTypes, outerTypes)}}); since
> {{float}} widens to {{double}}, {{double(2)}} is discarded and the narrowing
> {{float(2)}} is kept. The same rule cannot apply to the varargs pair, because
> {{float[]}} and {{double[]}} are not assignable to each other -- which is
> consistent with {{m(z, d)}} still being ambiguous on 5.x.
> So there are two distinct problems:
> * in 4.0.4 a working {{double}} selection became either a silent narrowing to
> {{float}} or an error, depending on how many integral arguments accompany the
> {{BigDecimal}} (varargs), and
> * in 5.0.0 a long-standing *detected* ambiguity became a silent narrowing
> conversion (scalar).
> The second is arguably the more serious, since it replaces a loud failure
> with silent precision loss.
> h2. Scope
> Deliberately not reported here: an integral argument also prefers {{float}}
> over {{double}}, but that is long-standing (identical in 3.0.25) and javac
> selects {{float}} too, since JLS 4.10.1 makes {{float}} more specific than
> {{double}}. Only the {{BigDecimal}} behaviour is new, and Java has no
> equivalent because the conversion does not exist implicitly there.
> h2. Suggested direction
> Restoring the pre-4.0.4 varargs applicability -- scoping GROOVY-8045's
> coercion change to non-numeric component types -- would fix the varargs rows
> but not {{m2(z, d)}}, which narrows through the 5.0.0 tie-break on parameters
> that were always applicable. Addressing both probably means weighting lossy
> conversions in the distance calculation, so that a narrowing such as
> {{BigDecimal}} -> {{float}} can neither tie with nor be selected over a
> widening such as {{BigDecimal}} -> {{double}}.
> h2. Impact
> Found in Deephaven Community Core (Apache-2.0), whose generated
> {{io.deephaven.function.Numeric}} offers {{max}}/{{min}} over the full
> primitive ladder. In a Groovy console {{max(5, 2, 3, 1, 5.0)}} changed from
> {{Double}} to {{Float}} and {{max(1, 5.0)}} began throwing. Libraries that do
> not expose both {{float}} and {{double}} overloads are unaffected.
> For us this specifically blocks JDK 27 support. We run fine on 3.0.25 under
> JDK 17, 21 and 25, but the two requirements for 27 are mutually exclusive:
> reading JDK 27 class files needs the ASM 9.10.1 bump present only in 4.0.33+
> / 5.0.7+, while preserving the previous selection needs 4.0.3 or earlier. No
> Groovy release does both, and the shaded {{groovyjarjarasm}} cannot be
> upgraded independently. So we stay on 3.0.25 and cannot add JDK 27 to our
> test matrix until this is resolved or we accept the precision change for our
> users.
> Workaround: a typed literal ({{5.0d}}) or a declared {{double}}, which
> selects {{double}} on every version from 3.0.25 through 6.0.0.
> Discussed on [email protected]:
> https://lists.apache.org/thread/79xony935rzccs93kmznzrvd4ros50x1
> h2. Related
> * GROOVY-1937 (fixed 1.1-beta-2) "Number conversion from BigDecimal to double
> does not work for Varargs" -- established {{BigDecimal}} -> {{double}} for
> varargs.
> * GROOVY-5490 (open, 2012) "Double Dispatch Distance Algorithm Incorrectly
> Implemented" -- the same minimum-distance algorithm, for inheritance distance
> among reference types.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
