groovy

Fix DGM toString for Map and Collection

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.6-rc-1
  • Fix Version/s: 1.6-rc-2
  • Component/s: None
  • Labels:
    None
  • Testcase included:
    yes
  • Patch Submitted:
    Yes
  • Number of attachments :
    1

Description

In Groovy code, the toString() method does not produce a Groovy formatted string for Map or Collection.

The problem is that the toString(Map) and toString(Collection) methods can't work because they're declared on interfaces and DGM methods must be declared on an implementation class in order to be dispatched.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jim White added a comment -

Fix applied to trunk (cs14814).

The solution is that those DGM methods are declared as toString(AbstractMap) and toString(AbstractCollection).

Folks who implement their own classes from scratch are on their own.

Show
Jim White added a comment - Fix applied to trunk (cs14814). The solution is that those DGM methods are declared as toString(AbstractMap) and toString(AbstractCollection) . Folks who implement their own classes from scratch are on their own.
Hide
Permalink
Jim White added a comment -

The first bit of fix (cs14814) applied to 1.6 branch (cs14815).

But there is a related problem for primitive arrays.

There is a toString(Object) in DGM, but it is scoped as protected so that it doesn't become a metamethod. I know there must be a reason for that, but we either need to make it public or implement toString for each of the primitive arrays. I've tried making it public and it seems to work OK, but I suspect there may not be test cases for the MOP cases that led to toString(Object) going with protected.

I'm tempted to commit the change and see what happens since it doesn't break the build, but I attached it as a patch here for the time being instead.

Show
Jim White added a comment - The first bit of fix (cs14814) applied to 1.6 branch (cs14815). But there is a related problem for primitive arrays. There is a toString(Object) in DGM, but it is scoped as protected so that it doesn't become a metamethod. I know there must be a reason for that, but we either need to make it public or implement toString for each of the primitive arrays. I've tried making it public and it seems to work OK, but I suspect there may not be test cases for the MOP cases that led to toString(Object) going with protected. I'm tempted to commit the change and see what happens since it doesn't break the build, but I attached it as a patch here for the time being instead.
Hide
Permalink
Alexander Veit added a comment -

Does it have performance implications if toString(Object) is made public? E.g. through excessive instanceof calls or increased call stack depth.

Show
Alexander Veit added a comment - Does it have performance implications if toString(Object) is made public? E.g. through excessive instanceof calls or increased call stack depth.
Hide
Permalink
Jim White added a comment -

I didn't notice anything obvious in the time for the test suite to run. And I seriously doubt performance is an issue, there are already 51 other metamethods defined on Object. Moreover the issue we're dealing with here is getting correct behavior. MOP performance is indeed a big deal but nothing specific to Object.toString AFAIK.

But there is something about toString be handled specially, but I don't know what it is. Some seemingly related issues are GROOVY-3236, GROOVY-2599, and GROOVY-2801.

Show
Jim White added a comment - I didn't notice anything obvious in the time for the test suite to run. And I seriously doubt performance is an issue, there are already 51 other metamethods defined on Object. Moreover the issue we're dealing with here is getting correct behavior. MOP performance is indeed a big deal but nothing specific to Object.toString AFAIK. But there is something about toString be handled specially, but I don't know what it is. Some seemingly related issues are GROOVY-3236 , GROOVY-2599 , and GROOVY-2801 .
Hide
Permalink
blackdrag blackdrag added a comment -

I would prefer toString on the concrete class... that means char[], byte[], long[], int[], float[] and double[]. The reason is not so much an increased call stack or something like that... But DGM methods that have to work with instanceof where a instanceof would not really be needed seems to be a design issue to me. Also this may show that toString needs a special handling like we do for the mathematic operators for example.

Show
blackdrag blackdrag added a comment - I would prefer toString on the concrete class... that means char[], byte[], long[], int[], float[] and double[]. The reason is not so much an increased call stack or something like that... But DGM methods that have to work with instanceof where a instanceof would not really be needed seems to be a design issue to me. Also this may show that toString needs a special handling like we do for the mathematic operators for example.
Hide
Permalink
Jim White added a comment -

Well, the instanceof checks are not really needed. They are obvious leftovers from folks trying to get toString to work on interfaces Map and Collection. They can be safely removed because InvokerHelper.toString takes care of that.

