This PR fixes #721, see issue for more details on the error that was occurring, its root cause, and the proposed fixes.
pylsp completion works in two steps: first a list of autocompletion suggestions is collected when the user requests it, and then the selected autocompletion candidate is further processed. This is implemented with hooks which multiple completion providers can implement, by default jedi, rope, or rope_autoimport. The hook for the first step is pylsp_completions, which should return a list, and the results for each provider which implements it are aggregated. The hook for the second step is pylsp_completion_item_resolve, which is called for each provider in an as far as I know undefined order until one of them returns not None. That hook might add additional information to the completion candidate, or it could leave it as it was.
One problem is that the jedi and rope providers do add additional information when they were the ones which suggested the candidate, but return the candidate as it was when they weren't, instead of returning None so other providers can have the chance to process it. This PR fixes that, ensuring that pylsp_completion_item_resolve is always processed by the same handler which provided the autocompletion suggestion. This was not strictly necessary to fix the original issue, but it's a clear improvement in consistency in the code I needed to touch to fix it, so that's what I decided to do. The original issue was just a KeyError in a dictionary access with no guard in the pylsp_completion_item_resolve implementation of both rope and jedi providers.
This change doesn't have any consequence with the built-in providers, as before there was only one active at a given time. With plugins, it enables other completion providers to be able to post-process their candidates properly. But by Hyrum's law, there might be a plugin which relies on the default providers handling the pylsp_completion_item_resolve step (rope_autoimport handler did, after all). So just in case I added a catch-all clause in the hook caller, and then I didn't need to implement it for rope_autoimport either.
Finally, both the previous and current implemention rely on the completion labels being unique, which might not be the case. But I think it's not a problem to keep that assumption for now, and fix it at a later point if it ever comes to be a problem.
Let me know what you think