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

[READY] Implement type/call hierarchy handling #4221

Merged
merged 34 commits into from
Jun 20, 2024

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Feb 11, 2024

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

This is the client side pull request related to ycm-core/ycmd#1733

Again, tests and documentations are missing. Documentation is the easy part, but tests would be dreadful...

popup look

I have not paid much attention to the look of the popup.
The look is slightly off - for deep hierarchies the location of references in the file (foo.cpp:8 for example) becomes misaligned.
The context is is not right aligned either.

Basically, it's functional but ugly. If someone would be up for it, please send patches my way.

How to use this thing?

The only publicly available function here is youcompleteme#hierarchy#StartRequest( kind ). It takes 'call' or 'type' as the argyment. From there, YCM will fire a "prepare hierarchy" request and then open a popup with the results.

While the popup is open, the following keys are used:

  • <Tab> to resolve a hierarchy item in the "subtypes" / "incoming" direction.
  • <S-Tab> to resolve a hierarchy item in the "supertypes"/"outgoing" direction.
  • <Up>/<Down> to select items in the popup.
  • <CR> to jump to the selected item and close the popup.
  • <Esc> to close the popup without jumping.

Maybe we want more key bindings here? <C-n>/<C-p> and j/k come to mind...

Oh yes, there's also <plug>(YCMCallHierarchy) and <plug>(YCMTypeHierarchy) that just call youcompletem#hierarchy#StartRequest(). Users should be using these, instead of directly calling the function.


This change is Reviewable

This makes the popup disappear if you keep typing or enter inser mode or
move the cursor etc.

I found it jarring that previously it would just move the cursor behind
the popup and such until you hit escape. Makes the popup more "modal"
but without actually stopping you from continueing.

Also:

- Use simliar up/down keys as the symbol finder (c-p, c-k, c-n, c-j
  etc.)
- SetupFoo -> SetUpFoo because the verb is "To set up" (rather than the
  noun "a setup".
- purge barbaric 80+ character lines
We try to be a bit consistent with the finder poppup, but
implementation-wise this is simpler. The idea is that there are 3
columns, each having 1/3 of the popup width. We fix the width of the
popup (like we do for the finder) and set the tabstop to 1/3 of the
internal width (core_width).

Then when displaying text, we truncate "columnns" according to that
tabstop (to avoid mess). To do this, we pass structured data from the
python layer to vimscript and construct the line text there. This will
also help later when we add in the syntax highlight (text properties)
like we have for the finder popup.
This basically involves moving the properties mapping to its own place,
and creating text properties to overlap the various parts of the popup
columns. Style matches, and feels correct.

Fiddly part is that we have to (for some reason) set cursorline every
time we move the cursor, but ain't nobody gonna be able to explain why,
and why that only necessary after launching the finder popup window
once. Some wierd vim bug is my guess.
This makes the columns seem less broken, particularly for leading
whitespace. This is particularly problematic due to our use of tabstop:
if the leading whitespace was a tab, it would go nuts.

It would still go nuts for any other literal tab in the description.
Perhaps we should fix that too.
@bstaletic bstaletic force-pushed the hierarchies branch 3 times, most recently from 7e16c1f to 9d8d267 Compare May 17, 2024 04:16
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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


autoload/youcompleteme/hierarchy.vim line 42 at r2 (raw file):

  if a:kind == 'call'
    let lines_and_handles = py3eval(
      \ 'ycm_state.InitializeCurrentHierarchy( ycm_state.SendCommandRequest( ' .

Rather than having magic in SendCommandRequest to skip the "result" handler, shouldn't we just call "GetCommandResponse" which is ... for that exact purpose (get the result without handling it?) same below


python/ycm/client/command_request.py line 222 at r2 (raw file):

                        buffer_command = DEFAULT_BUFFER_COMMAND,
                        extra_data = None,
                        skip_post_command_action = False ):

what's the difference between SendCommandRequest with skip_post_command_action=True and GetCommandResponse ?

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 6 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

a discussion (no related file):
Need to update README


Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic)

Previously, when changing direction of hierarchy and re-rooting the
hierarchy tree, we would retain the old node as the new root.
That works most of the time, but fails in the face of multiple
inheritance and switching to supertypes. In that, problematic, case, the
resulting hierarchy will be missing some supertypes.
Worse, if a call hierarchy contains a node with multiple locations,
re-rooting to that node behaved in a way that defies explanation.

