static foo(Closure) hides super.foo(Bar)

Ah, well the test works better when it doesn't have syntax errors.

Java does indeed allow static functions with the same name as a member in the parent (which is the way I thought it worked!).

I would like to add that this issue is not really related to inheritance (static method hiding the parent class implementation). The StaticImportVisitor is not resolving the method call correctly.

The following code also fails:

class SomeImplementorWithStatic{
    static Object build(String c) {return c} // if this static method is present, calls made as "build(1), this.build(1) fail
    void m(){
    	build(1) // this.build(1) also fails
    }
    public Object build(Integer i) {println "Call successful"}
}
new SomeImplementorWithStatic().m()

The problem is caused by StaticImportVisitor.transformMethodCallExpression(). It checks if the current class has a possible static method, and if it does, it maps the method call to that function. The problem is that it goes only by method name and number of arguments.

So, in above case, during compilation process, it finds static method "build" with 1 argument and incorrectly maps the call "build(1)" to this static method. At run time the call fails because the type of argument is different.

rgds,
Roshan

I think StaticImportVisitor needs to look at whether the method call expression, which it is trying to transform, is inside a static method or non-static method.

If it is happening inside a non-static method, it should check if there is a possible instance level method before trying to look for a possible static method.

If it is happening inside a static method, it should straight away do what it does currently - to see if there is a possible static method.

But I am a little unsure if this is the best that can be done, so, for now, waiting for more inputs.

I looked at the 1.7 patch and it does look good to me

I tried the proposed patch, and while it doesn't cause any failures on the existing test cases, it doesn't enable the test for this bug to pass.

I've packaged the test as a test case and it is attached as a diff (groovy-3163-test.txt).

I tested the patches again. Let me now explain that it is your new test case that has 2 bugs in it and there is no issue in patches. Please test it more carefully and I will be happy to look into any issues found in the patch.

public void testSuperOverStatic()
{
    def siws = new SomeImplementorWithStatic()

    assert (1 == siws.build(1)) // this goes through as it has no bugs :-)

/* 
    Roshan - In your original testcase, such assertions were in a 
    script (TestOverrideWithSuper.groovy), so siws.build(this) call was supposed 
    to map to SomeImplementorWithStatic.build(Script s). Now that assertion is inside 
    the class and "this" is no longer an instance of Script, so it can not map to build(Script), right? Which build version are you expecting it to get called? Integer / BigInteger / Class / Script / Closure version?
*/
    assert (this == siws.build(this))
/* 
    Roshan - Next assertion is wrong too. You are passing a closure and method SomeImplementorWithStatic.build(Closure) returns the closure itself but you are comparing it to a String. Of-course, the assertion will fail.

To, test the closure version call, use:

	    def cl = { 'foo' }
	    assert (cl == siws.build(cl))

*/

    assert ('foo' == siws.build { 'foo' })
}

Is there any way I can edit a comment after I post it? The previous comment has gone too much to the right and I could see it only after posting it. I would have liked editing it with proper line breaks.

In the meanwhile, can someone please put the line breaks in the previous comment for me?

Thanks.

If you are logged in, you should be able to edit your own comments. Unless I have slightly elevated permissions. I know I can't edit other people's comments.

Just click 'Edit' in the menu over on the far right: [ Permlink | Edit | Delete | « Hide ]

Looks like a permission thing. I see only [ Permlink | « Hide ] for my own comments as well.

Being able to edit my own comments is something that would be very useful. if I sometimes make any typing errors or want to reword something to avoid any confusion, it'll help very much.

Sorry for the bad test conversion. Just trying to make prompt use of your contribution even though I really need to spend my time on other projects right now.

Anyhow, I've fixed up the unit test (well, more like turned it into an ugly mess so it fails without the patch and succeeds with it.

Committed to trunk (cs14142).

I am glad that it is now found working as expected.

I am sorry if you found my last comment on the side of rudeness. How I wish I could have been able to re-word it a little to not make it too curt. I saw your comment first thing in the morning and got a little annoyed seeing that the patch was fine Please do not take any offense. From now, coffee first and then JIRA comments!

Also, I have a doubt regarding the process. Since the issue is found in all 1.5.8, 1.6 and 1.7 branches and patches are also supplied for all 3, at what stage do the changes make it to the other branches like 1.5.8/1.6? Does it need to follow some process (like testing of patch on trunk first) before changes are also applied on other branches?

thanks,
Roshan

I understand the difficulties in electronic communication and most definitely can relate to the need for caffeine!

I like to let changes that have the potential for unintended consequences (especially when I don't clearly know exactly what they are doing!) to sit a little while and start with the least stable branch and move it along to the more stable (trunk, then 1.6.x, then 1.5.x). I've taken it from Jochen's comment that he's OK with it being committed and apparently has left it to me (I'll assign it to myself to find out...).

Merged to 1.6.

Merged to 1.5.

to be closed?