improve call hierarchy

[wenbodong2015]

add commands for call hierarchy:

1. reopen hierarchy window.
2. Add a caller to current node manually.
3. PageUp and PageDown.
4. Close callers.
5. Remove callers.

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

• I have read and understood YCM's CONTRIBUTING document.
• I have read and understood YCM's CODE_OF_CONDUCT document.
• I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.
• I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.

Why this change is necessary and useful

When you need to navigate back and forth among many functions in the call stack, the ability to reopen the call stack window will be very useful.
When the call stack is large and complex, you can simplify things by closing and deleting call nodes.

The benefits of adding a call stack are as follows:
When YCMCallHierarchy cannot find the actual caller, '(YCMAddCallHierarchy)' will be very useful. For example, when tracking the caller of callback functions in C language, YCMCallHierarchy may not be able to find the true caller; instead, it may trace related registration functions or initialization functions. The relevant code passes the callback function to a function pointer, resulting in a call stack that may not be what you are looking for. In this case, you can manually find the function that calls the function pointer, which is the true caller, and use '(YCMAddCallHierarchy)' to manually add the true caller to the call stack, thus extending the call stack.

---

This change is 

[bstaletic]

Wow, I am pleasantly surprised by a non-maintainer working on this!

I am not able to review this today, but some initial thoughts:

Reopening could indeed be very useful.
Adding and removing, I do need to look more carefully into.

I will do a proper review tomorrow.

[wenbodong2015]

Wow, I am pleasantly surprised by a non-maintainer working on this!

I am not able to review this today, but some initial thoughts:

Reopening could indeed be very useful. Adding and removing, I do need to look more carefully into.

I will do a proper review tomorrow.

Hello, I just fixed several bugs related to reopen and remove. I am looking forward to your review.

[bstaletic]

@wenbodong2015 Sorry for the delay. I had some personal stuff that I had to take care off.
I swear I have not forgotten about this!
I'll have time to review this pull request when I come home from work. In the mean time, I'll let CI run tests with your changes.

[bstaletic]

Finally getting around to reviewing this.
Once again, thanks for working on this!

I will have to review manual adding and removing of nodes later. I'm still worried about that part for two reasons:

• Previously only the root node was special. Now any node could be a "preparation item".
• I also need some convincing we should allow users that freedom.

I'd also love to hear @puremourning's thoughts.

Reviewed 1 of 5 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @wenbodong2015)

---

autoload/youcompleteme.vim line 1773 at r1 (raw file):

      \ <cmd>call youcompleteme#hierarchy#StartRequest( 'resume' )<cr>
silent! nnoremap <silent> <plug>(YCMAddCallHierarchy)
      \ <cmd>call youcompleteme#hierarchy#StartRequest( 'addcall' )<cr>

Is addcall the best name?
This should just work (tm) for type hierarchies as well.

---

autoload/youcompleteme/hierarchy.vim line 51 at r1 (raw file):

    endif
    return
  endif

I love how simple resume turned out to be.

Code quote:

  if a:kind == 'resume'
    if s:lines_and_handles != v:null
      call s:SetUpMenu()
    endif
    return
  endif

---

autoload/youcompleteme/hierarchy.vim line 62 at r1 (raw file):

    let s:lines_and_handles = lines_and_handles
    call s:SetUpMenu()
    return

I missed these early returns which confused me for a moment.

I would rather not call StartRequest(), especially for resuming.

My suggestion would be:

• Keep the old StartRequest() as it were, perhaps with a different name.
• Put the resume logic in its own function.
• Put addcall logic to its own function.

---

autoload/youcompleteme/hierarchy.vim line 86 at r1 (raw file):

    let s:select = -1
    let s:kind = ''
  endif

Was this necessary? I'm not totally against it, just wondering if you had hit a problem that was fixed by this.

Code quote:

  else
    let s:lines_and_handles = v:null
    let s:select = -1
    let s:kind = ''
  endif

---

autoload/youcompleteme/hierarchy.vim line 194 at r1 (raw file):

    call s:ResolveItem( selection, 'close', a:result[ 2 ] )
  elseif operation == 'resolve_remove'
    call s:ResolveItem( selection, 'remove', a:result[ 2 ] )

For close and remove, ShouldResolveItem() returns True.
That does not seem right, since resolving an item implies talking to the server.

