Community
Participate
Working Groups
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 Build Identifier: I20090611-1540 The new suboption "missing override annotation for interface method implementation" introduced as a consequence of bug 163194 results in incorrect behaviour when the return type of a method has an error. Reproducible: Always Steps to Reproduce: public interface I extends J{ Object foo(); } interface J{ String foo(); } In the above case, I.foo() is erroneous, as the return type is incompatible with that of J.foo(). Yet the warning "The method foo() of type I should be tagged with @Override since it actually overrides a superinterface method" is shown.
Seems like the problem was already present before the new suboption was introduced. "The method foo() of type I should be tagged with @Override since it actually overrides a *superclass* method" warning is also observed in case both I and J above are classes and not interfaces.
(In reply to comment #0) > In the above case, I.foo() is erroneous, as the return type is incompatible > with that of J.foo(). Yet the warning "The method foo() of type I should be > tagged with @Override since it actually overrides a superinterface method" is > shown. Why should this warning be wrong? I.foo() still overrides J.foo(), the error re return type incompatibility is a consequence of this overriding, it doesn't inhibit overriding.
IMO, unless the correct return type is used for the method, it can't be said to override the interface method as such. Although it seems plausible to assume that the user is indeed trying to override a method, but the missing @Override warning based on this assumption diverts his attention from the actual problem at hand! We should not go one step ahead of the user to predict what he's trying to do!
(In reply to comment #3) > IMO, unless the correct return type is used for the method, it can't be said to > override the interface method as such. According to JLS 9.4.1: "An instance method m1 declared in an interface I overrides another instance method, m2, declared in interface J iff both of the following are true: - I is a subinterface of J. - The signature of m1 is a subsignature (§8.4.2) of the signature of m2." In 8.4.2 we find: "Two methods have the same signature if they have the same name and argument types" and: "The signature of a method m1 is a subsignature of the signature of a method m2 if either - m2 has the same signature as m1, or - the signature of m1 is the same as the erasure of the signature of m2." etc. So: no mentioning of the return type, which is only inspected _after_ detecting overriding (see how 9.4.1 continues after the quoted part). I remember this being discussed on some other bug recently, but can't remember which. > the missing @Override > warning based on this assumption diverts his attention from the actual problem > at hand! We should not go one step ahead of the user to predict what he's > trying to do! I see your point, but since both messages refer to overriding in some way, the user is not badly diverted, I think. The situation has two possible solutions: a) fix the return type and add @Override b) change the signature so that the method does not override an inherited one Both solutions are actually related to whether or not @Override has been declared. I see reason to change the behavior.
It looks like we are reporting missing Override too early. For this test case: public interface I extends J { @Override Object foo(); } interface J { String foo(); } using javac 1.6, I get: I.java:3: foo() in I clashes with foo() in J; attempting to use incompatible return type found : java.lang.Object required: java.lang.String Object foo(); ^ I.java:2: method does not override or implement a method from a supertype @Override ^ 2 errors We only report incompatible return types.
If we look at these cases together : interface I extends J { Object foo(); String foo2(); } interface J { String foo(); Object foo2(); } abstract class A extends B { abstract Object foo(); abstract String foo2(); } abstract class B { abstract String foo(); abstract Object foo2(); } 1. Ayushman is correct in comment #1 that this issue already existed on classes before the suboption was added - its not new. 2. The whole notion of overrides in the UI (from the little ^ in the outliner/editor) ignores this issue of incompatible return types. 3. But we do try to avoid reporting 'secondary' errors so users can focus on the 'real' problems that must be fixed. IMO, its helpful to know that the compiler believes you're trying to override the inherited method & since the missing @Override 'warning' will usually appear as a warning and not an error... I would say that we leave it as is. The incompatible return type errors should focus the user on what needs to be fixed & the additional missing @Override markers make it obvious that we believe you're overriding inherited declarations and you need to fix the return types. So my vote is to leave as is. Anyone disagree loudly ? FYI : if the @Override are added, javac complains about the return type incompability AND the incorrect @Override. I accept that we'll be different & only report the incompatible return types in this case.
@Kent Sounds reasonable. I concur. Though there's still one consequence of leaving it as it is which we could discuss about: Suppose in your above example of abstract classes A and B, I have the save action "add missing @Override" turned on, then for both methods foo and foo2, I'll get @Override annotations as soon as i do CTRL+S. This will be ok in case i'm actually trying to override and changing the return type would fix everythin, but in the case that i didn't intend to override and want to correct my mistake by changing the signature only, i'll also have to go and remove the @Override annotation!
Yes I agree that we're adding an extra 'step' IF our assumption is wrong. We'll never be 100% correct whichever way we decide, but I can live with 70-30. But IMO we'll be correct more often if we add the @Override vs. assuming the return type mismatch means the user meant a completely different method signature. Closing
Verified for 3.6M3
