Bug 122442 - [search] API inconsistency with IJavaSearchConstants.IMPLEMENTORS and SearchPattern
Summary: [search] API inconsistency with IJavaSearchConstants.IMPLEMENTORS and SearchP...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 minor (vote)
Target Milestone: 3.2 M5   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-01-01 17:04 EST by Markus Keller CLA
Modified: 2006-02-15 04:32 EST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2006-01-01 17:04:54 EST
I20051215-1506

Javadoc of IJavaSearchConstants.IMPLEMENTORS says that it works for searches for TYPE, CLASS, or INTERFACE. But all the references to IMPLEMENTORS in SearchPattern and SearchEngine tell that it only works for interfaces.

I checked with the Java Search dialog, and it looks like the search engine currently follows the behavior defined in IJavaSearchConstants.IMPLEMENTORS (which is nice). The other javadoc references should also tell that they work for classes too.
Comment 1 Frederic Fusier CLA 2006-01-02 07:15:52 EST
We will not modify current Search Engine behavior, so fix to this bug will consist to only modify javadoc comments of methods where IJavaSearchConstants.IMPLEMENTORS is referenced...

So, bug 122443 is not really blocked by this one => do you want me to remove dependency?
Comment 2 Markus Keller CLA 2006-01-03 04:44:37 EST
Removing bug 122443 from dependents. I just wanted to make sure that we don't remove our restriction before it was 100% sure that the search engine will not be changed in this regard.
Comment 3 Martin Aeschlimann CLA 2006-01-04 10:34:05 EST
I couldn't find an example with results.
class A extends B {
}
class B {
}

Implementors on B: none

Frederic, I guess thats the intended behaviour, right?

Please reopen if I missed something.
Comment 4 Martin Aeschlimann CLA 2006-01-04 10:36:43 EST
Sorry wrong bug. The comment should have gone to bug 122443. However, Frederic what is the intended behaviour of the search engine?
Comment 5 Markus Keller CLA 2006-01-04 12:27:04 EST
Hm I just tried it with the anonymous classes through which I came upon the problem, but I didn't check the normal case. Here, a search for implementors of Klass/Interface returns the anonymous in both cases:

class Klass {}
interface Interface {}

class User {
    void m() {
        new Klass() {};
        new Interface() {};
    }
}

So, to be consistent, the search engine should behave the same regardless whether the subtype is anonymous or not.
Comment 6 Frederic Fusier CLA 2006-01-05 11:40:23 EST
As I said in comment 1 (before markus noticed a problem with anonymous), current behavior search engine is correct: it should work for interfaces, classes and both of them when using TYPE constant...

I'll modify javadoc text for IMPLEMENTORS references and look at comment 5 issue...
Comment 7 Markus Keller CLA 2006-01-05 12:43:52 EST
The current behavior is:
- finds anonymous subtypes of a class or an interface
- does *not* find normal (non-anonymous) subtypes of classes

So if you say that IJavaSearchConstants.IMPLEMENTORS is correctly doc'd, then the normal classes are the problematic ones, not the anonymous.

What I said in comment 0 only applied to my too simple-minded tests with only anonymous subclasses.
Comment 8 Frederic Fusier CLA 2006-01-05 13:20:52 EST
I think there's a misunderstanding here. IMPLEMENTORS only appplies to interfaces, I mean there's no sense to search for implementors of a class. The 'search for' flavor (ie. TYPE, CLASS or INTERFACE) is only used to distinguish returned matches: classes which implement the selected interface and/or interfaces which extend the selected interface.

That means that martin was right in comment 3: we do not expect any result while searching implementors for class B! (note that menu item Search->Implementors is not accessible when you select class...). However, I agree that this is not the case for anonymous classes => this is a problem which must be fixed...

I also noticed that Search Engine does NOT behave as specified in IJavaSearchConstants.IMPLEMENTORS javadoc. In fact it always find both classes which implement _and_ interfaces which extends selected interface (ie. always works as if TYPE was specified). This problem must be also fixed...

