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

Dedup redundant type/kind info by simplifying full signature generation #179

Merged
merged 2 commits into from Nov 19, 2021
Merged

Dedup redundant type/kind info by simplifying full signature generation #179

merged 2 commits into from Nov 19, 2021

Conversation

ericvw
Copy link
Contributor

@ericvw ericvw commented Nov 19, 2021

For completions that don't have a signature, BaseName.description
provides type, name, and typing information if it was explicitly
written. For completions that provide signatures, .to_string provides
the parameters and typing information that needs to be appended to the
type.

get_type_hints() is an expensive call that calls into jedi.infer(),
even for completions that have explicit typing information.

This partially reverts commit 8ee2138
by restoring the former expected behavior for how the header is rendered
when hovering over a module due to the simplified approach for obtaining
the full signatures.

Closes: #178

@pappasam
Copy link
Owner

I agree, there's some redundancy that we should address. That said, there are a couple issues with this PR:

  1. The return type is no longer included in the function signature for functions. I believe the return type is useful to include if available.
  2. There are a couple edge cases where it's nice to get type information in the signature instead of just the Jedi type. For example, in your screenshot, SEEK_CUR benefits from the get_type_hint call that has been removed in this PR.

image

Based on your feedback, I think I have a clear idea of what we're looking for. I'll create a new PR later today with some updates and add you as a reviewer for feedback.

@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

Thanks for the feedback, and I agree that having the return type and hinted type is helpful.

If I find some time as well, I may iterate upon this PR to see if I can extend to cover the two issues you highlighted, if you don't mind.

@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

@pappasam I updated the PR with a WIP commit addressing the two issues. However, it appears that calling get_type_hints() is noticeably slower. Try completing:

import os

os.

In Neovim, I am resolving eagerly to get the CompletionItem.detail in the initial list of completions because completionItem/resolve is not supported yet (see neovim/neovim#13199). Initially, my goal was to match similar behavior to YouCompleteMe to get signatures in the completion list.

What about splitting the difference? Use the original method for normal completion requests and use get_type_hints() for resolve requests.

@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

The slowness is especially felt when I start using nvim-cmp for autocompletion.

I'll keep experimenting if there is a cheaper way to get the return type if it is present in the signature.

@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

Invoking get_type_hints() calls into jedi.infer(), which is expensive.

If the code is explicitly typed, .description() and signature.to_string() include the type information. In other words, the completion results return exactly what is specified in the file.

@pappasam, how about this: What if we evolve the default completion request to provide the CompletionItem.detail with the cheap approach using .description and signature.to_string() and use get_type_hints() for the completion resolve request?

For completions that don't have a signature, `BaseName.description`
provides type, name, and typing information if it was explicitly
written.  For completions that provide signatures, `.to_string` provides
the parameters and typing information that needs to be appended to the
type.

`get_type_hints()` is an expensive call that calls into `jedi.infer()`,
even for completions that have explicit typing information.
This partially reverts commit 8ee2138
by restoring the former expected behavior for how the header is rendered
when hovering over a module due to the simplified approach for obtaining
the full signatures.
@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

I've updated this PR blending both approaches to address having return types in function signatures if they are explicitly typed, but it doesn't address the second concern because of the cost of get_type_hints().

With this approach, the completion text is verbatim what is in the source code and this plays with with autocompletion plugins when fetching completions while typing.

@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

Woah — I discovered that .description does include the typing information for those completions which provide it. I see SEEK_CUR: int in the completion with the PR as it currently stands 🥳 .

@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

vim-jedi-lsp-updated

Copy link
Owner

@pappasam pappasam left a comment

Choose a reason for hiding this comment

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

lgtm! I think that, given the performance trade-offs you've identified, we should move forward with this as soon as possible

@pappasam pappasam merged commit d82e677 into pappasam:main Nov 19, 2021
@ericvw ericvw deleted the dedup-redundant-type branch November 19, 2021 21:04
@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

Thanks! Glad I could help out and I learned a few things along the way 😄 .

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.

CompletionItem.detail provides redundant information that is already provided in CompletionItem.kind
2 participants