From f4f6c229bde8f42721482469bd5a3d050d254d82 Mon Sep 17 00:00:00 2001 From: Haojian Wu Date: Sat, 8 Jul 2023 22:55:00 +0200 Subject: [PATCH] [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 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 175 +++++++++++++-------------- clang-tools-extra/clangd/ClangdLSPServer.h | 32 +++-- clang-tools-extra/clangd/ClangdServer.cpp | 64 +++++++++- clang-tools-extra/clangd/ClangdServer.h | 44 ++++++- clang-tools-extra/clangd/Protocol.h | 10 -- 5 files changed, 213 insertions(+), 112 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 64489f7..65ee603 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -886,8 +887,8 @@ void ClangdLSPServer::onDocumentDidClose( Server->removeDocument(File); { - std::lock_guard Lock(FixItsMutex); - FixItsMap.erase(File); + std::lock_guard Lock(DiagRefMutex); + DiagRefMap.erase(File); } { std::lock_guard HLock(SemanticTokensMutex); @@ -1010,72 +1011,70 @@ static std::optional asCommand(const CodeAction &Action) { void ClangdLSPServer::onCodeAction(const CodeActionParams &Params, Callback 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 ToLSPDiags; + ClangdServer::CodeActionInputs Inputs; - // We provide a code action for Fixes on the specified diagnostics. - std::vector 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> Tweaks) mutable { - if (!Tweaks) - return Reply(Tweaks.takeError()); - - std::vector 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 Fixits) mutable { + if (!Fixits) + return Reply(Fixits.takeError()); + std::vector 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 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 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 ClangdLSPServer::getFixes(llvm::StringRef File, - const clangd::Diagnostic &D) { - std::lock_guard Lock(FixItsMutex); - auto DiagToFixItsIter = FixItsMap.find(File); - if (DiagToFixItsIter == FixItsMap.end()) - return {}; +std::optional +ClangdLSPServer::getDiagRef(StringRef File, const clangd::Diagnostic &D) { + std::lock_guard 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 Fixes) { - std::vector CodeActions; - for (const auto &Fix : Fixes) - CodeActions.push_back(toCodeAction(Fix, Notification.uri, - Notification.version, - SupportsDocumentChanges, - SupportsChangeAnnotation)); - + [&](clangd::Diagnostic LSPDiag, llvm::ArrayRef Fixes) { if (DiagOpts.EmbedFixesInDiagnostics) { - Diag.codeActions.emplace(CodeActions); - if (Diag.codeActions->size() == 1) - Diag.codeActions->front().isPreferred = true; + std::vector 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 Lock(FixItsMutex); - FixItsMap[File] = LocalFixIts; + std::lock_guard 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 6cf1db4..a52e970 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -197,7 +197,8 @@ private: Callback Reply); void bindMethods(LSPBinder &, const ClientCapabilities &Caps); - std::vector getFixes(StringRef File, const clangd::Diagnostic &D); + std::optional 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 @@ private: /// Used to indicate the ClangdLSPServer is being destroyed. std::atomic IsBeingDestroyed = {false}; - std::mutex FixItsMutex; - typedef std::map, - LSPDiagnosticCompare> - DiagnosticToReplacementMap; - /// Caches FixIts per file and diagnostics - llvm::StringMap - 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 + DiagnosticToDiagRefMap; + /// Caches the mapping LSP and clangd-naive diagnostics per file. + llvm::StringMap + DiagRefMap; + // Last semantic-tokens response, for incremental requests. std::mutex SemanticTokensMutex; llvm::StringMap LastSemanticTokens; diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index d8df4c6..1451adc 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -52,11 +52,16 @@ #include #include #include +#include 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 CB) { + auto Action = [Params, CB = std::move(CB), + FeatureModules(this->FeatureModules)]( + Expected 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 { + 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 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 Filter, Callback> 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 InpAST) mutable { diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index ea35b34..9b77984 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -37,8 +37,7 @@ #include #include #include -#include -#include +#include #include namespace clang { @@ -351,8 +350,49 @@ public: 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 RequestedActionKinds; + + /// Diagnostics attached to the code action request. + std::vector Diagnostics; + + /// Tweaks where Filter returns false will not be checked or included. + std::function TweakFilter; + }; + struct CodeActionResult { + std::string Version; + struct QuickFix { + DiagRef Diag; + Fix F; + }; + std::vector QuickFixes; + std::vector 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 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 Filter, Callback> CB); diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index 09fcfb5..23a48e0 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 &); -- 2.7.4