The right thing is to have have Object.toString() be a metamethod just like all the rest unless there is a specific problem caused by doing that. As I said, this patch which does just that causes no problem with the test suite nor any obvious performance difference (although I've done no microbenchmark specifically to find what, if any, difference there is).

Show
Jim White added a comment - Well, the instanceof checks are not really needed. They are obvious leftovers from folks trying to get toString to work on interfaces Map and Collection . They can be safely removed because InvokerHelper.toString takes care of that. The right thing is to have have Object.toString() be a metamethod just like all the rest unless there is a specific problem caused by doing that. As I said, this patch which does just that causes no problem with the test suite nor any obvious performance difference (although I've done no microbenchmark specifically to find what, if any, difference there is).
Hide
Permalink
Jim White added a comment -

I agree that it would be nice to optimize performance on the individual primitive arrays. The current code is just about maximally slow (they are converted to a List using reflection - yikes!), so eliminating the instanceof calls won't help primitive arrays and for Map and Collection we've already got the vast majority of cases via AbstractMap and AbstractCollection.

Right now I'm trying to establish the correct API and behavior. Tuning the implementation with a slew of toString and InvokerHelper.format methods will come later.

Show
Jim White added a comment - I agree that it would be nice to optimize performance on the individual primitive arrays. The current code is just about maximally slow (they are converted to a List using reflection - yikes!), so eliminating the instanceof calls won't help primitive arrays and for Map and Collection we've already got the vast majority of cases via AbstractMap and AbstractCollection . Right now I'm trying to establish the correct API and behavior. Tuning the implementation with a slew of toString and InvokerHelper.format methods will come later.
Hide
Permalink
Jim White added a comment -

Added the DGM.toString methods for the various primitive arrays.

Committed to trunk (cs14852).

Show
Jim White added a comment - Added the DGM.toString methods for the various primitive arrays. Committed to trunk (cs14852).
Hide
Permalink
Jim White added a comment -

Merged to 1.6 branch (cs14868).

Show
Jim White added a comment - Merged to 1.6 branch (cs14868).
Hide
Permalink
Jim White added a comment -

Turns out we really need to make Object.toString() a metamethod.

If we don't then if someone implements a Map or Collection that doesn't not extend AbstractMap or AbstractCollection, then any time Groovy converts it to String internally (like {{"$

{mymap}

"}} or [mycoll].join() then we'll use our own formatter. But if they call toString() on their object then they'll get either Object.toString() or their own method if they have one.

This is actually probably a bug and may mean we need to do that instanceof Map/Collection differently.

Show
Jim White added a comment - Turns out we really need to make Object.toString() a metamethod. If we don't then if someone implements a Map or Collection that doesn't not extend AbstractMap or AbstractCollection , then any time Groovy converts it to String internally (like {{"$ {mymap} "}} or [mycoll] .join() then we'll use our own formatter. But if they call toString() on their object then they'll get either Object.toString() or their own method if they have one. This is actually probably a bug and may mean we need to do that instanceof Map/Collection differently.
Hide
Permalink
Jim White added a comment - - edited

Additional Map or Collection classes that don't extend AbstractMap/Collection:

Collection:

  • java.beans.beancontext.BeanContextSupport
  • java.beans.beancontext.BeanContextServicesSupport
  • java.util.concurrent.CopyOnWriteArrayList

Map:

  • java.util.jar.Attributes
  • java.util.Hashtable
  • java.awt.RenderingHints
  • javax.management.openmbean.TabularDataSupport

I'll open another JIRA on this more general problem.

Show
Jim White added a comment - - edited Additional Map or Collection classes that don't extend AbstractMap/Collection: Collection: java.beans.beancontext.BeanContextSupport java.beans.beancontext.BeanContextServicesSupport java.util.concurrent.CopyOnWriteArrayList Map: java.util.jar.Attributes java.util.Hashtable java.awt.RenderingHints javax.management.openmbean.TabularDataSupport I'll open another JIRA on this more general problem.
Hide
Permalink
Jim White added a comment -

A case demonstrating that we really need Object.toString() to be a metamethod (at least as the code is now) is to try out "this.binding.variables.toString()" in groovyConsole. A result of "this.binding.variables" is fine!

Committed the change (already on trunk - cs14873) to 1.6 branch (cs14946).

Show
Jim White added a comment - A case demonstrating that we really need Object.toString() to be a metamethod (at least as the code is now) is to try out " this.binding.variables.toString() " in groovyConsole. A result of " this.binding.variables " is fine! Committed the change (already on trunk - cs14873) to 1.6 branch (cs14946).
Hide
Permalink
Guillaume Laforge added a comment -

So, what's the status of this issue now?

Show
Guillaume Laforge added a comment - So, what's the status of this issue now?
Hide
Permalink
Jim White added a comment -

I had held off marking this resolved because I noticed a comment about DefaultGroovyStaticMethods and it makes me think some of these methods in DGM are supposed to be in there.

But things seem to be working, and if that (DGSM) is an issue, we can open one for that.

Show
Jim White added a comment - I had held off marking this resolved because I noticed a comment about DefaultGroovyStaticMethods and it makes me think some of these methods in DGM are supposed to be in there. But things seem to be working, and if that (DGSM) is an issue, we can open one for that.

People

  • Assignee:
    Jim White
    Reporter:
    Jim White
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: