Community
Participate
Working Groups
Created attachment 104897 [details] proposed patch (simpler variant, perhaps incomplete) Build ID: I20080530-1730 The AndPattern and OrPattern classes both override a method matchReportReference from PatternLocator, but they seem to override the wrong version. See in PatternLocator: protected void matchReportReference(ASTNode reference, IJavaElement element, IJavaElement localElement, IJavaElement[] otherElements, Binding elementBinding, int accuracy, MatchLocator locator) throws CoreException { matchReportReference(reference, element, elementBinding, accuracy, locator); } i.e., the "long version" delegates to the "short" one, but mentioned classes override only the long version, thus missing to intercept calls to the short version. When instead the short version is overridden, all calls are intercepted. However, some other subclasses of PatternLocator do override the long version (and seem to require the full argument list). For those to properly work, it might be necessary to have And/OrPattern actually override both versions? I detected this while debugging an adapted version of MethodLocator, which was never called when embedded into an OrPattern (during search all occurrences on behalf of a rename method refactoring). Instead of MethodLocator the default behavior in PatternLocator was triggered. Attaching a proposed patch (tests are currently running ...)
Well spotted! However, I do not agree with your patch. We still need to carry localElement and otherElements arguments value to the or/and patterns if they are not null... Instead, it would be better to override the matchReportReference(ASTNode, IJavaElement, Binding, int, MatchLocator) method as follows both in AndLocator and OrLocator classes: protected void matchReportReference(ASTNode reference, IJavaElement element, Binding elementBinding, int accuracy, MatchLocator locator) throws CoreException { matchReportReference(reference, element, null, null, elementBinding, accuracy, locator); } I'll post a complete patch with additional test.
Hmmm, I have some difficulties to find a path to reproduce the issue. Could you give me your test case for which the problem occurred? TIA
(In reply to comment #1) > Instead, it would be better to override ... Totally agree. I was trying to be too smart ;-) (In reply to comment #2) > Hmmm, I have some difficulties to find a path to reproduce the issue. Could you > give me your test case for which the problem occurred? The original test case involves some non-java constructs, so it cannot be used in a plain JDT. FYI, it occurred when renaming a method with overrides and with different references from clients => searching for references to Super.m() OR Sub.m(). As I can't find a test case to reproduce in a plain JDT, I could see the following implicit design rules: * Methods in MatchLocator must only invoke the long version of matchReportReference, the short version is only used as a utility when delegating within PatternLocator and Subclasses. * All methods from PatternLocator that may call the short version of matchReportReference must be overridden in And/OrPattern (all these relate to static things like packages/imports/supertypes). Since the And/OrPatterns will be the first to receive calls from MatchLocator before they can then delegate to other PatternLocators, overriding the long version effectively suffices, ... ... until someone adds code to MatchLocator that violates the above implicit rules. That's exactly what happened in our case :)
(In reply to comment #3) Thanks for the feedback. As MatchLocator currently matches these rules and you also couldn't find a plain JDT test case, this issue should not go into the maintenance but only in the next release...
Created attachment 105136 [details] New proposed patch As discussed in previous comments, there's no test case for this patch. It's just a protection against possible misbehavior if the rules Stephan described in comment 3 would not be respected in the future (or by some users overriding internal code...;-)).
Released for 3.5M1.
Verified for 3.5M1 using I20080805-1307
