Bugzilla – Full Text Bug Listing
| Summary: | [search] Search API got wrong result, when searching for method references, where the parameter is a member type of another type. | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | AndyFang <javaeecoding> | ||||||||||||||||
| Component: | Core | Assignee: | Manoj N Palat <manoj.palat> | ||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||
| Severity: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | jarthana, malaperle, manoj.palat, markus.kell.r, shankhba, simeon.danailov.andreev, srikanth_sankaran | ||||||||||||||||
| Version: | 4.3 | Flags: | jarthana:
review+
|
||||||||||||||||
| Target Milestone: | 4.5 M3 | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Windows 7 | ||||||||||||||||||
| See Also: |
https://bugs.eclipse.org/bugs/show_bug.cgi?id=88997 https://bugs.eclipse.org/bugs/show_bug.cgi?id=552774 |
||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Created attachment 245177 [details] Proposed Patch textual comparison is found insufficient to differentiate the above inner types. The existing source name checking code is retained as a safety net for any (if any)corner case. [note: of course, this fix does not break the fix for bug 41018] Manoj, I see four problems: (0) The patch does not apply as is. (1) The test is in the wrong suite. This has nothing to do with Java 8 and so should not be added to JavaSearchBugs8Tests ? (2) The bug summary says: [1.8][compiler] NPE when creating LambdaMethod element for lambda expressions with errors - this is wrong. (3) While the newly created test passes, I am not sure of the behavior in the IDE. With the patch adjusted and applied, launch an inner IDE and cut and paste the code from comment#0 into project. Click on the first query and open the search dialog via ^H and search for all occurrences. It still shows query(Bar.InnerKey key); ?? thanks for the review Srikanth. Moving to M2 to check the solution further Created attachment 246151 [details] Proposed Patch With added test case and solving the issues discussed in comment 2 (In reply to Srikanth Sankaran from comment #2) > (3) While the newly created test passes, I am not sure of the behavior in the > IDE. With the patch adjusted and applied, launch an inner IDE and cut and > paste the code from comment#0 into project. Click on the first query and > open the search dialog via ^H and search for all occurrences. It still shows > query(Bar.InnerKey key); ?? With the latest patch, I am still seeing problems: Launch an inner IDE with the example code in comment#0. Select the second query, open the search dialog via ^H and search for all occurrences. It shows one result, clicking on the result takes you to the wrong method. Created attachment 246299 [details]
Proposed Patch
Added logic to use the qualifier as well in getMethodBinding0().
The latest patch still shows problems:
Given:
// --
interface I {
public void query(Foo.Key key);// Search result of method query(Foo.Key) returns the method query(Bar.Key) too
public void query(Key key);
}
class Foo {
static class Key {
}
public static void foo(I i, Key key) {
i.query(key);
}
}
class Key {
}
class Bar {
public static void bar(I i, Key key) {
i.query(key);
}
}
class X {
public static I getInstance() {
return null;
}
}
(1) Select I.query(Foo.Key key), ^H, search for all occuraences:
Shows 3 results ==> wrong.
(2) Select I.query(Key key), ^H, search for all occuraences:
Shows 4 results => wrong.
Created attachment 246984 [details]
Proposed Patch
Additional test cases added - changed the algorithm to choose the most appropriate method from the list of available methods.
Try the following simple variant of the comment#0 test case: interface MyIF { public void query(Foo.InnerKey fk, Bar.InnerKey bk, String s); public void query(Bar.InnerKey fk, Bar.InnerKey bk, String s); } class Foo { static class InnerKey { } } class Bar { static class InnerKey { } public static void bar(MyIF i, Foo.InnerKey fk, Bar.InnerKey bk) { i.query(fk, bk, ""); } } (1) Select the second query of MyIF (2) Search all references (Ctrl+Shift+G) (3) Shows the query call inside Bar.bar ==> wrong. (In reply to Srikanth Sankaran from comment #9) > Try the following simple variant of the comment#0 test case: This is even simpler and closer to the comment#0 test case that still shows the problem: interface MyIF { public void query(Foo.InnerKey fk, String s); public void query(Bar.InnerKey fk, String s); } class Foo { static class InnerKey { } } class Bar { static class InnerKey { } public static void bar(MyIF i, Foo.InnerKey fk) { i.query(fk, ""); } } Same steps: (1) Select the second query of MyIF (2) Search all references (Ctrl+Shift+G) (3) Shows the query call inside Bar.bar ==> wrong. Created attachment 247051 [details] Proposed Patch Thanks Srikanth for the reviews. In the present patch, all the methods are iterated to check for a text match - if yes, that method is returned else a possible method is selected and returned since a simple text mismatch may exclude the possibility mentioned in bug 41018.TIA. (In reply to Manoj Palat from comment #11) > Created attachment 247051 [details] > Proposed Patch > > Thanks Srikanth for the reviews. In the present patch, all the methods are > iterated to check for a text match - if yes, that method is returned else a > possible method is selected and returned since a simple text mismatch may > exclude the possibility mentioned in bug 41018.TIA. Looking at the code change in MatchLocator, I conclude this patch is still not correct: Try the following: // -- interface MyIF { public void query(Foo.InnerKey fk, Bar.InnerKey bk, String s); public void query(Bar.InnerKey fk, Bar.InnerKey bk, String s); } class Foo { static class InnerKey { } } class Bar { static class InnerKey extends Foo.InnerKey { } public static void bar(MyIF i, Foo.InnerKey fk, Bar.InnerKey bk) { i.query(fk, bk, ""); } } // -- Search for all occurrences of MyIF.query(Foo.InnerKey, Bar.InnerKey, String) This shows: MyIF.query(InnerKey, InnerKey, String) ==> Wrong. Created attachment 247094 [details]
Proposed Patch
Replaced isCompatible() with isEquivalentTo()
Thanks Manoj - I'll take a look at this in early M3. Jay, could you review this for M3 please ? TIA. Patch mostly looks alright, but there's still one case where we have a problem:
interface MyIF {
public void query(Foo.InnerKey key);
public void query(Bar.InnerKey key);
class Foo {
static class InnerKey {}
}
class Bar {
static class InnerKey {}
}
}
Select the first query() and search for declarations. You get one result and when you double click from the search results view, you are taken to the second query(). However, this doesn't happen when you search on the second method. This also doesn't happen when the query() methods are declared/defined in different scopes.
Created attachment 248117 [details] Proposed Patch Jay: Thanks for the review. There is another bug (bug 88997) which has the similar (almost identical) case mentioned. Since these two touch different parts of the code, I am planning to break this into two and address the references issue (which was the original comment) in this bug. Have added a testcase (16) that captures the scenario which you have given, but is currently disabled - to be enabled when the declarations duplicate issue (bug 88997) is addressed. Patch looks good to me. (In reply to Jayaprakash Arthanareeswaran from comment #18) > Patch looks good to me. Thanks Jay. Fixed via commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=dbfcb8d7fd5ffcddbe22e6b1ba82be9083ee4d44 Manoj, was this really so serious that it needed to go into M3 after the test pass? In milestone weeks, regular commits should be suspended or put into a separate branch until the milestone is declared. (In reply to Markus Keller from comment #20) > Manoj, was this really so serious that it needed to go into M3 after the > test pass? In milestone weeks, regular commits should be suspended or put > into a separate branch until the milestone is declared. Markus, Manoj did speak to me and I gave a go for this since we are still doing our testing as of today. We will have two more builds to verify this fix as we complete our testing. Verified for 4.5 M3 using I20141029-2000 build. This seems to have introduced a regression, see bug 469320. Also introduced bug 552774 from what I can tell. |
org.eclipse.jdt.core search API got wrong result, when searching for method references, where the parameter is a member type of another type. For example: interface MyIF { // Search result of method query(Foo.InnerKey) returns the method query(Bar.InnerKey) too public void query(Foo.InnerKey key); public void query(Bar.InnerKey key); } class Foo { static class InnerKey {} } class Bar { static class InnerKey {} } * Here is a discussion on stackoverflow: http://stackoverflow.com/questions/22317878/eclipse-open-call-hierarchy-got-wrong-result * It seems that the bug was introduced by fixing JDT Bug 41018. https://bugs.eclipse.org/bugs/show_bug.cgi?id=41018