groovy
  1. groovy
  2. GROOVY-4163

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

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.1, 1.7.2
    • Fix Version/s: 1.7.4, 1.8-beta-1
    • Component/s: Compiler
    • Labels:
      None
    • Environment:
      linux x86_64 - Fedora 12,
      Groovy 1.7.2 (also 1.7.1)
      Java 1.6.0_18 (64-bit)
    • Number of attachments :
      2

      Description

      Groovy source file is attached.

      I am trying to compile this file with the command:
      groovy Temp.groovy

      Compiler's output is as follows:

      org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
      Temp.groovy: 4: Can't have an abstract method in a non-abstract class. The class 'Temp' must be declared abstract or the method 'void run()' must be implemented.
      @ line 4, column 1.
      class Temp implements Runnable {
      ^

      1 error

      1. groovy4163_delegateWithImplementsC.patch
        7 kB
        Paul King
      2. Temp.groovy
        0.2 kB
        Pawel Dolega

        Activity

        Hide
        blackdrag blackdrag added a comment -

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

        Show
        blackdrag blackdrag added a comment - This could be solved if @Delegate runs before that check is done... only question is, if that is possible.
        Hide
        Pawel Dolega added a comment -

        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 !

        Show
        Pawel Dolega added a comment - 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 !
        Hide
        blackdrag blackdrag added a comment -

        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.

        Show
        blackdrag blackdrag added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

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

        Show
        Roshan Dawrani added a comment - Jochen, regarding your 1st comment here - @Delegate does already run before(canonicalization) the check takes place (classgen).
        Hide
        blackdrag blackdrag added a comment -

        then why is there no run method?

        Show
        blackdrag blackdrag added a comment - then why is there no run method?
        Hide
        Roshan Dawrani added a comment - - edited

        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
            }
        }
        
        Show
        Roshan Dawrani added a comment - - edited 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 } }
        Hide
        blackdrag blackdrag added a comment -

        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

        Show
        blackdrag blackdrag added a comment - 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
        Hide
        Roshan Dawrani added a comment -

        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)

        Show
        Roshan Dawrani added a comment - 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)
        Hide
        blackdrag blackdrag added a comment -

        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.

        Show
        blackdrag blackdrag added a comment - 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.
        Hide
        Roshan Dawrani added a comment - - edited

        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.

        Show
        Roshan Dawrani added a comment - - edited 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.
        Hide
        blackdrag blackdrag added a comment -

        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.

        Show
        blackdrag blackdrag added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

        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) {
                }
        
        Show
        Roshan Dawrani added a comment - 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 ) { }
        Hide
        blackdrag blackdrag added a comment -

        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.

        Show
        blackdrag blackdrag added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

        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)

        Show
        Roshan Dawrani added a comment - 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)
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Paul King added a comment - - edited

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

        Show
        Paul King added a comment - - edited Potential patch - but breaks GroovyShellTestCase which uses @Delegate with a protected field - investigating that now.
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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.
        Hide
        Paul King added a comment - - edited

        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.

        Show
        Paul King added a comment - - edited 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.
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

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

        Show
        Roshan Dawrani added a comment - That seems like a harmless restriction and makes sense too, but what is the issue if Object methods are not filtered?
        Hide
        Paul King added a comment -

        Stops this:

        You are not allowed to override the final method wait(long) from class 'java.lang.Object'.
        
        Show
        Paul King added a comment - Stops this: You are not allowed to override the final method wait(long) from class 'java.lang.Object'.
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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.
        Hide
        Paul King added a comment - - edited

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

        Show
        Paul King added a comment - - edited Revised patch - this one gives supplied method (direct or inherited) precedence over delegated method.
        Hide
        Paul King added a comment -

        Patch applied

        Show
        Paul King added a comment - Patch applied
        Hide
        Roshan Dawrani added a comment - - edited

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

        A.groovy
        class A {
             final def foo(){}
        }
        
        B.groovy
        class B extends A {
             @Delegate C c = new C()
             static main(args){
                c.foo()
             }
        }
        
        C.groovy
        class C {
             def foo(){}
        }
        
        Expected error
        Apparent variable 'c' was found in a static scope but doesn't refer to a local variable, static field or class.
        
        Actual error
        Caught: java.lang.VerifyError: (class: B, method: main signature: ([Ljava/lang/String;)V) Incompatible type for getting or setting field
        
        Show
        Roshan Dawrani added a comment - - edited I was testing the example from my last comment, but made a typo and it highlighted an issue that has been introduced. A.groovy class A { final def foo(){} } B.groovy class B extends A { @Delegate C c = new C() static main(args){ c.foo() } } C.groovy class C { def foo(){} } Expected error Apparent variable 'c' was found in a static scope but doesn't refer to a local variable, static field or class. Actual error Caught: java.lang.VerifyError: (class: B, method: main signature: ([Ljava/lang/ String ;)V) Incompatible type for getting or setting field
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Paul King added a comment -

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

        Show
        Paul King added a comment - OK, found the issue. It was a missing line from GROOVY-4228 .
        Hide
        Paul King added a comment -

        Can you try again?

        Show
        Paul King added a comment - Can you try again?
        Hide
        Roshan Dawrani added a comment -

        Seems fine now.

        Show
        Roshan Dawrani added a comment - Seems fine now.
        Hide
        Roshan Dawrani added a comment -

        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.

        A.groovy
        class A {
             static foo(){println "A->foo()"}
        }
        
        B.groovy
        class B extends A {
             @Delegate C c = new C()
             static main(args){
                new B().foo()
             }
        }
        
        C.groovy
        class C {
             def foo(){println "C->foo()"}
        }
        
        Show
        Roshan Dawrani added a comment - 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. A.groovy class A { static foo(){println "A->foo()" } } B.groovy class B extends A { @Delegate C c = new C() static main(args){ new B().foo() } } C.groovy class C { def foo(){println "C->foo()" } }
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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.
        Hide
        Paul King added a comment -

        A new issue might be best

        Show
        Paul King added a comment - A new issue might be best
        Hide
        Roshan Dawrani added a comment -

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

        Show
        Roshan Dawrani added a comment - Raised GROOVY-4265 for the issue related to @Delegate wrongly dealing with static matches.

          People

          • Assignee:
            Paul King
            Reporter:
            Pawel Dolega
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: