[#GROOVY-2829] Groovy's class verification doesn't allow for covariant return types

groovy

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

Created: 16/May/08 04:52 AM Updated: 29/May/08 12:20 AM

Component/s:

None

Affects Version/s:

1.6-beta-1

Fix Version/s:

1.5.7, 1.6-beta-2

Time Tracking:

Not Specified

File Attachments:

groovy2829_additional.patch (4 kb)

Description

« Hide

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.

All

Comments

Work Log

Change History

Sort Order:

Ascending order - Click to sort in descending order

[ Permlink | « Hide ]

Jochen Theodorou - 16/May/08 06:05 AM

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.

[ Permlink | « Hide ]

Paul King - 16/May/08 06:30 AM - 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.

[ Permlink | « Hide ]

Jochen Theodorou - 16/May/08 06:52 AM - 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.

[ Permlink | « Hide ]

Jochen Theodorou - 16/May/08 07:11 AM

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

[ Permlink | « Hide ]

Jochen Theodorou - 16/May/08 07:14 AM

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.

[ Permlink | « Hide ]

Paul King - 16/May/08 07:33 AM

OK, I will await your patch and then compare.

[ Permlink | « Hide ]

Paul King - 17/May/08 09:14 AM

I think there are further cases.

[ Permlink | « Hide ]

Paul King - 17/May/08 09:14 AM

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

[ Permlink | « Hide ]

Paul King - 17/May/08 09:44 AM - 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.

[ Permlink | « Hide ]

Jochen Theodorou - 17/May/08 10:39 AM

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.

[ Permlink | « Hide ]

Paul King - 17/May/08 05:05 PM

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.

[ Permlink | « Hide ]

Paul King - 29/May/08 12:20 AM

was fixed about a week ago I believe