groovy

Incorrect array type created when calling vararg method

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.5.6
  • Fix Version/s: 1.6-rc-1, 1.5.8, 1.7-beta-1
  • Component/s: None
  • Labels:
    None
  • Environment:
    using groovy via gmaven-runtime-default 1.0-rc-3
  • Number of attachments :
    1

Description

I have a constructor:

Foo( String foo, long bar, double... values )

in groovy, i am calling it with a String, a Long, and an Integer.

ParameterTypes.fitToVarArgs correctly determines that the array is a Double.

however, when MetaClassHelper.makeArray is called, it does:

        Class baseClass = secondary;
        if (obj != null) {
            baseClass = obj.getClass();
        }

which overrides the Double.class that is secondary, with Integer.class from the incoming value.

The constructor invocation then fails since the correct type was not passed.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Roshan Dawrani added a comment -

Hi,
Attaching patches for the affected groovy versions and a test case.

The fix is basically to make coercing of double-type arguments consistent between DoubleCachedClass and ArrayCachedClass.

DoubleCachedClass allows all subclasses of java.lang.Number to be coerced into double type but ArrayCachedClass for varargs of double type allowed coercing only from float/double/BigDecimal types and not from other Number sub-classes and failed with "argument type mismatch" error.

rgds,
Roshan

Show
Roshan Dawrani added a comment - Hi, Attaching patches for the affected groovy versions and a test case. The fix is basically to make coercing of double-type arguments consistent between DoubleCachedClass and ArrayCachedClass. DoubleCachedClass allows all subclasses of java.lang.Number to be coerced into double type but ArrayCachedClass for varargs of double type allowed coercing only from float/double/BigDecimal types and not from other Number sub-classes and failed with "argument type mismatch" error. rgds, Roshan
Hide
Permalink
blackdrag blackdrag added a comment -

hmm... your fix does fix the problem, no doubt. But your test case made me think... 56.0 is a BigDecimal, no double. So a call with an array containing that should not succeed, because the BigDecimals might be bigger than the double. If you have mixed numbers in the arguments, then the array class will be Number... that is bad. It is bad, because with Number we don't know anything about if we should allow the number or not. If you give a BigDecimal into a double array, then this should cause an exception. I am aware, that if the array contains only BigDecimals, that then we will currently convert the array to a double array, but I come to think, that this is wrong.

So fixing this issue might require a bit more than just this patch

Show
blackdrag blackdrag added a comment - hmm... your fix does fix the problem, no doubt. But your test case made me think... 56.0 is a BigDecimal, no double. So a call with an array containing that should not succeed, because the BigDecimals might be bigger than the double. If you have mixed numbers in the arguments, then the array class will be Number... that is bad. It is bad, because with Number we don't know anything about if we should allow the number or not. If you give a BigDecimal into a double array, then this should cause an exception. I am aware, that if the array contains only BigDecimals, that then we will currently convert the array to a double array, but I come to think, that this is wrong. So fixing this issue might require a bit more than just this patch
Hide
Permalink
Roshan Dawrani added a comment -

I think when I worked on it, I tried that it is on the similar lines as coercing of a argument to a double (as it happens in DoubleCachedClass.coerceArgument()). I thought if coercing for double array and a single double both are consistent, it should be ok.

