Outline and CodeActions don't show up if the LSP response has fields with null values

[karthiknadig]

Issue Type: Bug

A typical DocumentSymbol in response to documentSymbol request currently looks like this:

[
    {
        "name": "SampleClass",
        "kind": 5,
        "range": {
            "start": {
                "line": 17,
                "character": 0
            },
            "end": {
                "line": 19,
                "character": 12
            }
        },
        "selectionRange": {
            "start": {
                "line": 17,
                "character": 6
            },
            "end": {
                "line": 17,
                "character": 17
            }
        },
        "detail": null,
        "children": [
            {
                "name": "__init__",
                "kind": 6,
                "range": {
                    "start": {
                        "line": 18,
                        "character": 4
                    },
                    "end": {
                        "line": 19,
                        "character": 12
                    }
                },
                "selectionRange": {
                    "start": {
                        "line": 18,
                        "character": 8
                    },
                    "end": {
                        "line": 18,
                        "character": 16
                    }
                },
                "detail": null,
                "children": [],
                "deprecated": false
            }
        ],
        "deprecated": false
    }
]

Notice the "detail": null, looks like VS Code does not like that. Outline does not show up in VS Code UI when "detail": null. Replacing the "detail": null with "detail": "" gets the Outline view to show up. That seems to suggest that the client is strict about the null field. I am not sure that the spec suggests that optional fields cannot be represented by null values. It seems like LS client could be lenient about this.

Similar issue occurs with CodeAction:

{
        "title": "Extract expression into function 'func_njmjvxyh'",
        "kind": "refactor.extract",
        "diagnostics": null,
        "edit": {
            "changes": null,
            "documentChanges": [
                {
                    "textDocument": {
                        "uri": "file:///c:/GIT/repro/lsptest/tests/test_math.py",
                        "version": null
                    },
                    "edits": [
                        {
                            "range": {
                                "start": {
                                    "line": 18,
                                    "character": 8
                                },
                                "end": {
                                    "line": 18,
                                    "character": 8
                                }
                            },
                            "newText": "func_njmjvxyh(self):\n        \r\n        return \n\n    def "
                        },
                        {
                            "range": {
                                "start": {
                                    "line": 19,
                                    "character": 12
                                },
                                "end": {
                                    "line": 19,
                                    "character": 14
                                }
                            },
                            "newText": " = self.func_njmjvxyh()\n"
                        }
                    ]
                }
            ]
        },
        "command": null
    }

VS Code version: Code 1.52.1 (ea3859d4ba2f3e577a159bc91e3074c5d85c0523, 2020-12-16T16:34:46.910Z)
OS version: Windows_NT x64 10.0.19041

System Info

Item	Value
CPUs	Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz (16 x 2304)
GPU Status	2d_canvas: enabled flash_3d: enabled flash_stage3d: enabled flash_stage3d_baseline: enabled gpu_compositing: enabled multiple_raster_threads: enabled_on oop_rasterization: disabled_off opengl: enabled_on protected_video_decode: unavailable_off rasterization: enabled skia_renderer: disabled_off_ok video_decode: enabled vulkan: disabled_off webgl: enabled webgl2: enabled
Load (avg)	undefined
Memory (System)	63.78GB (44.00GB free)
Process Argv	--folder-uri file:///c%3A/GIT/s%20p/vscode-python --crash-reporter-id 8a06823c-8219-469b-a5be-5ad36c5c7a1c
Screen Reader	no
VM	0%

Extensions (15)

Extension	Author (truncated)	Version
vscode-eslint	dba	2.1.14
gitlens	eam	11.2.1
EditorConfig	Edi	0.16.4
prettier-vscode	esb	5.8.0
vscode-pull-request-github	Git	0.22.0
vscode-docker	ms-	1.9.1
python	ms-	2021.2.529311076-dev
vscode-pylance	ms-	2021.2.0
jupyter	ms-	2020.12.414227025
remote-containers	ms-	0.155.1
remote-ssh	ms-	0.63.0
remote-ssh-edit	ms-	0.63.0
remote-wsl	ms-	0.52.0
cpptools	ms-	1.2.0
code-spell-checker	str	1.10.2

A/B Experiments

vsliv368cf:30146710
vsreu685:30147344
openlogontheside:30221877
python383cf:30185419
vspyt653:30253241
vspor879:30202332
vspor708:30202333
vspor363:30204092
vswsl492cf:30211402
wsl2promptcf:30224613
vstry914:30250197
pythonvsdeb440:30248342
unusedpromptcf:30224611
folderexplorer:30224614
openfilemenucf:30224648
pythonvsded773:30248341
vstes627:30244334
pythonvspyt875:30251590

Related: openlawlibrary/pygls#145

[dbaeumer]

From a specification point of view null is not equal to the absence of a value. If null is allowed as a value then it is explicitly mentioned in the specification using detail?: string | null. We used this consequently in the spec and I actually don't want to deviate from this unless we make this explicit.

The same is true for the command in the code action.

[karthiknadig]

@dbaeumer thanks for the explanation.

Then I see only one issue. the version: null is not respected, even though the spec for TextDocumentEdit says the textDocument field is of type OptionalVersionedTextDocumentIdentifier which allows version: interger | null.

i.e., This JSON still fails. The only null here is on the version field and it should be valid as per spec.

    {
        "title": "Extract expression into function 'func_njmjvxyh'",
        "kind": "refactor.extract",
        "edit": {
            "documentChanges": [
                {
                    "textDocument": {
                        "uri": "file:///c:/GIT/repro/lsptest/tests/test_math.py",
                        "version": null
                    },
                    "edits": [
                        {
                            "range": {
                                "start": {
                                    "line": 18,
                                    "character": 8
                                },
                                "end": {
                                    "line": 18,
                                    "character": 8
                                }
                            },
                            "newText": "func_njmjvxyh(self):\n        \r\n        return \n\n    def "
                        },
                        {
                            "range": {
                                "start": {
                                    "line": 19,
                                    "character": 12
                                },
                                "end": {
                                    "line": 19,
                                    "character": 14
                                }
                            },
                            "newText": " = self.func_njmjvxyh()\n"
                        }
                    ]
                }
            ]
        },
    }

[dbaeumer]

@karthiknadig can you explain what you mean with 'it fails'. The VS Code LSP client ignores that value anyways because VS Code has no support for it. https://github.com/microsoft/vscode-languageserver-node/blob/master/client/src/common/protocolConverter.ts#L954

[karthiknadig]

I don't see this:

if refactoring option if version value is null or if it is removed. It only shows up if the version value is a number. As a work around I am forcing the value of version to be 0.

[dbaeumer]

This works as expected in my testbed here: https://github.com/microsoft/vscode-languageserver-node/blob/master/testbed/server/src/server.ts#L416

Do you have more details. A repository I can clone that demos this.

[karthiknadig]

I will share a VSIX with the repro for this. It may take a day or two.

[karthiknadig]

@dbaeumer this is working as you have explained. In my repro case I was removing version field when it was null. As per spec, when it is null it is working.

[dbaeumer]

OK. Great to hear that you could fix it.