[clangd] add an extension field to LSP to transfer the diagnostic's category

[arphaman]

Summary

This patch adds a 'category' extension field to the LSP diagnostic that's sent by Clangd. This extension is always on by default.

[ilya-biryukov]

clangd/Diagnostics.h

40

Maybe store the string name of the category directly here? This struct was never optimized for efficiency, but aims to be independent of clang, so that the clients might interpret it without calling any clang-specific code (we do reuse the Level enum, though, but that's merely for code reuse)

clangd/Protocol.h

549

Maybe add a brief example of category names here, i.e. "semantic issue", etc.?

[arphaman]

Address review comments.

[ilya-biryukov]

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

[joaotavora]

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

This isn't in the spec and broke the LSP client eglot (https://github.com/joaotavora/eglot/pull/81). Why don't you put this in the "source" field, or concat it to the "message" field. Who can even use this information if it's not in the spec? Are clients supposed to code against every LSP server's whim?

[arphaman]

In D50571#1205650, @joaotavora wrote:

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

This isn't in the spec and broke the LSP client eglot (https://github.com/joaotavora/eglot/pull/81). Why don't you put this in the "source" field, or concat it to the "message" field. Who can even use this information if it's not in the spec? Are clients supposed to code against every LSP server's whim?

Thanks for the feedback. I'll make a patch that turns this off by default so that clients can opt-in into it.

[joaotavora]

In D50571#1206020, @arphaman wrote:

In D50571#1205650, @joaotavora wrote:

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

This isn't in the spec and broke the LSP client eglot (https://github.com/joaotavora/eglot/pull/81). Why don't you put this in the "source" field, or concat it to the "message" field. Who can even use this information if it's not in the spec? Are clients supposed to code against every LSP server's whim?

Thanks for the feedback. I'll make a patch that turns this off by default so that clients can opt-in into it.

Thank you very much, and sorry if I came across a bit hostile. What is this category field good for?

[arphaman]

In D50571#1208635, @joaotavora wrote:

In D50571#1206020, @arphaman wrote:

In D50571#1205650, @joaotavora wrote:

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

This isn't in the spec and broke the LSP client eglot (https://github.com/joaotavora/eglot/pull/81). Why don't you put this in the "source" field, or concat it to the "message" field. Who can even use this information if it's not in the spec? Are clients supposed to code against every LSP server's whim?

Thanks for the feedback. I'll make a patch that turns this off by default so that clients can opt-in into it.

Thank you very much, and sorry if I came across a bit hostile. What is this category field good for?

NP! Fixed in r340449.

clangd/ClangdLSPServer.cpp

toLSPDiags(Diag, [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
           URIForFile URI{File};
           WE.changes = {{URI.uri(), std::vector<TextEdit>(Fix.Edits.begin(),
                                                           Fix.Edits.end())}};
           ClangdFixes.push_back(
               json::Object{{"edit", toJSON(WE)}, {"title", Fix.Message}});
         }
         LSPDiag["clangd_fixes"] = std::move(ClangdFixes);
       }
+      if (!Diag.category.empty())
+        LSPDiag["category"] = Diag.category;
       DiagnosticsJSON.push_back(std::move(LSPDiag));
 
       auto &FixItsForDiagnostic = LocalFixIts[Diag];
       std::copy(Fixes.begin(), Fixes.end(),
                 std::back_inserter(FixItsForDiagnostic));
     });
   }
 

