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 support for callees based on implementors #2780

Open
gayanper opened this issue Jul 28, 2023 · 6 comments · May be fixed by #2792
Open

Add support for callees based on implementors #2780

gayanper opened this issue Jul 28, 2023 · 6 comments · May be fixed by #2792

Comments

@gayanper
Copy link
Contributor

Add support for the following feature which exist in Eclipse IDE into JDT.LS as well.

eclipse-jdt/eclipse.jdt.ui@5f69112

Summary:
When the call hierarchy finds a interface, this feature will try to find implementations and then continue on with the implementation methods to show the callees.

@gayanper
Copy link
Contributor Author

@rgrunber @jjohnstn How do you suggest to implement this ?

@rgrunber
Copy link
Contributor

rgrunber commented Aug 1, 2023

Is there a concrete example of this ? Is there a case where an interface method ends up in the call hierarchy ? If I just perform call hierarchy on an interface method, I see the implementors are already used.

@gayanper
Copy link
Contributor Author

gayanper commented Aug 2, 2023

I will provide a sample scenario

@gayanper
Copy link
Contributor Author

gayanper commented Aug 6, 2023

package com.example.demo;

import java.util.ArrayList;
import java.util.List;

public class CallH {
  static StringList names = new StringArrayList();

  public interface StringList {
    String get(int index);
  }

  public static class StringArrayList implements StringList {
    private List<String> strings = new ArrayList<>();

    @Override
    public String get(int index) {
      return strings.get(index);
    }
  }

  public static String fetchName(int pos) {
    return names.get(pos);
  }

  public static void main(String[] args) {
    fetchName(0);
  }
}

Now try to create outgoing call tree on String com.example.demo.CallH.fetchName(int pos).

This is vscode
image

This is Eclipse which I expect vscode could do the same
image

@gayanper gayanper linked a pull request Aug 8, 2023 that will close this issue
@rgrunber
Copy link
Contributor

rgrunber commented Sep 5, 2023

Is there any reason this wasn't enabled by default in Eclipse ?

I think it's reasonable to set this. So if an outgoing call is done through an interface, where there's only 1 implementation, the call hierarchy should follow the single implementor and make it the child of the given element.

The one downside is the hierarchy is "techically" not correct. The 2 get methods should be replaced by just the get(..) from StringArrayList right ?

@gayanper
Copy link
Contributor Author

gayanper commented Oct 16, 2023

Is there any reason this wasn't enabled by default in Eclipse ?

I think we added that in Eclipse so that if users find it heavy when generating the hierarchy for certain workspaces, then they can opt out of that feature. We can do the same in vscode as well by introducing the setting as we did for chain completions isn't it ?

I think it's reasonable to set this. So if an outgoing call is done through an interface, where there's only 1 implementation, the call hierarchy should follow the single implementor and make it the child of the given element.

Isn't it gaining to make it unclear for a user who is discovering that hierarchy for the first time in the code base ?

The one downside is the hierarchy is "techically" not correct. The 2 get methods should be replaced by just the get(..) from StringArrayList right ?

Even if we find only one implementation, should we make this decision that the fetchName uses StringArrayList#get ? In runtime it could be replaced by something else since the reference is a interface type right ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants