Details

    • Type: Sub-task Sub-task
    • Status: Open Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.5.4
    • Fix Version/s: None
    • Component/s: groovy-runtime
    • 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

          Activity

          Hide
          Alexander Veit added a comment -

          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 - 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
          blackdrag blackdrag added a comment - - 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 - - edited I updated the title, because this issue is not about GString, it is about the toString() method in general
          Hide
          blackdrag blackdrag added a comment -

          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 - 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
          Alexander Veit added a comment -
          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 - 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
          blackdrag blackdrag added a comment -

          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 - 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
          Martin Meel added a comment - - 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 - - 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
          Paul King added a comment -

          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 - 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.
          Hide
          Ravi Teja added a comment -

          FYI: A call to toString{} would print the metaClassed values

          Show
          Ravi Teja added a comment - FYI: A call to toString{} would print the metaClassed values

            People

            • Assignee:
              Unassigned
              Reporter:
              Lee Butts
            • Votes:
              7 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated: