[
https://issues.apache.org/jira/browse/GROOVY-12439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18125244#comment-18125244
]
ASF GitHub Bot commented on GROOVY-12439:
-----------------------------------------
paulk-asert commented on PR #2958:
URL: https://github.com/apache/groovy/pull/2958#issuecomment-6068869632
AI read:
> **Why not adjust `PRIMITIVE_DISTANCE_TABLE`?** Distances are summed over
all arguments, and every integral argument leans 2 points towards `float`
(`int`?`float` is 5, `int`?`double` is 7). With `n` integral arguments plus one
`BigDecimal`, the totals are:
>
> - `float`: `5n + x`, where `x` is the table value for `BigDecimal ? float`
> - `double`: `7n + 3`
>
> So `float` wins once `2n > x - 3`, whatever finite `x` the table holds.
For example, raising `BigDecimal ? float` from 5 to 10 fixes the calls in the
issue, but `m(1, 2, 3, 4, d)` picks `float` again. `BigInteger` is already 14
vs 12 in the table and still regresses at two `int` arguments.
>
> To always lose, the value has to exceed anything the other arguments can
add up to. At that point it isn't a ranking within the table any more but a
separate penalty, so I kept it out of the table and named it.
>
> **Why 2^19, i.e. a shift of 40?** You're right that `(dist << 21) + (1L <<
40)` is the same as adding 2^19 to the table value. The value only has to sit
between two bounds:
>
> - **Above** the largest total that the primitive and object distances of
the other arguments can reach. Each primitive distance is at most 19 << 21
(about 2^25.3), and object distances stay well below 2^34. So 2^40 can't be
overtaken by any realistic number of arguments, and a lossy candidate can't tie
with a non-lossy one again.
> - **Below** `VARGS_SHIFT` (44), so the existing varargs ordering is kept.
A lossy non-varargs method still beats a non-lossy varargs one, exactly as
before. Bits 40-43 also leave room for up to 15 lossy arguments before the
penalty spills into the varargs bits.
>
> Bits 34-43 were already unused in practice, so I narrowed the documented
object range to 23-39 and gave the penalty bits 40-43. Any value between about
2^34 and 2^43 would work; 40 is just the most obviously safe choice.
>
> If you'd prefer the penalty in the table, it could be a named constant
(for example `LOSSY = 1 << 19`) added to the `BigDecimal`/`BigInteger` ?
`float`/`Float` cells. The behaviour is identical, but those four cells would
hold 524,000-odd among values of 0-19, so I thought a separate penalty was
clearer.
>
> One point that's a real choice rather than an implementation detail:
should a lossy non-varargs method beat a lossy-free varargs one? For example,
with m(float) and m(double...), a BigDecimal argument currently picks m(float).
The patch keeps that, which is the conservative option. Moving the penalty
above VARGS_SHIFT would pick m(double...) instead. Jochen may have a view on
that.
> 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)
