Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method to find extending interfaces of target interface #65

Closed
geoand opened this issue Jul 31, 2019 · 8 comments · Fixed by #184
Closed

Add method to find extending interfaces of target interface #65

geoand opened this issue Jul 31, 2019 · 8 comments · Fixed by #184
Assignees
Milestone

Comments

@geoand
Copy link
Contributor

geoand commented Jul 31, 2019

I would like to be able to (efficiently) look up all the interfaces that extends a target interface.
Say for example that I have

public interface SomeInterface {

}

and there is an interface that extends is:

public interface SomeExtendingInterface extends SomeInterface {

}

Neither index.getAllKnownSubclasses() nor index.getAllKnownImplementors() covers the aforementioned usecase so I am forced do the following (inefficient) lookup:

    List<ClassInfo> getAllInterfacesExtending(DotName target, IndexView index) {
        List<ClassInfo> result = new ArrayList<>();
        Collection<ClassInfo> knownClasses = index.getKnownClasses();
        for (ClassInfo clazz : knownClasses) {
            if (!Modifier.isInterface(clazz.flags())) {
                continue;
            }
            List<DotName> interfaceNames = clazz.interfaceNames();
            boolean found = false;
            for (DotName interfaceName : interfaceNames) {
                if (interfaceName.equals(target)) {
                    found = true;
                    break;
                }
            }
            if (found) {
                result.add(clazz);
            }
        }
        return result;
    }
@mkouba
Copy link
Contributor

mkouba commented Feb 15, 2022

@Ladicek I think that this one would deserve your attention ;-)

@Ladicek
Copy link
Contributor

Ladicek commented Feb 28, 2022

Good point, indeed neither getAllKnownSubclasses nor getAllKnownImplementors can be used for this. Adding something like getAllKnownSubinterfaces is certainly a good idea.

@Ladicek Ladicek added this to the 3.0.0 milestone Feb 28, 2022
@Ladicek
Copy link
Contributor

Ladicek commented Apr 13, 2022

Here's a funny thing: getKnownDirectImplementors returns direct subinterfaces, but getAllKnownImplementors doesn't return interfaces at all. This is inconsistent and counter-intuitive, but I hesitate to unify that. That would be a pretty serious behavioral breakage that can't be caught during compilation.

I still intend to add getKnownDirectSubinterfaces and getAllKnownSubinterfaces (actually working on that right now), but I'd welcome any opinions about the above.

@geoand
Copy link
Contributor Author

geoand commented Apr 13, 2022

Sounds reasonable.

@mkouba
Copy link
Contributor

mkouba commented Apr 13, 2022

Hm, another funny thing: the javadoc of getAllKnownImplementors() was pretty clear that it only returns classes, not interfaces, but for some reason "interfaces" was replaced with "methodParameters" in 44d77cf#diff-cf6e0f736d92b7d69934c1308f902a9c5440c9075761997bc05a6bab7e0a5c6b. The same applies to getKnownDirectImplementors(). So it was intentional.

@n1hility can tell us more ;-).

@Ladicek
Copy link
Contributor

Ladicek commented Apr 13, 2022

I don't think that methodParameters replacement was intentional, I'm fixing that as part of this work. (Thanks for pointing to the commit. I found a few more places that need fixing.)

I agree that it was intentional for getKnownDirectImplementors to return interfaces and getAllKnownImplementors to not return interfaces, I just find it very weird and unexpected. I feel a strong urge to fix that in one way or another, but I'm not exactly sure about it :-)

@Ladicek
Copy link
Contributor

Ladicek commented Apr 13, 2022

I have an implementation over in #184. It doesn't change existing behavior, though I could be persuaded otherwise :-)

@Ladicek
Copy link
Contributor

Ladicek commented Apr 14, 2022

Done in #184.

@Ladicek Ladicek closed this as completed Apr 14, 2022
@Ladicek Ladicek self-assigned this Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants