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

GH-508: Support Call Hierarchy #1306

Merged
merged 9 commits into from
Dec 18, 2019
Merged

Conversation

testforstephen
Copy link
Contributor

@testforstephen testforstephen commented Dec 13, 2019

close #508

I have two PRs to update lsp4j to adopt the new call hierarchy protocol.

please use redhat-developer/vscode-java#1194 PR to integrate test this feature.

Thank @kittaakos for contributing the base PR #925, the new PR is based on that, and add changes to adopt the latest LSP for call hierarchy.

@testforstephen testforstephen force-pushed the GH-508 branch 2 times, most recently from 70ac5fd to 92bd75a Compare December 16, 2019 02:50
@fbricon
Copy link
Contributor

fbricon commented Dec 16, 2019

Recursive calls are not handled properly, recursion should be limited. Compare vscode:
Screen Shot 2019-12-16 at 7 59 55 PM

to Eclipse:
Screen Shot 2019-12-16 at 8 01 38 PM

Akos Kitta and others added 8 commits December 18, 2019 21:13
…JDT UI to LS.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
…rPlugin`.

Also, reused the `StringMatcher` from the CA instead of `jdt.ui.util`.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
Signed-off-by: Akos Kitta <kittaakos@typefox.io>
Closes eclipse-jdtls#508

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
@testforstephen
Copy link
Contributor Author

testforstephen commented Dec 18, 2019

About fixing the recursive call problem, the server has to add some cache to record the visited history and use that to detect a ring. The jdt.ls just needs cache the method wrapper, and then call methodWrapper.isRecursive() method to check if it's ring. So for the cache mechanism, we must need some signal to clean up it too.

This works well for the normal "Show Call Hierarchy" view. Because it will send a new prepareCallHierarchy request whenever you hovers at a new position, click the buttons of refresh or switching the direction in the top right corner. So the server is able to leverage the prepareCallHierarchy request to clean up the cache.

But in "Peek Call Hierarchy" view, only hovers at a new position, VS Code will send a prepareCallHierarchy request. Clicking direction button at the top right corner will not send prepare request. So the cache has no chance to be clean up, and it may cause problems after you switch several times. See the issue microsoft/vscode#87176. Given the issue, i justdisable the recursion check by default, it's easy to add it back.

IMO, The resolving of the next level is driven by users, e.g by expanding tree nodes, the recursion may be not a big problem. In current stage, the correctness is more important than this enhancement. However, i will keep discussing with VS Code about the right solution to fix the recursion.

@fbricon How do you think?

…rchy

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
@fbricon
Copy link
Contributor

fbricon commented Dec 18, 2019

IMO, The resolving of the next level is driven by users, e.g by expanding tree nodes, the recursion may be not a big problem. In current stage, the correctness is more important than this enhancement. However, i will keep discussing with VS Code about the right solution to fix the recursion.

@fbricon How do you think?

Yeah we can probably live without recursion guard for now, and maybe try to figure out how to do it properly next

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 this pull request may close these issues.

Support for Call Hierarchy
2 participants