[llvm-branch-commits] [clang-tools-extra] fed9af2 - [clangd] Publish config file errors over LSP

Sam McCall via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Mon Dec 7 02:07:42 PST 2020


Author: Sam McCall
Date: 2020-12-07T11:07:32+01:00
New Revision: fed9af29c2b5f289744254390c7372e8871e45e5

URL: https://github.com/llvm/llvm-project/commit/fed9af29c2b5f289744254390c7372e8871e45e5
DIFF: https://github.com/llvm/llvm-project/commit/fed9af29c2b5f289744254390c7372e8871e45e5.diff

LOG: [clangd] Publish config file errors over LSP

We don't act as a language server for these files (e.g. don't get open/close
notifications for them), but just blindly publish diagnostics for them.

This works reasonably well in coc.nvim and vscode: they show up in the
workspace diagnostic list and when you open the file.
The only update after the file is reparsed, not as you type which is a bit
janky, but seems a lot better than nothing.

Fixes https://github.com/clangd/clangd/issues/614

Differential Revision: https://reviews.llvm.org/D92704

Added: 
    clang-tools-extra/clangd/test/config.test

Modified: 
    clang-tools-extra/clangd/ClangdServer.cpp
    clang-tools-extra/clangd/ClangdServer.h
    clang-tools-extra/clangd/ConfigProvider.h
    clang-tools-extra/clangd/ConfigYAML.cpp
    clang-tools-extra/clangd/Diagnostics.cpp
    clang-tools-extra/clangd/Diagnostics.h
    clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
    clang-tools-extra/clangd/unittests/ConfigTesting.h
    clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index aa19a9485e17..8b0d0591abe7 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -139,7 +139,7 @@ ClangdServer::Options::operator TUScheduler::Options() const {
 ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const ThreadsafeFS &TFS, const Options &Opts,
                            Callbacks *Callbacks)
-    : ConfigProvider(Opts.ConfigProvider), TFS(TFS),
+    : ConfigProvider(Opts.ConfigProvider), TFS(TFS), ServerCallbacks(Callbacks),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex,
                                      Opts.CollectMainFileRefs)
@@ -829,16 +829,44 @@ Context ClangdServer::createProcessingContext(PathRef File) const {
     Params.Path = PosixPath.str();
   }
 
