groovy

String.valueOf(BigInteger) doesn't call String.valueOf(Object)

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.5.7, 1.6-rc-1
  • Fix Version/s: 1.6-rc-2, 1.5.8, 1.7-beta-1
  • Component/s: None
  • Labels:
    None
  • Testcase included:
    yes
  • Number of attachments :
    1

Description

Somehow Groovy is calling some formatting method rather than String.valueOf(Object) for:

String.valueOf(new BigInteger('1000'))

You can see the difference with:

assert (String.valueOf(new BigInteger('1000'))) == (new BigInteger('1000').toString())

which is a good test because String.valueOf(Object) is defined to be the same as String.toString() if the object is not null.

Issue Links

Activity

Hide
Jim White added a comment - - edited

I think this is another static method resolution bug.

Groovy is preferring String.valueOf(double) over String.valueOf(Object) and doing a conversion when the parameter is a BigInteger. That doesn't appear to be happening for BigDecimal from what I've tried so far, so this isn't some logic just about Number.

Show
Jim White added a comment - - edited I think this is another static method resolution bug. Groovy is preferring String.valueOf(double) over String.valueOf(Object) and doing a conversion when the parameter is a BigInteger. That doesn't appear to be happening for BigDecimal from what I've tried so far, so this isn't some logic just about Number.
Hide
Roshan Dawrani added a comment - - edited

When, as per casting rules, multiple methods match a particular invocation, groovy uses relative parameter distances to see which method is the best match.

When the invocation "String.valueOf(new BigDecimal('1000'))" is made, 2 method matches are found:
1) String.valueOf(double) -> double to BigDecimal distance = 3
2) String.valueOf(Object) -> Object to BigDecimal distance = 2
In this case, shortest distance match is String.valueOf(Object).
[/*distances for BigDecimal*/{14, 15, 12, 13, 10, 11, 8, 9, 7, 5, 6, 3(double), 4, 0, 1, 2(Object),}]

But when the invocation "String.valueOf(new BigInteger('1000'))" is made, 3 method matches are found:
1) String.valueOf(double) -> double to BigInteger distance = 3
2) String.valueOf(int) -> int to BigInteger distance = 10
3) String.valueOf(Object) -> Object to BigInteger distance = 7
In this case, shortest distance match is String.valueOf(double).
[/*distances for BigInteger*/{14, 15, 12, 13, 10(int), 11, 8, 9, 0, 1, 2, 3(double), 4, 5, 6, 7(Object),}]

So, as per the parameter distances decided, for BigInteger, matches are preferred in the order 1) double, 2) Object, 3) int, so it picks up valueOf(double), which makes it wrong!

There seems to be inconsistency between distances set for BigInteger and BigDecimal.

  • For BigDecimal, best matches go in the order BigDecimal, Number, Object, double, Double....
  • Whereas for BigInteger, best matches go in the order BigInteger, float, Float, double, Double...Number, Object...Shouldn't even BigInteger be closer to Number/Object compared to primitive types?
Show
Roshan Dawrani added a comment - - edited When, as per casting rules, multiple methods match a particular invocation, groovy uses relative parameter distances to see which method is the best match. When the invocation "String.valueOf(new BigDecimal('1000'))" is made, 2 method matches are found: 1) String.valueOf(double) -> double to BigDecimal distance = 3 2) String.valueOf(Object) -> Object to BigDecimal distance = 2 In this case, shortest distance match is String.valueOf(Object). [/*distances for BigDecimal*/{14, 15, 12, 13, 10, 11, 8, 9, 7, 5, 6, 3(double), 4, 0, 1, 2(Object),}] But when the invocation "String.valueOf(new BigInteger('1000'))" is made, 3 method matches are found: 1) String.valueOf(double) -> double to BigInteger distance = 3 2) String.valueOf(int) -> int to BigInteger distance = 10 3) String.valueOf(Object) -> Object to BigInteger distance = 7 In this case, shortest distance match is String.valueOf(double). [/*distances for BigInteger*/{14, 15, 12, 13, 10(int), 11, 8, 9, 0, 1, 2, 3(double), 4, 5, 6, 7(Object),}] So, as per the parameter distances decided, for BigInteger, matches are preferred in the order 1) double, 2) Object, 3) int, so it picks up valueOf(double), which makes it wrong! There seems to be inconsistency between distances set for BigInteger and BigDecimal.
  • For BigDecimal, best matches go in the order BigDecimal, Number, Object, double, Double....
  • Whereas for BigInteger, best matches go in the order BigInteger, float, Float, double, Double...Number, Object...Shouldn't even BigInteger be closer to Number/Object compared to primitive types?
Hide
Jim White added a comment - - edited

I think Groovy's resolution and conversion logic should produce the same results as Java's. I realize that may be hard to do given the current implementation, but certainly we should be able to fix up whatever is going wrong here.

Java's rules are here:

http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#20448
http://java.sun.com/docs/books/jls/third_edition/html/conversions.html

Do we have specifications and/or issues documenting what is expected (aside from the code)?

Show
Jim White added a comment - - edited I think Groovy's resolution and conversion logic should produce the same results as Java's. I realize that may be hard to do given the current implementation, but certainly we should be able to fix up whatever is going wrong here. Java's rules are here: http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#20448 http://java.sun.com/docs/books/jls/third_edition/html/conversions.html Do we have specifications and/or issues documenting what is expected (aside from the code)?
Hide
blackdrag blackdrag added a comment -

a distance of 7 from BigInteger to Object sounds wrong... at last it should be the same as from BigDecimal to Object. The order should be something like BigInteger, Number, Object, long, Long, int, Integer, short, Short, byte, Byte, double, Double, float, Float...

So this needs tweaking...

As for the Java rules... Groovy is a language where a number 1 is at the same time the int 1 and the Integer 1. So if you have an overloaded method foo taking one time a long and one time an Integer it would be very strange to prefer foo(long) over foo(Integer). But that is exactly what Java would do. Java tries to have a strict separation between primitive types and Objects, so it makes sense there. But in Groovy this is different, if not the opposite. We try to hide primitives as best as we can. For example is 1 instanceof Integer true and that is not an implementation detail, it is an elementary thought here.

Show
blackdrag blackdrag added a comment - a distance of 7 from BigInteger to Object sounds wrong... at last it should be the same as from BigDecimal to Object. The order should be something like BigInteger, Number, Object, long, Long, int, Integer, short, Short, byte, Byte, double, Double, float, Float... So this needs tweaking... As for the Java rules... Groovy is a language where a number 1 is at the same time the int 1 and the Integer 1. So if you have an overloaded method foo taking one time a long and one time an Integer it would be very strange to prefer foo(long) over foo(Integer). But that is exactly what Java would do. Java tries to have a strict separation between primitive types and Objects, so it makes sense there. But in Groovy this is different, if not the opposite. We try to hide primitives as best as we can. For example is 1 instanceof Integer true and that is not an implementation detail, it is an elementary thought here.
Hide
Jim White added a comment -

I'm not going to delve into Groovy's model here, but I think you'll agree that we need for Groovy to resolve this type of overloading in the same way as Java 5's autoboxing does.

I point out the Java spec because it would be a good idea for someone to review Groovy's internals and analyze them with respect to the Java spec in order to determine if there are any other problems such as this one. Also we don't want to fix this issue and cause another.

Show
Jim White added a comment - I'm not going to delve into Groovy's model here, but I think you'll agree that we need for Groovy to resolve this type of overloading in the same way as Java 5's autoboxing does. I point out the Java spec because it would be a good idea for someone to review Groovy's internals and analyze them with respect to the Java spec in order to determine if there are any other problems such as this one. Also we don't want to fix this issue and cause another.
Hide
blackdrag blackdrag added a comment -

Jim, that my wonder you, but I don't agree that we should do that the same as java5 autoboxing.

def foo(long) {1}
def foo(Integer) {2}
def foo(Object o) {3}
// groovy
assert foo(new Long(1)) == 1
// java
//assert foo(new Long(1)) == 3

Also we have primtive BigDecimals and BigIntegers, unlike Java. So we extended the widening and narrowing rules to them too

