Fix DGM toString for Map and Collection

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.

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.

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

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.

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.

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).

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.

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

Committed to trunk (cs14852).

Merged to 1.6 branch (cs14868).

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 {{"$

"}} 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.

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.

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).

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

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.