Bugzilla – Full Text Bug Listing
| Summary: | [navigation] Open Implementation hyperlink does not show multiple implementations | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Markus Keller <markus.kell.r> | ||||||||||||||||||
| Component: | Text | Assignee: | Raksha Vasisht <raksha.vasisht> | ||||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||
| Priority: | P3 | CC: | daniel_megert, deepakazad, markus.kell.r, raksha.vasisht | ||||||||||||||||||
| Version: | 3.6 | ||||||||||||||||||||
| Target Milestone: | 3.6 M4 | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Markus Keller
Created attachment 149619 [details]
Patch
Increased the scope of the search for implementors to the super most type of the reciever.
The patch completely breaks the feature:
Just test this:
public class T {
void bar() {
bar();
}
}
click on bar() call ==> Ctrl+T opens instead of directly going to the declaration.
(In reply to comment #2) > The patch completely breaks the feature: > > Just test this: > > public class T { > void bar() { > bar(); > } > } > > click on bar() call ==> Ctrl+T opens instead of directly going to the > declaration. The fix would be to take care of case where there are no superInterfaces, ie superInterface.length== 0, build hierarchy on the type and separate superInterface.length >1 to open the quick type hierarchy. Sorry, I accidentally reassigned the bug to Deepak. Raksha, I didn't look at the code changes yet but I would expect that not only the fact that there are super types is checked but also that they indeed contain the method we're looking for. Created attachment 150467 [details]
Patch
Added code to check for the selected method in the super interface.
The patch does not work. Example:
public class T implements I1, I2 {
void bar() {
bar();
}
}
interface I1 {}
interface I2 {}
==> should not open the Quick Hierarchy just because T implements some random interfaces.
Creating the super type hierarchy can take some time and using 'null' as progress monitor is wrong. The whole code has to go into the IRunnableWithProgress which we already have.
Instead of using helpers from 'Check' it's probably better/simpler to use the 'MethodOverrideTester'.
Created attachment 151587 [details] Updated Patch (In reply to comment #6) > The patch does not work. Example: > > public class T implements I1, I2 { > void bar() { > bar(); > } > } > interface I1 {} > interface I2 {} > > ==> should not open the Quick Hierarchy just because T implements some random > interfaces. > This is fixed now, we now check if the method is declared in the super interfaces. We only open the Quick Hierarchy when there is more than one super interface declaring the method. If there are none, then we create the hierarchy on the declaring type itself as before. This patch can't work as it only handles the root interfaces i.e. if you have e.g. A > I1 > I2 you simply ignore I1, which of course is wrong. - why are you not using one of the MethodOverrideTester.find* methods? - are you 100% sure that 'javaElement' is always an instance of IMethod? - the runnable becomes almost unreadable with the newly added method ==> move it outside, make it private and static - it is better to reassign the local variable 'type' with the super type (In reply to comment #8) > This patch can't work as it only handles the root interfaces i.e. if you have > e.g. > A > I1 > I2 you simply ignore I1, which of course is wrong. > > - why are you not using one of the MethodOverrideTester.find* methods? Because these methods search from the type and upwards to superClass and then superInterfaces and return the first overridden method found. We need to search the other way to find the top most type which declares the method. Ofcourse we could use superTypeHierarchy.getSuperTypes(type) to get all the super types, but we might have to add more complex code to find out the top most type. > - are you 100% sure that 'javaElement' is always an instance of IMethod? Yes. we do a check in org.eclipse.jdt.internal.ui.javaeditor.JavaElementHyperlinkImplementationDetector.createHyperlink > - are you 100% sure that 'javaElement' is always an instance of IMethod?
>Yes. we do a check in
>org.eclipse.jdt.internal.ui.javaeditor.JavaElementHyperlinkImplementationDetector>.createHyperlink
In that case it is a bug to use IJavElement instead of IMethod in the constructor.
We only need to find the real method declaration and hence org.eclipse.jdt.internal.corext.util.MethodOverrideTester.findDeclaringMethod(IMethod, boolean) should do the trick, shouldn't it?
Even if you wanted to implement it on your own, it's still wrong as you only check the root interfaces i.e. there's no code in your patch that really goes down the hierarchy. Or did I miss something?
(In reply to comment #10) > We only need to find the real method declaration and hence > org.eclipse.jdt.internal.corext.util.MethodOverrideTester.findDeclaringMethod(IMethod, > boolean) should do the trick, shouldn't it? > Yes I tried to use that method, but it searches the hierarchy of the declaring type of the method not the reciever type, so it might not work in the cases where the reciever type is different from the declaring type (such as example given in the description). Ill try to modify the code to handle this case. > Even if you wanted to implement it on your own, it's still wrong as you only > check the root interfaces i.e. there's no code in your patch that really goes > down the hierarchy. Or did I miss something? Yes you are right. There should be a check for all super types not only root interfaces. Created attachment 152156 [details] Patch with discussed changes (In reply to comment #8) > - it is better to reassign the local variable 'type' with the super type This is not possible since 'type' has to be final. Made changes to use the receiver type when the method is abstract in the receiver type, find declaring type otherwise. Created attachment 152163 [details]
Pls take this one.
>> - it is better to reassign the local variable 'type' with the super type >This is not possible since 'type' has to be final. Indeed. Sorry about that. Unfortunately the patch is still not good. 1. for the following example: public class A { void test() { A a= new A(); a.toString(); } } it creates the full type hierarchy of Object, which is very slow and not required instead. Then it opens the Quick Hierarchy instead of just selecting Object.toString() as it currently correctly does. 2. the following example (also broken in HEAD): package p; public class A { void test() { A a= new A(); a.toString(); } } is not fixed: it should open A.toString() instead of the Quick Hierarchy. 3. I don't think you need to compute the declaring type but rather it should be enough to call findOverriddenMethodInHierarchy - or did I miss something? 5. if the receiver type is a class then we can simply search in its full or sub-type hierarchy see next item. 6. we don't always need to search in the full hierarchy: if there is no overridden method above the receiver then a subtype hierarchy is good enough (solves 2.). Hint: use different variant of SearchEngine.createHierarchyScope(). Minor issues: >> - are you 100% sure that 'javaElement' is always an instance of IMethod? >>Yes. we do a check in >>org.eclipse.jdt.internal.ui.javaeditor.JavaElementHyperlinkImplementationDetector>>.createHyperlink >In that case it is a bug to use IJavElement instead of IMethod in the >constructor. Still not fixed. I don't like the name of getDeclaringMethod and how the local variables are named - they aren't reflecting what they stand for. (In reply to comment #14) > Unfortunately the patch is still not good. > > 1. for the following example: > public class A { > void test() { > A a= new A(); > a.toString(); > } > } > it creates the full type hierarchy of Object, which is very slow and not > required instead. Then it opens the Quick Hierarchy instead of just selecting > Object.toString() as it currently correctly does. The current fix checks only for abstract declarations or no declaration of method in the receiver type. In which case we compute the overriden method in hierarchy and search in its hierarchy. The toString() is a special case where the overriden method is java.lang.Object.toString(). If we generalize this case to receiver being a class and we search only in its hierarchy (as pointed in 5) then the problem in the above case can be overcome. > > 2. the following example (also broken in HEAD): > package p; > > public class A { > void test() { > A a= new A(); > a.toString(); > } > } > is not fixed: it should open A.toString() instead of the Quick Hierarchy. This example should be: public class A { void test() { A a= new A(); a.toString(); } @Override public String toString(){ return ""; } } see last comment. > 3. I don't think you need to compute the declaring type but rather it should be > enough to call findOverriddenMethodInHierarchy - or did I miss something? Yeah this method might just be enough since it returns the first overridden method in the super type hierarchy.We need not go all the way to find the top most declaring type. > > 5. if the receiver type is a class then we can simply search in its full or > sub-type hierarchy see next item. > > 6. we don't always need to search in the full hierarchy: if there is no > overridden method above the receiver then a subtype hierarchy is good enough > (solves 2.). Hint: use different variant of > SearchEngine.createHierarchyScope(). But in case 2 there is a overriden method in java.lang.Object.toString(). Wouldnt it be wrong to search only in the subtypes? Created attachment 152464 [details] Updated Patch In reply to comment #15) As discussed, The latest patch differentiates between the receiver type being a class vs a interface. In the first case, we search the implementors in the scope of the reciever type's hierarchy, whereas in the latter we use the declaring type of the method. In addition the search is restricted to only sub types in the hierarchy of the receiver type by default, unless the receiver type *does not* have a concrete implementation of the given method, in which case we search all the types in the hierarchy. We do not yet handle special cases (ex mentioned in bug 268763) where there are more than one super interfaces (for a given interface which is the declaring type for the method) which declare the method and there are one or more classes implementing the method. Created attachment 152469 [details]
Updated Patch
Added a minor tweak to check the match found after search for instance of IMethod.
Sorry, I won't review that patch as it introduces compiler errors on org.eclipse.jdt.ui. Created attachment 152565 [details]
Patch
Forgot to add one line change in another file into the patch.
Raksha, the patch works but needs polish: - why does it create a supertype hierarchy in case of the interface? - why did you add the check "!links.contains(methodFound)"? - fCreateSubtypeHierarchyScope does not follow the naming conventions - using a static field is not needed ==> - simply do the interface check outside the new helper method - create the hierarchy inside the helper method - return a boolean from the helper method - call the helper isFullHierarchyNeeded(...) - if needed add the receiver method outside the helper method Please make those changes and commit to HEAD. (In reply to comment #20) > - why did you add the check "!links.contains(methodFound)"? This check is needed in cases like this : public class sup implements I{ void test() { sup a = new sup(); a.toString(); } @Override public String toString() { return ""; } public class sub extends sup{ @Override public String toString() { return ""; } } } interface I { @Override public String toString(); } where the sub type also implements the method. We add the method once to the list and then while searching in the sub type hierarchy, the parent method gets added again. To prevent that, I have added a check. Made the rest of the changes and committed to HEAD. >This check is needed in cases like this :
No it is not if you don't add it in the case where it's not needed in the first place ;-)
Also, the check for isAbstract(...) is not needed and causes this example to fail:
public abstract class Try {
static void foo() {
Try t= null;
t.toString();
}
public void bar() {
toString();
}
public abstract String toString();
}
==> it goes to Object.toString() which is wrong.
I've fixed the above issues in HEAD plus reworked the code so it's easier to understand.
Please fix those remaining issues:
- if there are 0 matches and the method is abstract then we should go to that
method (also if it's coming from an interface) instead of opening the Quick
Type Hierarchy
- convert the helpers on the OMT to static so we don't need to create a useless
super type hierarchy and then remove the progress monitor changes
Some other comments to the commit:
- we normally put the progress monitor at (or near) the end of the method
signature
- a method that takes a progress monitor can assume he owns it i.e. the caller
should create the sub-progress monitor if needed (like it's done for the
search engine
(In reply to comment #22) > >This check is needed in cases like this : > No it is not if you don't add it in the case where it's not needed in the first > place ;-) Sorry I dont agree, because the fix fails for this case : public class sup implements K{ void test() { sup a = new sup(); a.toString(); } @Override public String toString() { return ""; } public abstract class sub extends sup{ @Override public abstract String toString() ; } } interface K { @Override public String toString(); } It should directly go to sup.toString(), but it opens QTH. (In reply to comment #22) > Please fix those remaining issues: > - if there are 0 matches and the method is abstract then we should go to that > method (also if it's coming from an interface) instead of opening the Quick > Type Hierarchy Fixed and Released to HEAD. > - convert the helpers on the OMT to static so we don't need to create a useless > super type hierarchy and then remove the progress monitor changes Working on it. Will resolve the bug once this is fixed. (In reply to comment #23) > (In reply to comment #22) > > >This check is needed in cases like this : > > No it is not if you don't add it in the case where it's not needed in the first > > place ;-) > > Sorry I dont agree, because the fix fails for this case : > It should directly go to sup.toString(), but it opens QTH. On further investigation, found that the bug is in search. Raised a bug 295894 and to track this issue. Added back the check to the HEAD as a workaround, and filed bug 295897 to track it. (In reply to comment #24) > > - convert the helpers on the OMT to static so we don't need to create a useless > > super type hierarchy and then remove the progress monitor changes > > > Working on it. Will resolve the bug once this is fixed. These methods in OMT cannot be made static since the hierarchy is used in case of generics to compute the substitutions for type/method parameters.Resolving the bug with the existing fix. Open implementation hyperlink jumps to single implementation and open quick hierarchy for multiple implementations. Verified for 3.6 M4 with I20091207-1800 |