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

Class nested inside function causes exception in textDocument/documentSymbol #170

Closed
pyscripter opened this issue Oct 22, 2021 · 4 comments · Fixed by #171
Closed

Class nested inside function causes exception in textDocument/documentSymbol #170

pyscripter opened this issue Oct 22, 2021 · 4 comments · Fixed by #171

Comments

@pyscripter
Copy link
Contributor

pyscripter commented Oct 22, 2021

The following valid python code causes an exception in the language server when you request

def myfunc():
    x = 3
    class MyClass(object):
        y = 4
        def __init__(self):
           pass
    return MyClass

print(myfunc().y)

Traceback:

Traceback (most recent call last):
  File "C:\ProgramData\PyScripter\Lsp\jls\jedilsp\pygls\protocol.py", line 331, in _handle_request
    self._execute_request(msg_id, handler, params)
  File "C:\ProgramData\PyScripter\Lsp\jls\jedilsp\pygls\protocol.py", line 260, in _execute_request
    method_name, method_type, msg_id, handler(params))
  File "C:\ProgramData\PyScripter\Lsp\jls\jedilsp\jedi_language_server\server.py", line 388, in document_symbol
    document_symbols = jedi_utils.lsp_document_symbols(names)
  File "C:\ProgramData\PyScripter\Lsp\jls\jedilsp\jedi_language_server\jedi_utils.py", line 167, in lsp_document_symbols
    parent_symbol = _name_lookup[parent]
KeyError: <Name name='MyClass', description='class MyClass'>

If you repace the __init__ constructor with a function, e.g.

def myfunc():
    x = 3
    class MyClass(object):
        y = 4
        def f(self):
           pass
    return MyClass

print(myfunc().y)

there is no exception but information about the nested class is not included.

Fix
The attached contains a change in lsp_utils.lsp_document_symbols that resolves both issues (avoids the crash and includes info about classes and functions nested inside functions).

jedi_utils.zip

@pappasam
Copy link
Owner

@pyscripter thanks for the suggestion, the examples, and the sample code!

  1. I agree with the first issue and its resolution: we shouldn't have an exception here and this should be fixed.
  2. I'm torn about including classes nested inside functions. This is inconsistent with how we treat other variables that are local to functions. Why not also include y and x in the returned symbols? I initially implement this, but it resulted in an incredibly noisy document symbols tree (as you might imagine).

My preference would be to find a way to omit namespaces held privately inside functions (without raising exceptions, of course): values defined in a function namespace, be they classes / functions / other, are not available to users outside of the function namespace and therefore (arguably) don't belong in the document symbols.

@pyscripter
Copy link
Contributor Author

pyscripter commented Oct 25, 2021

The LSP provides for SymboInformation (flat view) or DocumentSymbol (hierarchical view). So it is reasonable to assume that clients expect the DocumentSymbol results to include some lower levels of the hierarchy.

I agree that there is a danger of getting noise in the results (this is why for instance I did not include variables defined inside functions, function parameters etc.) which Jedi provides.

On the other hand the symbols are used to present the user with a hierarchical view of the namespace, which is used among other things for navigation in the code. For example PyScripter has a docked view called Code Explorer. It would be useful to show nested types (classes) in that view for instance. Also classes nested inside other classes (which are currently also missing) or functions are not that common, so the extra noise will be minimal.

Can we make it an initialization option? (e.g. nested_types)

@pyscripter
Copy link
Contributor Author

By the way and as you may have guessed, PyScripter now uses your project for IntelliSense.

@pappasam
Copy link
Owner

Ok, I agree with your reasoning! Also, as you've pointed out, the current implementation is somewhat broken. I'm going to take your suggestion and just implement it now as the default. In the future, if someone has a huge problem with seeing classes nested in functions, we can create an option to let them override that behavior.

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 a pull request may close this issue.

2 participants