Show
blackdrag blackdrag added a comment - Jim, that my wonder you, but I don't agree that we should do that the same as java5 autoboxing.
def foo(long) {1}
def foo(Integer) {2}
def foo(Object o) {3}
// groovy
assert foo(new Long(1)) == 1
// java
//assert foo(new Long(1)) == 3
Also we have primtive BigDecimals and BigIntegers, unlike Java. So we extended the widening and narrowing rules to them too
Hide
Jim White added a comment -

Well, that is exactly the issue we're facing here.

Because Groovy doesn't behave the same as Java wrt to overloading there is no way to use a properly designed Java class (like PrintWriter and unlike your foo example).

How do you propose we solve this problem? I regard it as a blocker for 1.6. Notice the test case in GROOVY-3237 that led to discovering this bug.

Show
Jim White added a comment - Well, that is exactly the issue we're facing here. Because Groovy doesn't behave the same as Java wrt to overloading there is no way to use a properly designed Java class (like PrintWriter and unlike your foo example). How do you propose we solve this problem? I regard it as a blocker for 1.6. Notice the test case in GROOVY-3237 that led to discovering this bug.
Hide
Roshan Dawrani added a comment -

So, shall I try the modified distances for BigInteger as suggested above (BigInteger, Number, Object, long, Long, int, Integer, short, Short, byte, Byte, double, Double, float, Float) and see if it breaks anything. Or, the modification to distances needs to be discussed / thought about more.

Show
Roshan Dawrani added a comment - So, shall I try the modified distances for BigInteger as suggested above (BigInteger, Number, Object, long, Long, int, Integer, short, Short, byte, Byte, double, Double, float, Float) and see if it breaks anything. Or, the modification to distances needs to be discussed / thought about more.
Hide
blackdrag blackdrag added a comment -

Jim, I don't think that a bug, that exists since Groovy 1.0 does qualify as blocker. Also the distance table is obviously wrong, so correcting it will also lead to the same result as for Java in this case. Of course you can easily consruct a case where JAva will behave different from Groovy, but you have the same with dynamic typing too.

Roshan, I think modifying the table as I sad and test if it breaks something is ok.. an additional testcase is also needed to ensure we won't change it back

Show
blackdrag blackdrag added a comment - Jim, I don't think that a bug, that exists since Groovy 1.0 does qualify as blocker. Also the distance table is obviously wrong, so correcting it will also lead to the same result as for Java in this case. Of course you can easily consruct a case where JAva will behave different from Groovy, but you have the same with dynamic typing too. Roshan, I think modifying the table as I sad and test if it breaks something is ok.. an additional testcase is also needed to ensure we won't change it back
Hide
Roshan Dawrani added a comment -

Hi Jochen,
I tried the change in distances and it did not break anything and fixed the issue of this JIRA.

Shall I commit the change? It needs to be fixed on all 1.5.8, 1.6-rc-2 and trunk, right?

One little thing is that in the new distances you had given, BigDecimal was absent. I have positioned that as in [BigInteger, Number, Object, long, Long, int, Integer, short, Short, byte, Byte, BigDecimal, double, Double, float, Float]. Let me know if that does not look alright for some reason.

rgds,
Roshan

Show
Roshan Dawrani added a comment - Hi Jochen, I tried the change in distances and it did not break anything and fixed the issue of this JIRA. Shall I commit the change? It needs to be fixed on all 1.5.8, 1.6-rc-2 and trunk, right? One little thing is that in the new distances you had given, BigDecimal was absent. I have positioned that as in [BigInteger, Number, Object, long, Long, int, Integer, short, Short, byte, Byte, BigDecimal, double, Double, float, Float]. Let me know if that does not look alright for some reason. rgds, Roshan
Hide
blackdrag blackdrag added a comment -

no.that does look to me, so apply the patch to all versions.

Show
blackdrag blackdrag added a comment - no.that does look to me, so apply the patch to all versions.
Hide
Roshan Dawrani added a comment -

Fixed by correcting the primitive distances for BigInteger type as discussed with Jochen.

Show
Roshan Dawrani added a comment - Fixed by correcting the primitive distances for BigInteger type as discussed with Jochen.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: