Community
Participate
Working Groups
Hi. The following code contains a bug in inheritance of generic methods: abstract class A<T> { void g1 (T t) { System.out.println("g1 base: " + t); } void g2 (T t) { System.out.println("g2 base: " + t); } } public class B extends A<java.util.List<Number>> { void g1 (java.util.List<?> t) { System.out.println("g1 derived: " + t); } void g2 (java.util.List<Number> t) { System.out.println("g2 derived: " + t); } public static void main (String[] args) { B b = new B(); b.g1(new java.util.ArrayList<Number>()); b.g2(new java.util.ArrayList<Number>()); } } Notice that B.g1() does not override A.g1() because it declares a type parameter incompatible with the bound T->List<Number> established by class B. The JDT compiler is working correctly; output is: g1 base: [] g2 derived: [] and in the bytecode, B has a stub for erasure delegation g2(Object)->g2(List) but not for g1(Object)->g1(List). The problem is in the editor. There is a marker saying that B.g1() overrides A.g1(); it's wrong as there is no overriding in this case, only overloading. I performed some experiments with variations of this example, and it seems that the source editor ignores completely the parameters' generic types to infer inheritance, even though it considers generic type info from class hierarchy: if I declare B.g2(String), the overriding marker disappears because String is not compatible with B's T->List<Number>. If I tag B's methods with the annotation @Override, the behavior is correct. The annotation is OK for g2(), but at g1() the compiler flags an error: "The method g1(List<?>) of type B must override a superclass method". This shows that the compiler and the editor's code responsible for marker generation are not using the same strategy to validate method overriding. This wrong marker is an usability problem, but a relatively severe one because it induces the programmer to error. I just had this kind of error in my app and it was difficult to analyse because I failed to inherit a method, so the wrong method was being invoked, but inspection of the source didn't reveal the problem because I blindly trusted the overriding marker -- shame on me for not using the @Override tag, I know -- so I only caught this bug after laborious step-by-step debugging.
Another case related to bug 97027
Markus, as discussed the problem is with taking the erasure of the left hand side in equals. Although the fix is trivial it is risky since the method is called in various other situations as well and we have to understand how these clients use the method. We should investigate for 3.1.1
Created attachment 23829 [details] Proposed path
If we decide to keep our implementation of the override checking code, then we should also rename isEqualMethod(..) to isSubsignature(..) and check all clients and both implementations whether they are only used for override checking and only in the right direction. Furthermore, bug 97027 should also be fixed then (we currently don't compare method type parameters). The alternative would be to switch to IMethodBinding#overrides(..) in cases where we have bindings. Clients that have less strict requirements such as quick fix (doesn't want to check return types) should then still use the old code, unless IMethodBinding#overrides(..) gets loosened to not checking return types.
Created attachment 25674 [details] Better patch
The patch from comment 5 also fixes the override indicator bug 97027. I updated all clients of isEqualMethod(IMethodBinding method, String methodName, ITypeBinding[] parameters) to isSubsignature(IMethodBinding, IMethodBinding) where I think it is safe.
Released the patch to HEAD.
Although this is not a one line fix, I opt to put it into 3.1.1 if we see that it works fine in the 3.2 branch. Markus, here are some comments/questions regarding the fix: Class Bindings: - isSubSignature: to avoid some false positives shouldn't we only take the erasure of the m1Parameters when the parameter itself contains a type variable. Otherwise I think it isn't needed. - areSubTypeCompatible: please check the method with Tobias and look if we can remove it in favour of isSubSignature. The current implementation of areSubTypeCompatible doesn't look quite right either ;-) Class StubUtility2 - findOverridingMethod: checking both isSubSignature and areSubTypeCompatible looks strange to me. Please check with Tobias. Martin, Tobias can you please have a look at the patch as well and cast your votes.
Regarding StubUtility2#findOverridingMethod: The conditional expression in this method can be replaced by Bindings#isSubSignature only, because the method Bindings#areOverriddenMethods effectively should implement the same behavior as Bindings#isSubSignature. The implementation suggested by the patch works, but does or'ed checks for the same thing. Method invocations to Bindings#areOverriddenMethods should therefore be replaced by isSubSignature Regarding Bindings#areSubTypeCompatible The method occurrence in areOverriddenMethods can be eliminated (see above). The occurrence in isSignatureEquivalentConstructor actually corresponds to a call to isSubSignature, but without checking for the method name. With the current API of isSubSignature, this is not possible. IMO we should be more flexible and offer an API where the components of a signature (name, parameters, exception, etc.) can be passed in separately. +1 for 3.1.1
*** Bug 97027 has been marked as a duplicate of this bug. ***
patch looks ok to me.
Created attachment 26185 [details] Patch 3 This (last) patch only takes the erasure where the parameters contain type variables (Dirk's comment 8) and fixes several other issues found while writing test cases. The only changes w.r.t. "Better Patch" are in Bindings#isSubsignature(..) and three new private methods.
Created attachment 26186 [details] Tests for Patch 3
Improved patch looks OK to me.
patch is OK. We now also have much more test cases -> ok to release.
Released to 3.1.1 (and HEAD).
verifying...
verified scenarios in comment 0 and bug 97027 work correctly.