-  auto DiagnosticHandler = [](const llvm::SMDiagnostic &Diag) {
-    if (Diag.getKind() == llvm::SourceMgr::DK_Error) {
-      elog("config error at {0}:{1}:{2}: {3}", Diag.getFilename(),
-           Diag.getLineNo(), Diag.getColumnNo(), Diag.getMessage());
-    } else {
-      log("config warning at {0}:{1}:{2}: {3}", Diag.getFilename(),
-          Diag.getLineNo(), Diag.getColumnNo(), Diag.getMessage());
+  llvm::StringMap<std::vector<Diag>> ReportableDiagnostics;
+  auto ConfigDiagnosticHandler = [&](const llvm::SMDiagnostic &D) {
+    // Ensure we create the map entry even for note diagnostics we don't report.
+    // This means that when the file is parsed with no warnings, we'll
+    // publish an empty set of diagnostics, clearing any the client has.
+    auto *Reportable = D.getFilename().empty()
+                           ? nullptr
+                           : &ReportableDiagnostics[D.getFilename()];
+    switch (D.getKind()) {
+    case llvm::SourceMgr::DK_Error:
+      elog("config error at {0}:{1}:{2}: {3}", D.getFilename(), D.getLineNo(),
+           D.getColumnNo(), D.getMessage());
+      if (Reportable)
+        Reportable->push_back(toDiag(D, Diag::ClangdConfig));
+      break;
+    case llvm::SourceMgr::DK_Warning:
+      log("config warning at {0}:{1}:{2}: {3}", D.getFilename(), D.getLineNo(),
+          D.getColumnNo(), D.getMessage());
+      if (Reportable)
+        Reportable->push_back(toDiag(D, Diag::ClangdConfig));
+      break;
+    case llvm::SourceMgr::DK_Note:
+    case llvm::SourceMgr::DK_Remark:
+      vlog("config note at {0}:{1}:{2}: {3}", D.getFilename(), D.getLineNo(),
+           D.getColumnNo(), D.getMessage());
+      break;
     }
   };
-  Config C = ConfigProvider->getConfig(Params, DiagnosticHandler);
+  Config C = ConfigProvider->getConfig(Params, ConfigDiagnosticHandler);
+  // Blindly publish diagnostics for the (unopened) parsed config files.
+  // We must avoid reporting diagnostics for *the same file* concurrently.
+  // Source file diags are published elsewhere, but those are 
diff erent files.
+  if (!ReportableDiagnostics.empty()) {
+    std::lock_guard<std::mutex> Lock(ConfigDiagnosticsMu);
+    for (auto &Entry : ReportableDiagnostics)
+      ServerCallbacks->onDiagnosticsReady(Entry.first(), /*Version=*/"",
+                                          std::move(Entry.second));
+  }
   return Context::current().derive(Config::Key, std::move(C));
 }
 

diff  --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 35ba4686cc9a..ff2fc8578103 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -363,6 +363,8 @@ class ClangdServer {
   config::Provider *ConfigProvider = nullptr;
 
   const ThreadsafeFS &TFS;
+  Callbacks *ServerCallbacks = nullptr;
+  mutable std::mutex ConfigDiagnosticsMu;
 
   Path ResourceDir;
   // The index used to look up symbols. This could be:

diff  --git a/clang-tools-extra/clangd/ConfigProvider.h b/clang-tools-extra/clangd/ConfigProvider.h
index 67ab833ee551..25d9450f28a7 100644
--- a/clang-tools-extra/clangd/ConfigProvider.h
+++ b/clang-tools-extra/clangd/ConfigProvider.h
@@ -46,6 +46,8 @@ struct Params {
 /// Used to report problems in parsing or interpreting a config.
 /// Errors reflect structurally invalid config that should be user-visible.
 /// Warnings reflect e.g. unknown properties that are recoverable.
+/// Notes are used to report files and fragments.
+/// (This can be used to track when previous warnings/errors have been "fixed").
 using DiagnosticCallback = llvm::function_ref<void(const llvm::SMDiagnostic &)>;
 
 /// A chunk of configuration that has been fully analyzed and is ready to apply.

diff  --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index a3bb1eb29adb..022d890a9687 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -273,10 +273,16 @@ std::vector<Fragment> Fragment::parseYAML(llvm::StringRef YAML,
       Fragment Fragment;
       Fragment.Source.Manager = SM;
       Fragment.Source.Location = N->getSourceRange().Start;
+      SM->PrintMessage(Fragment.Source.Location, llvm::SourceMgr::DK_Note,
+                       "Parsing config fragment");
       if (Parser(*SM).parse(Fragment, *N))
         Result.push_back(std::move(Fragment));
     }
   }
+  SM->PrintMessage(SM->FindLocForLineAndColumn(SM->getMainFileID(), 0, 0),
+                   llvm::SourceMgr::DK_Note,
+                   "Parsed " + llvm::Twine(Result.size()) +
+                       " fragments from file");
   // Hack: stash the buffer in the SourceMgr to keep it alive.
   // SM has two entries: "main" non-owning buffer, and ignored owning buffer.
   SM->AddNewSourceBuffer(std::move(Buf), llvm::SMLoc());

diff  --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index db4e2e61b86c..7b59cedb0f67 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -377,6 +377,32 @@ CodeAction toCodeAction(const Fix &F, const URIForFile &File) {
   return Action;
 }
 
+Diag toDiag(const llvm::SMDiagnostic &D, Diag::DiagSource Source) {
+  Diag Result;
+  Result.Message = D.getMessage().str();
+  switch (D.getKind()) {
+  case llvm::SourceMgr::DK_Error:
+    Result.Severity = DiagnosticsEngine::Error;
+    break;
+  case llvm::SourceMgr::DK_Warning:
+    Result.Severity = DiagnosticsEngine::Warning;
+    break;
+  default:
+    break;
+  }
+  Result.Source = Source;
+  Result.AbsFile = D.getFilename().str();
+  Result.InsideMainFile = D.getSourceMgr()->FindBufferContainingLoc(
+                              D.getLoc()) == D.getSourceMgr()->getMainFileID();
+  if (D.getRanges().empty())
+    Result.Range = {{D.getLineNo() - 1, D.getColumnNo()},
+                    {D.getLineNo() - 1, D.getColumnNo()}};
+  else
+    Result.Range = {{D.getLineNo() - 1, (int)D.getRanges().front().first},
+                    {D.getLineNo() - 1, (int)D.getRanges().front().second}};
+  return Result;
+}
+
 void toLSPDiags(
     const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
     llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
@@ -404,6 +430,9 @@ void toLSPDiags(
   case Diag::ClangTidy:
     Main.source = "clang-tidy";
     break;
+  case Diag::ClangdConfig:
+    Main.source = "clangd-config";
+    break;
   case Diag::Unknown:
     break;
   }

diff  --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index 4a00abfc450f..e1c3bcafcbf7 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -20,6 +20,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Support/SourceMgr.h"
 #include <cassert>
 #include <string>
 
@@ -86,10 +87,11 @@ struct Note : DiagBase {};
 struct Diag : DiagBase {
   std::string Name; // if ID was recognized.
   // The source of this diagnostic.
-  enum {
+  enum DiagSource {
     Unknown,
     Clang,
     ClangTidy,
+    ClangdConfig,
   } Source = Unknown;
   /// Elaborate on the problem, usually pointing to a related piece of code.
   std::vector<Note> Notes;
@@ -98,6 +100,8 @@ struct Diag : DiagBase {
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D);
 
+Diag toDiag(const llvm::SMDiagnostic &, Diag::DiagSource Source);
+
 /// Conversion to LSP diagnostics. Formats the error message of each diagnostic
 /// to include all its notes. Notes inside main file are also provided as
 /// separate diagnostics with their corresponding fixits. Notes outside main

diff  --git a/clang-tools-extra/clangd/test/config.test b/clang-tools-extra/clangd/test/config.test
new file mode 100644
index 000000000000..eb556feefd95
--- /dev/null
+++ b/clang-tools-extra/clangd/test/config.test
@@ -0,0 +1,66 @@
+# We specify a custom path in XDG_CONFIG_HOME, which only works on some systems.
+# UNSUPPORTED: system-windows
+# UNSUPPORTED: system-darwin
+
+# RUN: rm -rf %t
+# RUN: mkdir -p %t/clangd
+
+# Create a config file that injects a flag we can observe, and has an error.
+# RUN: echo 'Foo: bar' > %t/clangd/config.yaml
+# RUN: echo 'CompileFlags: {Add: -DFOO=BAR}' >> %t/clangd/config.yaml
+
+# RUN: env XDG_CONFIG_HOME=%t clangd -lit-test -enable-config < %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","text":"int x = FOO;"}}}
+# First, the errors from the config file.
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "message": "Unknown Config key Foo",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 3,
+# 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:        "source": "clangd-config"
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ],
+# CHECK-NEXT:    "uri": "file://{{.*}}/config.yaml"
+# CHECK-NEXT:  }
+# Next, the error from the main file. BAR rather than FOO means we used config.
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "code": "undeclared_var_use",
+# CHECK-NEXT:        "message": "Use of undeclared identifier 'BAR'",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 11,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": 8,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "severity": 1,
+# CHECK-NEXT:        "source": "clang"
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ],
+# CHECK-NEXT:    "uri": "file://{{.*}}/foo.c",
+# CHECK-NEXT:    "version": 0
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}

diff  --git a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
index d6af5856cc49..b2217bbc55da 100644
--- a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -96,21 +96,26 @@ TEST(ProviderTest, FromYAMLFile) {
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
               ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
   FS.Files["foo.yaml"] = AddBarBaz;
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+  Diags.clear();
 
   FS.Files.erase("foo.yaml");
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Missing file is not an error";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 }
 
@@ -133,21 +138,26 @@ TEST(ProviderTest, FromAncestorRelativeYAMLFiles) {
 
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
               ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(Diags.Files,
+              ElementsAre(testPath("a/foo.yaml"), testPath("a/b/c/foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo", "bar", "baz"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   Cfg = P->getConfig(AParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached config";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
   FS.Files.erase("a/foo.yaml");
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
 }
 
@@ -169,7 +179,7 @@ TEST(ProviderTest, Staleness) {
   EXPECT_THAT(Diags.Diagnostics,
               ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   // Stale value reused by policy.
   FS.Files["foo.yaml"] = AddBarBaz;
@@ -211,14 +221,14 @@ TEST(ProviderTest, SourceInfo) {
   auto Cfg = P->getConfig(Bar, Diags.callback());
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   // This should be a relative match/exclude hence baz/bar.h should be excluded.
   P = Provider::fromAncestorRelativeYAMLFiles("foo.yaml", FS);
   Cfg = P->getConfig(Bar, Diags.callback());
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
-  Diags.Diagnostics.clear();
+  Diags.clear();
 }
 } // namespace
 } // namespace config

diff  --git a/clang-tools-extra/clangd/unittests/ConfigTesting.h b/clang-tools-extra/clangd/unittests/ConfigTesting.h
index 5541c6fffe59..46c51e80b6bc 100644
--- a/clang-tools-extra/clangd/unittests/ConfigTesting.h
+++ b/clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -24,6 +24,12 @@ namespace config {
 struct CapturedDiags {
   std::function<void(const llvm::SMDiagnostic &)> callback() {
     return [this](const llvm::SMDiagnostic &D) {
+      if (Files.empty() || Files.back() != D.getFilename())
+        Files.push_back(D.getFilename().str());
+
+      if (D.getKind() > llvm::SourceMgr::DK_Warning)
+        return;
+
       Diagnostics.emplace_back();
       Diag &Out = Diagnostics.back();
       Out.Message = D.getMessage().str();
@@ -50,7 +56,13 @@ struct CapturedDiags {
           << D.Message << "@" << llvm::to_string(D.Pos);
     }
   };
-  std::vector<Diag> Diagnostics;
+  std::vector<Diag> Diagnostics;  // Warning or higher.
+  std::vector<std::string> Files; // Filename from diagnostics including notes.
+
+  void clear() {
+    Diagnostics.clear();
+    Files.clear();
+  }
 };
 
 MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }

diff  --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
index 1da0f3a1cc71..54ef7d1d85cd 100644
--- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -67,6 +67,7 @@ CompileFlags: { Add: [foo, bar] }
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, ElementsAre("config.yaml"));
   ASSERT_EQ(Results.size(), 4u);
   EXPECT_FALSE(Results[0].If.HasUnrecognizedCondition);
   EXPECT_THAT(Results[0].If.PathMatch, ElementsAre(Val("abc")));


        


More information about the llvm-branch-commits mailing list