kadircet created this revision.
Herald added subscribers: usaxena95, arphaman.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
Fixes only allow modifications to local file containing the diagnostics,
with CodeActions we'll be able to provide changes that can fix the issue
outside of the local file (e.g. build file/header modificiations).
As an implementation detail, we now covert all the fixes to codeactions
directly at publish time, rather than onCodeAction requests. So this
implies some extra copies on each diag release.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D96245
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/Diagnostics.h
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -693,7 +693,8 @@
// Transform diagnostics and check the results.
std::vector<std::pair<clangd::Diagnostic, std::vector<clangd::Fix>>> LSPDiags;
toLSPDiags(D, MainFile, Opts,
- [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes) {
+ [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes,
+ ArrayRef<CodeAction>) {
LSPDiags.push_back(
{std::move(LSPDiag),
std::vector<clangd::Fix>(Fixes.begin(), Fixes.end())});
@@ -712,7 +713,8 @@
LSPDiags.clear();
Opts.EmitRelatedLocations = true;
toLSPDiags(D, MainFile, Opts,
- [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes) {
+ [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes,
+ ArrayRef<CodeAction>) {
LSPDiags.push_back(
{std::move(LSPDiag),
std::vector<clangd::Fix>(Fixes.begin(), Fixes.end())});
@@ -1334,16 +1336,14 @@
D.InsideMainFile = true;
N.InsideMainFile = false;
toLSPDiags(D, {}, Opts,
- [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
- EXPECT_EQ(LSPDiag.range, D.Range);
- });
+ [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>,
+ ArrayRef<CodeAction>) { EXPECT_EQ(LSPDiag.range, D.Range); });
D.InsideMainFile = false;
N.InsideMainFile = true;
toLSPDiags(D, {}, Opts,
- [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
- EXPECT_EQ(LSPDiag.range, N.Range);
- });
+ [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>,
+ ArrayRef<CodeAction>) { EXPECT_EQ(LSPDiag.range, N.Range); });
}
} // namespace
Index: clang-tools-extra/clangd/Diagnostics.h
===================================================================
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -97,6 +97,7 @@
std::vector<Note> Notes;
/// *Alternative* fixes for this diagnostic, one should be chosen.
std::vector<Fix> Fixes;
+ std::vector<CodeAction> Actions;
};
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D);
@@ -107,9 +108,11 @@
/// separate diagnostics with their corresponding fixits. Notes outside main
/// file do not have a corresponding LSP diagnostic, but can still be included
/// as part of their main diagnostic's message.
-void toLSPDiags(
- const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
- llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn);
+void toLSPDiags(const Diag &D, const URIForFile &File,
+ const ClangdDiagnosticOptions &Opts,
+ llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>,
+ llvm::ArrayRef<CodeAction>)>
+ OutFn);
/// Convert from Fix to LSP CodeAction.
CodeAction toCodeAction(const Fix &D, const URIForFile &File);
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -9,6 +9,7 @@
#include "Diagnostics.h"
#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
#include "Compiler.h"
+#include "Plugin.h"
#include "Protocol.h"
#include "SourceCode.h"
#include "support/Logger.h"
@@ -403,9 +404,11 @@
return Result;
}
-void toLSPDiags(
- const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
- llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
+void toLSPDiags(const Diag &D, const URIForFile &File,
+ const ClangdDiagnosticOptions &Opts,
+ llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>,
+ llvm::ArrayRef<CodeAction>)>
+ OutFn) {
clangd::Diagnostic Main;
Main.severity = getSeverity(D.Severity);
@@ -437,7 +440,7 @@
break;
}
if (Opts.EmbedFixesInDiagnostics) {
- Main.codeActions.emplace();
+ Main.codeActions = D.Actions;
for (const auto &Fix : D.Fixes)
Main.codeActions->push_back(toCodeAction(Fix, File));
if (Main.codeActions->size() == 1)
@@ -462,7 +465,7 @@
Main.relatedInformation->push_back(std::move(RelInfo));
}
}
- OutFn(std::move(Main), D.Fixes);
+ OutFn(std::move(Main), D.Fixes, D.Actions);
// If we didn't emit the notes as relatedLocations, emit separate diagnostics
// so the user can find the locations easily.
@@ -474,7 +477,7 @@
Res.severity = getSeverity(Note.Severity);
Res.range = Note.Range;
Res.message = noteMessage(D, Note, Opts);
- OutFn(std::move(Res), llvm::ArrayRef<Fix>());
+ OutFn(std::move(Res), {}, {});
}
}
@@ -744,6 +747,8 @@
LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(),
ExtraFixes.end());
}
+ llvm::append_range(LastDiag->Actions,
+ Plugin::collectFixes(DiagLevel, Info));
} else {
// Handle a note to an existing diagnostic.
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -163,7 +163,7 @@
/// hierarchy.
void onMemoryUsage(Callback<MemoryTree>);
- std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D);
+ 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
@@ -210,7 +210,8 @@
std::atomic<bool> IsBeingDestroyed = {false};
std::mutex FixItsMutex;
- typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare>
+ typedef std::map<clangd::Diagnostic, std::vector<CodeAction>,
+ LSPDiagnosticCompare>
DiagnosticToReplacementMap;
/// Caches FixIts per file and diagnostics
llvm::StringMap<DiagnosticToReplacementMap> FixItsMap;
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -27,6 +27,7 @@
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/iterator_range.h"
@@ -1050,7 +1051,7 @@
if (KindAllowed(CodeAction::QUICKFIX_KIND)) {
for (const Diagnostic &D : Params.context.diagnostics) {
for (auto &F : getFixes(File.file(), D)) {
- FixIts.push_back(toCodeAction(F, Params.textDocument.uri));
+ FixIts.emplace_back(std::move(F));
FixIts.back().diagnostics = {D};
}
}
@@ -1580,8 +1581,8 @@
Server->profile(MT.child("clangd_server"));
}
-std::vector<Fix> ClangdLSPServer::getFixes(llvm::StringRef File,
- const clangd::Diagnostic &D) {
+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())
@@ -1647,9 +1648,14 @@
DiagnosticToReplacementMap LocalFixIts; // Temporary storage
for (auto &Diag : Diagnostics) {
toLSPDiags(Diag, Notification.uri, DiagOpts,
- [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
+ [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes,
+ llvm::ArrayRef<CodeAction> Actions) {
auto &FixItsForDiagnostic = LocalFixIts[Diag];
- llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
+ llvm::copy(Actions, std::back_inserter(FixItsForDiagnostic));
+ for (const auto &Fix : Fixes) {
+ FixItsForDiagnostic.emplace_back(
+ toCodeAction(Fix, Notification.uri));
+ }
Notification.diagnostics.push_back(std::move(Diag));
});
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
