hokein updated this revision to Diff 541009. hokein marked an inline comment as done. hokein added a comment.
address review comments. 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 @@ -962,16 +962,6 @@ }; 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 &); 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,45 @@ std::string Title; /// A single-line message to show in the UI. llvm::StringLiteral Kind; }; + + // Contains fields (LSP-form) to identify corresponding 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); + } + }; + + 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,6 +52,7 @@ #include <optional> #include <string> #include <type_traits> +#include <vector> namespace clang { namespace clangd { @@ -643,6 +644,70 @@ return std::move(Result); } +void ClangdServer::codeAction(const CodeActionInputs &Params, + Callback<CodeActionResult> CB) { + static constexpr trace::Metric TweakAvailable( + "code_action_tweak_available", trace::Metric::Counter, "tweak_id"); + 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 &LSPDiag) -> const clangd::Diag * { + for (const auto &Diag : InpAST->AST.getDiagnostics()) { + if (Diag.Range == LSPDiag.Range && + // FIMXE: this is tricky + llvm::StringRef(LSPDiag.Message) + .starts_with_insensitive(Diag.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) { Index: clang-tools-extra/clangd/ClangdLSPServer.h =================================================================== --- clang-tools-extra/clangd/ClangdLSPServer.h +++ clang-tools-extra/clangd/ClangdLSPServer.h @@ -197,7 +197,6 @@ Callback<llvm::json::Value> Reply); void bindMethods(LSPBinder &, const ClientCapabilities &Caps); - std::vector<CodeAction> getFixes(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 +229,6 @@ /// 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; // 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 @@ -885,10 +885,6 @@ PathRef File = Params.textDocument.uri.file(); Server->removeDocument(File); - { - std::lock_guard<std::mutex> Lock(FixItsMutex); - FixItsMap.erase(File); - } { std::lock_guard<std::mutex> HLock(SemanticTokensMutex); LastSemanticTokens.erase(File); @@ -1010,72 +1006,65 @@ 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(".")); - }); + ClangdServer::CodeActionInputs Inputs; + for (const auto &D : Params.context.diagnostics) + Inputs.Diagnostics.push_back(ClangdServer::DiagRef{D.range, D.message}); + 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, Diags = Params.context.diagnostics, Reply = std::move(Reply), + 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)); + for (const auto &D : Diags) { + if (QF.Diag == ClangdServer::DiagRef{D.range, D.message}) { + CAs.back().diagnostics = {D}; + break; + } } } - } - - // 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 (Diags.size() == 1 && Diags.front().range == Selection) + OnlyFix->diagnostics = {Diags.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,21 +1693,6 @@ 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 {}; - - const auto &DiagToFixItsMap = DiagToFixItsIter->second; - auto FixItsIter = DiagToFixItsMap.find(D); - if (FixItsIter == DiagToFixItsMap.end()) - return {}; - - return FixItsIter->second; -} - // A completion request is sent when the user types '>' or ':', but we only // want to trigger on '->' and '::'. We check the preceding text to make // sure it matches what we expected. @@ -1747,32 +1721,23 @@ PublishDiagnosticsParams Notification; Notification.version = decodeVersion(Version); Notification.uri = URIForFile::canonicalize(File, /*TUPath=*/File); - DiagnosticToReplacementMap LocalFixIts; // 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)); - if (DiagOpts.EmbedFixesInDiagnostics) { - Diag.codeActions.emplace(CodeActions); + std::vector<CodeAction> CodeActions; + for (const auto &Fix : Fixes) + CodeActions.push_back(toCodeAction( + Fix, Notification.uri, Notification.version, + SupportsDocumentChanges, SupportsChangeAnnotation)); + Diag.codeActions.emplace(std::move(CodeActions)); if (Diag.codeActions->size() == 1) Diag.codeActions->front().isPreferred = true; } - LocalFixIts[Diag] = std::move(CodeActions); Notification.diagnostics.push_back(std::move(Diag)); }); } - // Cache FixIts - { - std::lock_guard<std::mutex> Lock(FixItsMutex); - FixItsMap[File] = LocalFixIts; - } // Send a notification to the LSP client. PublishDiagnostics(Notification);
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
