microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.71k stars 29.08k forks source link

Peek call hierarchy should resend prepareCallHierarchy request after switch the direction #87176

Open testforstephen opened 4 years ago

testforstephen commented 4 years ago

In the normal "Show Call Hierarchy" view, both switching the direction and refresh will resend a prepareCallHierarchy request.

But in the "Peek Call Hierarchy" view, switching the direction doesn't resend a prepareCallHierarchy request. It's better to keep consistent with the normal call hierarchy view. Generally we will do some clean up in the prepareCallHierarchy.

jrieken commented 4 years ago

Generally we will do some clean up in the prepareCallHierarchy.

Can you explain that a little more? Is that clean up from a previous session so that the very last is never cleaned? The fact that "Show Call Hierarchy" always starts with a prepare-call is bit of an implementation detail...

testforstephen commented 4 years ago

We have a requirement about stopping expanding the recursive calls. For example, below is code snippet for recursive call.

public void foo() {
   bar();
}

public void bar() {
   foo();
}

When hover at public void <|>foo() { to show call hierarchy, we want the call hierarchy tree to be like something below.

foo()
 |
 --- bar()
      | 
      --- foo() // Stop at here when continue expanding foo()

In order to detect the recursive ring, we have to add some cache to record the visited elements at past. And every time when expanding the call hierarchy, it will check if the node is already in the visited graph. And if it's yes, then stop expanding the children. That's why we need some signal to clean up the cache.

Recursive call looks like a general problem for all languages. If the language client can deal it directly, that seems be more reasonable. Then the server is just be stateless.

jrieken commented 4 years ago

Recursive call looks like a general problem for all languages. If the language client can deal it directly, that seems be more reasonable. Then the server is just be stateless.

Yeah, our API idea is that servers don't bother about recursion - that's why the API only asks for the next level. We know that this doesn't prevent recursion but since the resolving of the next level is driven by users, e.g by expanding tree nodes, we think it's not a real problem.

Did you hit anything when implementing/experimenting with this or why did you add the recursion check?