clangd/Diagnostics.h

 /// Contains basic information about a diagnostic.
 struct DiagBase {
   std::string Message;
   // Intended to be used only in error messages.
   // May be relative, absolute or even artifically constructed.
   std::string File;
   clangd::Range Range;
   DiagnosticsEngine::Level Severity = DiagnosticsEngine::Note;
+  unsigned Category;
   // Since File is only descriptive, we store a separate flag to distinguish
   // diags from the main file.
   bool InsideMainFile = false;
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D);
 
 /// Represents a single fix-it that editor can apply to fix the error.
 struct Fix {

clangd/Diagnostics.cpp

 
 void toLSPDiags(
     const Diag &D,
     llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
   auto FillBasicFields = [](const DiagBase &D) -> clangd::Diagnostic {
     clangd::Diagnostic Res;
     Res.range = D.Range;
     Res.severity = getSeverity(D.Severity);
+    if (D.Category)
+      Res.category = DiagnosticIDs::getCategoryNameFromID(D.Category).str();
     return Res;
   };
 
   {
     clangd::Diagnostic Main = FillBasicFields(D);
     Main.message = mainMessage(D);
     OutFn(std::move(Main), D.Fixes);
   }
void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   auto FillDiagBase = [&](DiagBase &D) {
     D.Range = diagnosticRange(Info, *LangOpts);
     llvm::SmallString<64> Message;
     Info.FormatDiagnostic(Message);
     D.Message = Message.str();
     D.InsideMainFile = InsideMainFile;
     D.File = Info.getSourceManager().getFilename(Info.getLocation());
     D.Severity = DiagLevel;
+    D.Category = DiagnosticIDs::getCategoryNumberForDiag(Info.getID());
     return D;
   };
 
   auto AddFix = [&](bool SyntheticMessage) -> bool {
     assert(!Info.getFixItHints().empty() &&
            "diagnostic does not have attached fix-its");
     if (!InsideMainFile)
       return false;

clangd/Protocol.h

struct Diagnostic {
 
   /// A human-readable string describing the source of this
   /// diagnostic, e.g. 'typescript' or 'super lint'.
   /// Note: Not currently used by clangd
   // std::string source;
 
   /// The diagnostic's message.
   std::string message;
+
+  /// The diagnostic's category. Can be omitted.
+  /// An LSP extension that's used to send the name of the category over to the
+  /// client.
+  std::string category;
 };
 
 /// A LSP-specific comparator used to find diagnostic in a container like
 /// std:map.
 /// We only use the required fields of Diagnostic to do the comparsion to avoid
 /// any regression issues from LSP clients (e.g. VScode), see
 /// https://git.io/vbr29
 struct LSPDiagnosticCompare {

test/clangd/compile-commands-path-in-initialize.test

 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [
 # CHECK-NEXT:       {
+# CHECK-NEXT:         "category": "#pragma message Directive",
 # CHECK-NEXT:         "message": "MACRO is one",
 ---
 {"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [
 # CHECK-NEXT:       {
+# CHECK-NEXT:         "category": "#pragma message Directive",
 # CHECK-NEXT:         "message": "MACRO is two",
 ---
 {"jsonrpc":"2.0","id":10000,"method":"shutdown"}

test/clangd/compile-commands-path.test

 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [
 # CHECK-NEXT:       {
+# CHECK-NEXT:         "category": "#pragma message Directive",
 # CHECK-NEXT:         "message": "MACRO is not defined",
 ---
 {"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [
 # CHECK-NEXT:       {
+# CHECK-NEXT:         "category": "#pragma message Directive",
 # CHECK-NEXT:         "message": "MACRO is one",
 ---
 {"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [
 # CHECK-NEXT:       {
+# CHECK-NEXT:         "category": "#pragma message Directive",
 # CHECK-NEXT:         "message": "MACRO is two",
 ---
 {"jsonrpc":"2.0","id":10000,"method":"shutdown"}

test/clangd/diagnostics.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"void main() {}"}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
+# CHECK-NEXT:        "category": "Semantic Issue",
 # CHECK-NEXT:        "message": "Return type of 'main' is not 'int'",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 4,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 0,

test/clangd/did-change-configuration-params.test

 # CHECK-NEXT:    "uri": "file://{{.*}}/bar.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c": {"workingDirectory":"/clangd-test2", "compilationCommand": ["clang", "-c", "foo.c", "-Wall", "-Werror"]}}}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
+# CHECK-NEXT:        "category": "Semantic Issue",
 # CHECK-NEXT:        "message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 28,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,

test/clangd/execute-command.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
+# CHECK-NEXT:        "category": "Semantic Issue",
 # CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,

test/clangd/extra-flags.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main() { int i; return i; }"},"metadata":{"extraFlags":["-Wall"]}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
+# CHECK-NEXT:        "category": "Semantic Issue",
 # CHECK-NEXT:        "message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 28,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.c","version":2},"contentChanges":[{"text":"int main() { int i; return i+1; }"}]}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
+# CHECK-NEXT:        "category": "Semantic Issue",
 # CHECK-NEXT:        "message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 28,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,

test/clangd/fixits.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
 #      CHECK:    "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
+# CHECK-NEXT:        "category": "Semantic Issue",
 # CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,