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

The code field in the Diagnostics structure is unexpectedly converted to a String #24084

Closed
Markiewic opened this issue Feb 1, 2025 · 4 comments
Labels
bug [core label]

Comments

@Markiewic
Copy link

Discussed in #24081

Originally posted by Markiewic February 1, 2025
I'm working on an Angular extension. I'm currently solving issue nathansbradshaw/zed-angular#16, which says that Angular Language Server's Code Actions don't work in Zed.

After checking, I found that Zed converts the code field to a string in messages with the "textDocument/codeAction" method, which is a problem for Angular, since it uses strict error code comparison when looking for Code Actions. Here's an example of the message Zed gets when asking for diagnostics, and here's what it sends later.

Examples

Pay attention to the path params.context.diagnostics[*].code.

Received from ALS:

{
  "jsonrpc": "2.0",
  "method": "textDocument/publishDiagnostics",
  "params": {
    "uri": "file:///Users/a.khodov/vscodeprojects/leak-test/src/app/app.component.html",
    "diagnostics": [
      {
        "range": {
          "start": { "line": 5, "character": 2 },
          "end": { "line": 5, "character": 22 }
        },
        "message": "'app-data-carrier' is not a known element:\n1. If 'app-data-carrier' is an Angular component, then verify that it is included in the '@Component.imports' of this component.\n2. If 'app-data-carrier' is a Web Component then add 'CUSTOM_ELEMENTS_SCHEMA' to the '@Component.schemas' of this component to suppress this message.",
        "severity": 1,
        "code": -998001,
        "source": "ngtsc",
        "relatedInformation": [
          {
            "location": {
              "uri": "file:///Users/a.khodov/vscodeprojects/leak-test/src/app/app.component.ts",
              "range": {
                "start": { "line": 8, "character": 24 },
                "end": { "line": 8, "character": 36 }
              }
            },
            "message": "Error occurs in the template of component AppComponent."
          }
        ]
      },
      {
        "range": {
          "start": { "line": 8, "character": 7 },
          "end": { "line": 8, "character": 32 }
        },
        "message": "In the two-way binding syntax the parentheses should be inside the brackets, ex. '[(ngModel)]=\"(someValue)\"'.\n        Find more at https://angular.dev/guide/templates/two-way-binding",
        "severity": 2,
        "code": -998101,
        "source": "ngtsc",
        "relatedInformation": [
          {
            "location": {
              "uri": "file:///Users/a.khodov/vscodeprojects/leak-test/src/app/app.component.ts",
              "range": {
                "start": { "line": 8, "character": 24 },
                "end": { "line": 8, "character": 36 }
              }
            },
            "message": "Error occurs in the template of component AppComponent."
          }
        ]
      }
    ]
  }
}

Sent to ALS:

{
  "jsonrpc": "2.0",
  "id": 86,
  "method": "textDocument/codeAction",
  "params": {
    "textDocument": {
      "uri": "file:///Users/a.khodov/vscodeprojects/leak-test/src/app/app.component.html"
    },
    "range": {
      "start": { "line": 5, "character": 22 },
      "end": { "line": 5, "character": 22 }
    },
    "context": {
      "diagnostics": [
        {
          "range": {
            "start": { "line": 5, "character": 2 },
            "end": { "line": 5, "character": 22 }
          },
          "severity": 1,
          "code": "-998001",
          "source": "ngtsc",
          "message": "'app-data-carrier' is not a known element:\n1. If 'app-data-carrier' is an Angular component, then verify that it is included in the '@Component.imports' of this component.\n2. If 'app-data-carrier' is a Web Component then add 'CUSTOM_ELEMENTS_SCHEMA' to the '@Component.schemas' of this component to suppress this message."
        }
      ],
      "only": ["quickfix"]
    }
  }
}

As said @SomeoneToIgnore, the conversion to String happens here:

let code = diagnostic.code.as_ref().map(|code| match code {
lsp::NumberOrString::Number(code) => code.to_string(),
lsp::NumberOrString::String(code) => code.clone(),
});

Compared to VS Code, it doesn't do that, it passes the code as it received it.

I intervened inside ALS and checked if this is really the reason - it turned out that it is (details in the attached issue).

I'm looking for a way to intervene in the RPC messages with an extension and convert the code back to a number. Or some other way to make sure the code's type doesn't change.

The LSP spec allows both a string and a number in this field. I believe that in this case, it is the language server that has the right to choose in what format it should accept and return numbers, and the editor should take into account either format.

@SomeoneToIgnore SomeoneToIgnore added the bug [core label] label Feb 2, 2025
@probably-neb
Copy link
Contributor

Hmm this is wierd. Just tried to use our LSP notification parsing on the json you provided and we correctly parsed the code as a number. Seems that somewhere in the middle we convert it to a string? Don't see why that makes any sense at all...

@Markiewic
Copy link
Author

Yes, Zed receives the code as a number, converts it to a string (I showed where exactly in the start message), and passes it to the language server as string.

This is important for the Angular language server, since it searches for suitable code actions based on the error code with strict comparison. All of the Angular error codes are stored as numbers.

Image

@mikayla-maki
Copy link
Contributor

mikayla-maki commented Feb 4, 2025

Reading the LSP spec, it states:

[Diagnostics] are provided so that the server knows which errors are currently presented to the user for the given range. There is no guarantee that these accurately reflect the error state of the resource.

Under a very strict reading of this, our behavior is acceptable.

That said, I think we should still do this. Passing the diagnostic code's type through faithfully stops this class of errors at the source.

@SomeoneToIgnore
Copy link
Contributor

Fixed in #24347

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug [core label]
Projects
None yet
Development

No branches or pull requests

4 participants