groovy

this was found while porting an application from groovy version 1.6.3 to 1.7.0

I looked a bit into it.

m[null] = null selects DGM#putAt(Object, String, Object) over DGM#putAt(Map, Object, Object) because key in the subscript operator is supposed to be String. Because of that DGM selection, it goes to MetaClassImpl#setProperty(), which tries to use the key's hashcode in meta-property look-up in MethodIndex maps. Since the key is null, it results in NPE.

So,
Option 1) Specially handle key = null and MC.isMap = true in the beginning of MetaClassImpl#setProperty() and delegate it to Map.put(key, value)

Option 2) Introduce DGM#putAt(Map, String, Object) so that it gets selected over DGM#putAt(Object, String, Object).

Which way looks better?

Attaching both the potential patches for review.

for null the most general class should be selected, and that is Object, not String. So if String is used, it is wrong here. That sounds like a method selection bug

And indeed.... while

def foo(Map x, Object o, Object p) {1}
def foo(Map x, String s, Object p) {2}
assert foo([:],null,null)==1

works, this fails

def foo(Object o, Object p) {1}
def foo(String s, Object p) {2}
assert foo(null,null)==1

The later case is what is used to select the DGM method. So we have definitely a method selection bug here

Yes, method selection is the problem overall. I had first looked at the method selection area itself.

When MetaClassImpl#calculateParameterDistance() looks at both
DGM(Map, Object, Object) and
DGM(Object, String, Object), it first calculates the distance for DGM(Object, String, Object), finds the distance 0, and simply selects that method.

It doesn't even calculate the distance for DGM(Map, Object, Object). That's where the error is coming from.

Jochen, here is another proposed patch, which fixes the method selection.

It now looks at parameter distances of all candidate methods and chooses the most specific one - instead of jumping out pre-maturely on seeing a distance of 0.

Jochen, do you see any issue with the parameter distance patch? Or shall I go ahead with it?

Does it really fix the issue? Because distance 0 means a direct match, meaning there should be no other method which does fit it better. But I wonder why the distance is 0. It is just wrong that null causes a zero distance. Well, ok, if we say that null causes a zero distance, then of course there oculd be multiple methods with distance 0... but then the method selection would result in a method selection error. So for me the error here is not jumping out prematurely, it is that this should not give the distance 0 for mathching String agains null. Probably the method selection process needs a refinement here in using the distance to Object as distance element for this. not of the argument class as usal, but of the parameter class. Then String is less fitting than Object and the Object version should be selected

It seems to fix the issue. Didn't break any existing test and fixes the issue here.

But you know better about the parameter distance calculations.

I can tell you where the distance 0 is coming from. Let's see if it helps.

When it calculates the distance for (null, null) against (String, Object),

• it starts with objectDistance-- (dist = -1) and then adds 1 * 2 (dist = 1) (because String is 1 level away from Object). Dist offset for first parameter 1 << 23

• it then looks at next parameter (Object) - does objectDistance-- (dist = -1), nothing else gets added because parameter type Object is 0 level away from Object. Distance offset for this parameter -1 << 23.

Now overall for both the parameters, distance becomes 1 << 23 + -1 << 23 = 0, making it the perfect match, because of which it prematurely exits not even looking at other candidates.

It is already using the distance to Object as a distance element for this. And it is not looking at the argument as usual, but of the parameter class - because arguments are null.

I don't know why it first does that (objectDistance – ) in the beginning when the argument is null. If it doesn't do that, then we won't have this (+x) + (-x) situation choosing wrong perfect matches. But I don't have the background of that objectDistance --.

if any partial distance lower than 0 is returned, then this is absolutely wrong. So the distance calculation for null is wrong. 1 is right for String, but for object it should return 0, not -1.

As of why there is objectDistance -.... no idea, maybe a relict from a refactoring... and it made sense back then.

The partial distance of -1 << 23 is coming due that objectDistance --, which makes it -1 to being with when it deals with nulls.

If you also suspect that it may not make sense now, may be I will try removing that to attempt the 4th patch.

yes, please

I removed objectDistance--. Now there is no partial < 0 distance and it fixes the issue.

Below is the patch - 4th time lucky?

Index: src/main/org/codehaus/groovy/runtime/MetaClassHelper.java
===================================================================
--- src/main/org/codehaus/groovy/runtime/MetaClassHelper.java	(revision 19174)
+++ src/main/org/codehaus/groovy/runtime/MetaClassHelper.java	(working copy)
@@ -303,8 +303,6 @@
             // choose the distance to Object if a parameter is null
             // this will mean that Object is preferred over a more
             // specific type
-            // remove one to dist to be sure Object is preferred
-            objectDistance--;
             Class clazz = parameter.getTheClass();
             if (clazz.isPrimitive()) {
                 objectDistance += 2;
Index: src/test/groovy/bugs/Groovy4029Bug.groovy
===================================================================
--- src/test/groovy/bugs/Groovy4029Bug.groovy	(revision 0)
+++ src/test/groovy/bugs/Groovy4029Bug.groovy	(revision 0)
@@ -0,0 +1,9 @@
+package groovy.bugs
+
+class Groovy4029Bug extends GroovyTestCase {
+    void testAddNullKeyEntryInMapUsingSubscriptNotation() {
+        Map m = [:]
+        m[null] = null
+        assert m.size() == 1
+    }
+}
\ No newline at end of file

Hi Jochen.

Shall I apply this last patch (not doing objectDistance-- to not have any -ive partial distances)?

4 patches and more than that requests for final review.

This JIRA should really be closed by now

sorry, yes, zhe patch is fine

Fixed.