The solution is to throw away everything and start a new hierarchy
completely every time a user requests re-rooting.

This trades some performance for correctness, but this is not at all
performance sensitive.
Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some tests locally, but...

not quite LGTM. There's one more bug to solve.

Re-rooting the call hierarchy , such that we go from asking for foo's callers to foo's callees is broken.
Solving this will require a slight API modification.

The "incoming calls" response looks something like this:

{
   'from': hierarchy_item
   'fromRanges': locations
}

locations is what ycmd API currently provides.
hierarchy_item['range'] is what we need when going from callers to callees.

Example:

int f();
int g() {
        return f() + f();
}

Asking for callers of f returns something like this:

{
  'from': hierarchy_item # hierarchy_item['range'] represents `g` location
  'fromRanges': [
    first_f,
    second_f
  ]
}

If g should be the new root, we definitely want the from.range.start for the new request. Currently, instead, we get fromRanges[N].

All that to say... what should the additional location be called in the ycmd API?
from is just an awful property name.
parent_location? Seems very specific to incoming calls. (we have no use for it, but we can provide the location of to in outgoing calls.)
root_location? Seems like it should only be used when changing root. (That's the only current purpose.)

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

a discussion (no related file):

Previously, puremourning (Ben Jackson) wrote…

Need to update README

Done, thanks to your pull request.



autoload/youcompleteme/hierarchy.vim line 42 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Rather than having magic in SendCommandRequest to skip the "result" handler, shouldn't we just call "GetCommandResponse" which is ... for that exact purpose (get the result without handling it?) same below

Magic is gone, but like I said, I think we want another utility in command_request.py.


python/ycm/youcompleteme.py line 147 at r4 (raw file):

    response = self.GetCommandRequest( request_id ).Response()
    self.FlushCommandRequest( request_id )
    return response

Here's an example of how I'm currently performing sync requests.
This is the need for a new utility.

Locally I have implemented that new utility and called it GetRawCommandResponse().

Would that be a good name?


python/ycm/youcompleteme.py line 441 at r4 (raw file):

    final_arguments = []
    for argument in arguments:
      if isinstance( argument, str ):

Now that I'm passing hierarchy items (dict) as an argument for /run_completer_command, we now need this check as well.

Yet, this function will never add any extra_data to these requests.

Taking into account the GetRawCommandResponse(), we could just skip calling _GetCommandResponseArguments() and then revert this change.


python/ycm/client/base_request.py line 263 at r4 (raw file):

    'column_num': column,
    'working_dir': GetCurrentDirectory(),
    'file_data': file_data

Really not sure if file_data is correct here. I think it is...

The point of this change is, when re-rooting the hierarchy, we do not want a request for the cursor location, but whatever is in the hierarchy node.

Also, location being a (file, line, column) tuple seems ugly.
At one point I have named it HierarchyLocation, but that's even worse - why would BuildRequestDataForLocation know about hierarchy types!


python/ycm/client/command_request.py line 222 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

what's the difference between SendCommandRequest with skip_post_command_action=True and GetCommandResponse ?

GetCommandResponse returns a string and returns an empty string for lists. I.e. request.Response() vs request.StringResponse().

Right now, I have done away with it, but am using SendCommandRequestAsync and immediately calling Response() on it.

We probably want an additional utility in our command_request.py.

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


autoload/youcompleteme/hierarchy.vim line 42 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Magic is gone, but like I said, I think we want another utility in command_request.py.

As agreed, I have added GetRawCommandResponse.


python/ycm/client/base_request.py line 263 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Really not sure if file_data is correct here. I think it is...

The point of this change is, when re-rooting the hierarchy, we do not want a request for the cursor location, but whatever is in the hierarchy node.

Also, location being a (file, line, column) tuple seems ugly.
At one point I have named it HierarchyLocation, but that's even worse - why would BuildRequestDataForLocation know about hierarchy types!

This is definitely broken when location refers to a buffer we have never opened.


python/ycm/client/command_request.py line 222 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

GetCommandResponse returns a string and returns an empty string for lists. I.e. request.Response() vs request.StringResponse().

Right now, I have done away with it, but am using SendCommandRequestAsync and immediately calling Response() on it.

We probably want an additional utility in our command_request.py.

Added GetRawCommandResponse().

In order to send a ycmd request for an unloaded buffer one must first...
load the buffer.
That is the only way to reliably determine the buffer's filetype.
However, we do not want to switch to the buffer, so everything needs to
be done "in the background".

`:badd` adds a buffer as unloaded, if it were not present before.
`bufload()` loads a buffer that has previously not been loaded.
Previously all new nodes were counted as 1.
With call hierarchies, there may be more locations/lines per hierarchy
node. In that case, the cursorline would be set to the wrong line.

Instead, we need to add up the lengths of all the item locations.
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 92.54499% with 29 lines in your changes missing coverage. Please review.

Project coverage is 89.78%. Comparing base (4556062) to head (25ec3c9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4221      +/-   ##
==========================================
+ Coverage   89.67%   89.78%   +0.11%     
==========================================
  Files          34       37       +3     
  Lines        4445     4779     +334     
==========================================
+ Hits         3986     4291     +305     
- Misses        459      488      +29     

Changelog:

1. Upgrade all subservers
2. Hierarchy support
@bstaletic bstaletic changed the title [RFC] Implement type/call hierarchy handling [READY] Implement type/call hierarchy handling Jun 18, 2024
Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to the latest ycmd.
Still need help with the tests. Manually performing the steps in the tests works.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

puremourning and others added 3 commits June 19, 2024 22:34
Previously the tests were using async result checking, but this is
intended for insert mode only. In fact we were running off the end of
the test and then the check callbacks were firing.

Simplified by just making it the same but sequential, and replacding
FeedAndCheck* with direct calls to feedkeys(..., 'xt').
indexof was added so recently that even vim-helptag-versions website
doesn't have it. Replace with a loop.
There were some unnecessary `WaitForAssert()` calls and unnecessary
`silent` commands.
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


python/ycm/client/base_request.py line 253 at r5 (raw file):

def BuildRequestDataForLocation( location ):
  file, line, column = location

any reason not to accept file, line, location as arguments here rather than a tuple? Callers can always do *location


python/ycm/client/base_request.py line 255 at r5 (raw file):

  file, line, column = location
  current_buffer = vim.current.buffer
  vim.command( f'badd {file }' )

may be slightly more consistent to use vim.eval( 'bufadd(...)' ) rather than cmd/function mix.

I'm also fairly sure that either this or the buflload can throw vim.error E325 ATTENTION! if there is a swap file.


python/ycm/client/base_request.py line 257 at r5 (raw file):

  vim.command( f'badd {file }' )
  vim.eval( f'bufload( "{ file }" )' )
  buffer_number = vimsupport.GetBufferNumberForFilename( file )

Looking at this, I recall that this function has a create_buffer_if_needed argument. We could do that (first) and then call bufload, skipping badd call above.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 7 files at r4, 3 of 7 files at r5, 3 of 3 files at r6, 3 of 4 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


python/ycm/client/base_request.py line 253 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

any reason not to accept file, line, location as arguments here rather than a tuple? Callers can always do *location

No good reason. Fixed.


python/ycm/client/base_request.py line 255 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

may be slightly more consistent to use vim.eval( 'bufadd(...)' ) rather than cmd/function mix.

I'm also fairly sure that either this or the buflload can throw vim.error E325 ATTENTION! if there is a swap file.

Dropped badd in favour of GetBufferNumberForFile() with create_if_needed = True.

E325 is handled. It can only be thrown from bufload(). I hope I have handled it correctly.


python/ycm/client/base_request.py line 257 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Looking at this, I recall that this function has a create_buffer_if_needed argument. We could do that (first) and then call bufload, skipping badd call above.

Good catch.
Done.

- `badd` is unnecessary since we have `GetBufferNumberForFilename()`
- Instead of passing `location`, we can pass `file`, `line` and `column`
  separately.
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Jun 20, 2024
Copy link
Contributor

mergify bot commented Jun 20, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit 3a5ee90 into ycm-core:master Jun 20, 2024
11 of 13 checks passed
@bstaletic bstaletic deleted the hierarchies branch June 20, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants