diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/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: diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/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,40 @@ 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> 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 = 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()); + 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()); + 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 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/ConfigProvider.h b/clang-tools-extra/clangd/ConfigProvider.h --- a/clang-tools-extra/clangd/ConfigProvider.h +++ b/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; /// 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 --- a/clang-tools-extra/clangd/ConfigYAML.cpp +++ b/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()); diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h --- 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 #include @@ -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 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 diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/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)> 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; } diff --git a/clang-tools-extra/clangd/test/config.test b/clang-tools-extra/clangd/test/config.test new file mode 100644 --- /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 --- a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp +++ b/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 diff --git a/clang-tools-extra/clangd/unittests/ConfigTesting.h b/clang-tools-extra/clangd/unittests/ConfigTesting.h --- a/clang-tools-extra/clangd/unittests/ConfigTesting.h +++ b/clang-tools-extra/clangd/unittests/ConfigTesting.h @@ -24,6 +24,12 @@ struct CapturedDiags { std::function 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 Diagnostics; + std::vector Diagnostics; // Warning or higher. + std::vector 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 --- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp +++ b/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")));