python-lsp / python-lsp-server

Fork of the python-language-server project, maintained by the Spyder IDE team and the community
MIT License
1.76k stars 186 forks source link

Hover should not show docstring of inferred type but help for hover position #444

Open HedgehogCode opened 9 months ago

HedgehogCode commented 9 months ago

Try the following script:

bar = 10
"""A variable named bar"""

bar

When using auto-complete while typing "bar" the documentation of the variable is resolved correctly. However, if I hover over "bar" it shows the documentation of int() which is not helpful at all.

The current implementation of hover uses Script.infer. Using Script.help gives the expected hover content.

krassowski commented 9 months ago

Thank you, this was bugging me for a while too. If the performance-wise there is no penalty in using help() compared to infer() this is a good idea indeed.

ccordoba12 commented 9 months ago

@HedgehogCode, could you post a screenshot of what you see when using Script.help on this case?

HedgehogCode commented 9 months ago

Script.infer: Screenshot from 2023-09-20 09-07-09

Script.help: Screenshot from 2023-09-20 09-11-25

I did not check the complete API for Jedi yet. Therefore, I am not sure that all the post-processing of the Script.infer return value is also applicable for Script.help (in my example it just worked).

ccordoba12 commented 9 months ago

Thanks @HedgehogCode! I agree that this would be an improvement, but it'd be better if we'd map the docstring returned by Script.help to its Python type so we could return instead a message saying

bar is an integer variable

which I think is more helpful.

Actually, I have a branch that implements a similar improvement in Spyder itself. We plan to have that in Spyder 6, but I could move that code to the server instead.

HedgehogCode commented 9 months ago

I agree, that this is more useful if no docstring is attached to the variable. However, if someone took the time to write a docstring for the variable I think it should be shown to the user.

Pyright for example shows both (result from a "textDocument/hover" request):

{
    "contents": {
        "kind": "markdown",
        "value": "```python\n(variable) bar: Literal[10]\n```\n---\nA variable named bar"
    },
    "range": [...]
}

Note that the variable docstrings are not a language feature but the format is generally accepted (Jedi, sphinx, Pyright, and probably more tools support it).

ccordoba12 commented 9 months ago

However, if someone took the time to write a docstring for the variable I think it should be shown to the user.

Sure, that's a good point too. But I don't know how to do that and show the variable type with Jedi unless (I guess) users also add a type when declaring the variable.

SigmaTel71 commented 9 months ago

I had started working in this direction. I found this help tooltip rather annoying than helpful, so I had to change this. As a result, I got a functioning prototype of variable lookup tooltip. If you want to try it, it's available in my fork of pylsp. More changes coming soon!

image.png

image.png

HedgehogCode commented 9 months ago

Yes. Showing the value of a variable instead of the docstring of the built-in type is much more useful. However, I think we should also show the docstring of the variable itself if it has one (like in my initial example). I also started working in this direction here but I want to do some more testing and provide some examples for hovering different elements. @SigmaTel71, maybe we can combine both approaches.

ccordoba12 commented 9 months ago

Yes. Showing the value of a variable instead of the docstring of the built-in type is much more useful

If this is done for builtins only, then it's fine. I say it because trying to get values of objects using repr is a bad idea since many libraries out there have buggy or not performant repr's

SigmaTel71 commented 9 months ago

If this is done for builtins only, then it's fine. I say it because trying to get values of objects using repr

I don't do that via repr, but via description field of BaseName object.

HedgehogCode commented 9 months ago

Note that I created the PR #452 to solve this ticket. The PR shows some screenshots of new hover results.

The proposed changes do not include the approach by @SigmaTel71 because I came to the conclusion that the definition of a statement should not be shown in the hover result. The language server protocol has a separate method to find the definition of an object: "textDocument/definition". However, I am open to discuss that.

krassowski commented 9 months ago

I came to the conclusion that the definition of a statement should not be shown in the hover result. The language server protocol has a separate method to find the definition of an object: "textDocument/definition". However, I am open to discuss that.

I believe this reasoning is incorrect, the intent of the definition request is not to present it in a hover but to enable go to/jump behaviour. It only returns a location object which does not have a field for text representation of the definition.

SigmaTel71 commented 9 months ago

However, I am open to discuss that.

I started implementing variable definition lookup on hover because this is what pyright does, but I don't want to use pyright just for that, given the fact that I moved from VSCode to Sublime Text for better workflow experience when working on Python code. image Module file from example in screenshot above is available here.

HedgehogCode commented 9 months ago

I believe this reasoning is incorrect, the intent of the definition request is not to present it in a hover but to enable go to/jump behaviour. It only returns a location object which does not have a field for text representation of the definition.

I see. That makes sense. So the question is more, if we really want to show a definition lookup in the hover result.

I started implementing variable definition lookup on hover because this is what pyright does.

I could be wrong, but I think the pyright results are types rather than definition lookups. I put some type inference into my PR but Jedi is less strict (does not use the type Literal[<val>] if a variable is constant).


If we decide on showing a definition lookup we should answer the following questions:

I am open to suggestions for enhancing my PR.

SigmaTel71 commented 9 months ago

For me using BaseName#description looks more like a hack, because it shows the full line of code assigned to the variable, which looks more like Visual Studio's approach when you hover a macro in C/C++. image

  • In which cases do we want to show the definition? Only for builtin types?

From what I see in pyright, the definition is shown when the value is literal, no matter if it's a constant or variable. dict and list don't show its contents.

Various pyright behavior Same goes with constants.
  • How should it be shown? Instead or next to the type information?

I think it will be effective enough to show it in order like 'type, value (if applicable), docstring (if exists)'.