Groovyc is unable to compile a class which implements interface and uses @Delegate annotation

This could be solved if @Delegate runs before that check is done... only question is, if that is possible.

Well, it compiles with no problem under Eclipse...
I need to compile groovy sources in my Gradle build, so Eclipse build is of no help to me actually but I thought it may be useful to look how guys are doing the compilation there.

Thanks !

the eclipse plugin in is not the reference. They do what they need to do and sometimes their solution is good for eclipse, but not for Groovy. Also their are largely using the normal compiler. So if it works there now, there is no guarantee it will work there tomorrow too.

Jochen, regarding your 1st comment here - @Delegate does already run before(canonicalization) the check takes place (classgen).

Explanation in the comments inline:

class DelegateASTTransformation -> 
private void addDelegateMethod(...Map<String, MethodNode> ownMethods, 
    Map.Entry<String, MethodNode> e, ...) {
    MethodNode method = e.getValue();

    if (!method.isPublic() || method.isStatic() || 0 != (method.getModifiers () & Opcodes.ACC_SYNTHETIC))
        return;

    if (!method.getAnnotations(DEPRECATED_TYPE).isEmpty() && !deprecated)
        return;

    MethodNode existingNode = ownMethods.get(e.getKey());

    /*
        Roshan: the following condition is not met for run() and hence it is not added
        as a delegate method

        ownMethods collection here is filled up for class Temp's interfaces, superclasses and itself
        and hence has run() already from java.lang.Runnable
    */
    if (existingNode == null || existingNode.getCode() == null) {
        // add the delegate method
    }
}

I think the test for getCode()==null is wrong. Every method from a precompiled class will have that being abstract or not. If the method really is from Runnable and not from Thread, then this test should be replaced with a test for abstract instead. Because then the runnable method should be ignored and Thread#run method should be used. Of course, if for some reason the Threadd#run method does not even appear, that is yet another part of this bug

Precompiled classes never seem to have getCode()==null. org.codehaus.groovy.vmplugin.v5.Java5#setMethodDefaultValue() ensures that all the methods of precompiled classes have getCode() != null.

I already tried adding existingNode.isAbstract() in there. That helped in this case but broke other cases because then methods of GroovyObject (getMetaClass()/setMetaClass(), etc) also started coming in loop (DelegateAstTransformation comes before Verifier - so by the time @Delegate is happening, if we add isAbstract() check, then this adds getMetaClass()/setMetaClass() delegate methods as well, which interferes with what Verifier tries to do later - provide the standard getMetaClass()/setMetaClass(), etc)

I didn't mean to add it if abstract, I did mean to ignore it if abstract. This way an interface method should not be added ever. There should be an addition of the run method only because of an existing run method in Thread.

No, no that is opposite of how it should be. In DelegateASTTransformation#addDelegateMethod(), it is looping over java.lang.Thread's method. It does not add Thread's run() to class Temp because it sees an existing run() on Temp (coming from Temp implementing Runnable). So what ends up being is that when it comes to ClassCompletionVerifier, it sees that Temp has an abstract run() (Runnable's) but no implementation of run() (because Thread's run() didn't get added to it).

The solution will be to add the delegate run() to Temp's method in DelegateASTTransformation#addDelegateMethod() and not ignore it.

Currently it is being ignored already and that is what is raising the issue in the first place. And it is being ignored because existingNode != null and existingNode.getCode() != null for run().

And the getCode() != null because of that stupid org.codehaus.groovy.vmplugin.v5.Java5#setMethodDefaultValue() adding a return statement even to abstract methods of precompiled interfaces.

it does not add Thread#run, because there is an existing run method? The code was

import groovy.lang.Delegate;


class Temp implements Runnable {
	@Delegate 
	private final Thread runnable
	
	def static main(args) {
		def thread = Thread.currentThread();
		def temp = new Temp(runnable: thread)
	}
}

If you now imply that the "implements Runnable" is causing a run method to appear on Temp, then this is clearly not the intention. Unless the method is declared in Temp itself, the method should be override-able through the delegate. This of course creates a problem for GroovyObject implementing Java classes. @Delegate would then have to skip those special groovy methods.

ownMethods collection that addDelegateMethod() uses for the following check comes from ClassNode#getDeclaredMethodsMap() and it has methods from all its interfaces too. So, when Temp doesn't provide an implementation, the above collection will still have a run() coming from Runnable. Looking at that, DelegateASTTransformation doesn't add the overriding run() that comes from Thread - causing ClassCompletionVerifier to see it as an abstract method whose implementation has not been provided.

        MethodNode existingNode = ownMethods.get(e.getKey());

        if (existingNode == null || existingNode.getCode() == null) {
        }

and I think that getDelcaredMethodsMap is maybe the wrong way, Either it should be only all non abstract methods or only the methods implemented in the current class that are used for this check. So for sure abstract methods originating from interfaces should be filtered out or other means to get the methods we need should be used... for example only getMethods(). Of yourse the later would mean a delegate would be overriding a method defined in the superclass if I am not wrong. Actually we need first discuss on the lists if this is correct behavior. Judging from backwards compatibility it surely is not. So in the end filtering our the "abstract methods not declared in the delegate using class" should be the way to go.

Yes, now it seems on the right track - only that I don't know how to explain it all on the mailing list Someone with more background of current Delegate design can better ask that - like, what all it is designed to let the user override, etc (super class methods, for ex, as you point out)

The workaround for this case is to leave out the implements clause, e.g.:

class Temp {
    @Delegate
    private Thread runnable
    
    static main(args) {
        def thread = Thread.currentThread()
        def temp = new Temp(runnable: thread)
    }
}

//println Temp.class.interfaces

The Delegate transform will add in any interfaces found on the delegates class unless "interfaces=false" is provided as a param to the annotation.

Potential patch - but breaks GroovyShellTestCase which uses @Delegate with a protected field - investigating that now.

Y'day I tried multiple ways of fixing it too, but all got stuck with that @Delegate use in GroovyShellTestCase Sometimes it couldn't find setUp() on NullObject, other times, some interference started happening with those special methods of GroovyObject and its getMetaClass(), etc started getting affected.

Then only I resorted to discussing it with Jochen.

Yes, not much doco and poor test coverage in this territory!
Revised attached patch is better.
We'll have to make coverage testing more visible and maybe GSoC spec project can help us in spec area.

Possibly it would be better to skip methods from Object too - or at least prohibit Object from being the delegate type, i.e. give a warning if you leave out the type.

That seems like a harmless restriction and makes sense too, but what is the issue if Object methods are not filtered?

Stops this:

You are not allowed to override the final method wait(long) from class 'java.lang.Object'.

But doesn't that point to the breaking change that Jochen was pointing to? That after this change we will start overriding methods of super classes (because we now use ClassNode.getMethods() and not getDeclaredMethodMap())

What if we filter out Object but we still have a structure like below:

class A {
     final def foo(){}
}

class B extends A {
     @Delegate C c
}

class C {
     def foo(){}
}

Now you have a base class A that provides the final method foo() that that base class is not Object. How do we take care of it? I think that is the breaking change Jochen was pointing to.

Revised patch - this one gives supplied method (direct or inherited) precedence over delegated method.

Patch applied

I was testing the example from my last comment, but made a typo and it highlighted an issue that has been introduced.

class A {
     final def foo(){}
}

class B extends A {
     @Delegate C c = new C()
     static main(args){
        c.foo()
     }
}

class C {
     def foo(){}
}

Apparent variable 'c' was found in a static scope but doesn't refer to a local variable, static field or class.

Caught: java.lang.VerifyError: (class: B, method: main signature: ([Ljava/lang/String;)V) Incompatible type for getting or setting field

Does the fragment below also fail for you? Doesn't seem related to @Delegate if that is the case.

class B {
  def c = new Object()
  static main(args) {
    c.foo()
  }
}

I suspect it was introduced when we split out StaticVerifier and we mustn't have coverage for that case.

OK, found the issue. It was a missing line from GROOVY-4228.

Can you try again?

Seems fine now.

Possibly one more issue here. The check introduced before deciding whether to add a delegate a method or not is based on type descriptor of the method alone and that may not be sufficient.

In the code below, shouldn't the call from B#main() go to C#foo()?

So, if for a delegate candidate method (which can only be an instance method), if in DelegateASTTransformation#addDelegateMethod(), a match(existingNode) is found but is static, it should be ignored.

class A {
     static foo(){println "A->foo()"}
}

class B extends A {
     @Delegate C c = new C()
     static main(args){
        new B().foo()
     }
}

class C {
     def foo(){println "C->foo()"}
}

I just checked that the static/instance issue last raised is present in 1.7.3 too.

Should I open a new issue? I am not doing it right away to also check if others see it is a valid issue.

A new issue might be best

Raised GROOVY-4265 for the issue related to @Delegate wrongly dealing with static matches.