Community
Participate
Working Groups
Pull Up Method refactoring does not delete method from the original class. Steps to reproduce: 0. Include three files below in a project in "pullup" package. 1. Invoke Pull Up refactoring on "pullUp" method in class "PullUpBug" shown below. 2. The method "pullUp" will not be deleted from "PullUpBug", which leads to a compilation error. 3. Note that the method "pullUp" is not deleted from "PullUpBug" even if method "mb" is removed (although there will be no compilation error). Note the following is relevant (on my machine): (1) two classes (see below) must be in separate files; (2) there must be 11 methods with annotations (any annotation will be sufficient to reproduce the bug), (3) package-info.java must exist, (4) comment with "@param" must exist on one of the methods. package-info.java ---------- package pullup; ---------- PullUpBug.java ---------- package pullup; class PullUpBug extends A { void mb() { pullUp(); } // INVOKE Pull Up REFACTORING ON "pullUp" private void pullUp() { } } ---------- A.java ---------- package pullup; class A { @Deprecated void m0() { } @Deprecated void m1() { } @Deprecated void m2() { } @Deprecated void m3() { } @Deprecated void m4() { } @Deprecated void m5() { } @Deprecated void m6() { } @Deprecated void m7() { } @Deprecated void m8() { } @Deprecated void m9() { } /** * @param */ @Deprecated void m10() { } } ----------
Wow, thanks a lot for isolating this test case! The underlying problem is that the type hierarchy on A does not contain the subtype PullUpBug in this case. If I e.g. remove the annotation from A#m0(), then the type hierarchy is complete again, and the Pull Up refactoring behaves as expected.
The problem is the call to HierarchyResolver#accept(ICompilationUnit, AccessRestriction) sets the ignoreFurtherInvestigation to true on the CompilationUnitDeclaration 'A'. Now I don't really know why this should be reported as a problem. And here is the reason why it works when we have less than 10 annotations: code at SourceTypeConverter.java:119, forces a diet parse, which sets the ProblemReporter#referenceContext and this results in the CompilationUnitDeclaraton being tagged with errors (ProblemHandler.java:129). So, the question is, why should the HierarchyResolver#accept report the problem?
(In reply to comment #2) > The problem is the call to HierarchyResolver#accept(ICompilationUnit, > AccessRestriction) sets the ignoreFurtherInvestigation to true on the > CompilationUnitDeclaration 'A'. Now I don't really know why this should be > reported as a problem. > > And here is the reason why it works when we have less than 10 annotations: > code at SourceTypeConverter.java:119, forces a diet parse, which sets the > ProblemReporter#referenceContext Where exactly is ProblemReporter#referenceContext assigned? From a brief look I only see assignment to Parser#referenceContext. > and this results in the > CompilationUnitDeclaraton being tagged with errors (ProblemHandler.java:129). > > So, the question is, why should the HierarchyResolver#accept report the > problem? I assume the main reason for abortDueToInternalError() is to trigger an Abort exception, right? Maybe we can indeed throw without going through the ProblemReporter? Did you see whether AbortCompilation was thrown or AbortType? Would that fix the issue?
(In reply to comment #3) > Where exactly is ProblemReporter#referenceContext assigned? > From a brief look I only see assignment to Parser#referenceContext. Parser.problemReporter() line: 10304 JavadocParser(AbstractCommentParser).parseParam() line: 874 JavadocParser.parseParam() line: 708 JavadocParser.parseTag(int) line: 607 JavadocParser(AbstractCommentParser).commentParse() line: 241 JavadocParser.checkDeprecation(int) line: 100 Parser.checkComment() line: 1139 Parser.consumeModifiers() line: 4898 Parser.consumeRule(int) line: 6436 Parser.parse() line: 9683 Parser.parse(ICompilationUnit, CompilationResult, int, int) line: 9922 Parser.parse(ICompilationUnit, CompilationResult) line: 9879 Parser.dietParse(ICompilationUnit, CompilationResult) line: 8445 > I assume the main reason for abortDueToInternalError() is to trigger an > Abort exception, right? Maybe we can indeed throw without going through the > ProblemReporter? Did you see whether AbortCompilation was thrown or > AbortType? > > Would that fix the issue? It's "Abort". We mark the severity with Fatal and that make the ProblemHandler#handle() choose Abort from based on the stopOnFirstError policy. And I don't see abort exception being thrown without going through the problem reporter anywhere. Did I miss something?
*** Bug 394957 has been marked as a duplicate of this bug. ***
*** Bug 362775 has been marked as a duplicate of this bug. ***
*** Bug 392171 has been marked as a duplicate of this bug. ***
we also have a problem with incomplete type hierarchies. the interesting thing here is, that once i open the missing type in a java editor in eclipse, it will be part of the hierarchy from then on. Before opening the file it is missing. Is this the same problem? The hierarchy in question is regarding to JUnit4 tests, so there are a lot of annotations here and there ;)
This has come up quite a few times. We need to take a look at the hardcoded number there and see what the motivation was and what the alternatives could be etc. Jesper, this could be an interesting task to work on. TIA.
Ok, I think I understand the bug now. The optimization regarding the 10 annotations is a trade-off between the speed of the parser and the speed of decoding the stored annotation information from the compiled information, as long as full information is not required. I'm not able to tell if this is still a valid optimization or not, as I lack the project history / knowledge of specific performance tests. The net effect is that if there are more than 10 annotations, an actual parse is carried out, and this causes the SourceTypeConverter's problemReporter.referenceContext to point to the parsed compilation unit. This is due to the reporting of the incorrect Javadoc warning for the missing text in the @param annotation in pullup.A.m10. JavadocParser calls sourceParser.problemReporter() to prepare for reporting, but the call is never made (inside javadocMissingParamName), since compiler options suppress this warning in this case. Thus, Thus, the referenceContext is now set, and the parse returns. Next up, during hierarchy computation, the supertype is tested for deprecation (org.eclipse.jdt.internal.compiler.ast.TypeReference.internalResolveType(Scope)), and the package-info.java is fetched for examination. However, HierarchyResolver doesn't want to visit other compilation units somehow, and so reports an error. Since the problemReporter is hooked up with the compilationUnit, this results in the class being marked as in error, and the type hierarchy is never completed. Possible, but unlikely fix #0: Remove "more than 10 annotations"-test and live with the (possible ??) performance degradation. Haven't tried this. Possible, but unlikely fix #1a: Fix the setting of referenceContext during problemReporting. Why can't the referenceContext somehow follow the call, not the "set by side-effect" ProblemReporter. Haven't tried this approach. Possible fix #1b: Make sure that methods which 'may' report problems also clear ProblemReporter.referenceContext. Haven't tried this either. Possible fix #2: Live with this clumsy side effect of the error reporting during parse, and simply null this.problemReporter.referenceContext inside SourceTypeConverter, after dietParse (apprx. line 124, (BETA_JAVA8 branch)). That way, errors found during hierarchy examination won't mark CU as bad. Tried this, it works. Possible fix #3: Allow HierarchyResolver.visit to visit other compilation units, even if it's only 'package-info.java'. Tried this as well, it works. Will be attaching test case and fix #2 and #3 for evaluation.
Created attachment 227907 [details] Test as addendum to TypeHierarchyTests Using the original example - thanks again, Milos!
Created attachment 227908 [details] Patch for fix #2 Simply make sure the referenceContext is nulled after the dietParse
Created attachment 227910 [details] Patch for fix #3 - simply allow the visit to package-info.java
Please review which approach (#2 or #3) is the better.
I think we can settle for fix#2 if it is found to solve the issues reported in the duplicates Bug 394957, Bug 362775 Bug 392171. Could you add junits for these and see if they pass alright ? TIA.
Is it more defensive to clear the reference context at the end of Parser.parse() ?
(In reply to comment #16) > Is it more defensive to clear the reference context at the end of > Parser.parse() ? I'm not sure how to judge which condition is more problematic - that the HierarchyResolver reports problems like that, or that the problems are propagated, as I don't know the intended invariant conditions for ProblemReporter.referenceContext. One could argue that the real problem is that the AbortException ends up marking the referenceContext as in error, since this abort is really from something entirely external.
(In reply to comment #17) > (In reply to comment #16) > > Is it more defensive to clear the reference context at the end of > > Parser.parse() ? Rephrased: For fix#2, Is it more defensive to clear the reference context at the end of Parser.parse() ? > I'm not sure how to judge which condition is more problematic - that the > HierarchyResolver reports problems like that, This looks very deliberate, though I don't know the background. or that the problems are > propagated, as I don't know the intended invariant conditions for > ProblemReporter.referenceContext. ProblemReporter.referenceContext is supposed to be "volatile" - we don't have the infrastructure in place to push/pop. It is a reasonable guess that in the absence of such facility, refresh everytime, use and clear is the chosen strategy - That strategy fails in the current scenario because the agent that clears it i.e ProblemHandler.handle never gets reached.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > Is it more defensive to clear the reference context at the end of > > > Parser.parse() ? > > > Rephrased: For fix#2, Is it more defensive to clear the reference context > at the end of Parser.parse() ? Given the volatility of the referenceExpression, I'd say yes. After all, it has done its job at that stage.
Fix and tests released here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=c761974440acb731c195132472f655f4e933900d Thanks Jesper.
VERIFIED for build eclipse-SDK-I20130310-2000.
This change introduced bug 407376.