Does it make more sense ?
Comment 9 Markus Keller CLA 2006-01-05 13:55:40 EST
I agree with the analysis in comment 8.

Both the search dialog and the search menu actions only allow to searchFor TYPE, so the CLASS and INTERFACE options are never used in Eclipse. We don't use IMPLEMENTORS search anywhere else, so changes will not affect our test cases.

However, as a user, I would find it much nicer if IMPLEMENTORS would also return subtypes of classes. Currently, it just returns no matches when I enter a class name in the search dialog. But if you think this would be too much a violation of the term IMPLEMENTORS, then I can also live without it.
Comment 10 Frederic Fusier CLA 2006-01-06 06:01:23 EST
(In reply to comment #9)
> I agree with the analysis in comment 8.
:-)
 
> Both the search dialog and the search menu actions only allow to searchFor
> TYPE, so the CLASS and INTERFACE options are never used in Eclipse. We don't
> use IMPLEMENTORS search anywhere else, so changes will not affect our test
> cases.
I think this problem must be fixed first as it is publicly documented (not used yet but perhaps will be soon...)

> However, as a user, I would find it much nicer if IMPLEMENTORS would also
> return subtypes of classes. Currently, it just returns no matches when I enter
> a class name in the search dialog. But if you think this would be too much a
> violation of the term IMPLEMENTORS, then I can also live without it.
Yes, we think this would be a violation of implementors concept which only applies for interface as said in javadoc comment. Report subclasses would need a new IJavaSearchConstants value. You may enter a new enhancement bug for this if you think there will be any need for it in the future...
Comment 11 Frederic Fusier CLA 2006-01-19 06:47:34 EST
I was wrong... Search Engine does find implementors of classes but only when they are qualified. I guess that in comment 3 test case, martin created A (or B) in default package.

If you change it like:
  A.java
    package pack;
    class A extends B {}
    class B {}
Then Search Engine finds implementors for B...

So, forget my last answer about return subtypes of classes for IMPLEMENTORS. This will not be a violation, it's currently designed for but had a but in the implementation => I will also fix it...
Comment 12 Frederic Fusier CLA 2006-01-19 16:35:45 EST
Fixed and released in HEAD.

Search engine now correctly find implementors depending on searchFor flavor set while creating string search pattern. It behaves the same for anonymous classes than for other ones.

Changes done in SearchPattern, SuperTypeReferencePattern and SuperTypeReferenceLocator.

Test cases added in JavaSearchBugsTests
Comment 13 Markus Keller CLA 2006-01-20 06:00:33 EST
I'm sorry to be a nuisance, but I think we're back to field 1 now regarding the javadoc. I verified that the implementation now finds subtypes of classes as well as of interfaces for IMPLEMENTORS and TYPE.

But the javadoc in IJavaSearchConstants#IMPLEMENTORS still says that it only works for interfaces. It should be something like:
/**
 * The search result is a type that implements an interface or extends a class. 
 * Used in conjunction with either TYPE or CLASS or INTERFACE, it will
 * respectively search for any type implementing/extending the type,
 * or rather exclusively search for classes implementing/extending the type, or
 * interfaces extending the type.
 */

And likewise for references:
{@link IJavaSearchConstants#IMPLEMENTORS}: for types, will find all types which directly implement/extend a given type.


class Klass {}
interface Interface {}

class User {
    void m() {
        new Klass() {};
        new Interface() {};
    }
}

class Subklass extends Klass { }
class Implementor implements Interface { }
interface Subinterface extends Interface { }
interface CantExtendClass extends Klass { }
Comment 14 Frederic Fusier CLA 2006-01-20 09:24:52 EST
I do never think that review was a nuisance.
Thanks for time you take to make this feedback :-)

=> I'v released proposed changes which were obviously missing in my fix.
Comment 15 David Audel CLA 2006-02-15 04:32:19 EST
Verified for 3.2 M5 using build I20060214-0010

There is still an issue with this behavior, see bug 124645.