[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