Inconsistencies in `Show call Hierarchy`

[FMorschel]

Repro:

/// Foo is [foo].
/// Here
/// Is
/// A
/// Really
/// Big
/// Comment
void foo() {
  foo();
}


void bar() {
  // This is a comment
  foo();
}


/// This doesnt call [foo].
void other() {}

It shows the call iteratively on itself forever, maybe we should stop that?

And it also shows non-calls (only references in comments) which I think should be removed from this result too.

CC @DanTup @bwilkerson

[bwilkerson]

I agree that it shouldn't show comment references as if they were 'calls', because they aren't.

I don't know how other languages handle recursive invocations, but our current behavior isn't necessarily wrong (and is arguably right, even if there's limited value in it).

[DanTup]

I tested TypeScript, and it shows the recursion too. I think this makes sense, because if you're reviewing the hierarchy to understand where the code calls, knowing it recurses might be useful information.

I agree it shouldn't include references in docs. Though I'm less sure about tear-offs like this:

void other() {
  var x = foo;
  x();
}

I think these should still be included?

[FMorschel]

I tested TypeScript, and it shows the recursion too. I think this makes sense, because if you're reviewing the hierarchy to understand where the code calls, knowing it recurses might be useful information.

I guess my biggest problem with that was that it was also showing bar and other for every inner fold. Seeing it is recursive is fine, but duplicating (or triplicating, etc) the tree like this shows little to no information. This is what I was asking for us to stop iterating (JIC I wasn't clear enough). The tree can get really hard to read and traverse if that happens and may confuse the user.

Some rule like: Is the parent the same method? Any child who is different than both can be removed.

This could be applied (if we have this information) only to the top-level case and leave inner calls to foo inside bar, for example, iterating normally and showing all calls.

Its fine if we decide to do nothing about this, I just wanted to share my initial thoughts on this recursion since it seemed a bit confusing.

[DanTup]

I guess my biggest problem with that was that it was also showing bar and other for every inner fold.

We don't actually build this tree as you see it here (probably if we did we would run forever 😄). When you expand a node, VS Code just asks us "what are the outgoing (or incoming) calls for this method" and adds the results to the node you expanded. In the case where a function appears in multiple places in the tree, expanding any of them would send the same request to the server and therefore get the same results.

It might be possible to work around this by trying to round-trip some custom data to keep track of the tree structure, but I don't think it's worth the complexity just to avoid something that other languages do anyway. If this should be solved, I think the right place would be in the UI, perhaps showing the identical child styled differently (maybe with a little arrow showing it returns back to the parent) that isn't expandable.

[FMorschel]

I think the right place would be in the UI, perhaps showing the identical child styled differently (maybe with a little arrow showing it returns back to the parent) that isn't expandable.

I've opened microsoft/vscode#258860 for that. Thanks!