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

Don't send not specified properties #124

Closed
muffinmad opened this issue May 13, 2020 · 8 comments
Closed

Don't send not specified properties #124

muffinmad opened this issue May 13, 2020 · 8 comments
Labels
bug Something isn't working

Comments

@muffinmad
Copy link
Contributor

E.g.

types.Completion('defaultdict')

will result in sending all other attributes along with label:

server-reply (id:21) Wed May 13 14:25:56 2020:
(:id 21 :jsonrpc "2.0" :result
     (:isIncomplete :json-false :items
		    [(:label "defaultdict" :kind nil :detail nil :documentation nil :deprecated :json-false :preselect :json-false :sortText nil :filterText nil :insertText nil :insertTextFormat nil :textEdit nil :additionalTextEdits nil :commitCharacters nil :command nil :data nil)]))
@rwols
Copy link

rwols commented May 14, 2020

Important notions from the LSP spec:

  • There can be an optional key with a valid value.
  • There can be a mandatory key with an optional value. doesn't seem to be the case

@rwols
Copy link

rwols commented May 14, 2020

There's also a "range": null in the response to the textDocument/hover request. I'm guessing it's the same underlying issue.

This library needs to distinguish between optional keys with typed values and mandatory keys.

@rwols
Copy link

rwols commented May 14, 2020

I haven't tested this but this might work as serializer:

def default_serializer(o):
    """JSON serializer for complex objects."""
    if isinstance(o, enum.Enum):
        return o.value
    d = {}
    for k, v in o.__dict__.items():
        if not k.startswith('_') and v is not None:
            d[k] = v
    return d

@rwols
Copy link

rwols commented May 14, 2020

You should also account for cases where a None response is allowed:

  • textDocument/hover is allowed to respond with None if there's no hover.
  • textDocument/signatureHelp is allowed to respond with None if there's no sighelp.
  • ...

@danixeee danixeee added the bug Something isn't working label May 18, 2020
@rwols
Copy link

rwols commented May 28, 2020

Another example where this breaks an editor is for textDocument/rename. We do a check here to see if we're dealing with new-style documentChanges or old-style changes:

def parse_workspace_edit(workspace_edit: Dict[str, Any]) -> Dict[str, List[TextEdit]]:
    changes = {}  # type: Dict[str, List[TextEdit]]
    if 'changes' in workspace_edit:
        for uri, file_changes in workspace_edit.get('changes', {}).items():
            changes[uri_to_filename(uri)] = list(parse_text_edit(change) for change in file_changes)
    if 'documentChanges' in workspace_edit:
        for document_change in workspace_edit.get('documentChanges', []):
            if 'kind' in document_change:
                debug('Ignoring unsupported "resourceOperations" edit type')
                continue
            uri = document_change.get('textDocument').get('uri')
            version = document_change.get('textDocument').get('version')
            text_edit = list(parse_text_edit(change, version) for change in document_change.get('edits'))
            changes[uri_to_filename(uri)] = text_edit
    return changes

However, the payload from this framework is

{'changes': None, 'documentChanges': [{'edits': [{'newText': "                filename = cls.name()\n                if filename.lower().startswith('lsp-'):\n                    filename = filename[len('lsp-'):]\n                sublime.find_resources('*{}.sublime-settings'.format(filename))\n", 'range': {'end': {'character': 0, 'line': 68}, 'start': {'character': 0, 'line': 64}}}], 'textDocument': {'version': 149, 'uri': 'file:///home/raoul/.config/sublime-text-3/Packages/LSP/plugin/core/main.py'}}]}

changes is in the returned params, so we will break in if 'changes' in workspace_edit. It's not allowed per spec to have None as value.

@muffinmad
Copy link
Contributor Author

@rwols Because both 'changes' and 'documentChanges' are processed, simple workaround may help, e.g.:

 def parse_workspace_edit(workspace_edit: Dict[str, Any]) -> Dict[str, List[TextEdit]]:
     changes = {}  # type: Dict[str, List[TextEdit]]
-    if 'changes' in workspace_edit:
-        for uri, file_changes in workspace_edit.get('changes', {}).items():
+    _changes = workspace_edit.get('changes')
+    if _changes:
+        for uri, file_changes in _changes.items():
             changes[uri_to_filename(uri)] = list(parse_text_edit(change) for change in file_changes)
-    if 'documentChanges' in workspace_edit:
-        for document_change in workspace_edit.get('documentChanges', []):
+    _changes = workspace_edit.get('documentChanges')
+    if _changes:
+        for document_change in _changes:
             if 'kind' in document_change:
                 debug('Ignoring unsupported "resourceOperations" edit type')
                 continue

@pappasam
Copy link
Contributor

pappasam commented Aug 20, 2020

@danixeee Another option is to have a library-specific value for an optionally undefined attribute:

UNDEFINED = object()

In the serializer, you can do an identity is check for UNDEFINED and not return it if the object is undefined:

def default_serializer(o):
    """JSON serializer for complex objects."""
    if isinstance(o, enum.Enum):
        return o.value
    d = {}
    for k, v in o.__dict__.items():
        if v is not UNDEFINED:
            d[k] = v
    return d

The above will still allow None to be placed into the dictionary.

I was drawn to this conversation by the following issue: pappasam/jedi-language-server#38 (comment)

Here's a mypy-friendly way of achieving the above: https://www.python.org/dev/peps/pep-0484/#support-for-singleton-types-in-unions

@danixeee
Copy link
Contributor

Solved with #139.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants