Community
Participate
Working Groups
JDT/Summit Compiler topic.
Created attachment 52077 [details] Proposed fix
You shouldn't tag the method binding, but rather the method declaration using an ASTNode.flag instead. Also overridingMethodWithoutSupercall --> overridingMethodWithoutSuperInvocation Should also cache scope.methodScope(), this isn't a free op.
Batch command line argument should be "super" (shorter). Should also be suppressable using @SuppressWarnings("super")
Created attachment 52122 [details] New patch
Additional testcase: abstract class Y { void foo(); } public class X extends Y { void foo() { // should not complain for missing super call, since overriding // abstract method } } same issue with implementing interface method, though I suspect this is covered by the fact we do not set AccOverriding in this case.
for implementing abstract superclass method, you want to check that AccImplementing is not set. i.e. (modifiers & AccOverriding|AccImplementing) == AccOverriding
other than that, patch looks good. bit5 is fine, no collision.
Created attachment 52261 [details] Last patch + regression tests
Released for 3.3M3. Regression tests added in org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest#113/119
Maybe I msised this at the JDT summit, but I don't think this really makes sense. I don't think you can say that every method that override should call super. There are contract that say that you have to override and provide an own implemenation. In that case calling super is not required and maybe not even allowed if the super implemenation also has side effects. What I always suggested was a new annotation that would allow me to specify that super needs to be called.
It is up to the user to decide if this needs to be enabled. By default it is disabled.
It was discussed at the summit under the label "more advanced warnings:" * NotNull, Nullable * overriding methods that do not call super i.e. we discussed means to spec such advanced warnings in the code via some sort of "annotation" and then be able to check those. Without this mechanism I agree with Martin that it does not make much sense (too many false positives).
+1 for comment 10 and comment 12. I would remove this warning again, until we have something like @OverrideCallSuper.
As long as an annotation like @OverrideCallSuper is not standardized, we cannot use it. The actual code is fine and has almost no impact on the batch compiler. So I propose to keep the code inside the batch compiler (since the implementation simply needs to be tuned once the @OverrideCallSuper is defined) and close the one relative to the addition of the UI option as RESOLVED/LATER. Same idea for the null checks. @Null or @NotNull will help to improve it. See jsr305 for this.
Re: need for annotation. @OverrideCallSuper should be placed on overriden method, and force overriding ones to invoke super. Would need to figure if it propagates down for free (likely not).
Verified for 3.3 M3 using build I20061030-0010
Instead of waiting for something like 'java.lang.OverrideCallSuper' or 'java.lang.NotNull' to go into the JDK libraries, we should allow the user to create such an annotation by himself in his project and add a preference where the user can tell the compiler the qualified type name of that annotation. Same for NotNull ect. As soon as Java 7 gets the annotation added, we will simple set its qualified name as the preferences default value. So for example we might at some point add such an annotation for Eclipse 'org.eclipse.core.apis.SuperCallRequired'. If we want to go a step further, we let clients provide a AnnotationEvaluator that does the lookup for annotations, providing something like isAnnotated(methodOrTypeOrField, "SuperCallRequired"). A default implementation of an AnnotationEvaluator would test for a certain Java 5 annotation. A client's implementation might consider Javadoc tags or even use information from a metadata file. This then of course would work also for 1.4 or less compliance. So I really think we either evolve this feature further or remove the option again. It just doesn't make sense in the current state. It gives people the impression that we don't know the diffenerce between 'extending' and 'overriding' a method or gives a bad example for Java novices. Do you know of any projects that can use this setting?
JSR305 is about standardizing more annotation types. We are not removing the support from the compiler, simply not surfacing it in UI for now. As to providing a custom pref to define user marker annotations, we thought of it already; still thinking about the investment value add vs. waiting on JSR305 (where we will need to redo it). I think when JSR305 occurs, we will need more than just default to its values. We would rather need to support both (standard+user custom ones).
>still thinking about the investment value add vs. waiting on JSR305 >(where we will need to redo it). We should not only look at the investment on our side but also on the benefit for the users that are locked into using 1.4 or even older.
