Community
Participate
Working Groups
build : HEAD (Pls remove the temp workaround in line 213 with this bug id as comment : " && !links.contains(methodFound)" to reproduce) The code in org.eclipse.jdt.internal.ui.javaeditor.JavaElementImplementationHyperlink.openImplementations(...) searches for implementations in sub types of a focus type and creates the hierarchy scope using API : org.eclipse.jdt.core.search.SearchEngine.createHierarchyScope(IJavaProject project, IType type, boolean onlySubtypes, boolean noMemberTypes, WorkingCopyOwner owner) with the 'onlySubtypes' flag set to true. But with nested types, the search also shows the implementation in focus type when the subtype has a declaration of the method. For ex: public class sup { void test() { sup a = new sup(); a.toString(); } public String toString() { return ""; } public abstract class sub extends sup{ public abstract String toString() ; } } Expected : When you open implementation on a.toString(), the search results must return only sub.toString(). Current Behaviour: shows sup.toString(), sub.toString(). Which is wrong according to the API documentation. PS: Same behaviour is seen even when sub has a concrete implementation of toString() rather than abstract.
Raksha, your code in HEAD calls SearchEngine.createHierarchyScope(..) with noMemberTypes == false. Doesn't it work as you expect when you set it to true?
(In reply to comment #1) > Raksha, your code in HEAD calls SearchEngine.createHierarchyScope(..) with > noMemberTypes == false. Doesn't it work as you expect when you set it to true? Ahh yes! The boolean noMemberTypes also filters enclosing types which could be used in this case. It works as expected with that flag set to true. Thanks Markus!
>Doesn't it work as you expect when you set it to true? That's not the point of this bug. The bug is about returning results for the focus type. According to the Javadoc it should not return results for the focus type: * This method, however, allows to restrict the hierarchy to true subtypes, * neither including supertypes nor the focus type itself. Now, the flag you mention can be set to hide results for member types but that shouldn't override the general spec to not return results for the focus type and especially not when the focus type is the top-level class. Reopening to fix.
And there seems to be wrong interpretation of noMemberTypes too.
My first analysis is that there are two issues here: a) comment 0: when the onlySubtypes param is set to true, the SearchEngine should never return the focus type b) comment 3: we missed the peculiar case where a sub-type is an inner class and onlySubtypes==true and noMemberTypes==true. Which parameter should be taken into account first...!? I agree with Dani that changing the noMemberTypes value should not change the behavior of the onlySubtypes flag. But I'd add that it should also take the precedence on that flag: e.g. with the peculiar case I described earlier, no match at all should be returned...
Created attachment 152929 [details] Tests showing the problem Here are some additional tests to show and reproduce the problem easily...
Do we have a use case for noMemberTypes == false? I think not. This was just a bug in the old methods, but we couldn't fix them because the bug was also in the specification. I would just remove that parameter (always set it to true). That would also simplify the documentation. It could e.g. say: Unlike the other <code>createHierarchyScope</code> methods, this method creates scopes that only contain types that actually span the hierarchy of the focus type, but do not include additional enclosing or member types.
+1 for comment 7.
Has anybody started working on a patch or should I have a look, since this is a follow up of bug 215139? (In reply to comment #5) > b) comment 3: we missed the peculiar case where a sub-type is an inner class > and onlySubtypes==true and noMemberTypes==true. Which parameter should be taken > into account first...!? I agree with Dani that changing the noMemberTypes value > should not change the behavior of the onlySubtypes flag. But I'd add that it > should also take the precedence on that flag: e.g. with the peculiar case I > described earlier, no match at all should be returned... According to the original intention I'd say that noMemberTypes should never exclude an actual subtype. (In reply to comment #7) > Do we have a use case for noMemberTypes == false? I think not. This was just a > bug in the old methods, but we couldn't fix them because the bug was also in > the specification. > > I would just remove that parameter (always set it to true). That would also > simplify the documentation. It could e.g. say: > > Unlike the other <code>createHierarchyScope</code> methods, this method creates > scopes that only contain types that actually span the hierarchy of the focus > type, but do not include additional enclosing or member types. I fully agree.
(In reply to comment #9) > Has anybody started working on a patch or should I have a look, > since this is a follow up of bug 215139? > I just wrote the tests, so if you have time to have a look on it, I will sincerely be happy :-) > (In reply to comment #5) > According to the original intention I'd say that noMemberTypes should > never exclude an actual subtype. > > (In reply to comment #7) > > Do we have a use case for noMemberTypes == false? I think not. This was just a > > bug in the old methods, but we couldn't fix them because the bug was also in > > the specification. > > > > I would just remove that parameter (always set it to true). That would also > > simplify the documentation. It could e.g. say: > > > > Unlike the other <code>createHierarchyScope</code> methods, this method creates > > scopes that only contain types that actually span the hierarchy of the focus > > type, but do not include additional enclosing or member types. > > I fully agree. So, if there's no real use case for the false value and it was not included in the initial intention, then I also agree to remove this parameter...
At a closer look the implementation is correct wrt the specification: With these options: onlySubtypes=true noMemberTypes=false type sup from comment 0 is correctly found because it is the enclosing type of a true sub type (note that noMemberTypes covers both direction of containment - probably a misnomer to begin with ...) Following comment 7, however, this is a non-issue since the combination (onlySubtypes=true,noMemberTypes=false) seems to be useless. The main improvement in this bug will thus be simplification by removing a difficult-to-explain parameter.
Created attachment 152945 [details] API simplification This patch contains the API simplification suggested in comment 7, but DO NOT BLINDLY APPLY THIS PATCH (see below). I've tried to update the tests, too. Firstly, my patch is not usable for the tests since I'm still affected by bug 214085 comment 4!! Perhaps some more votes could help here? Secondly, with the simplified API some tests no longer apply. Should some of those tests from bug 215139 be removed or be disabled and kept for documentation?
>Firstly, my patch is not usable for the tests since I'm still affected >by bug 214085 comment 4!! Perhaps some more votes could help here? Bug 214085 is valid BUT the main issue here is with the test project itself: if it uses special characters encoded in ISO-8859-1 (because most JDT Core developers use Windows XP) then it must explicitly set the project specific encoding to ISO-8859-1 otherwise there is not way for the editor or patch wizard to guess this. Frédéric, please fix all JDT Core projects that have such special chars.
>With these options: > onlySubtypes=true > noMemberTypes=false >type sup from comment 0 is correctly found because it is the enclosing >type of a true sub type (note that noMemberTypes covers both direction >of containment - probably a misnomer to begin with ...) Such a behavior would be misleading and I would consider it broken but as we agreed this parameter will go away anyway ;-) It's also wrong to exclude the focus type: the 80% case is to search an element in a hierarchy of T and of course a result in T itself is naturally expected to appear. With the existing API the client is forced to manually do the test himself. Yes, it's easy to do, but it is additional useless work that the search engine should do for me. This is also better aligned with the other createHierarchyScope(*) methods which include the focus type. So, the correct fix for this is to have parameter that tells whether to include the focus type or not.
(In reply to comment #14) > >With these options: > > onlySubtypes=true > > noMemberTypes=false > >type sup from comment 0 is correctly found because it is the enclosing > >type of a true sub type (note that noMemberTypes covers both direction > >of containment - probably a misnomer to begin with ...) > Such a behavior would be misleading and I would consider it broken but as we > agreed this parameter will go away anyway ;-) The catch is: pre bug 215139 the hierarchy scope ALWAYS contained all enclosing types of types in the hierarchy. And since reasons exist to maintain that behavior, HierarchyScope.encloses() MUST still be able to compute that set as well. In that light the patch in bug 215139 was designed in terms of additional parameters that incrementally change the behavior of a HierarchyScope, not a full re-design ;-) If one of the factory methods in SearchEngine starts to produce behavioral differences that are not controlled by an explicit parameter, this might actually be confusing, too. > So, the correct fix for this is to have parameter that tells whether to > include the focus type or not. If there's sufficient demand for that parameter that should be easy to add. Also note, that the new API is already consumed by PDE/UI. Any changes will have to be propagated.
>And since reasons exist >to maintain that behavior, HierarchyScope.encloses() MUST still be able >to compute that set as well Which ones? When adding a new API there's no need to take over anything unless there are good reasons/demands. >If there's sufficient demand for that parameter that should be easy to add. Yes, PDE needs it and there is no API that would allow them to remove the focus type from the IJavaSearchScope, hence we have to add the parameter. >Also note, that the new API is already consumed by PDE/UI. Any changes will >have to be propagated. I know and it's also used by JDT Text. Once we have a patch we agree on, we can coordinate the changes, that's no problem.
NOTE: Given the method is in M3, we should leave it in for M4 but deprecate it with a comment that it will be removed for 3.6 (M5).
(In reply to comment #13) > >Firstly, my patch is not usable for the tests since I'm still affected > >by bug 214085 comment 4!! Perhaps some more votes could help here? > Bug 214085 is valid BUT the main issue here is with the test project itself: if > it uses special characters encoded in ISO-8859-1 (because most JDT Core > developers use Windows XP) then it must explicitly set the project specific > encoding to ISO-8859-1 otherwise there is not way for the editor or patch > wizard to guess this. > > Frédéric, please fix all JDT Core projects that have such special chars. > JDT/Core usually didn't have such characters, but I introduced the '§' char when working on Java Search tests and it seems it was definitely not a good idea... So, I change to use an ASCII character ('!') instead and avoid this kind of limitation in our projects. I also set specific 'UTF-8' encoding for a file containing specific UTF-8 characters used by the EncodingTests test suite... The changes have been released in HEAD, hence it should be OK to create the patch even if the bug 214085 is still not fixed.
Created attachment 153058 [details] Patch to fix encoding issue Patch corresponding to the change described in comment 18 and which has been released in HEAD...
(In reply to comment #16) > >And since reasons exist > >to maintain that behavior, HierarchyScope.encloses() MUST still be able > >to compute that set as well > Which ones? When adding a new API there's no need to take over anything unless > there are good reasons/demands. Hierarchy scopes created through the old API must contain members and enclosings for performance sake (document based search). HierarchyScope.encloses() is used on all hierarchy scopes, not matter through which API they are created. According to bug 215196 there were differences between cached results and actual search results, because encloses() did not fully comply with the spec. This is fixed. All this is about the implementation in HierarchyScope. Of course the new API is free to provide whatever functionality is desired, but then documentation must be very clear about any difference that is hidden/not represented by a parameter.
(In reply to comment #15) > > So, the correct fix for this is to have parameter that tells whether to > > include the focus type or not. > > If there's sufficient demand for that parameter that should be easy to add. Any votes from JDT/Core on this parameter?
>Any votes from JDT/Core on this parameter? The parameter is needed unless you can explain a simple way how PDE can build their scope as they currently do.
(In reply to comment #22) > >Any votes from JDT/Core on this parameter? > The parameter is needed unless you can explain a simple way how PDE can build > their scope as they currently do. Still: without a GO from a JDT/Core committer I can do nothing.
+1 for adding a parameter.
>Still: without a GO from a JDT/Core committer I can do nothing. Stephan, I'm the overall JDT lead, so whatever I say counts ;-)
Created attachment 153865 [details] combined changes This patch combines the removal of parameter 'noMemberTypes' with the addition of a parameter 'includeFocusType'. After adding and removing a boolean argument, we end up with the same signature as before. I assume that consumers of this API currently call it with 'noMemberTypes=true', is that right? When linking against the new API, where this 'true' now goes into the 'includeFocusType' parameter, this would mean that the behavior regarding the focus type will change. According to comment 14 this should be the desired behavior for the 80% case. => Is this semantic change acceptable at this point? Otherwise a new method name might be necessary. For bug 61185 (PDE/UI) I think excluding the focus type is more natural, but that's a minor change. (behavior regarding member types will not change in the above situation). Thanks to the avoidance of non-ascii chars I could produce a sound patch including the tests ;-) I slightly adjusted Frederics 3 tests in JavaSearchBugsTest. Also for 2 tests in JavaSearchTests I created a variant challenging the new parameter, plus adjusted existing tests. 3 Tests became obsolete be removing 'noMemberTypes'. I only disabled those (by '_' prefix), but you may want to remove those as well.
(In reply to comment #26) > [...] > Otherwise a new method name might be necessary. Given the different semantics of those factory methods, I'd actually suggest to name the new API something like: createStrictHierarchyScope where "strict" refers to the exclusion of members/enclosings. Then we can also cleanly mark the previous version as deprecated etc. Should I update the patch in this way?
(In reply to comment #27) > suggest to name the new API something like: > createStrictHierarchyScope That's a very good idea. I also felt uneasy about having overloaded versions of createHierarchyScope and just one of them has a slightly different behavior. +1 from my side (but I'm not a JDT/Core committer).
+1 for me and I am a JDT Core committer :-)
Created attachment 153938 [details] updated patch: new method name Voila! * retain previous API and mark it as @deprecated - retain also some tests that can only be expressed with the previous API * create new API as createStrictHierarchyScope - update tests to using the new API where appropriate * map both APIs to one extended ctor in HierarchyScope * slightly clarify the now internal flags regarding members & enclosings And: thanks, I guess by now I know who's who in JDT ;-)
+1. I like that we use a new name. > - retain also some tests that can only be expressed with the previous API They should be removed as we will remove the deprecated API that was introduced during 3.6.
Anything keeping us from committing the patch (comment 30) and closing the bug?
(In reply to comment #32) > Anything keeping us from committing the patch (comment 30) and closing the bug? Just remove the API added in 3.6 instead of set as deprecated as Dani requested in comment 31. Also remove the static TESTS_PREFIX in JavaSearchBugTest (my fault as I let it in my initial patch...) and repost the patch, then I'll release it. Many thanks for the good job Stephan (as usual) and for your patience! :-)
(In reply to comment #33) > Just remove the API added in 3.6 instead of set as deprecated as Dani requested > in comment 31. I had understood that the old API should stay as deprecated for the duration of one milestone to give clients sufficient time to adjust (see comment 16 & comment 17). If this understanding is wrong I will post an updated patch later today.
>I had understood that the old API should stay as deprecated for the >duration of one milestone to give clients sufficient time to adjust >(see comment 16 & comment 17). Correct. In comment 31 I only asked to remove the tests that still reference that method. I would write in the deprecation comment that the method will get removed shortly before M5 and also send a note to the mailing list.
(In reply to comment #35) > I would write in the deprecation comment that the method will get > removed shortly before M5 Really before M5? So, the deprecation mark will be visible for less than 2 weeks, is that sufficient time? > and also send a note to the mailing list. No problem, which one do you mean? jdt-core-dev?
>Really before M5? So, the deprecation mark will be visible for >less than 2 weeks, is that sufficient time? I doubt that there's anyone using it outside the SDK. I would fix it in HEAD today and announce on the lists that this will be gone in the next I-build (and hence also M5). After all adopting the new method is quickly done. >No problem, which one do you mean? jdt-core-dev? No, that list is for jdt-core developers which aren't affected by the change. I would send it to eclipse-dev and cross-project-issues-dev. I would start the message like this: If you don't call JDT Core's SearchEngine.createHierarchyScope(...) method you can disregard this message. ... and mention that the remove method has been introduces during 3.6 and also tell how to replace the call.
Created attachment 155994 [details] patch v4 This patch removes tests that used the now deprecated method. Removed the static TEST_PREFIX. Also a comment was added mentioning that the deprecated method will be removed shortly before M5. While cleaning the tests I noticed other tests that also call deprecated API, but did not touch those.. Mail will be sent shortly.
Comment on attachment 155994 [details] patch v4 Patch looks good to me. It was not really necessary to depreciate the method in BasicSearchEngine as it is internal, but that's not a problem, we'll remove it at the same time than for SearchEngine...
(In reply to comment #39) > (From update of attachment 155994 [details]) > Patch looks good to me. It was not really necessary to depreciate the method in > BasicSearchEngine as it is internal, but that's not a problem, we'll remove it > at the same time than for SearchEngine... Yes, and by marking it as deprecated it's easier to remember removing it ;-) FYI: Mail has been sent to both lists as suggested by Dani.
Created attachment 156008 [details] Same patch with copyright updates
Released for 3.6M5 on the behalf of Frédéric.
The new method does not work as expected and hence I cannot use it. The problem is that it does not find matches if I include the focus type but that type has no subtype: public class A { void test() { A a= new A(); a.toString(); } @Override public String toString(){ return ""; } } If I create a hierarchy scope for A including the focus type and subtypes only then it won't find A.toString() which is wrong. However, if I simply add class B extends A {} to the example then it finds A.toString(). Please fix ASAP since we asked clients to adopt this new method.
(In reply to comment #40) > (In reply to comment #39) > > (From update of attachment 155994 [details] [details]) > > Patch looks good to me. It was not really necessary to depreciate the method in > > BasicSearchEngine as it is internal, but that's not a problem, we'll remove it > > at the same time than for SearchEngine... > > Yes, and by marking it as deprecated it's easier to remember removing it ;-) > My remark wasn't clear enough. I wanted to say that the BasicSearchEngine method could have been removed instead of being depreciated. Because it is in a internal class, no client is supposed to used it. Hence we do not have to give time to migrate on the correct method as we have to do for API method...
Created attachment 156074 [details] additional test (In reply to comment #43) > The new method does not work as expected and hence I cannot use it. The problem > is that it does not find matches if I include the focus type but that type has > no subtype: > > public class A { > void test() { > A a= new A(); > a.toString(); > } > @Override > public String toString(){ > return ""; > } > } > > If I create a hierarchy scope for A including the focus type and subtypes only > then it won't find A.toString() which is wrong. However, if I simply add > class B extends A {} > to the example then it finds A.toString(). > > Please fix ASAP since we asked clients to adopt this new method. I cannot reproduce. The patch adds your example to the tests and in my workspace it passes.
(In reply to comment #44) > My remark wasn't clear enough. I wanted to say that the BasicSearchEngine > method could have been removed instead of being depreciated. Because it is in a > internal class, no client is supposed to used it. Hence we do not have to give > time to migrate on the correct method as we have to do for API method... But the deprecated method in SearchEngine still uses the deprecated method in BasicSearchEngine. I cannot remove the internal one before removing the deprecated API. Am I missing something?
>I cannot reproduce. The patch adds your example to the tests >and in my workspace it passes. Maybe you do a different search. I can give you more details if your on Sametime.
(In reply to comment #47) > >I cannot reproduce. The patch adds your example to the tests > >and in my workspace it passes. > Maybe you do a different search. I can give you more details if your on > Sametime. If Sametime is some kind of platform, no. Never heard of. How about email? ;-) Have you looked at the test? What's different?
(In reply to comment #46) > > But the deprecated method in SearchEngine still uses the deprecated > method in BasicSearchEngine. I cannot remove the internal one > before removing the deprecated API. Am I missing something? The deprecated API can call the new BasicSearchEngine method and then you can remove the deprecated method on BasicSearchEngine. It should not heart anyone...
>If Sametime is some kind of platform, no. Never heard of. >How about email? ;-) My response was for Frédéric, who knows what Sametime is ;-) FYI: it's a chat client. I didn't look at the test but I suspect we perform the search a bit differently. Frédéric is looking at it.
(In reply to comment #49) > (In reply to comment #46) > > > > But the deprecated method in SearchEngine still uses the deprecated > > method in BasicSearchEngine. I cannot remove the internal one > > before removing the deprecated API. Am I missing something? > > The deprecated API can call the new BasicSearchEngine method and then you can > remove the deprecated method on BasicSearchEngine. It should not heart > anyone... No ;-) The deprecated API needs to passe the parameter noMemberTypes, which is not exposed by the new method in BasicSearchEngine. Alternatively, the deprecated API could create the HierarchyScope directly without using BasicSearchEngine, but why the hassle? It'll all go by M5, right?
(In reply to comment #51) > (In reply to comment #49) > > (In reply to comment #46) > > > > > > But the deprecated method in SearchEngine still uses the deprecated > > > method in BasicSearchEngine. I cannot remove the internal one > > > before removing the deprecated API. Am I missing something? > > > > The deprecated API can call the new BasicSearchEngine method and then you can > > remove the deprecated method on BasicSearchEngine. It should not heart > > anyone... > > No ;-) > The deprecated API needs to passe the parameter noMemberTypes, which is > not exposed by the new method in BasicSearchEngine. > That's right, I was puzzled with the boolean which does not have the same meaning for the two methods... > Alternatively, the deprecated API could create the HierarchyScope > directly without using BasicSearchEngine, but why the hassle? > It'll all go by M5, right? Sure... :-)
(In reply to comment #50) > >If Sametime is some kind of platform, no. Never heard of. > >How about email? ;-) > My response was for Frédéric, who knows what Sametime is ;-) > FYI: it's a chat client. > > I didn't look at the test but I suspect we perform the search a bit > differently. Frédéric is looking at it. Yes, I'm reproducing the Dani's problem. So, I'm currently trying to writing a test to highlight it...
Created attachment 156084 [details] Additional test highlighting the problem described by Dani The difference is when using compilation unit instead of working copy. The hierarchy scope encloses(String) method does not return true for the A.java file, hence the index returns no match and the search request fails... It sounds to be a problem in the HierarchyScope.buildResourceVector method which does not add any elements if there is no subtypes. Perhaps the test this.subTypes == null should also test whether the array is empty or the focus type should be explicitely added to the elements when the flag includeFocusType is true...
Created attachment 156088 [details] Additional test highlighting the problem described by Dani New patch fixing the expected search results...
Created attachment 156091 [details] proposed fix re comment 54 (In reply to comment #54) > Created an attachment (id=156084) [details] > Additional test highlighting the problem described by Dani I see. > The difference is when using compilation unit instead of working copy. The > hierarchy scope encloses(String) method does not return true for the A.java > file, hence the index returns no match and the search request fails... > > It sounds to be a problem in the HierarchyScope.buildResourceVector method > which does not add any elements if there is no subtypes. > > Perhaps the test this.subTypes == null should also test whether the array is > empty or the focus type should be explicitely added to the elements when the > flag includeFocusType is true... Sounds like it. I propose this patch which adds the focus type right after calling this.hierarchy.getAllSubtypes(this.focusType), which reflects the asymmetry between getAllSubTypes() and getAllTypes(): the latter includes the focus type, the former excludes it. I also added a test similar to yours but using the old API.
Created attachment 156098 [details] same patch - one more test I added one more test as a variation of testBug295894c3(): By placing A and B in different files the test fails (without the patch in HierarchyScope) even though A has a subtype. I must admit that the includeFocusType flag wasn't evaluated when searching for resources (using the path) rather than types. The proposed patch fixes this. (Same patch as before).
Comment on attachment 156098 [details] same patch - one more test Looks good. I also verified that with this fix, the JDT/UI test case works well :-)
(In reply to comment #57) > Created an attachment (id=156098) [details] > same patch - one more test > Released for 3.6M5 in HEAD stream.
Thanks Stephan and Frédéric, works now.
Verified for 3.6M5 using Build id: I20100122-0800
Verified.
(In reply to comment #37) > I doubt that there's anyone using it outside the SDK. I would fix it in HEAD > today and announce on the lists that this will be gone in the next I-build (and > hence also M5). After all adopting the new method is quickly done. Did we just miss the last train home (due to API freeze)? Or can we still remove the deprecated method for Helios? Note, that the PDE/UI usage of the deprecated method has been fixed in bug 304157 (builds > 20100303).
(In reply to comment #63) > Did we just miss the last train home (due to API freeze)? > Or can we still remove the deprecated method for Helios? With PMC approval, it is always possible to have API change post API freeze. This is why the API freeze check has an exclusion list to remove "approved" API changes post API freeze.
I completely forgot this case.
(In reply to comment #65) > I completely forgot this case. Me too, until I read Chris' blog post on API freeze today.
(In reply to comment #66) > (In reply to comment #65) > > I completely forgot this case. > > Me too, until I read Chris' blog post on API freeze today. I opened bug 305755 to take care of this.
