groovy

Hi,
I am attaching a zip that has patches for all 3 affected groovy versions for this issue.
The patch has changes to the following 2 files:

1) src/main/org/codehaus/groovy/ast/ClassNode.java - Its hasPossibleStaticMethod() now handles finding the static methods having parameters with default values.

2) src/test/groovy/StaticImportTest.groovy - Added a few test cases for this issue.

Hope it helps.
Roshan

say.. if I have foo() and a static foo(x,y=1,z=2){}, then this patch would select the method, because it has default arguments. If that were a precompiled class, then we would have three foo methods, but none of them would be selected, because there is no exact match in the number of parameters and arguments. See ClassNode:1160

so I think a small modification to the patch is needed

I am not sure if I understand your scenario correctly. So, let me confirm:

So, let's say, there is the following class ConsoleUI and it is pre-compiled

class ConsoleUI
{
     static void foo(x, y = 1, z = 2) {}
     /*
      When pre-compiled, 3 foo() are generated in the class file:
      1) foo(x, y, z) {}
      2) foo(x, y) { /* invokes foo(x, y, 2) */}
      3) foo(x) { /* invokes foo(x, 1, 2) */}
     */
}

and there is another class like

import static ConsoleUI.*
class Settings
{
    static void initialize() { 
           foo("valX") // goes through and invokes version 3 of foo above
           foo() // is not supposed to go through
    }
}

Settings.initialize()

So, now what is reported in current bug is that a call like foo("valX") does not go through. That goes through with the patch I have supplied. It invokes version 3 of foo() mentioned above. It matches that version of foo.

You have mentioned a foo() in your comment. If that is what you are expecting to go through, I don't think that is correct because ConsoleUI.foo() has a parameter "x" without a default value, so that method needs to be called with at least one parameter.

If I have not understood your comment, please elaborate the scenario and I will be glad to look into it.

rgds,
Roshan

better let us use the example

class X {
  static foo(x,y,z=1) {1}
}

which produces foo(x,y,z) and foo(x,y){foo(x,y,1)}. If I have now:

import static X.*
foo(1,2)
foo(1)
foo()

then before the your patch none of these foo methods would have worked if X was not precompiled. With your patch each of these methods is linked to X. If X is precompiled, then only foo(1,2) is linked to X, because for the other two we won't find a method with a matching number of arguments. Your patch does not change anything here, because in a precompiled class there will be no default value.

That means with your patch foo(1,2) and addiionally foo(1) and foo() will be linked to X if X is not precompiled. If X is prcompiled then it is only foo(1,2).

This is a mismatch. so right, foo(1) and foo() are not supposed to go throughin the sense, that it will fail with a MissingMethodException. But if X is precompiled, then the exception would report ScriptXY#foo(int) and ScriptXY#foo() could not be found, while if the X is not precompiled it will report X#foo(int) and X#foo(). I think that shouldn't happen.

I suggest you remove the number of default parameters from the number of overall parameters and check that the resulting number is lower than count. If parameters.length>count and count>nonDefaultParameters, then it is a match, because there will be a method with the matching number of arguments.

I tested my patch with both pre-compiled and not pre-compiled versions of the X and it seems to be working fine. Have you tested and reported the mismatch in the previous comments or is it a code review kind of comment? If it is a code review kind of comment, you may have misread ( ! ..) conditions.

Just to make sure that we are on the same page version-wise - I am first trying to look into your comments with version 1.5.8. Are you also reporting it from the same version?

Now coming to details of why the patch is working, the check it performs is :-

if (method.isStatic() && count < method.getParameters().length) {
    int countOfParamsNotPassed = method.getParameters().length - count;
    boolean foundAParamWithNoDefualtVal = false;
    // if all arguments that have not been passed have default values, it is still a match
    for(int i = count; i <= method.getParameters().length - 1; i++) {
        if(!method.getParameters()[i].hasInitialExpression()) foundAParamWithNoDefualtVal = true;
    }
    if(!foundAParamWithNoDefualtVal) return true;
}

For the call foo(1), this block sets foundAParamWithNoDefualtVal = true both in case of pre-compiled and not-pre-compiled cases and hence it does not report foo(1) as a match in either case (if foundAParamWithNoDefualtVal is true, it does not return true to report a match).

For pre-compiled X, is has no initial expression for 2nd parameter and hence foundAParamWithNoDefualtVal = true (pre-compiled classes have no default value/initial expression)

For not pre-compiled X also, 2nd param has no initial expression (foo(x,y,z=1)) and hence it also sets foundAParamWithNoDefualtVal = true.

Same explanation applies to foo() also. Also, I didn't notice any difference in the compiler errors it throws for pre-compiled X and not pre-compiled X.

rgds,
Roshan

Roshan, may I suggest for the future, that you change your code style a little bit to avoid "if (!". Because this is at last the second time I did misread the patch... Just remove a bit negation. I mean, your have "foundAParamWithNoDefualtVal" containing a "No", you have if(!method.getParameters()[i].hasInitialExpression()) containing the "if(Unable to render embedded object: File (", that is sometimes hard to read and if(!foundAParamWithNoDefualtVal) return true;, which is a double problem, because again you have "if() not found." and then you simply return a boolean constant value... You could have done for example "return !foundAParamWithNoDefualtVal;" instead, but that still looks like having a strange negation. Then there is another part in your code style that is a bit problematic "i <= method.getParameters().length - 1;" usually that is written as "i < method.getParameters().length ;"

And yes, it was just from the review, not from actually trying it out... and I looked at 1.7 only. Ok, let me review that again....

I did also overlook that we start with i=count and not i=0... sigh... ok, then let me show you this:

class Y {
  def foo(x=100,y,z) {x+y+z}
}

import Y.*
assert foo(10,1) == 111

this code will still produce two methods, but this time it is not the last parameter that has a default, but the first parameter. If I did read your patch right this time, then you skip the first two parameters and so you will never know that there is a default value. Instead you will skip x and also y, because count is 2 and then check only z, which has no default value. So for this case your code won't work.

Ok, I will change the patch to take care of the code review comments and re-send them.

The example code that you have shown now has default values for parameters and then more parameters without default values. I did not know that it was allowed. I thought it was like varargs and default values could be supplied for only parameters at the end and no parameter without the default value could come after ones with default value. Hence I coded it the way I coded it. Thanks for correcting that assumption.

I will correct the patches for these 2 things and re-send.

Attaching the newer set of patches with the following changes based on the comments:

1) Conditions modified to not use negation.

2) No assumption that parameters with default values will only be specified at the end in parameter list of a static method.

rgds,
Roshan

much better now

Thank you. Trying negation had a side-advantage though. I have some insider information on one thing that you can get wrong in reviewing code

I had moved duplicate isStatic() calls to one place in hasPossibleStaticMethod. Missed doing that in 1.7 patch. So, just re-attaching that patch.

Sorry for a little clutter.

patch applied