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

Error on triggering completions in import context #121

Closed
rchl opened this issue Nov 25, 2021 · 7 comments · Fixed by #122
Closed

Error on triggering completions in import context #121

rchl opened this issue Nov 25, 2021 · 7 comments · Fixed by #122
Milestone

Comments

@rchl
Copy link
Contributor

rchl commented Nov 25, 2021

  1. Create a document:
import c
  1. Trigger completions with the cursor after c.

Results in a error on triggering textDocument/completions:

LSP-pylsp: 2021-11-25 11:14:53,289 CET - ERROR - pylsp_jsonrpc.endpoint - Failed to handle request 43
LSP-pylsp: Traceback (most recent call last):
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/pylsp/plugins/jedi_completion.py", line 311, in resolve_label
LSP-pylsp:     sig = completion.get_signatures()
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/jedi/api/classes.py", line 582, in get_signatures
LSP-pylsp:     for s in self._get_signatures()
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/jedi/api/classes.py", line 570, in _get_signatures
LSP-pylsp:     names = convert_names([self._name], prefer_stubs=True)
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/jedi/inference/gradual/conversion.py", line 152, in convert_names
LSP-pylsp:     return _python_to_stub_names(names, fallback_to_python=prefer_stubs)
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/jedi/inference/utils.py", line 16, in wrapper
LSP-pylsp:     return list(func(*args, **kwargs))
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/jedi/inference/gradual/conversion.py", line 126, in _python_to_stub_names
LSP-pylsp:     for x in _python_to_stub_names([n], fallback_to_python=fallback_to_python):
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/jedi/inference/utils.py", line 16, in wrapper
LSP-pylsp:     return list(func(*args, **kwargs))
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/jedi/inference/gradual/conversion.py", line 132, in _python_to_stub_names
LSP-pylsp:     v = name.get_defining_qualified_value()
LSP-pylsp:   File "/Users/../Package Storage/LSP-pylsp/lib/python3.9/site-packages/jedi/inference/names.py", line 251, in get_defining_qualified_value
LSP-pylsp:     if context.is_module() or context.is_class():
LSP-pylsp: AttributeError: 'NoneType' object has no attribute 'is_module'

It works with import p for example so it's something about specific completion items causing it to crash.

I'm using it through LSP-pylsp in ST.
It uses default settings more or less but you can check all settings here https://github.com/sublimelsp/LSP-pylsp/blob/6cc56fa6f1b60649f2f59bffb9fe1f9beafc738f/LSP-pylsp.sublime-settings.

@ccordoba12
Copy link
Member

ccordoba12 commented Nov 25, 2021

Hey @rchl, thanks for reporting. Please report this issue in the Jedi repo because it's a bug with the latest Jedi (as far as I can tell).

The only thing we can do here is to catch and pass on that error, which I plan to do for our next release.

@rchl
Copy link
Contributor Author

rchl commented Nov 25, 2021

Created davidhalter/jedi#1822

Are you sure catching is a good idea? Then the completions would not work without indication what went wrong.

Or... I guess you meant reporting this as an LSP error which is fine but I feel like printing the stack trace would also be good.

@ccordoba12
Copy link
Member

Created davidhalter/jedi#1822

You're very diligent, thanks! Sorry for the hassle but this turned out to be an error in our side (we were incorrectly formatting a log message).

Could you mention that on that Jedi issue and close it? Thanks again!

Are you sure catching is a good idea?

Yes, it is. We're trying to resolve labels for completion objects, but some (like modules) don't have one. That's why we need to pass when an error like that is raised.

@davidhalter
Copy link

davidhalter commented Nov 25, 2021

@ccordoba12 And why is this not a Jedi bug? I kind of disagree that you should ever catch an error :)

Maybe you should catch InvalidPythonEnvironment, InternalError, or RefactoringError, but other than that I do not see a reason to catch anything. Maybe I'm wrong, because I don't understand your codebase here, but it feels like Jedi is to blame here and you shouldn't really change anything.

@ccordoba12
Copy link
Member

And why is this not a Jedi bug?

Sorry, I thought it wasn't because we're trying to get signatures of completion objects here:

def resolve(self, completion):
try:
sig = completion.get_signatures()
return self.callback(completion, sig)
except Exception as e: # pylint: disable=broad-except
log.warning(f'Something went wrong when resolving label for {completion}: {e}')
return self.resolve_on_error

and I assumed that's not something Jedi can provide of modules.

However, as you can see, we're catching any exception while doing that. So the cause of this bug was us not formatting the log message correctly.

Maybe you should catch InvalidPythonEnvironment, InternalError, or RefactoringError, but other than that I do not see a reason to catch anything. Maybe I'm wrong, because I don't understand your codebase here, but it feels like Jedi is to blame here and you shouldn't really change anything.

Of course we can do that, but the reality is we don't have the man power to handle too many bug reports related to Jedi. The thing is few people know this project is a thin interface over Jedi's functionality, so they come here to report errors that have nothing to do with it.

@davidhalter
Copy link

I can understand that :). All I'm saying is that this is wrong: but this turned out to be an error in our side. It is not an error on your side. It is very much a Jedi error. You can obviously catch Exception and just hide it. That might make sense sometimes. However, I would like to encourage you to send people over if there are actual issues like this one.

@ccordoba12
Copy link
Member

All I'm saying is that this is wrong: but this turned out to be an error in our side

Oh yeah, that was half the truth, sorry for that.

That might make sense sometimes. However, I would like to encourage you to send people over if there are actual issues like this one.

Ok, will do. Actually, we have very few broad exceptions like this, so I think Jedi is working really well. By the way, thanks a lot for your hard work on it, it's really awesome!

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.

3 participants