[#GROOVY-2599] unable to overwritel toString() via MOP in some cases - jira.codehaus.org

Details

Type: Bug
Status: Open
Priority: Major
Resolution: Unresolved
Affects Version/s: 1.5.4
Fix Version/s: None
Component/s: None
Labels:
None

Testcase included:
yes
Number of attachments :
0

Description

If toString is overridden via the metaClass then "${obj}" != obj.toString().

Test case below

class DateTest extends GroovyTestCase {

    void testToString(){

        Date.metaClass.toString = { ->
            'boogie'
        }
       
        def date = new Date()
        assertEquals('boogie', date.toString())
        assertEquals( date.toString(), "${date}") //fails
    }
}

Paul King gave this pointer as to where the problem is:

"Yes, the final line of InvokerHelper.format() should be something like:

return (String) invokeMethod(arguments, "toString", EMPTY_ARGS);
//return arguments.toString();

to make your example work."

Issue Links

This issue is duplicated by:
GROOVY-2732	GString doesn't call correct toString method when it's been changed via the metaClass mechanism

This issue relates to:
GROOVY-2801	Override toString in coerced Map fails for interfaces but is ok for concrete classes

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Alexander Veit added a comment - 10/Aug/08 6:01 PM

There are other cases that do not work. Some of them require additional code to be fixed.

Date.metaClass.toString = {-> "silly"}

assert new Date().toString() == "silly"

// similar
assert "".plus(new Date()) != "silly"
assert [new Date()].toString() != ["silly"].toString()

// but also e.g.
assert "".leftShift(new Date()).toString() != "silly"
assert "silly".isCase(new Date()) == false

def s = new StringBuffer("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
s[1..5] = new Date()
assert s.indexOf("silly") == -1

But since

String.metaClass.toString = {-> "silly"}
StringBuffer.metaClass.toString = {-> "silly"}
StringBuilder.metaClass.toString = {-> "silly"}

assert "hello world".toString() == "hello world"
assert new StringBuffer("hello world").toString() == "hello world"
assert new StringBuilder("hello world").toString() == "hello world"

what seems to be intent, it is probably a good idea to have a faster invocation for classes that don't allow overriding toString().

Show

Alexander Veit added a comment - 10/Aug/08 6:01 PM There are other cases that do not work. Some of them require additional code to be fixed.

Date.metaClass.toString = {-> "silly"}

assert new Date().toString() == "silly"

// similar
assert "".plus(new Date()) != "silly"
assert [new Date()].toString() != ["silly"].toString()

// but also e.g.
assert "".leftShift(new Date()).toString() != "silly"
assert "silly".isCase(new Date()) == false

def s = new StringBuffer("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
s[1..5] = new Date()
assert s.indexOf("silly") == -1

But since

String.metaClass.toString = {-> "silly"}
StringBuffer.metaClass.toString = {-> "silly"}
StringBuilder.metaClass.toString = {-> "silly"}

assert "hello world".toString() == "hello world"
assert new StringBuffer("hello world").toString() == "hello world"
assert new StringBuilder("hello world").toString() == "hello world"

what seems to be intent, it is probably a good idea to have a faster invocation for classes that don't allow overriding toString().

Hide

Permalink

blackdrag blackdrag added a comment - 11/Aug/08 9:39 AM - edited

I updated the title, because this issue is not about GString, it is about the toString() method in general

Show

blackdrag blackdrag added a comment - 11/Aug/08 9:39 AM - edited I updated the title, because this issue is not about GString, it is about the toString() method in general

Hide

Permalink

blackdrag blackdrag added a comment - 11/Aug/08 9:47 AM

as reason why toString here does not work as you seem to expect it to work. Whenever Javacode is calling the toString method we don't get the Groovy version you added here using EMC. As for Lists this happens because the format method uses the Java code only. In guess we would need to rewrite that code in Groovy.

As for the second block... these calls should work I think... which means all of the three asserts should fail

Show

blackdrag blackdrag added a comment - 11/Aug/08 9:47 AM as reason why toString here does not work as you seem to expect it to work. Whenever Javacode is calling the toString method we don't get the Groovy version you added here using EMC. As for Lists this happens because the format method uses the Java code only. In guess we would need to rewrite that code in Groovy. As for the second block... these calls should work I think... which means all of the three asserts should fail

Hide

Permalink

Alexander Veit added a comment - 11/Aug/08 1:07 PM

assert "".plus(new Date()) != "silly"
assert [new Date()].toString() != ["silly"].toString()

would both work as expected with the fix to InvokerHelper.format() mentioned above, whereas e.g.

assert "".leftShift(new Date()).toString() != "silly"
assert "silly".isCase(new Date()) == false

would require to modify the respective method's code.

I'm somewhat concerned about Groovy's performance and the scalability of applications that use Groovy as a central technology (e.g. Grails). So I'm not sure if it would be a good idea to let users override e.g. String#toString(), StringBuffer#toString(), and other prominent methods of such classes. Compared to Java, a method call in Groovy is really expensive. Running through the MOP twice, e.g. for a simple string concatenation, surely does not improve this situation.

Show

Alexander Veit added a comment - 11/Aug/08 1:07 PM

assert "".plus(new Date()) != "silly"
assert [new Date()].toString() != ["silly"].toString()

would both work as expected with the fix to InvokerHelper.format() mentioned above, whereas e.g.

assert "".leftShift(new Date()).toString() != "silly"
assert "silly".isCase(new Date()) == false

would require to modify the respective method's code. I'm somewhat concerned about Groovy's performance and the scalability of applications that use Groovy as a central technology (e.g. Grails). So I'm not sure if it would be a good idea to let users override e.g. String#toString(), StringBuffer#toString(), and other prominent methods of such classes. Compared to Java, a method call in Groovy is really expensive. Running through the MOP twice, e.g. for a simple string concatenation, surely does not improve this situation.

Hide

Permalink

blackdrag blackdrag added a comment - 12/Aug/08 5:23 PM

leftShift is a GDK method... if leftShift calls the dynamic toString method, then there is no problem, because of course you can replace the toString method, you just need code that respects the dynamic aspect. of course the same goes for isCase

Show

blackdrag blackdrag added a comment - 12/Aug/08 5:23 PM leftShift is a GDK method... if leftShift calls the dynamic toString method, then there is no problem, because of course you can replace the toString method, you just need code that respects the dynamic aspect. of course the same goes for isCase

Hide

Permalink

Martin Meel added a comment - 06/Oct/10 3:24 AM - edited

I stumbled over this problem the last days and found this issue.

It seems that the solution is posted in ~~GROOVY-2732~~ and a patch is supplied there.

Can anyone apply the patch and commit the source so this patch is available in the next release.

Best wishes,
Martin Meel
____
Edit:
Got the source now and saw the comment in InvokerHelper.java

Show

Martin Meel added a comment - 06/Oct/10 3:24 AM - edited I stumbled over this problem the last days and found this issue. It seems that the solution is posted in ~~GROOVY-2732~~ and a patch is supplied there. Can anyone apply the patch and commit the source so this patch is available in the next release. Best wishes, Martin Meel ____ Edit: Got the source now and saw the comment in InvokerHelper.java

Hide

Permalink

Paul King added a comment - 06/Oct/10 5:36 AM

The patch fixes some issues but breaks others - 40 breakages in the build for instance including StackOverflow errors. The patch should be regarded as no more than a signpost pointing towards a proper fix. Further contributions/discussion welcome of course.

Show

Paul King added a comment - 06/Oct/10 5:36 AM The patch fixes some issues but breaks others - 40 breakages in the build for instance including StackOverflow errors. The patch should be regarded as no more than a signpost pointing towards a proper fix. Further contributions/discussion welcome of course.

People

Assignee:

Unassigned

Reporter:

Lee Butts

Vote (5)

Watch (8)

Dates

Created:

13/Feb/08 1:59 AM

Updated:

30/Mar/11 7:16 AM

groovy

unable to overwritel toString() via MOP in some cases

Details

Description

Issue Links

Activity

People

Dates