groovy
  1. groovy
  2. GROOVY-2829

Groovy's class verification doesn't allow for covariant return types

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6-beta-1
    • Fix Version/s: 1.5.7, 1.6-beta-2
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      1

      Description

      Given these Java interfaces:

      import java.util.List;
      
      public interface Base {
          List foo();
      }
      
      import java.util.ArrayList;
      
      public interface Child extends Base {
          ArrayList foo();
      }
      

      Java allows the return type for methods to be the most specified or a derived type:

      import java.util.ArrayList;
      
      public class JavaChildImpl implements Child {
          public ArrayList foo() {
              return null;
          }
      }
      

      But compiling this Groovy script:

      class GroovyChildImpl implements Child {
          public ArrayList foo() {
              return null
          }
      }
      

      yields:

      org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed, GroovyChildImpl.groovy: 2: the return type is incompatible with java.util.List foo() in Base.
      Node: org.codehaus.groovy.ast.MethodNode. At [2:5]  @ line 2, column 5.
             public ArrayList foo() {
             ^
      
      1 error
      

      Attached patch fixes this problem.

        Activity

        Hide
        blackdrag blackdrag added a comment -

        I guess the check (if it is allowed and if a bridge method has to be added) respects super classes, but not interfaces being implemented by the current class.

        Show
        blackdrag blackdrag added a comment - I guess the check (if it is allowed and if a bridge method has to be added) respects super classes, but not interfaces being implemented by the current class.
        Hide
        Paul King added a comment - - edited

        I had a patch but it doesn't handle a particular case that I just noticed. I'll have a bit more of a look and then get you to comment on the patch once amended and attached. When you say "respects super classes, but not interfaces being implemented by the current class" I think I understand but do you want to explain further what you mean.

        Currently the patch modifies two classes. When ClassCompletionVerifier is checking for abstract methods it skips reporting an error if a derived method exists. Then in getCovariantImplementation in Verifier, instead of checking for equality between the overridingMethod and oldMethod class nodes it checks whether the type classes are assignable. Problem is that the return type for the overriding method may not be resolved yet, so there is more work to do.

        Show
        Paul King added a comment - - edited I had a patch but it doesn't handle a particular case that I just noticed. I'll have a bit more of a look and then get you to comment on the patch once amended and attached. When you say " respects super classes, but not interfaces being implemented by the current class " I think I understand but do you want to explain further what you mean. Currently the patch modifies two classes. When ClassCompletionVerifier is checking for abstract methods it skips reporting an error if a derived method exists. Then in getCovariantImplementation in Verifier , instead of checking for equality between the overridingMethod and oldMethod class nodes it checks whether the type classes are assignable. Problem is that the return type for the overriding method may not be resolved yet, so there is more work to do.
        Hide
        blackdrag blackdrag added a comment - - edited

        you can not check for equality, because:

        class X{def foo(){}}
        class Y{String foo(){}}

        In this case we want covariation to kick in and let String foo() overwrite Object foo() by adding a bridge method for Object foo() that calls String foo(). Since the overwritten method can not return anything that its return type does not allow, I use assignable to check that. At the phase where this is called, the return type is usually resolved already, since it happens after ResolveVisitor is called.

        Show
        blackdrag blackdrag added a comment - - edited you can not check for equality, because: class X{def foo(){}} class Y{ String foo(){}} In this case we want covariation to kick in and let String foo() overwrite Object foo() by adding a bridge method for Object foo() that calls String foo() . Since the overwritten method can not return anything that its return type does not allow, I use assignable to check that. At the phase where this is called, the return type is usually resolved already, since it happens after ResolveVisitor is called.
        Hide
        blackdrag blackdrag added a comment -

        I read "Attached patch fixes this problem."... is there a patch?

        Show
        blackdrag blackdrag added a comment - I read "Attached patch fixes this problem."... is there a patch?
        Hide
        blackdrag blackdrag added a comment -

        anyway, I will commit a fix for this in a few minutes. The problem was that in the code checking for covariance isDerivedFrom was used. but isDerivedFrom checks only super classes, no interfaces.... Maybe that should be considered as bug too.. We have also declaresInterface, so I guess it does not have to be seen as bug. Anyway, since only isDerivedFrom was used, only super classes where checked, and not interfaces. My patch will fix that and the test case will pass then.

        Show
        blackdrag blackdrag added a comment - anyway, I will commit a fix for this in a few minutes. The problem was that in the code checking for covariance isDerivedFrom was used. but isDerivedFrom checks only super classes, no interfaces.... Maybe that should be considered as bug too.. We have also declaresInterface, so I guess it does not have to be seen as bug. Anyway, since only isDerivedFrom was used, only super classes where checked, and not interfaces. My patch will fix that and the test case will pass then.
        Hide
        Paul King added a comment -

        OK, I will await your patch and then compare.

        Show
        Paul King added a comment - OK, I will await your patch and then compare.
        Hide
        Paul King added a comment -

        I think there are further cases.

        Show
        Paul King added a comment - I think there are further cases.
        Hide
        Paul King added a comment -

        Altered the test cases and supplied a potential patch to make them pass.

        Show
        Paul King added a comment - Altered the test cases and supplied a potential patch to make them pass.
        Hide
        Paul King added a comment - - edited

        I should have mentioned. I am not sure whether the change I had to make to GroovyInterceptableTest is desirable. I expect not. Perhaps an improved fix is required. Perhaps we need both a implementsInterface and implementsInterfaceDirectOrIndirect.

        Show
        Paul King added a comment - - edited I should have mentioned. I am not sure whether the change I had to make to GroovyInterceptableTest is desirable. I expect not. Perhaps an improved fix is required. Perhaps we need both a implementsInterface and implementsInterfaceDirectOrIndirect .
        Hide
        blackdrag blackdrag added a comment -

        first.. if you find another bug in this, please open a new issue, since this one has been resolved. But I agree, that the isAssignable test might not cover all of it.... for example did we test

        interface I1{}
        interface I2 extends I2{}
        class X implements I2{}
        class Foo {
          I1 foo(){}
        }
        class Bar extends Foo {
          X foo(){}
        }
        

        In that example X does not directly implement I1, but the method in Foo should still be overwritten by the one in Bar. Also I am very for a isAssignable method on ClassNode working with a ClassNode, not more String based patchwork here.. in fact that stupid string method (declaresInterface was it I think) should vanish. If you want to do that, feel free to do so... just the change to GroovyInterceptableTest should be undone.

        Show
        blackdrag blackdrag added a comment - first.. if you find another bug in this, please open a new issue, since this one has been resolved. But I agree, that the isAssignable test might not cover all of it.... for example did we test interface I1{} interface I2 extends I2{} class X implements I2{} class Foo { I1 foo(){} } class Bar extends Foo { X foo(){} } In that example X does not directly implement I1, but the method in Foo should still be overwritten by the one in Bar. Also I am very for a isAssignable method on ClassNode working with a ClassNode, not more String based patchwork here.. in fact that stupid string method (declaresInterface was it I think) should vanish. If you want to do that, feel free to do so... just the change to GroovyInterceptableTest should be undone.
        Hide
        Paul King added a comment -

        I only reopened because I realised that my original testcase wasn't complete enough. I will add in my patch as it covers the case you mention above.

        Show
        Paul King added a comment - I only reopened because I realised that my original testcase wasn't complete enough. I will add in my patch as it covers the case you mention above.
        Hide
        Paul King added a comment -

        was fixed about a week ago I believe

        Show
        Paul King added a comment - was fixed about a week ago I believe
        Hide
        Raffael Herzog added a comment -

        Looks like the problem has returned in 1.8.6. I'm running into this, exactly as described in the original posting.

        Show
        Raffael Herzog added a comment - Looks like the problem has returned in 1.8.6. I'm running into this, exactly as described in the original posting.
        Hide
        blackdrag blackdrag added a comment -

        Raffael, can you please fill a new issue and explain there what exactly your issue is? Do you mean the example in this issue no longer works?

        Show
        blackdrag blackdrag added a comment - Raffael, can you please fill a new issue and explain there what exactly your issue is? Do you mean the example in this issue no longer works?
        Hide
        Raffael Herzog added a comment -

        Done: https://jira.codehaus.org/browse/GROOVY-5418

        And no, the issue is very similar, but the example in this issue should still work, AFAICS.

        Show
        Raffael Herzog added a comment - Done: https://jira.codehaus.org/browse/GROOVY-5418 And no, the issue is very similar, but the example in this issue should still work, AFAICS.

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Paul King
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: