Auto-imports broken in Emacs lsp-mode by commit 62d97d9

[traviscross]

The commit 62d97d9 ("Draft completion hashing") to rust-analyzer breaks the automatic adding of imports in Emacs + lsp-mode.

That commit, found via bisection, was part of this PR:

• Hash completion items to properly match them during /resolve #18653

(Note that this is a second and seemingly-separate breakage from the earlier one that also affected Emacs + lsp-mode in #18653 and which was resolved in #18503.)

The behavior remains broken in the latest current commit to r-a (a77cf8e).

Others have also reported this breakage in #18363 (comment) (cc @xitep @monoid @noonebtw).

Testing procedure: Open a file in Emacs, start LSP, and type:

fn main() {
    let x: Pin// Wait, then select `% Pin Pin<{unknown}> (Struct)`.
}

At and after commit 62d97d9, this does not cause Pin to be imported, and the following error is logged (after typing "Pin" and before making a selection):

Error processing message (error "invalid type: null, expected a boolean").

This is testing with lsp-mode version 20241227.533 at commit emacs-lsp/lsp-mode@9de4050.

For details on how to set up an environment in a container suitable to reproduce this issue, see the instructions in #18504 (comment).

No assessment is being made in this report of whether r-a or lsp-mode is responsible, at a protocol level, for this regression.

cc @Veykril @SomeoneToIgnore

[monoid]

I confirm it reproduces since rust-analyzer 2024-12-16.

[dfireBird]

Enabling client server message tracing reveals that response for textDocument/completion has a null value which is sent back by lsp-mode to rust-analyzer. On which rust-analyzer returns an error, which is what lsp-mode shows.

Response of textDocument/completion (trimmed):

{
  "isIncomplete": true,
  "items": [
    {
      "label": "PanicInfo",
      "labelDetails": {
        "detail": " (use core::panic::PanicInfo)",
        "description": "PanicInfo<'_>"
      },
      "kind": 22,
      "deprecated": null,
      "sortText": "80000000",
      "filterText": "PanicInfo",
      "textEdit": {
        "newText": "PanicInfo",
        "insert": {
          "start": {
            "line": 33,
            "character": 4
          },
          "end": {
            "line": 33,
            "character": 5
          }
        },
        "replace": {
          "start": {
            "line": 33,
            "character": 4
          },
          "end": {
            "line": 33,
            "character": 5
          }
        }
      },
      "additionalTextEdits": [],
      "data": {
        "position": {
          "textDocument": {
            "uri": "file:///home/.../src/main.rs"
          },
          "position": {
            "line": 33,
            "character": 5
          }
        },
        "imports": [
          {
            "full_import_path": "core::panic::PanicInfo",
            "imported_name": "PanicInfo"
          }
        ],
        "version": 0,
        "trigger_character": null,
        "for_ref": null,
        "hash": "aCm5vuCqiy5hrerIGGQuoh/1+nI="
      }
    },
...

Update: My bad, it's not returned by r-a, it what lsp-mode has parsed from the response.

[Veykril]

If it errors because we sent null for deprecated then that's a bug in lsp-mode: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItem declares the property as deprecated?: bool which means null, false and true are valid values (and in which case the PR that changed behavior on our side is #18698)

[dfireBird]

If it errors because we sent null for deprecated

My bad for not clarifying, it errors on the null value of for_ref, which is supposed to have bool (according to r-a ext struct)

[Veykril]

I assume we send the field populated with either true or false though right? (otherwise this would be a bug in serde/serde_json which I somewhat doubt)

So that is still an lsp-mode bug, it has no business modifying the resolve data payload

[dfireBird]

So that is still an lsp-mode bug, it has no business modifying the resolve data payload

It's more so of a problem with falsehood¹ in ELisp, nil is the only way it can represent falsy values. For known boolean values, i.e value that is specified as boolean in spec, lsp-mode can interpret the said nil into false.

But for the "data", it can't make such an interpretation, since it's type is LSPAny: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionList

Arguing further on the topic, that our "data" here can be considered LSPObject, whose value is LSPAny and can be null. So for any data that has type LSPAny we should allow null

1: https://www.gnu.org/software/emacs/manual/html_node/eintr/Truth-_0026-Falsehood.html

[michaelpj]

declares the property as deprecated?: bool which means null, false and true are valid values

I think that's not true. The LSP spec is essentially Typescript type definitions, and I think an optional property has to be either present and with the correct type, or absent. Present and null isn't correct, I believe.

[Veykril]

Well maybe, but that doesn't matter here either way, the issue is that lspmode turns false into null, whether it turns it into null or omits the field instead doesn't matter as it should be preserving the resolve data as is.

[Veykril]

Either way, in this instance we should should just be lenient on the resolve data (in fact we can omit the for_ref field if its not true, have a missing one be implied false. But lsp-mode should really fix that.

The fix in that case is to just put a #[serde(default)] on the field definition I think