astral-sh / ruff-lsp

A Language Server Protocol implementation for Ruff.
Other
1.31k stars 46 forks source link

Code action with non-fixable diagnostics returns an error #128

Closed dhruvmanila closed 1 year ago

dhruvmanila commented 1 year ago

So, I was testing this (https://github.com/charliermarsh/ruff/issues/4746) out and I've Neovim configured to show a lightbulb symbol if there are any code actions available. This is done by sending a textDocument/codeAction request wherever the cursor is present.

Request:

{
  id = 5,
  jsonrpc = "2.0",
  method = "textDocument/codeAction",
  params = {
    context = {
      diagnostics = { {
          code = "PLE0117",
          data = {
            noqa_row = 8
          },
          message = "Nonlocal name `x` found without binding",
          range = {
            ["end"] = {
              character = 18,
              line = 7
            },
            start = {
              character = 17,
              line = 7
            }
          },
          severity = 2,
          source = "Ruff"
        } }
    },
    range = {
      ["end"] = <1>{
        character = 12,
        line = 7
      },
      start = <table 1>
    },
    textDocument = {
      uri = "file:///Users/dhruv/playground/ruff/src/PLE0117.py"
    }
  }
}

Response:

{
  error = {
    code = -32602,
    data = "{'traceback': ['  File \"/Users/dhruv/.local/pipx/venvs/ruff-lsp/lib/python3.11/site-packages/pygls/protocol.py\", line 340, in _handle_request\\n    self._execute_request(msg_id, handler, params)\\n', '  File \"/Users/dhruv/.local/pipx/venvs/ruff-lsp/lib/python3.11/site-packages/pygls/protocol.py\", line 264, in _execute_request\\n    self._send_response(msg_id, handler(params))\\n                                ^^^^^^^^^^^^^^^\\n', '  File \"/Users/dhruv/.local/pipx/venvs/ruff-lsp/lib/python3.11/site-packages/ruff_lsp/server.py\", line 477, in code_action\\n    fix = cast(DiagnosticData, diagnostic.data)[\"fix\"]\\n          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^\\n']}",
    message = "KeyError: 'fix'"
  },
  id = 2,
  jsonrpc = "2.0"
}

Extracting the traceback:

  File "/Users/dhruv/.local/pipx/venvs/ruff-lsp/lib/python3.11/site-packages/pygls/protocol.py", line 340, in _handle_request
    self._execute_request(msg_id, handler, params)
  File "/Users/dhruv/.local/pipx/venvs/ruff-lsp/lib/python3.11/site-packages/pygls/protocol.py", line 264, in _execute_request
    self._send_response(msg_id, handler(params))
                                ^^^^^^^^^^^^^^^
  File "/Users/dhruv/.local/pipx/venvs/ruff-lsp/lib/python3.11/site-packages/ruff_lsp/server.py", line 477, in code_action
    fix = cast(DiagnosticData, diagnostic.data)["fix"]
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^

This is happening because the diagnostic doesn't contain a fix but the server expects the fix key to be present (line 506):

https://github.com/astral-sh/ruff-lsp/blob/8dca712c84fee384a477271127ab33b7da955877/ruff_lsp/server.py#L503-L507

MichaReiser commented 1 year ago

Hmm, that's strange. I would expect fix to always be present (except for very old Ruff versions) because serde should serialize None if no fix is present:

https://github.com/charliermarsh/ruff/blob/7ab349aece1a19bc6e9e6b75ac2a910b021e7372/crates/ruff/src/message/json.rs#L55-L63

dhruvmanila commented 1 year ago

Ok, so I found the problem.

tldr, It's got to do with how lua interacts with nil and how Neovim's LSP client decodes the JSON response from the server.

So, in lua there's only one data structure called a table which is used to represent everything (arrays, hashmaps, classes, etc.). Now, you can index into a table through [] or using attribute access. If the key doesn't exists, it'll return nil and not raise any error.

https://github.com/neovim/neovim/blob/cbbda3bcd77595eeabcc0fb70cee513e473833e6/runtime/lua/vim/lsp/rpc.lua#L382

Here, Neovim decodes the body with a specific parameter which says to convert JSON null to lua nil, basically eliminating the entry as they're the same in lua:

vim.json.decode('{"data": {"fix": null, "noqa_row": 8}}')
{
  data = {
    -- `vim.NIL` is a special object used to handle the very bug we're facing
    fix = vim.NIL,
    noqa_row = 8
  }
}

vim.json.decode('{"data": {"fix": null, "noqa_row": 8}}', {luanil = {object = true}})
{
  data = {
    noqa_row = 8
  }
}

So, now when the Neovim client sends the code action request, the diagnostic data doesn't contain the fix key.


I think we should handle such cases appropriately.