Community
Participate
Working Groups
N20080911-2000 Scenario: Client implements interface ISomething. It needs to implement all methods. Now the developer of ISomething decides to deprecate a method on the interface with it's corresponding return type (eg. public void DeprecatedReturnType doSomething(); ). Option "Signal overriding or implementing deprecated method" is off. The problem here is that the warning for the implemented (and deprecated) method is not flagged but the return type. As the client need to implement the interface we force to produce a warning. Current workaround: Add the @deprecated tag to the implementing method. See the following snippet: ---- E.java ---- package test1; public class E implements I{ public I2 foo() { return null; } } interface I { /** * @deprecated */ public I2 foo(); } ---- I2.java package test1; /** * @deprecated */ public interface I2 { } While there is no warning on interface I for I2 because foo() is deprecated, there is still a warning on E#foo as the deprecated flag of the interface method is not concerned. javac produces two warnings in this scenario which looks like the following: E.java:4: warning: [deprecation] test1.I2 in test1 has been deprecated public I2 foo() { ^ E.java:4: warning: [deprecation] foo() in test1.I has been deprecated public I2 foo() { ^ 2 warnings In my eyes the deprecated return type should not be flagged if the interface method declaration is already deprecated.
Created attachment 112476 [details] test cases Added two new testcases to DeprecatedTest
Srikanth - pls have a look
Created attachment 124141 [details] Preliminary patch This patch will undergo some more changes as I test further. It will also undergo some cleanup, but ... It is ready for review though and it fixes the problem and passes the attached test (Thanks Benjamin!) and the regression tests (except for a couple of failures due to white space differences in messages that are being looked into.) Essentially this patch flags a method as being "implicitly deprecated" if it implements an abstract method _and_ the abstract method is tagged as being deprecated. The deprecation warnings for method arguments, return types and exception types originally used to be issued under CompilationUnitScope.faultInTypes(). This is too early to know if a method implements an abtract interface spec as CompilationUnitScope.verifyMethods has not been run at that point (which is the stage when the set of inherited methods and overridden methods are computed). Current patch introduces a new step called CompilationUnitScope.reportDeprecatedTypesInSignatures() that runs after both CompilationUnitScope.faultInTypes() and CompilationUnitScope.verifyMethods have run and so can say for sure whether a method should be considered implicitly deprecated due to its being an implementation of an abstract deprecated interface spec.
Created attachment 124262 [details] Proposed fix & tests This fixes the problem and passes the tests. Philippe, please review and help take this to commit - thanks. Comment #3 outlines the approach to the solution.
(In reply to comment #4) > Created an attachment (id=124262) [details] > Proposed fix & tests > This fixes the problem and passes the tests. Yes, but there some scenarios untested by the regression test harness which are not handled well by this patch. (i.e, some deprecation warnings that are currently issued will cease to be). This is under investigation.
Created attachment 124532 [details] Take 2 : An alternate fix with tests The earlier patch posted via attachment (id=124262) is bad. While the problem diagnosis is correct (i.e, the deprecation messages are emitted prematurely, before method verification step is done (only after which we can tell for sure if a method is implementing an abstract deprecated interface)), the solution does not handle some cases correctly: For example, that patch introduces the new method reportDeprecatedUsagesFor which is called in a separate pass after method verification is done. private void reportDeprecatedUsagesFor(MethodBinding method) { ... ... ... ... // Report deprecation on argument types as needed. Argument[] arguments = methodDecl.arguments; if (arguments != null) { int size = arguments.length; for (int i = 0; i < size; i++) { Argument anArgument = arguments[i]; if (anArgument != null && anArgument.type != null && anArgument.type.resolvedType != null) { if (!anArgument.type.resolvedType.isValidBinding()) continue; if (anArgument.isTypeUseDeprecated(anArgument.type.resolvedType, methodDecl.scope)) { methodDecl.scope.problemReporter().deprecatedType(anArgument.type.resolvedType, anArgument.type); } } } } ... ... ... ... } Here we are checking whether the argument's resolvedType is deprecated. This is insufficient as the argument (or the exception, return type, type parameter etc) could be of any of the several subtypes of TypeReference. For example if argument is of type OuterClass.InteriorClass.InnerClass, the deprecation check has to happen for every one of the components of the QualifiedTypeReference and not just for InnerClass. (Similarly for other subtypes of TypeReference). Given the degree of possible self and mutual references, it appears that it would be a non-trivial task to introduce a separate pass that models the original full blown traversal and ensure that - None of the diagnostics which would have been originally emitted or indavertantly missed out in the second pass. - None of the diagnostics are emitted twice. - No diagnostic is emitted that wouldn't have been emitted originally. - No other side effect is introduced due to the second AST traversal. The current patch completely side steps all these issues and takes a swipe at the problem very laterally. Let me know what you think.
Are you sure it's a good idea to do this at all? The spec for the @deprecated Javadoc tag is sparse, but for the @Deprecated annotation, JLS3 9.6.1.6 says that "A Java compiler must produce a warning when a deprecated type, method, field, or constructor is used (overridden, invoked, or referenced by name) unless [..]". A method that overrides a deprecated method should in most cases also be marked as deprecated, and then this isn't an issue any more.
> A method that overrides a deprecated method should in most cases also be marked > as deprecated, and then this isn't an issue any more. We have this behavior as well. It is controlled via an option, and disabled by default.
(In reply to comment #8) > > A method that overrides a deprecated method should in most cases also be marked > > as deprecated, and then this isn't an issue any more. > We have this behavior as well. It is controlled via an option, and disabled by > default. In the way of clarification, the option referred to (signal overriding or implementing a deprecated method) produces a warning when enabled, but does not flag the overriding/implementing method as being implicitly deprecated. The current patch flags a method as being implicitly deprecated if it is implementing an abstract method specification (originating from a super class or interface) (and leaves the overriding a concrete deprecated method case alone), but yes, this behavior requires some stretching of the first "unless" clause i.e "The use is within an entity that itself is is annotated with the annotation @Deprecated" It just "feels" right, albeit an interpretation nevertheless. Philippe ?
(In reply to comment #7) > Are you sure it's a good idea to do this at all? > > The spec for the @deprecated Javadoc tag is sparse, but for the @Deprecated > annotation, JLS3 9.6.1.6 says that "A Java compiler must produce a warning when > a deprecated type, method, field, or constructor is used (overridden, invoked, > or referenced by name) unless [..]". I took another look at this issue and the relevant sections in the JLS and I am inclined to think that we should leave the behavior as it is. There are enough linguistic & extra-linguistic help available to suppress this warning that, we don't need to risk implementing a behavior that is mandated in no uncertain terms ( ... must produce a warning ...)
Resolving as INVALID as current behavior matches JLS3 9.6.1.6 and javac.
Verified for 3.7M1.