I know that was very convenient and required less code, but I'd again prefer separation. How about s:CloseItem() and s:RemoveItem()?

Code quote:

  elseif operation == 'resolve_close'
    call s:ResolveItem( selection, 'close', a:result[ 2 ] )
  elseif operation == 'resolve_remove'
    call s:ResolveItem( selection, 'remove', a:result[ 2 ] )

---

doc/youcompleteme.txt line 2165 at r1 (raw file):

  the symbol under cursor. Expand down to callers and up to callees.

- Resume hierarchy '<Plug>(YCMResumeHierarchy)': Reopen the Hierarchy window.

This file is generated from the README.
You should update the README.md and a bot will update the youcompleteme.txt after the pull request is merged.

---

python/ycm/hierarchy_tree.py line 127 at r1 (raw file):

  def _CloseNode( self, node: HierarchyNode ):
    nodes = self._down_nodes

This looks wrong in case of closing outgoing calls.

Clangd does not support those, but gopls does.

---

python/ycm/hierarchy_tree.py line 131 at r1 (raw file):

      for subindex in node._references:
        if nodes[ subindex ]:
          self._CloseNode( nodes[ subindex ])

Do we need to recursively close all child nodes? Can't we just drop that whole branch and let python's GC handle the rest?

---

python/ycm/hierarchy_tree.py line 138 at r1 (raw file):

  def CloseNode( self, handle : int):
    current_index = handle_to_index( handle )
    nodes = self._down_nodes

Again, this looks wrong in case of closing outgoing calls.

This same assumption was made in AddNode() and RemoveNode().

[bstaletic]

Reviewed all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @wenbodong2015)

---

python/ycm/hierarchy_tree.py line 99 at r1 (raw file):

      return
    current_index = handle_to_index( handle )
    nodes = self._down_nodes

Again, we are hardcoding _down_nodes.

---

python/ycm/hierarchy_tree.py line 108 at r1 (raw file):

      node._references.extend( new_refs)
    else:
      node._references = new_refs

If I'm reading everything right, this looks like the expected interaction with the user is the following:

• User tries to expand a hierarchy and hits a wall.
• User closes the hierarchy window.
• User manually finds the next node somehow.
• User calls addcall, keeping in mind the layout of the currently closed hierarchy window.

If I'm getting that right, it feels quite awkward to work with.

Same concern goes for remove.CloseNode

Code quote:

    node = nodes[ current_index ]
    nodes.extend( [
      HierarchyNode( item, node._distance_from_root + 1, node )
      for item in items ] )
    new_refs = list( range( len( nodes ) - len( items ), len( nodes ) ) )
    if node._references:
      node._references.extend( new_refs)
    else:
      node._references = new_refs

[bstaletic]

Reviewed 4 of 5 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @wenbodong2015)

[wenbodong2015]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

python/ycm/hierarchy_tree.py line 108 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

If I'm reading everything right, this looks like the expected interaction with the user is the following:

• User tries to expand a hierarchy and hits a wall.
• User closes the hierarchy window.
• User manually finds the next node somehow.
• User calls addcall, keeping in mind the layout of the currently closed hierarchy window.

If I'm getting that right, it feels quite awkward to work with.

Same concern goes for remove.CloseNode

"Yes, that's what I mean. Function pointers are like that wall. Although this manual 'add' action may seem a bit strange and somewhat complicated to operate, without it, you would not be able to pass through that wall. Suppose you want to trace a 10-level function call stack, where two calls are implemented by function pointers. If you don't manually 'add', you will only be able to demonstrate three separate call stacks."
"Are you still not accepting the manual 'add' and 'remove' functionalities? In that case, I can remove them from the pull request. Also, I have implemented a feature for keeping a history record. Every time a new call stack is viewed, the current call stack is added to the history record. Later, you can select them again through a list box. Are you interested in this?"

[puremourning]

Hi, thanks for sending a PR!

I haven't reviewed this in a lot of detail, but one thing I would say is that I think it is trying to do too much in one PR.

Could we split the various functionalities out into individual pieces so that we can take each on their merit?

On the face of it it's not obvious why you would want to do some of the things that this adds and how much of a burden the current limitations really are. it would be easier to discuss and understand that in 5 smaller PRs than one multi-faceted PR