Coming to coercing of a argument to a double (DoubleCachedClass) - it does not simply reject the argument based on its type (if it is BigDecimal, let's say).

Currently, what it does is:

if (argument instanceof BigDecimal && res.isInfinite()) {
            throw new IllegalArgumentException(Double.class + " out of range while converting from BigDecimal");
}

Do you think, ArrayCachedClass, when handling coercing an array, which has mixed types of numbers, can be on the same lines - allowing an array element to be BigDecimal but rejecting it if it is infinitely large?

If you can define how coercing to a double array should be done when a mixed types of numbers is passed, I can re-look into it.

Show
Roshan Dawrani added a comment - I think when I worked on it, I tried that it is on the similar lines as coercing of a argument to a double (as it happens in DoubleCachedClass.coerceArgument()). I thought if coercing for double array and a single double both are consistent, it should be ok. Coming to coercing of a argument to a double (DoubleCachedClass) - it does not simply reject the argument based on its type (if it is BigDecimal, let's say). Currently, what it does is: if (argument instanceof BigDecimal && res.isInfinite()) { throw new IllegalArgumentException( Double .class + " out of range while converting from BigDecimal" ); } Do you think, ArrayCachedClass, when handling coercing an array, which has mixed types of numbers, can be on the same lines - allowing an array element to be BigDecimal but rejecting it if it is infinitely large? If you can define how coercing to a double array should be done when a mixed types of numbers is passed, I can re-look into it.
Hide
Permalink
blackdrag blackdrag added a comment -

The method to coerce a single double may do too much, but that s no problem if the method is not even selected. I think that maybe the idea to find a base class for all arguments is maybe not good enough

Show
blackdrag blackdrag added a comment - The method to coerce a single double may do too much, but that s no problem if the method is not even selected. I think that maybe the idea to find a base class for all arguments is maybe not good enough
Hide
Permalink
blackdrag blackdrag added a comment -

after having a bit thought about it I think we should do the following:

if the array component type we convert from is Number, then we know that got a mixed st of umbers that require coercion. This is a case special from the other cases so we can handle that different from all other cases. We could get the CachedClass for the goal component type and then call coerceArgument for each element of the argument array. I think we should not try restricting any conversion in this phase. The other advantage of this is, that we keep the coercion logic in the right Cached Class and do not spread conversions all over the place.

Then there are other cases as well... the argument array might be a Integer[], Long[], Byte[], Short[], Char[] or Float[].. and not to forget BigDecimal[] and BigInteger[]. But does it matter? I think the same logic applies then as if the array is a Number[]... again a per element conversion is needed. The method selection should have selected only elements we can convert from so extensive type checks before the conversion are not needed.

Now... even though we have a CachedClass for for example double we use nearly the same logic as in Double, meaning coerceArgument will return a Double instead of a double. We unbox that right away, so first boxing and then unboxing again looks really like a waste of time. that's why we use the methods in DefaultTypeTransformation instead.

The double conversion from BigDecimal there lacks the precision test, I think we should add that.

ArrayCachedClass has code in the form "if (paramComponent == boolean.class && argumentClass == Boolean[].class) {" and I think we can simply remove the && part to fix the problem.

Roshan, what do you think? I did no test run yet, it is just an idea

Show
blackdrag blackdrag added a comment - after having a bit thought about it I think we should do the following: if the array component type we convert from is Number, then we know that got a mixed st of umbers that require coercion. This is a case special from the other cases so we can handle that different from all other cases. We could get the CachedClass for the goal component type and then call coerceArgument for each element of the argument array. I think we should not try restricting any conversion in this phase. The other advantage of this is, that we keep the coercion logic in the right Cached Class and do not spread conversions all over the place. Then there are other cases as well... the argument array might be a Integer[], Long[], Byte[], Short[], Char[] or Float[].. and not to forget BigDecimal[] and BigInteger[]. But does it matter? I think the same logic applies then as if the array is a Number[]... again a per element conversion is needed. The method selection should have selected only elements we can convert from so extensive type checks before the conversion are not needed. Now... even though we have a CachedClass for for example double we use nearly the same logic as in Double, meaning coerceArgument will return a Double instead of a double. We unbox that right away, so first boxing and then unboxing again looks really like a waste of time. that's why we use the methods in DefaultTypeTransformation instead. The double conversion from BigDecimal there lacks the precision test, I think we should add that. ArrayCachedClass has code in the form "if (paramComponent == boolean.class && argumentClass == Boolean[].class) {" and I think we can simply remove the && part to fix the problem. Roshan, what do you think? I did no test run yet, it is just an idea
Hide
Permalink
Roshan Dawrani added a comment -

Fixed by re-using the existing conversion logic in DefaultTypeTransformation for var-args of primitive types.

Show
Roshan Dawrani added a comment - Fixed by re-using the existing conversion logic in DefaultTypeTransformation for var-args of primitive types.

People

  • Assignee:
    Roshan Dawrani
    Reporter:
    peter royal
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: