[
https://issues.apache.org/jira/browse/GROOVY-12439?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Stanley Brubaker updated GROOVY-12439:
--------------------------------------
Description:
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.
was:
A call that selected a {{double}} overload up to 4.0.3 now silently selects a
narrowing {{float}} one, losing about nine significant digits -- and the same
call in varargs form throws {{GroovyRuntimeException}} instead. Code that
worked on 4.0.3 either loses precision without warning or fails at runtime,
depending only on the shape of the call.
The cause is that a {{BigDecimal}} argument became applicable to a {{float}}
parameter in 4.0.4. Where both {{float}} and {{double}} overloads exist, the
same conceptual call now gives three different answers: {{double}} for a lone
{{BigDecimal}}, an ambiguity error for varargs, and a silently narrowing
{{float}} for two scalars.
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 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*|
|{{m2(z, d)}}|AMBIGUOUS|AMBIGUOUS|AMBIGUOUS|*float(2)*|
Expected: {{double}} in all three, as in 4.0.3.
h2. Cause
Two independent changes, each bisected to a single release.
*4.0.4 -- BigDecimal became applicable to float.*
{{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.
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 tie started resolving to float.* The GROOVY-11258 redundant-match
pruning added to {{MetaClassImpl.doChooseMostSpecificParams}} (absent in
4.0.33, present in 5.0.0) discards one of the tied matches, so the non-varargs
form silently selects the narrowing {{float(2)}} instead of erroring.
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
Either restore the pre-4.0.4 applicability so {{BigDecimal}} is not implicitly
narrowed to {{float}} while a {{double}} overload exists -- scoping
GROOVY-8045's coercion change to non-numeric component types -- or weight lossy
conversions so a narrowing cannot tie with, or be selected over, a widening.
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.
> 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
> 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)
