hokein updated this revision to Diff 541389.
hokein marked 2 inline comments as done.
hokein added a comment.
add LSPServer cache back and repurpose for lsp <=> naive diagnostic mapping,
per offline discussion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155173/new/
https://reviews.llvm.org/D155173
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.h
Index: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -972,6 +972,7 @@
return std::tie(LHS.range, LHS.message) < std::tie(RHS.range, RHS.message);
}
};
+
bool fromJSON(const llvm::json::Value &, Diagnostic &, llvm::json::Path);
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Diagnostic &);
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -37,8 +37,7 @@
#include <memory>
#include <optional>
#include <string>
-#include <type_traits>
-#include <utility>
+#include <tuple>
#include <vector>
namespace clang {
@@ -351,8 +350,49 @@
std::string Title; /// A single-line message to show in the UI.
llvm::StringLiteral Kind;
};
+
+ // Ref to the clangd::Diag.
+ struct DiagRef {
+ Range Range;
+ std::string Message;
+ bool operator==(const DiagRef &Other) const {
+ return std::tie(Range, Message) == std::tie(Other.Range, Other.Message);
+ }
+ bool operator<(const DiagRef &Other) const {
+ return std::tie(Range, Message) < std::tie(Other.Range, Other.Message);
+ }
+ };
+
+ struct CodeActionInputs {
+ std::string File;
+ Range Selection;
+
+ /// Requested kind of actions to return.
+ std::vector<std::string> RequestedActionKinds;
+
+ /// Diagnostics attached to the code action request.
+ std::vector<DiagRef> Diagnostics;
+
+ /// Tweaks where Filter returns false will not be checked or included.
+ std::function<bool(const Tweak &)> TweakFilter;
+ };
+ struct CodeActionResult {
+ std::string Version;
+ struct QuickFix {
+ DiagRef Diag;
+ Fix F;
+ };
+ std::vector<QuickFix> QuickFixes;
+ std::vector<TweakRef> TweakRefs;
+ };
+ /// Surface code actions (quick-fixes for diagnostics, or available code
+ /// tweaks) for a given range in a file.
+ void codeAction(const CodeActionInputs &Inputs,
+ Callback<CodeActionResult> CB);
+
/// Enumerate the code tweaks available to the user at a specified point.
/// Tweaks where Filter returns false will not be checked or included.
+ /// Deprecated, use codeAction instead.
void enumerateTweaks(PathRef File, Range Sel,
llvm::unique_function<bool(const Tweak &)> Filter,
Callback<std::vector<TweakRef>> CB);
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -52,11 +52,16 @@
#include <optional>
#include <string>
#include <type_traits>
+#include <vector>
namespace clang {
namespace clangd {
namespace {
+// Tracks number of times a tweak has been offered.
+static constexpr trace::Metric TweakAvailable(
+ "tweak_available", trace::Metric::Counter, "tweak_id");
+
// Update the FileIndex with new ASTs and plumb the diagnostics responses.
struct UpdateIndexCallbacks : public ParsingCallbacks {
UpdateIndexCallbacks(FileIndex *FIndex,
@@ -643,12 +648,66 @@
return std::move(Result);
}
+void ClangdServer::codeAction(const CodeActionInputs &Params,
+ Callback<CodeActionResult> CB) {
+ auto Action = [Params, CB = std::move(CB),
+ FeatureModules(this->FeatureModules)](
+ Expected<InputsAndAST> InpAST) mutable {
+ if (!InpAST)
+ return CB(InpAST.takeError());
+ auto KindAllowed =
+ [Only(Params.RequestedActionKinds)](llvm::StringRef Kind) {
+ if (Only.empty())
+ return true;
+ return llvm::any_of(Only, [&](llvm::StringRef Base) {
+ return Kind.consume_front(Base) &&
+ (Kind.empty() || Kind.startswith("."));
+ });
+ };
+
+ CodeActionResult Result;
+ Result.Version = InpAST->AST.version().str();
+ if (KindAllowed(CodeAction::QUICKFIX_KIND)) {
+ auto FindMatched = [&InpAST](const DiagRef &DR) -> const clangd::Diag * {
+ for (const auto &Diag : InpAST->AST.getDiagnostics())
+ if (Diag.Range == DR.Range && Diag.Message == DR.Message)
+ return &Diag;
+ return nullptr;
+ };
+ for (const auto &Diag : Params.Diagnostics) {
+ if (const auto *Match = FindMatched(Diag))
+ for (const auto &Fix : Match->Fixes)
+ Result.QuickFixes.push_back({Diag, Fix});
+ }
+ }
+
+ // Collect Tweaks
+ auto Selections = tweakSelection(Params.Selection, *InpAST, /*FS=*/nullptr);
+ if (!Selections)
+ return CB(Selections.takeError());
+ // Don't allow a tweak to fire more than once across ambiguous selections.
+ llvm::DenseSet<llvm::StringRef> PreparedTweaks;
+ auto DeduplicatingFilter = [&](const Tweak &T) {
+ return KindAllowed(T.kind()) && Params.TweakFilter(T) &&
+ !PreparedTweaks.count(T.id());
+ };
+ for (const auto &Sel : *Selections) {
+ for (auto &T : prepareTweaks(*Sel, DeduplicatingFilter, FeatureModules)) {
+ Result.TweakRefs.push_back(TweakRef{T->id(), T->title(), T->kind()});
+ PreparedTweaks.insert(T->id());
+ TweakAvailable.record(1, T->id());
+ }
+ }
+ CB(std::move(Result));
+ };
+
+ WorkScheduler->runWithAST("EnumerateTweaks", Params.File, std::move(Action),
+ Transient);
+}
+
void ClangdServer::enumerateTweaks(
PathRef File, Range Sel, llvm::unique_function<bool(const Tweak &)> Filter,
Callback<std::vector<TweakRef>> CB) {
- // Tracks number of times a tweak has been offered.
- static constexpr trace::Metric TweakAvailable(
- "tweak_available", trace::Metric::Counter, "tweak_id");
auto Action = [Sel, CB = std::move(CB), Filter = std::move(Filter),
FeatureModules(this->FeatureModules)](
Expected<InputsAndAST> InpAST) mutable {
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -197,7 +197,8 @@
Callback<llvm::json::Value> Reply);
void bindMethods(LSPBinder &, const ClientCapabilities &Caps);
- std::vector<CodeAction> getFixes(StringRef File, const clangd::Diagnostic &D);
+ std::optional<ClangdServer::DiagRef> getDiagRef(StringRef File,
+ const clangd::Diagnostic &D);
/// Checks if completion request should be ignored. We need this due to the
/// limitation of the LSP. Per LSP, a client sends requests for all "trigger
@@ -230,13 +231,19 @@
/// Used to indicate the ClangdLSPServer is being destroyed.
std::atomic<bool> IsBeingDestroyed = {false};
- std::mutex FixItsMutex;
- typedef std::map<clangd::Diagnostic, std::vector<CodeAction>,
+ // FIXME: The caching is a temporary solution to get corresponding clangd
+ // diagnostic from a LSP diagnostic.
+ // Ideally, ClangdServer can generate an identifier for each diagnostic,
+ // emit them via the LSP's data field (which was newly added in LSP 3.16).
+ std::mutex DiagRefMutex;
+ /// A map from LSP diagnostic to clangd-naive diagnostic.
+ typedef std::map<clangd::Diagnostic, ClangdServer::DiagRef,
LSPDiagnosticCompare>
- DiagnosticToReplacementMap;
- /// Caches FixIts per file and diagnostics
- llvm::StringMap<DiagnosticToReplacementMap>
- FixItsMap;
+ DiagnosticToDiagRefMap;
+ /// Caches the mapping LSP and clangd-naive diagnostics per file.
+ llvm::StringMap<DiagnosticToDiagRefMap>
+ DiagRefMap;
+
// Last semantic-tokens response, for incremental requests.
std::mutex SemanticTokensMutex;
llvm::StringMap<SemanticTokens> LastSemanticTokens;
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -41,6 +41,7 @@
#include <cstddef>
#include <cstdint>
#include <functional>
+#include <map>
#include <memory>
#include <mutex>
#include <optional>
@@ -886,8 +887,8 @@
Server->removeDocument(File);
{
- std::lock_guard<std::mutex> Lock(FixItsMutex);
- FixItsMap.erase(File);
+ std::lock_guard<std::mutex> Lock(DiagRefMutex);
+ DiagRefMap.erase(File);
}
{
std::lock_guard<std::mutex> HLock(SemanticTokensMutex);
@@ -1010,72 +1011,71 @@
void ClangdLSPServer::onCodeAction(const CodeActionParams &Params,
Callback<llvm::json::Value> Reply) {
URIForFile File = Params.textDocument.uri;
- // Checks whether a particular CodeActionKind is included in the response.
- auto KindAllowed = [Only(Params.context.only)](llvm::StringRef Kind) {
- if (Only.empty())
- return true;
- return llvm::any_of(Only, [&](llvm::StringRef Base) {
- return Kind.consume_front(Base) && (Kind.empty() || Kind.startswith("."));
- });
+ llvm::ArrayRef<clangd::Diagnostic> LSPDiags = Params.context.diagnostics;
+ std::map<ClangdServer::DiagRef, /*index of LSPDiags*/ unsigned>
+ ToLSPDiagsIndex;
+ ClangdServer::CodeActionInputs Inputs;
+
+ for (unsigned I = 0; I < LSPDiags.size(); ++I) {
+ if (auto DiagRef = getDiagRef(File.file(), LSPDiags[I])) {
+ ToLSPDiagsIndex[*DiagRef] = I;
+ Inputs.Diagnostics.push_back(*DiagRef);
+ }
+ }
+ Inputs.File = File.file();
+ Inputs.Selection = Params.range;
+ Inputs.RequestedActionKinds = Params.context.only;
+ Inputs.TweakFilter = [this](const Tweak &T) {
+ return Opts.TweakFilter(T);
};
-
- // We provide a code action for Fixes on the specified diagnostics.
- std::vector<CodeAction> FixIts;
- if (KindAllowed(CodeAction::QUICKFIX_KIND)) {
- for (const Diagnostic &D : Params.context.diagnostics) {
- for (auto &CA : getFixes(File.file(), D)) {
- FixIts.push_back(CA);
- FixIts.back().diagnostics = {D};
+ auto CB = [this, LSPDiags = Params.context.diagnostics,
+ DiagRefs = Inputs.Diagnostics, Reply = std::move(Reply),
+ ToLSPDiagsIndex = std::move(ToLSPDiagsIndex), File,
+ Selection = Params.range](
+ llvm::Expected<ClangdServer::CodeActionResult> Fixits) mutable {
+ if (!Fixits)
+ return Reply(Fixits.takeError());
+ std::vector<CodeAction> CAs;
+ auto Version = decodeVersion(Fixits->Version);
+ for (const auto &QF : Fixits->QuickFixes) {
+ CAs.push_back(toCodeAction(QF.F, File, Version, SupportsDocumentChanges,
+ SupportsChangeAnnotation));
+ if (auto It = ToLSPDiagsIndex.find(QF.Diag);
+ It != ToLSPDiagsIndex.end()) {
+ CAs.back().diagnostics = {LSPDiags[It->second]};
}
}
- }
-
- // Now enumerate the semantic code actions.
- auto ConsumeActions =
- [Diags = Params.context.diagnostics, Reply = std::move(Reply), File,
- Selection = Params.range, FixIts = std::move(FixIts), this](
- llvm::Expected<std::vector<ClangdServer::TweakRef>> Tweaks) mutable {
- if (!Tweaks)
- return Reply(Tweaks.takeError());
-
- std::vector<CodeAction> Actions = std::move(FixIts);
- Actions.reserve(Actions.size() + Tweaks->size());
- for (const auto &T : *Tweaks)
- Actions.push_back(toCodeAction(T, File, Selection));
-
- // If there's exactly one quick-fix, call it "preferred".
- // We never consider refactorings etc as preferred.
- CodeAction *OnlyFix = nullptr;
- for (auto &Action : Actions) {
- if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND) {
- if (OnlyFix) {
- OnlyFix = nullptr;
- break;
- }
- OnlyFix = &Action;
- }
- }
+ for (const auto &TR : Fixits->TweakRefs)
+ CAs.push_back(toCodeAction(TR, File, Selection));
+
+ // If there's exactly one quick-fix, call it "preferred".
+ // We never consider refactorings etc as preferred.
+ CodeAction *OnlyFix = nullptr;
+ for (auto &Action : CAs) {
+ if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND) {
if (OnlyFix) {
- OnlyFix->isPreferred = true;
- if (Diags.size() == 1 && Diags.front().range == Selection)
- OnlyFix->diagnostics = {Diags.front()};
+ OnlyFix = nullptr;
+ break;
}
+ OnlyFix = &Action;
+ }
+ }
+ if (OnlyFix) {
+ OnlyFix->isPreferred = true;
+ if (LSPDiags.size() == 1 && LSPDiags.front().range == Selection)
+ OnlyFix->diagnostics = {LSPDiags.front()};
+ }
- if (SupportsCodeAction)
- return Reply(llvm::json::Array(Actions));
- std::vector<Command> Commands;
- for (const auto &Action : Actions) {
- if (auto Command = asCommand(Action))
- Commands.push_back(std::move(*Command));
- }
- return Reply(llvm::json::Array(Commands));
- };
- Server->enumerateTweaks(
- File.file(), Params.range,
- [this, KindAllowed(std::move(KindAllowed))](const Tweak &T) {
- return Opts.TweakFilter(T) && KindAllowed(T.kind());
- },
- std::move(ConsumeActions));
+ if (SupportsCodeAction)
+ return Reply(llvm::json::Array(CAs));
+ std::vector<Command> Commands;
+ for (const auto &Action : CAs) {
+ if (auto Command = asCommand(Action))
+ Commands.push_back(std::move(*Command));
+ }
+ return Reply(llvm::json::Array(Commands));
+ };
+ Server->codeAction(Inputs, std::move(CB));
}
void ClangdLSPServer::onCompletion(const CompletionParams &Params,
@@ -1704,17 +1704,17 @@
Server->profile(MT.child("clangd_server"));
}
-std::vector<CodeAction> ClangdLSPServer::getFixes(llvm::StringRef File,
- const clangd::Diagnostic &D) {
- std::lock_guard<std::mutex> Lock(FixItsMutex);
- auto DiagToFixItsIter = FixItsMap.find(File);
- if (DiagToFixItsIter == FixItsMap.end())
- return {};
+std::optional<ClangdServer::DiagRef>
+ClangdLSPServer::getDiagRef(StringRef File, const clangd::Diagnostic &D) {
+ std::lock_guard<std::mutex> Lock(DiagRefMutex);
+ auto DiagToDiagRefIter = DiagRefMap.find(File);
+ if (DiagToDiagRefIter == DiagRefMap.end())
+ return std::nullopt;
- const auto &DiagToFixItsMap = DiagToFixItsIter->second;
- auto FixItsIter = DiagToFixItsMap.find(D);
- if (FixItsIter == DiagToFixItsMap.end())
- return {};
+ const auto &DiagToDiagRefMap = DiagToDiagRefIter->second;
+ auto FixItsIter = DiagToDiagRefMap.find(D);
+ if (FixItsIter == DiagToDiagRefMap.end())
+ return std::nullopt;
return FixItsIter->second;
}
@@ -1747,31 +1747,29 @@
PublishDiagnosticsParams Notification;
Notification.version = decodeVersion(Version);
Notification.uri = URIForFile::canonicalize(File, /*TUPath=*/File);
- DiagnosticToReplacementMap LocalFixIts; // Temporary storage
+ DiagnosticToDiagRefMap LocalDiagMap; // Temporary storage
for (auto &Diag : Diagnostics) {
toLSPDiags(Diag, Notification.uri, DiagOpts,
- [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
- std::vector<CodeAction> CodeActions;
- for (const auto &Fix : Fixes)
- CodeActions.push_back(toCodeAction(Fix, Notification.uri,
- Notification.version,
- SupportsDocumentChanges,
- SupportsChangeAnnotation));
-
+ [&](clangd::Diagnostic LSPDiag, llvm::ArrayRef<Fix> Fixes) {
if (DiagOpts.EmbedFixesInDiagnostics) {
- Diag.codeActions.emplace(CodeActions);
- if (Diag.codeActions->size() == 1)
- Diag.codeActions->front().isPreferred = true;
+ std::vector<CodeAction> CodeActions;
+ for (const auto &Fix : Fixes)
+ CodeActions.push_back(toCodeAction(
+ Fix, Notification.uri, Notification.version,
+ SupportsDocumentChanges, SupportsChangeAnnotation));
+ LSPDiag.codeActions.emplace(std::move(CodeActions));
+ if (LSPDiag.codeActions->size() == 1)
+ LSPDiag.codeActions->front().isPreferred = true;
}
- LocalFixIts[Diag] = std::move(CodeActions);
- Notification.diagnostics.push_back(std::move(Diag));
+ LocalDiagMap[LSPDiag] = {Diag.Range, Diag.Message};
+ Notification.diagnostics.push_back(std::move(LSPDiag));
});
}
- // Cache FixIts
+ // Cache DiagRefMap
{
- std::lock_guard<std::mutex> Lock(FixItsMutex);
- FixItsMap[File] = LocalFixIts;
+ std::lock_guard<std::mutex> Lock(DiagRefMutex);
+ DiagRefMap[File] = LocalDiagMap;
}
// Send a notification to the LSP client.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
