[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 ↗

(On Diff #188112)

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

clangd/Diagnostics.h

72 ↗

(On Diff #188112)

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

[ilya-biryukov]

clangd/ClangdUnit.cpp

380 ↗

(On Diff #188112)

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 ↗

(On Diff #188112)

+1, why do we need ID?

74 ↗

(On Diff #188112)

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 ↗

(On Diff #188112)

good catch, I missed this.

clangd/Diagnostics.cpp

381 ↗

(On Diff #188112)

See my comment above.

clangd/Diagnostics.h

72 ↗

(On Diff #188112)

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)

clang-tools-extra/trunk/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.S =
+        !CTContext->getCheckName(D.ID).empty() ? Diag::ClangTidy : Diag::Clang;
   // 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));
 }
 
buildPreamble(PathRef FileName, CompilerInvocation &CI,
 
   // When building the AST for the main file, we do want the function
   // bodies.
   CI.getFrontendOpts().SkipFunctionBodies = false;
 
   if (BuiltPreamble) {
     vlog("Built preamble of size {0} for file {1}", BuiltPreamble->getSize(),
          FileName);
+    std::vector<Diag> Diags = PreambleDiagnostics.take();
+    for (auto &Diag : Diags)
+      Diag.S = Diag::Clang;
     return std::make_shared<PreambleData>(
-        std::move(*BuiltPreamble), PreambleDiagnostics.take(),
+        std::move(*BuiltPreamble), std::move(Diags),
         SerializedDeclsCollector.takeIncludes(), std::move(StatCache),
         SerializedDeclsCollector.takeCanonicalIncludes());
   } else {
     elog("Could not build a preamble for file {0}", FileName);
     return nullptr;
   }
 }
 

clang-tools-extra/trunk/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.
+  enum Source {
+    Clang,
+    ClangTidy,
+  };
+  Source S = Clang;
   /// 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

clang-tools-extra/trunk/clangd/Diagnostics.cpp

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());

clang-tools-extra/trunk/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.S == 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(Diag::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"),
TEST(DiagnosticsTest, FlagsMatter) {
   TU.Filename = "Plain.c";
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       ElementsAre(AllOf(
           Diag(Test.range(), "return type of 'main' is not 'int'"),
           WithFix(Fix(Test.range(), "int", "change return type to 'int'")))));
 }
 
+TEST(DiagnosticsTest, DiagnosticPreamble) {
+  Annotations Test(R"cpp(
+    #include $[["not-found.h"]]
+  )cpp");
+
+  auto TU = TestTU::withCode(Test.code());
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              ElementsAre(testing::AllOf(
+                  Diag(Test.range(), "'not-found.h' file not found"),
+                  DiagSource(Diag::Clang))));
+}
+
 TEST(DiagnosticsTest, ClangTidy) {
   Annotations Test(R"cpp(
     #include $deprecated[["assert.h"]]
 
     #define $macrodef[[SQUARE]](X) (X)*(X)
     int main() {
       return $doubled[[sizeof]](sizeof(int));
       int y = 4;
       return SQUARE($macroarg[[++]]y);
     }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   TU.HeaderFilename = "assert.h"; // Suppress "not found" error.
   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(Diag::ClangTidy),
                 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(Diag::ClangTidy),
               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) {