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

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.

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.

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.

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

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.

OK, I will await your patch and then compare.

I think there are further cases.

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

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.

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.

was fixed about a week ago I believe

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

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?

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.