Community
Participate
Working Groups
Using HEAD as well as 3.2.2, we don't compile the following test case: interface I<U, V> { U foo(Object o, V v); } public class X<U, V> implements I<U, V> { public Object foo(Object o, Object v) { return null; } } We report: ---------- 1. ERROR in D:\tests_sources\X.java (at line 6) public Object foo(Object o, Object v) { ^^^^^^ The return type is incompatible with I<U,V>.foo(Object, V) ---------- 1 problem (1 error) where javac 1.5.0_12, jdk6_01 and jdk7_b10 report an unchecked warning: X.java:6: warning: foo(java.lang.Object,java.lang.Object) in X implements foo(java.lang.Object,V) in I; return type requires unchecked conversion found : java.lang.Object required: U public Object foo(Object o, Object v) { ^ 1 warning
Will base investigations upon HEAD+162370 (same code area).
Interestingly enough, javac also reports an error upon: interface I<U, V> { U foo(Object o, V v); } public class X<U, V> implements I<U, V> { public Object foo(Object o, V v) { return null; } } That is, when method signatures become identical, javac no more allows the return types discrepancy. IMHO, this probes for a careful check of the spec.
I think this problem is related to bug 150655. It appears we need to handle 'RAW' methods differently than methods which have been converted to use generics.
Per JLS 8.4.5, since Object = |U|, Object foo is return-type substituable for U foo in both test cases. I would then contend that both test cases should compile with an unchecked warning (hence that javac would be wrong on the test case of comment #2).
Created attachment 64144 [details] Tentative fix + test cases This patch follows the spec and javac on the original test case, and follows javac (hence is, IMO, non spec conformant - but passes more practical test cases) on the test case for comment 2. Kent, would you please let me know what you think?
Why call #equals() in areReturnTypesCompatible()? if (one.returnType.equals(two.returnType.erasure())) if (! one.parameters[i].equals(two.parameters[i])) Should this be == or areTypesEqual() ?
Basically, equals makes the test cases pass... but I'll be more than happy to hear about the following: - do we warranty that T1.equals(T2) implies T1 == T2? I would bet that we do not; - what are the exact relationships between equals, == and areTypesEqual for type bindings, and what are their relative merits? I would assume that it is safe to consider that == implies equals and areTypesEqual (will check the code). The existence of types that are equals or areTypesEqual without being == is somewhat likely (I'll read the code, and I'll attempt to exhibit test cases that proves that). Do we have a policy against the use of equals? I'll investigate those questions further myself today, time permitting. But in all cases your help in getting me up to speed will be much appreciated.
According to the javadoc for IBinding#equals, equals should not be used (no added value at all, recommends to use isEqualTo in case we are not sure the bindings are 'in the same cluster'). Conversely, the same suggests that we do not warranty that semantic equality implies == (except if the type bindings we use always fall into the same 'cluster' - would it be the case?). I also checked that neither TypeBinding nor any of its subclasses implements equals. Conclusion is that equals must not be used. Still need to decide which comparison makes sense. The current patch must rely on the default implementation of equals, which doesn't do much more than == I guess. Hence we could probably rely upon == in this instance. Doing so passes the test cases. As far as identified test cases are concerned, == fits and, since nothing else has any chance to be faster, this should be the way to go. However, could you please tell me more about the potential risk to have bindings that would answer true to isEqualTo but false to == in the context of the affected code? Looking at the code of areTypesEqual, I can see that it will answer true for types that are not ==. This does not impact the known test cases, but I will forge a few that show a difference and we'll decide what to do.
Created attachment 64277 [details] Fix + test cases New patch that adds two test cases and adopts the following changes: We can keep == on line 83, since we are comparing to an erasure (hence the extra case documented in areTypesEqual cannot happen). If we want to follow javac on (new) test cases 136 and 137, then we need to use areTypesEqual on line 86. Kent, pls let me know what you think.
Sorry I thought you knew this... org.eclipse.jdt.core.dom.IBinding has nothing to do with org.eclipse.jdt.internal.compiler.lookup.Binding. Make sure you're looking at the correct hierarchy. We've made all TypeBindings unique within a compilation session so == should always be used, except for the MethodVerifier which wants to avoid resolving types if it can (which is why we have #areTypesEqual).
The next issue is methods such as areReturnTypesCompatible() or areParametersEqual() should not report errors/warnings. They may be called from helper methods such as doesMethodOverride(). The boolean methods are intended to answer questions. The senders should detect the error cases.
(In reply to comment #10) ... > We've made all TypeBindings unique within a compilation session so == should > always be used, except for the MethodVerifier which wants to avoid resolving > types if it can (which is why we have #areTypesEqual). Thanks for the explanation. I use areTypesEqual anyway in the parameter types comparison because doing otherwise breaks some test cases. I'll now take for granted that, except for the X<?> vs X<? extends Object> (and similar) case(s), == is the way to go.
(In reply to comment #11) > The next issue is methods such as areReturnTypesCompatible() or > areParametersEqual() should not report errors/warnings. They may be called from > helper methods such as doesMethodOverride(). > > The boolean methods are intended to answer questions. > > The senders should detect the error cases. > Then we'd probably need a tristate here instead of a boolean (something along COMPATIBLE, CONVERTIBLE, INCOMPATIBLE) if we want the caller to report the error. What do you think?
Can this not be handled in checkInheritedReturnTypes() ? Here's the testcase (with javac's behaviour) I would like to focus on. Do we want the same behaviour? class A<U> { U foo() { return null; } U foo(U one) { return null; } U foo(U one, U two) { return null; } } class B<U> extends A<U> { @Override // does not override error Object foo() { return null; } // cannot override foo(), incompatible return type error @Override Object foo(Object one) { return null; } // unchecked conversion warning @Override // does not override error Object foo(Object one, U two) { return null; } } class C<U> extends A<U> { @Override // does not override error Object foo(U one) { return null; } // cannot override foo(U), incompatible return type error @Override // does not override error Object foo(U one, U two) { return null; } // cannot override foo(U,U), incompatible return type error }
(In reply to comment #14) > Can this not be handled in checkInheritedReturnTypes() ? Interestingly enough, areReturnTypesCompatible is called twice within MethodVerifier, and this is not the call from checkInheritedReturnTypes that needs to be more tolerant, but the one from checkAgainstInheritedMethods. Having said that, starting from your question, I suggest the following pattern: introduce checkReturnTypesCompatible(MethodBinding one, MethodBinding two) that calls areReturnTypesCompatible, the latter being left unchanged (it returns true iff return types are strictly compatible), the former emitting a warning and returning true if the return types are not strictly compatible but acceptable modulo an unchecked conversion. I will post a patch that follows those lines.
Created attachment 64412 [details] Fix + test cases This patch implements the course of action suggested in comment #15 above. It also adds a tests series drafted after the test case Kent proposed in comment #14, which essentially adds @Override annotations (and uses classes instead of interfaces). We currently behave as javac on all cases but test130, for which I could not see how the spec could support javac's behavior against ours.
Created attachment 64486 [details] Proposed patch Here's my suggestion. Why can we not report the warning in reportIncompatibleReturnTypeError(), where we already reporting it in another case?
I did not realize the exact intent behind reportIncompatibleReturnTypeError before reading your patch, which is better than mine. I'll drop a comment on MethodVerifier#reportIncompatibleReturnTypeError to clarify its intended behavior.
Created attachment 64568 [details] Fix + test cases Follows Kent's suggestion as implemented in attachment #64486 [details] but keeps test cases in the split format of attachment 64412 [details].
Released for 3.3 M7.
Thinking more about it, I do not like reportIncompatibleReturnTypeError not doing what its name suggests. Opened fup bug 183916 to handle this.
Verified for 3.3M7 using I20070427-0010
