sammccall updated this revision to Diff 309688.
sammccall added a comment.
Handle diagnostics without a filename
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92704/new/
https://reviews.llvm.org/D92704
Files:
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/test/config.test
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
clang-tools-extra/clangd/unittests/ConfigTesting.h
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -67,6 +67,7 @@
)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")));
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigTesting.h
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -24,6 +24,12 @@
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 @@
<< 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; }
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -96,21 +96,26 @@
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 @@
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 @@
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 @@
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
Index: clang-tools-extra/clangd/test/config.test
===================================================================
--- /dev/null
+++ 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"}
Index: clang-tools-extra/clangd/Diagnostics.h
===================================================================
--- clang-tools-extra/clangd/Diagnostics.h
+++ 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 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 @@
};
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
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -377,6 +377,32 @@
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 @@
case Diag::ClangTidy:
Main.source = "clang-tidy";
break;
+ case Diag::ClangdConfig:
+ Main.source = "clangd-config";
+ break;
case Diag::Unknown:
break;
}
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -273,10 +273,16 @@
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());
Index: clang-tools-extra/clangd/ConfigProvider.h
===================================================================
--- clang-tools-extra/clangd/ConfigProvider.h
+++ clang-tools-extra/clangd/ConfigProvider.h
@@ -46,6 +46,8 @@
/// 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.
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -363,6 +363,8 @@
config::Provider *ConfigProvider = nullptr;
const ThreadsafeFS &TFS;
+ Callbacks *ServerCallbacks;
+ mutable std::mutex ConfigDiagnosticsMu;
Path ResourceDir;
// The index used to look up symbols. This could be:
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -139,7 +139,7 @@
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 @@
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 DiagnosticHandler = [&](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);
+ // 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 different 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));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
