Summary: | [compiler] Deprecation problem on inherited return type | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Benjamin Muskalla <b.muskalla> | ||||||||||
Component: | Core | Assignee: | Srikanth Sankaran <srikanth_sankaran> | ||||||||||
Status: | VERIFIED INVALID | QA Contact: | |||||||||||
Severity: | normal | ||||||||||||
Priority: | P3 | CC: | daniel_megert, markus.kell.r, Olivier_Thomann, philippe_mulet | ||||||||||
Version: | 3.5 | ||||||||||||
Target Milestone: | 3.7 M1 | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Whiteboard: | |||||||||||||
Attachments: |
|
Description
Benjamin Muskalla
2008-09-12 20:24:37 EDT
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. |