LSP_METHODS_MAP: Correct server -> client types

[MatejKastak]

Hi,

I was playing with communication between the server (editor) and another server (lsp) in the context of tests. Both instances are based on pygls. I think the deserialization types are swapped. Consider the following:

SERVER -------- workspace/configuration --------> EDITOR

workspace/configuration request docs. Params should be ConfigurationParams and the result any[] according to spec. From the code I think the LSP_METHODS_MAP is in format (options, params, return). The original configuration is (None, List[Any], ConfigurationParams, ).

What I think is happening

When the client receives this request, it tries to deserialize params:

1. https://github.com/openlawlibrary/pygls/blob/master/pygls/protocol.py#L128
2. The check fails since __name__ is not present on typing.List https://github.com/openlawlibrary/pygls/blob/master/pygls/protocol.py#L131

Code @ https://github.com/openlawlibrary/pygls/blob/master/pygls/protocol.py#L127

        try:
            # I searched the code and get_params_type is still the same method across calls
            params_type = get_params_type(method) # params = typing.List[typing.Any]
            if params_type is None: # FALSE
                params_type = dict_to_object
            # this branch expect everything in the LSP_METHODS_MAP[*][1] to have __name__ field
            elif params_type.__name__ == ExecuteCommandParams.__name__: # Exception since typing.List throws on __name__
                params = deserialize_command(params)

>>> from typing import List; List[any].__name__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.9/typing.py", line 694, in __getattr__
    raise AttributeError(attr)
AttributeError: __name__

Are there any other instances of this issue?

[nav] In [1]: from pygls.lsp import LSP_METHODS_MAP

[nav] In [2]: for t in filter(lambda x: x is not None, map(lambda x: x[1], LSP_METHODS_MAP.values())):
         ...:     try:
         ...:         t.__name__
         ...:     except:
         ...:         print(t)
typing.List[typing.Any]
typing.Optional[typing.List[pygls.lsp.types.workspace.WorkspaceFolder]]

This is also true for the WORKSPACE_FOLDERS request.

Traceback

  Traceback (most recent call last):
    File "/usr/lib/python3.9/threading.py", line 954, in _bootstrap_inner
      self.run()
    File "/usr/lib/python3.9/threading.py", line 892, in run
      self._target(*self._args, **self._kwargs)
    File "/home/asd/tools/pygls/pygls/server.py", line 204, in start_io
      self.loop.run_until_complete(
    File "/usr/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
      return future.result()
    File "/home/asd/tools/pygls/pygls/server.py", line 75, in aio_readline
      proxy(b''.join(message))
    File "/home/asd/tools/pygls/pygls/protocol.py", line 456, in data_received
      json.loads(body.decode(self.CHARSET),
    File "/usr/lib/python3.9/json/__init__.py", line 359, in loads
      return cls(**kw).decode(s)
    File "/usr/lib/python3.9/json/decoder.py", line 337, in decode
      obj, end = self.raw_decode(s, idx=_w(s, 0).end())
    File "/usr/lib/python3.9/json/decoder.py", line 353, in raw_decode
      obj, end = self.scan_once(s, idx)
    File "/home/asd/tools/pygls/pygls/protocol.py", line 152, in deserialize_message
      deserialize_params(data, get_params_type)
    File "/home/asd/tools/pygls/pygls/protocol.py", line 131, in deserialize_params
      elif params_type.__name__ == ExecuteCommandParams.__name__:
    File "/usr/lib/python3.9/typing.py", line 694, in __getattr__
      raise AttributeError(attr)
  AttributeError: __name__

Impact

Since the problem only occurs when pygls is used as a client (editor), this should not be any problem for the users. However, it can be encountered during testing.

Evaluation

• What do you think?
• Is this reasonable change?
• Can this be caused because only client (editor) is expected to receive such requests and this is in correct order?
• Should we add check @ https://github.com/openlawlibrary/pygls/blob/master/pygls/protocol.py#L131 to assert params has attr __name__? So we can handle such cases better?
  Hope I am not missing something :)

Code review checklist (for code reviewer to complete)

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• Title summarizes what is changing
• Commit messages are meaningful (see this for details)
• Tests have been included and/or updated, as appropriate
• Docstrings have been included and/or updated, as appropriate
• Standalone docs have been updated accordingly
• CONTRIBUTORS.md was updated, as appropriate
• Changelog has been updated, as needed (see CHANGELOG.md)

[danixeee]

@MatejKastak I just found another bug partially related to this and made a PR. Let's wait to merge that first and we will check this issue again.

[danixeee]

@MatejKastak Could you rebase your branch with master and check if tests are passing?

[MatejKastak]

@danixeee Tests are passing, what do you think about this change?

[danixeee]

@MatejKastak Sorry, there are few more bugs there, but I won't have time to work on them this week. Probably the whole type-checking code needs to be refactored.

Maybe format (options, params, return) is not understandable enough, but we need to split if the server is receiving the request vs sending the request.

In case when receiving a request from the client, we expect params to be an instance of type "params" from the above format and result returned by the server to be an instance of type "return".

In case when sending a request to the client, we check what the client is expecting, which is "return" type in this case.

So, the format should be documented as (options, client-to-server, server-to-client), and regarding testing where pygls is both the client and the server, we should be able to turn off type-checking for the "pygls client".

Does this seem reasonable?

[MatejKastak]

Sorry, this slipped out of my mind.

Does this seem reasonable?

Totally reasonable.

So, the format should be documented as (options, client-to-server, server-to-client)

Yes. I somehow interpreted that differently. Changes in this PR do not adhere to this specific contract.

we should be able to turn off type-checking for the "pygls client".

Yes, I think this is an OK solution.

In summary, I think we can close this PR. Using pygls as a client(editor) is not the original goal of this project (at least what I am aware of). It is just a nice side-effect that we can reuse it in the tests.

Feel free to close this and create an issue with 'we should be able to turn off type-checking for the "pygls client"' if you want.