[clang-tools-extra] f4f6c22 - [clangd] Refine the workflow for diagnostic Fixits.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 18 08:25:15 PDT 2023
Author: Haojian Wu
Date: 2023-07-18T17:25:08+02:00
New Revision: f4f6c229bde8f42721482469bd5a3d050d254d82
URL: https://github.com/llvm/llvm-project/commit/f4f6c229bde8f42721482469bd5a3d050d254d82
DIFF: https://github.com/llvm/llvm-project/commit/f4f6c229bde8f42721482469bd5a3d050d254d82.diff
LOG: [clangd] Refine the workflow for diagnostic Fixits.
- No longer store the diagnostic fixits in the clangdLSPServer
- When propagating the fixit via the code action, we use the Diag
information stored in the ParsedAST (in clangdServer.cpp)
Differential Revision: https://reviews.llvm.org/D155173
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 64489f78d843df..65ee60382ba991 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/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 @@ void ClangdLSPServer::onDocumentDidClose(
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,70 @@ static std::optional<Command> asCommand(const CodeAction &Action) {
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("."));
- });
- };
+ std::map<ClangdServer::DiagRef, clangd::Diagnostic> ToLSPDiags;
+ ClangdServer::CodeActionInputs Inputs;
- // 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};
- }
+ for (const auto& LSPDiag : Params.context.diagnostics) {
+ if (auto DiagRef = getDiagRef(File.file(), LSPDiag)) {
+ ToLSPDiags[*DiagRef] = LSPDiag;
+ Inputs.Diagnostics.push_back(*DiagRef);
}
}
-
- // 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;
- }
- }
+ Inputs.File = File.file();
+ Inputs.Selection = Params.range;
+ Inputs.RequestedActionKinds = Params.context.only;
+ Inputs.TweakFilter = [this](const Tweak &T) {
+ return Opts.TweakFilter(T);
+ };
+ auto CB = [this,
+ Reply = std::move(Reply),
+ ToLSPDiags = std::move(ToLSPDiags), 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 = ToLSPDiags.find(QF.Diag);
+ It != ToLSPDiags.end()) {
+ CAs.back().diagnostics = {It->second};
+ }
+ }
+ 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 (ToLSPDiags.size() == 1 &&
+ ToLSPDiags.begin()->second.range == Selection)
+ OnlyFix->diagnostics = {ToLSPDiags.begin()->second};
+ }
- 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 +1703,17 @@ void ClangdLSPServer::profile(MemoryTree &MT) const {
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(toDiagKey(D));
+ if (FixItsIter == DiagToDiagRefMap.end())
+ return std::nullopt;
return FixItsIter->second;
}
@@ -1747,31 +1746,29 @@ void ClangdLSPServer::onDiagnosticsReady(PathRef File, llvm::StringRef Version,
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[toDiagKey(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.
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 6cf1db43063245..a52e97099635e3 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -197,7 +197,8 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
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,28 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
/// Used to indicate the ClangdLSPServer is being destroyed.
std::atomic<bool> IsBeingDestroyed = {false};
- std::mutex FixItsMutex;
- typedef std::map<clangd::Diagnostic, std::vector<CodeAction>,
- LSPDiagnosticCompare>
- DiagnosticToReplacementMap;
- /// Caches FixIts per file and diagnostics
- llvm::StringMap<DiagnosticToReplacementMap>
- FixItsMap;
+ // 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;
+ struct DiagKey {
+ clangd::Range Range;
+ std::string Message;
+ bool operator<(const DiagKey &Other) const {
+ return std::tie(Range, Message) < std::tie(Other.Range, Other.Message);
+ }
+ };
+ DiagKey toDiagKey(const clangd::Diagnostic &LSPDiag) {
+ return {LSPDiag.range, LSPDiag.message};
+ }
+ /// A map from LSP diagnostic to clangd-naive diagnostic.
+ typedef std::map<DiagKey, ClangdServer::DiagRef>
+ 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;
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index d8df4c6a69910d..1451adcbf4d4fe 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/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,65 @@ tweakSelection(const Range &Sel, const InputsAndAST &AST,
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 FindMatchedFixes =
+ [&InpAST](const DiagRef &DR) -> llvm::ArrayRef<Fix> {
+ for (const auto &Diag : InpAST->AST.getDiagnostics())
+ if (Diag.Range == DR.Range && Diag.Message == DR.Message)
+ return Diag.Fixes;
+ return {};
+ };
+ for (const auto &Diag : Params.Diagnostics)
+ for (const auto &Fix : FindMatchedFixes(Diag))
+ 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("codeAction", 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 {
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index ea35b34da4ad6d..9b779841e9ed45 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/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 @@ class ClangdServer {
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);
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index 09fcfb55f4911e..23a48e0a8e5f61 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -962,16 +962,6 @@ struct Diagnostic {
};
llvm::json::Value toJSON(const Diagnostic &);
-/// A LSP-specific comparator used to find diagnostic in a container like
-/// std:map.
-/// We only use the required fields of Diagnostic to do the comparison to avoid
-/// any regression issues from LSP clients (e.g. VScode), see
-/// https://git.io/vbr29
-struct LSPDiagnosticCompare {
- bool operator()(const Diagnostic &LHS, const Diagnostic &RHS) const {
- 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 &);
More information about the cfe-commits
mailing list