Community
Participate
Working Groups
I20061102-0010 To complete support from bug 141931: interface I { void m(); } public class Over implements I { public void m() { // should warn about missing @Override } }
This is on our list, but I wonder if we need a separate option for it (suboption actually: check implementations of interface methods). Maybe only conditionned by source 1.6 compliance. The part which worries me is about trashing code for which user finally realizes 1.5 is all he needs, and then realize that @Override has become a problem.
We should not add @Override. It is tolerated, but it is not mandatory. It is rather an implementation of the interface method than an overriding.
I think it's more logical to add @Override for implemented methods exactly like abstract methods... to say the method is from an interface but not a "new" method. I found this blog: http://blogs.sun.com/ahe/entry/override I would like to have this feature (not present in Eclipse 3.3RC3 yet), and I would like the automatic writing of @Override when clicking on "override/implement methods"... even for implementation :)
It seems inconsistent (and in fact an error) to me that if @Override for an interface method is handled just like @Override for a base class method (and by handled, I mean that when you have it, there really must be a method that it overrides a method in the base class or base interface), that the options for adding it and reporting it as a warning when it is absent doesn't handle them the same way. I agree with Philippe's comment that ideally there would be separate warning settings and separate cleanup options for them. If EMF is to support Java 6.0 code generation for users, we really need this kind of automatic diagnosis of missing @Overrides. I don't agree with Olivier's comment because an @Override that's present when it should not be present is treated as an error, so having it missing when it could be present should be detectable, reportable, and automatically fixable. Wouldn't life have been simpler if they'd just done it this way for Java 5.0? :-( I wonder if @Implements would have been a better approach? But then I suppose one could have both @Implements and @Override on the same darned method. Anyway, it's not something we can change.
interface I { void m(); //@Override - could warn about missing override public boolean equals(Object obj); } interface J extends I { //@Override - could warn about missing override void m(); } public class X implements J { //@Override - could warn about missing override public void m() { } } Post 3.4, since it would need adding a new sub preference to missing @Override annotation: 'also for interface methods implementation'
interface I { void m(); @Override boolean equals(Object obj); } interface J extends I { @Override void m(); // @Override error in 1.5, not in 6.0 } public class X implements J { @Override public void m() {} // @Override error in 1.5, not in 6.0 } FYI : @Override is 'accepted' in more places after 1.5
*** Bug 235750 has been marked as a duplicate of this bug. ***
This should be fixed in 3.5, such that dependencies (bug 242244, bug 235750) can be fixed as well. If this introduces a new problem ID or a new preference constant, then we need to reopen bug 235750 and update the quick fixes.
Kent - pls have a look
This could need new API, so it must be decided for M6.
I'm a little confused--should @override be added to interface implementations or not? The reason I ask is that I get bitten all the time when I try to export plugins (create deployable plugins and fragments) and I get compile errors when @override is used for interface implementations. Is it a bug that using the QuickFix for adding interface methods adds the @override, or is it a bug that the deploy compiler treats the annotation as an error? The normal incremental compiler doesn't warn me about it.
For Java 1.6 it's allowed but for 1.5 it's not allowed. So the answer is, it depends.
Ah, maybe that's my problem then. I'm running Eclipse with a 1.6vm and a 1.6 compiler, but I have the "compiler compliance" set to 1.5. So I guess Eclipse sees that I'm "using" the 1.6 jre and adds the @override calls, but there's some difference between the incremental compiler and the deploy compiler about how to handle 1.5 compliance.
Given Markus' comment, will this make 3.5 release?
I am running Eclipse 3.5 with my compiler compliance set to 1.6, but don't see this functionality. Is there any update on when this will be supported? At the minimum, it would be great to be able to have the @Override annotation automatically added for implemented interface methods.
Just to make sure we are on the same page, you are asking to get a warning when @Override is missing on an interface method and the project compliance is 1.6? The consequence of adding it would be that the code can no longer compile with the compliance 1.5. Without the @Override, it compiles fine with both compliances. So I don't see why this is a "good" thing to add it.
> So I don't see why this is a "good" thing to add it. Ed's comment 4 nicely explains this. If you want the compiler to flag inadvertent overrides in the future, then you need to have @Override at all places it is allowed. The only thing I can add are pointers to the current definition: http://java.sun.com/javase/7/docs/api/java/lang/Override.html and a blog entry that explains that this update is missing in the J2SE 6 docs: http://blogs.sun.com/ahe/entry/override_snafu
(In reply to comment #16) If you practice interface driven development, the interfaces seem to be constantly evolving. Without default @Override I have found in many cases that the dead code is left to rot for good in many implementing classes. The most common case is when interface as a whole is removed from existence: you remove 'implements' and all the dead methods are left in-place, specifically if the interface was somewhat prolific. As to your point about 1.5 compliance. @Override on classes will not compile on 1.1 or even 1.4 JDK, but that does not seem to bother people, because many 1.5 features are worth dropping 1.5 compiler support. Same can be said about 1.6. You commit to it because you want/like its features. Hence when I commit to 1.6 compliance level I want all the features of 1.6 work as advertized. So, if you have a control that says 'Missing @Override annotation' it should do exactly what it says. If you want to add more (un)support for 1.5 you can add more verbose explanation there that it will not check for presence of @Override annotation on interface methods.
I believe Alex in comment #18 is right on. I experience the situation he described exactly. As a result, I still believe this would be excellent functionality to add.
+1!
+1 on #18
Ayushman, please investigate.
*** Bug 271254 has been marked as a duplicate of this bug. ***
Created attachment 146743 [details] proposed fix v0.5+ tests Added a subpreference OPTION_ReportMissingOverrideAnnotationForInterfaceMethodImplementation. (It has been more or less handled in the same way as OPTION_ReportDeprecationInDeprecatedCode and so initialised/updated at all relevant places. Note that in org.eclipse.jdt.internal.compiler.batch.Main.handleWarningToken, the new option has been initialised only for java 1.6. ) Modified compiler/org/eclipse/jdt/internal/compiler/ast/MethodDeclaration. Now missing override annotations are reported for java 1.6 at all relevant place. Included regression tests 271a, b, c, d to test different scenarios. Remastered test013() in BatchCompilerTest One question- the error message for missing override annotations is of the form "The method m() of type B should be tagged with @Override since it actually overrides a superclass method" . Should the wording be changed to say "supertype"? However, quite a lot of existing regression tests will have to be remastered to do so. If its really worth the effort, maybe that change can be made. Please review.
I prefer to have a specific error message for the new warning. So we need a new IProblem id + corresponding message. If I use the warning (-warn:+over-ann), I cannot disable the new warning. I think the new warning should be triggered with a specific batch compiler option. We could have: allOver-ann and over-ann. This would behave like allDeprecation vs deprecation. Beside this it looks pretty good. Could you please make sure that you don't have trailing whitespaces at the end of line? Please submit a new patch that addresses these issues. Thanks.
Created attachment 146860 [details] modified fix + tests New batch compiler option allOver-ann added. Regression test BatchCompilerTest.test292_warn_options() added. Note that the description for the new option in message.properties says "all missing override annotations". Couldn't elaborate further here because of 80 character limit for the description. New problem id and message created to report missing override annotations. The message now says "supertype" instead of "superclass".
Created attachment 146881 [details] Proposed fix + regression tests Adjust some @since tags to reflect 3.6 instead of 3.5 + minor changes for the compiler options category and irritants. Ayushman, please let me know what you think. Kent, please review.
@Olivier, I have a few issues. First, for other "suboptions" such as 'signal use of deprecated API inside deprecated code', we don't have a separate irritant. Then is it imperative to have it for our new suboption? Second, even if we want to have a separate irritant, i think it should be initialised in group 2, with bit mask 3. This is because reading through the description for irritants tells me that they are stored in 8 groups, each of 29 bits. So group 1 and 2 are already full. Third, a couple of minor changes are required in resetDefaults() and toString() in CompilerOptions.I'll submit a patch with these changes according to what you say about having a new irritant.
Oh and btw, I found that the new functionality doesnt work if we have a separate irritant. No error is reported because somehow the getSeverity() method returns ProblemSeverities.Ignore . I think wer'e missing out on setting/updating the severity for the new irritant. Tried calling updateSeverity for the new suboption from CompilerOptions.set(Map), still didnt work. IMO, just like the suboption reportDeprecationInsideDeprecatedCode, we can link our new suboption to the irritant and severity of the main missingOverrideAnnotation option. Works fine that way.
ok, for reusing same irritant. Please provide a new patch for it that is fixing the @since 3.6 issues.
Markus, Please check the next patch in order to know if this is good enough for you to integrate it into the UI compiler option.
> Markus, > Please check the next patch in order to know if this is good enough for you to > integrate it into the UI compiler option. Did you really mean he should wait for the next patch or should he check the attached one?
Created attachment 147293 [details] proposed fix + regression tests The @since tags have been adjusted to say 3.6 instead of 3.5, and the expectedProblemAttributes in CompilerInvocationTests have been arranged lexicographically. Two additional(minor) changes are in toString() and resetDefaults() in CompilerOptions to handle the new suboption. Rest all is same as in patch 146881.
Please ignore comment 32.
Markus, please check the patch in comment 33 and let us know if the UI can be updated for the new option. Ideally the new option should not be enabled if the compliance is lower than 1.6. Thanks.
Should the new error message read : The method m() of type B should be tagged with @Override since it implements a superinterface method instead of : The method m() of type B should be tagged with @Override since it actually overrides a supertype method We still have the other message for overriding a superclass' method : The method m() of type B should be tagged with @Override since it actually overrides a superclass method
(In reply to comment #36) > Should the new error message read : > > The method m() of type B should be tagged with @Override since it implements a > superinterface method > > instead of : > > The method m() of type B should be tagged with @Override since it actually > overrides a supertype method +1. The method m() of type B should be tagged with @Override since it implements a superinterface method is better.
(In reply to comment #37) > The method m() of type B should be tagged with @Override since it implements a > superinterface method > is better. I dont think restricting the wording to superinterface is good here. Reading http://blogs.sun.com/ahe/entry/override can give one a clear picture of what all possible cases can have missing @Override annotation warning/error. So for a case like interface A { void m(); } interface B extends A { void m(); // overrides but doesn't implement } saying "implements a superinterface method" in the error message will make it incorrect(An interface cannot implement methods per se). Also, in the following case: public interface Test { void m(); } abstract class A implements Test { // m() is inherited from Test } class B extends A { //@Override public void m() {} } B.m()is NOT 'implementing a superinterface method', but rather 'overriding and implementing a supertype(superclass) method'. Thats why, i think the more general message 'The method m() of type B should be tagged with @Override since it actually overrides a supertype method' is better.
1. interface A { void m(); } interface B extends A { void m(); } Yes this example is valid code, but it doesn't happen very often because B.m() has no value whatsoever. But since its possible we change the message to : The method m() of type B should be tagged with @Override since it overrides a superinterface method 2. public interface Test { void m(); } abstract class A implements Test {} // m() is inherited from Test class B extends A { public void m() {} } >B.m()is NOT 'implementing a superinterface method', but rather 'overriding and >implementing a supertype(superclass) method'. How does A define an m() in your example, the only m() is defined by B And I don't see how its not implementing/overriding the interface method Test.m() I think we should make it clear that this new message deals with superinterface methods, and the existing message takes care of superclass methods.
(In reply to comment #39) > But since its possible we change the message to : > > The method m() of type B should be tagged with @Override since it overrides a > superinterface method This is definitely better than saying "implements". Can be done. > How does A define an m() in your example, the only m() is defined by B > > And I don't see how its not implementing/overriding the interface method > Test.m() Well I didn't mean A defines m(). Its correct that B.m() implements Test.m(), but IMO, the error message should be such that the writer of B, without bothering much about what interfaces A implements, can get the point and correct his mistake. Not much of an issue though. > I think we should make it clear that this new message deals with superinterface > methods, and the existing message takes care of superclass methods. So should i change the message to : The method m() of type B should be tagged with @Override since it overrides a superinterface method. ?
The only alternative would be to have only 1 message for all cases : 'The method m() of type B should be tagged with @Override since it actually overrides a supertype method' Would still have 2 options but just only 1 message & 1 method in ProblemReporter. Does anyone have a strong opinion for 1 message vs. 2 ? I don't
As long as the message clearly states what the problem is, I don't have a problem with using only one message.
Thats a good alternative. However, it'll require remastering of about a 100 regression tests. If you think its a worth the effort, I'll do it.
In this case I would keep the existing message and add a new message that clearly states it concerns superinterfaces methods. So you will have two problem ids, two methods in problem reporter and two different messages. If users find this too confusing, we can then easily merge to one message. But if we go with only one message immediately, it will be difficult to go back to two messages if users prefer two messages. So please provide an update using two messages that clearly handles superclass vs superinterface methods. We will then release that code and wait for feedbacks. Thanks Ayushman.
Created attachment 147758 [details] proposed fix with 2 msgs + regression tests Changed the message to say 'superinterface instead of 'supertype'.
No big deal - but why are there so many changes to CompilerInvocationTests.java in the patch ?
Released for 3.6M3. Feedbacks are welcome on this bug if you find anything that doesn't match what you expect. Regression tests added in: org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test274a org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test274b org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test274c org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test274d Updated org.eclipse.jdt.core.tests.compiler.regression.CompilerInvocationTests and org.eclipse.jdt.core.tests.compiler.regression.BatchCompilerTest.
Open bug 290135 to add the corresponding option in the UI.
w00t! Thanks a lot! Can't wait to install the next milestone! :)
Looks good, except for 2 details: 1. In the Javadoc of JavaCore.COMPILER_PB_MISSING_OVERRIDE_ANNOTATION_FOR_INTERFACE_METHOD_IMPLEMENTATION, the sentence "This option should be available only if the compliance is greater or equals to 1.6." is not clear enough ("should" says nothing, and what happens if clients still set it?). I would say: "This option only has an effect if the compiler compliance is 1.6 or greater." 2. The option should be enabled by default. The main option is 'Ignore' by default, so this will not affect people who don't care about @Override. But we had many requests to make this work for 1.6, and the revised Javadoc of the Override annotation also does not make one case "more special" than the other, so we should give users the complete support out of the box.
If I enable it by default, this means that I should not check this new option if the option COMPILER_PB_MISSING_OVERRIDE_ANNOTATION is disabled.
This is properly handled in the problem reporter when reporting the problem. The problem creation is ignored if the main option is not set to warning or error.
This also means that users with the main option set to warnings can now get additional warnings without changing anything.
> This also means that users with the main option set to warnings can now get > additional warnings without changing anything. Yes, and that's goodness, since users missed them.
(In reply to comment #50) > Looks good, except for 2 details: Both are fixed now.
(In reply to comment #46) > No big deal - but why are there so many changes to CompilerInvocationTests.java > in the patch ? the changes are only reshuffling of the problem attributes lexicographically, nothing major. Just one problem attribute corresponding to the new option has been added.
For the batch compiler, the warning option allOver-ann should be used to get the warnings for interface methods. over-ann warning continues to work as it did before.
A lot of tests on HEAD(121 approx.) are failing due to the above changes(enabling the option by default). I'm in the process of remastering them, will post the changes as soon as i can.
Sorry about that. I meant to run all tests last night, but I had issues with my laptop from home :-(.
Verified for 3.6M3 using build I20091025-2000
\o/ many thanks!
