[clangd] Emit source to Diagnostic.

[hokein]

Summary

Set "clang", "clang-tidy" as the source to the diagnostics, so that
client can distinguish diagnostics from clang-tidy.

[jkorous]

clangd/Diagnostics.cpp

381

Nit - is this really intended to be part of this patch?

clangd/Diagnostics.h

72

Nit - is this really intended to be part of this patch?

[ilya-biryukov]

clangd/ClangdUnit.cpp

380

Preamble diagnostics seem to be missing the source.
Could we add a test they also have the "source" set? "unresolved include" should be the easiest to get.

clangd/Diagnostics.h

72

+1, why do we need ID?

74

Let's introduce an enum with two values here and convert to string at the LSP boundaries.

[hokein]

Address comments.

[hokein]

add @kadircet as a reviewer, since @ilya-biryukov is OOO.

clangd/ClangdUnit.cpp

380

good catch, I missed this.

clangd/Diagnostics.cpp

381

See my comment above.

clangd/Diagnostics.h

72

Yes, we do need this ID to determine whether a diagnostic is from clang-tidy, CangTidyContext::getCheckName.

[kadircet]

LGTM

[hokein]

Don't emit the source to LSP level (as this is a UI change as least in vscode)

clangd/ClangdUnit.cpp

ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
   // CompilerInstance won't run this callback, do it directly.
   ASTDiags.EndSourceFile();
   // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF.
   // However Action->EndSourceFile() would destroy the ASTContext!
   // So just inform the preprocessor of EOF, while keeping everything alive.
   Clang->getPreprocessor().EndSourceFile();
 
   std::vector<Diag> Diags = ASTDiags.take();
+  // Populate diagnostic source.
+  for (auto& D : Diags)
+    D.Source = CTContext->getCheckName(D.ID).empty() ? "clang" : "clang-tidy";
   // Add diagnostics from the preamble, if any.
   if (Preamble)
     Diags.insert(Diags.begin(), Preamble->Diags.begin(), Preamble->Diags.end());
   return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
                    std::move(ParsedDecls), std::move(Diags),
                    std::move(Includes), std::move(CanonIncludes));
 }
 
 ParsedAST::ParsedAST(ParsedAST &&Other) = default;
 
 ParsedAST &ParsedAST::operator=(ParsedAST &&Other) = default;

clangd/Diagnostics.h

 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Fix &F);
 
 /// Represents a note for the diagnostic. Severity of notes can only be 'note'
 /// or 'remark'.
 struct Note : DiagBase {};
 
 /// A top-level diagnostic that may have Notes and Fixes.
 struct Diag : DiagBase {
+  // Diagnostic enum ID.
+  unsigned ID;
+  // The source of this diagnostic, e.g. 'clang', 'clang-tidy'.
+  std::string Source;
   /// Elaborate on the problem, usually pointing to a related piece of code.
   std::vector<Note> Notes;
   /// *Alternative* fixes for this diagnostic, one should be chosen.
   std::vector<Fix> Fixes;
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D);
 
 /// Conversion to LSP diagnostics. Formats the error message of each diagnostic

clangd/Diagnostics.cpp

auto FillBasicFields = [](const DiagBase &D) -> clangd::Diagnostic {
     Res.range = D.Range;
     Res.severity = getSeverity(D.Severity);
     return Res;
   };
 
   {
     clangd::Diagnostic Main = FillBasicFields(D);
     Main.message = mainMessage(D, Opts.DisplayFixesCount);
+    Main.source = D.Source;
     if (Opts.EmbedFixesInDiagnostics) {
       Main.codeActions.emplace();
       for (const auto &Fix : D.Fixes)
         Main.codeActions->push_back(toCodeAction(Fix, File));
     }
     if (Opts.SendDiagnosticCategory && !D.Category.empty())
       Main.category = D.Category;
 
auto AddFix = [&](bool SyntheticMessage) -> bool {
     return true;
   };
 
   if (!isNote(DiagLevel)) {
     // Handle the new main diagnostic.
     flushLastDiag();
 
     LastDiag = Diag();
+    LastDiag->ID = Info.getID();
     FillDiagBase(*LastDiag);
 
     if (!Info.getFixItHints().empty())
       AddFix(true /* try to invent a message instead of repeating the diag */);
     if (Fixer) {
       auto ExtraFixes = Fixer(DiagLevel, Info);
       LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(),
                              ExtraFixes.end());

clangd/Protocol.h

struct Diagnostic {
   int severity = 0;
 
   /// The diagnostic's code. Can be omitted.
   /// Note: Not currently used by clangd
   // std::string code;
 
   /// 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;
+  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. The category typically describes the compilation stage during
   /// which the issue was produced, e.g. "Semantic Issue" or "Parse Issue".

clangd/Protocol.cpp

llvm::json::Object Diag{
       {"range", D.range},
       {"severity", D.severity},
       {"message", D.message},
   };
   if (D.category)
     Diag["category"] = *D.category;
   if (D.codeActions)
     Diag["codeActions"] = D.codeActions;
+  if (!D.source.empty())
+    Diag["source"] = D.source;
   return std::move(Diag);
 }
 
 bool fromJSON(const llvm::json::Value &Params, Diagnostic &R) {
   llvm::json::ObjectMapper O(Params);
   if (!O || !O.map("range", R.range) || !O.map("message", R.message))
     return false;
   O.map("severity", R.severity);
   O.map("category", R.category);
+  O.map("source", R.source);
   return true;
 }
 
 bool fromJSON(const llvm::json::Value &Params, CodeActionContext &R) {
   llvm::json::ObjectMapper O(Params);
   return O && O.map("diagnostics", R.diagnostics);
 }
 

test/clangd/diagnostic-category.test

 # CHECK-NEXT:            "character": 22,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 17,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
-# CHECK-NEXT:        "severity": 1
+# CHECK-NEXT:        "severity": 1,
+# CHECK-NEXT:        "source": "clang"
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "message": "Previous use is here\n\nfoo.c:1:18: error: use of 'Point' with tag type that does not match previous declaration",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 12,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },

test/clangd/diagnostics.test

 # CHECK-NEXT:            "character": 4,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 0,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
-# CHECK-NEXT:        "severity": 2
+# CHECK-NEXT:        "severity": 2,
+# CHECK-NEXT:        "source": "clang"
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

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

 # 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": 1
+# CHECK-NEXT:        "severity": 1,
+# CHECK-NEXT:        "source": "clang"
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 #
 # ERR: Updating file {{.*}}foo.c with command [{{.*}}clangd-test2] clang -c foo.c -Wall -Werror
 # Don't reparse the second file:
 # ERR: Skipping rebuild of the AST for {{.*}}bar.c
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}
 
 

test/clangd/execute-command.test

 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
-# CHECK-NEXT:        "severity": 2
+# CHECK-NEXT:        "severity": 2,
+# CHECK-NEXT:        "source": "clang"
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
 # No command name
 {"jsonrpc":"2.0","id":3,"method":"workspace/executeCommand","params":{}}
 ---

test/clangd/fixits-codeaction.test

 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
-# CHECK-NEXT:        "severity": 2
+# CHECK-NEXT:        "severity": 2,
+# CHECK-NEXT:        "source": "clang"
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)"}]}}}
 #      CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",

test/clangd/fixits-command.test

 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
-# CHECK-NEXT:        "severity": 2
+# CHECK-NEXT:        "severity": 2,
+# CHECK-NEXT:        "source": "clang"
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)"}]}}}
 #      CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",

test/clangd/fixits-embed-in-diagnostic.test

 # CHECK-NEXT:            "character": 22,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 17,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
-# CHECK-NEXT:        "severity": 1
+# CHECK-NEXT:        "severity": 1,
+# CHECK-NEXT:        "source": "clang"
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "message": "Previous use is here\n\nfoo.c:1:18: error: use of 'Point' with tag type that does not match previous declaration",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 12,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },

unittests/clangd/DiagnosticsTests.cpp

 }
 
 MATCHER_P(EqualToLSPDiag, LSPDiag,
           "LSP diagnostic " + llvm::to_string(LSPDiag)) {
   return std::tie(arg.range, arg.severity, arg.message) ==
          std::tie(LSPDiag.range, LSPDiag.severity, LSPDiag.message);
 }
 
+MATCHER_P(DiagSource, Source, "") {
+  return arg.Source == Source;
+}
+
 MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) {
   if (arg.Message != Fix.Message)
     return false;
   if (arg.Edits.size() != Fix.Edits.size())
     return false;
   for (std::size_t I = 0; I < arg.Edits.size(); ++I) {
     if (arg.Edits[I].range != Fix.Edits[I].range ||
         arg.Edits[I].newText != Fix.Edits[I].newText)
o]]();
     }
   )cpp");
   EXPECT_THAT(
       TestTU::withCode(Test.code()).build().getDiagnostics(),
       ElementsAre(
           // This range spans lines.
           AllOf(Diag(Test.range("typo"),
                      "use of undeclared identifier 'goo'; did you mean 'foo'?"),
+                DiagSource("clang"),
                 WithFix(
                     Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
                 // This is a pretty normal range.
                 WithNote(Diag(Test.range("decl"), "'foo' declared here"))),
           // This range is zero-width and insertion. Therefore make sure we are
           // not expanding it into other tokens. Since we are not going to
           // replace those.
           AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"),
TU.ClangTidyChecks =
       "-*, bugprone-sizeof-expression, bugprone-macro-repeated-side-effects, "
       "modernize-deprecated-headers";
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       UnorderedElementsAre(
           AllOf(Diag(Test.range("deprecated"),
                      "inclusion of deprecated C++ header 'assert.h'; consider "
                      "using 'cassert' instead [modernize-deprecated-headers]"),
+                DiagSource("clang-tidy"),
                 WithFix(Fix(Test.range("deprecated"), "<cassert>",
                             "change '\"assert.h\"' to '<cassert>'"))),
           Diag(Test.range("doubled"),
                "suspicious usage of 'sizeof(sizeof(...))' "
                "[bugprone-sizeof-expression]"),
           AllOf(
               Diag(Test.range("macroarg"),
                    "side effects in the 1st macro argument 'X' are repeated in "
                    "macro expansion [bugprone-macro-repeated-side-effects]"),
+              DiagSource("clang-tidy"),
               WithNote(Diag(Test.range("macrodef"),
                             "macro 'SQUARE' defined here "
                             "[bugprone-macro-repeated-side-effects]"))),
           Diag(Test.range("macroarg"),
                "multiple unsequenced modifications to 'y'")));
 }
 
 TEST(DiagnosticsTest, Preprocessor) {