[PATCH] D53391: [clangd] Embed fixes as CodeAction, instead of clangd_fixes. Clean up serialization.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345119: [clangd] Embed fixes as CodeAction, instead of
clangd_fixes. Clean up… (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53391?vs=170153=170824#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53391
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/Diagnostics.cpp
clang-tools-extra/trunk/clangd/Diagnostics.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/test/clangd/fixits-embed-in-diagnostic.test
clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -549,14 +549,8 @@
std::vector Actions;
for (const Diagnostic : Params.context.diagnostics) {
for (auto : getFixes(Params.textDocument.uri.file(), D)) {
- Actions.emplace_back();
- Actions.back().title = F.Message;
- Actions.back().kind = CodeAction::QUICKFIX_KIND;
+ Actions.push_back(toCodeAction(F, Params.textDocument.uri));
Actions.back().diagnostics = {D};
- Actions.back().edit.emplace();
- Actions.back().edit->changes.emplace();
- (*Actions.back().edit->changes)[Params.textDocument.uri.uri()] = {
- F.Edits.begin(), F.Edits.end()};
}
}
@@ -724,36 +718,16 @@
void ClangdLSPServer::onDiagnosticsReady(PathRef File,
std::vector Diagnostics) {
- json::Array DiagnosticsJSON;
-
+ URIForFile URI(File);
+ std::vector LSPDiagnostics;
DiagnosticToReplacementMap LocalFixIts; // Temporary storage
for (auto : Diagnostics) {
-toLSPDiags(Diag, [&](clangd::Diagnostic Diag, ArrayRef Fixes) {
- json::Object LSPDiag({
- {"range", Diag.range},
- {"severity", Diag.severity},
- {"message", Diag.message},
- });
- // LSP extension: embed the fixes in the diagnostic.
- if (DiagOpts.EmbedFixesInDiagnostics && !Fixes.empty()) {
-json::Array ClangdFixes;
-for (const auto : Fixes) {
- WorkspaceEdit WE;
- URIForFile URI{File};
- WE.changes = {{URI.uri(), std::vector(Fix.Edits.begin(),
- Fix.Edits.end())}};
- ClangdFixes.push_back(
- json::Object{{"edit", toJSON(WE)}, {"title", Fix.Message}});
-}
-LSPDiag["clangd_fixes"] = std::move(ClangdFixes);
- }
- if (DiagOpts.SendDiagnosticCategory && !Diag.category.empty())
-LSPDiag["category"] = Diag.category;
- DiagnosticsJSON.push_back(std::move(LSPDiag));
-
- auto = LocalFixIts[Diag];
- llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
-});
+toLSPDiags(Diag, URI, DiagOpts,
+ [&](clangd::Diagnostic Diag, ArrayRef Fixes) {
+ auto = LocalFixIts[Diag];
+ llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
+ LSPDiagnostics.push_back(std::move(Diag));
+ });
}
// Cache FixIts
@@ -766,8 +740,8 @@
// Publish diagnostics.
notify("textDocument/publishDiagnostics",
json::Object{
- {"uri", URIForFile{File}},
- {"diagnostics", std::move(DiagnosticsJSON)},
+ {"uri", URI},
+ {"diagnostics", std::move(LSPDiagnostics)},
});
}
Index: clang-tools-extra/trunk/clangd/Diagnostics.cpp
===
--- clang-tools-extra/trunk/clangd/Diagnostics.cpp
+++ clang-tools-extra/trunk/clangd/Diagnostics.cpp
@@ -227,19 +227,37 @@
return OS;
}
-void toLSPDiags(const Diag ,
-function_ref)> OutFn) {
+CodeAction toCodeAction(const Fix , const URIForFile ) {
+ CodeAction Action;
+ Action.title = F.Message;
+ Action.kind = CodeAction::QUICKFIX_KIND;
+ Action.edit.emplace();
+ Action.edit->changes.emplace();
+ (*Action.edit->changes)[File.uri()] = {F.Edits.begin(), F.Edits.end()};
+ return Action;
+}
+
+void toLSPDiags(
+const Diag , const URIForFile , const ClangdDiagnosticOptions ,
+function_ref)> OutFn) {
auto FillBasicFields = [](const DiagBase ) -> clangd::Diagnostic {
clangd::Diagnostic Res;
Res.range = D.Range;
Res.severity = getSeverity(D.Severity);
-Res.category = D.Category;
return Res;
};
{
clangd::Diagnostic Main = FillBasicFields(D);
Main.message = mainMessage(D);
+if (Opts.EmbedFixesInDiagnostics) {
+ Main.codeActions.emplace();
+ for (const auto : D.Fixes)
+Main.codeActions->push_back(toCodeAction(Fix, File));
+}
+if
[PATCH] D53391: [clangd] Embed fixes as CodeAction, instead of clangd_fixes. Clean up serialization.
sammccall updated this revision to Diff 170153.
sammccall marked an inline comment as done.
sammccall added a comment.
Don't call URIForFile again, rebase.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53391
Files:
clangd/ClangdLSPServer.cpp
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/fixits-embed-in-diagnostic.test
unittests/clangd/ClangdUnitTests.cpp
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -199,11 +199,13 @@
// Transform dianostics and check the results.
std::vector>> LSPDiags;
- toLSPDiags(D, [&](clangd::Diagnostic LSPDiag,
-llvm::ArrayRef Fixes) {
-LSPDiags.push_back({std::move(LSPDiag),
-std::vector(Fixes.begin(), Fixes.end())});
- });
+ toLSPDiags(
+ D, URIForFile("/path/to/foo/bar/main.cpp"), ClangdDiagnosticOptions(),
+ [&](clangd::Diagnostic LSPDiag, llvm::ArrayRef Fixes) {
+LSPDiags.push_back(
+{std::move(LSPDiag),
+ std::vector(Fixes.begin(), Fixes.end())});
+ });
EXPECT_THAT(
LSPDiags,
Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- test/clangd/fixits-embed-in-diagnostic.test
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -1,12 +1,12 @@
# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"clangdFixSupport":true}}},"trace":"off"}}
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"codeActionsInline":true}}},"trace":"off"}}
---
{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"struct Point {}; union Point p;"}}}
-# CHECK:"method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:"params": {
-# CHECK-NEXT: "diagnostics": [
+# CHECK: "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT:"diagnostics": [
# CHECK-NEXT: {
-# CHECK-NEXT:"clangd_fixes": [
+# CHECK-NEXT:"codeActions": [
# CHECK-NEXT: {
# CHECK-NEXT:"edit": {
# CHECK-NEXT: "changes": {
@@ -27,6 +27,7 @@
# CHECK-NEXT:]
# CHECK-NEXT: }
# CHECK-NEXT:},
+# CHECK-NEXT:"kind": "quickfix",
# CHECK-NEXT:"title": "change 'union' to 'struct'"
# CHECK-NEXT: }
# CHECK-NEXT:],
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -323,9 +323,8 @@
/// workspace.symbol.symbolKind.valueSet
llvm::Optional WorkspaceSymbolKinds;
- /// Whether the client accepts diagnostics with fixes attached using the
- /// "clangd_fixes" extension.
- /// textDocument.publishDiagnostics.clangdFixSupport
+ /// Whether the client accepts diagnostics with codeActions attached inline.
+ /// textDocument.publishDiagnostics.codeActionsInline.
bool DiagnosticFixes = false;
/// Whether the client accepts diagnostics with category attached to it
@@ -536,6 +535,7 @@
};
bool fromJSON(const llvm::json::Value &, DocumentSymbolParams &);
+struct CodeAction;
struct Diagnostic {
/// The range at which the message applies.
Range range;
@@ -560,7 +560,12 @@
/// An LSP extension that's used to send the name of the category over to the
/// client. The category typically describes the compilation stage during
/// which the issue was produced, e.g. "Semantic Issue" or "Parse Issue".
- std::string category;
+ llvm::Optional category;
+
+ /// Clangd extension: code actions related to this diagnostic.
+ /// Only with capability textDocument.publishDiagnostics.codeActionsInline.
+ /// (These actions can also be obtained using textDocument/codeAction).
+ llvm::Optional> codeActions;
};
llvm::json::Value toJSON(const Diagnostic &);
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -210,8 +210,8 @@
if (auto *Diagnostics = TextDocument->getObject("publishDiagnostics")) {
if (auto CategorySupport = Diagnostics->getBoolean("categorySupport"))
R.DiagnosticCategory = *CategorySupport;
- if (auto ClangdFixSupport = Diagnostics->getBoolean("clangdFixSupport"))
-R.DiagnosticFixes = *ClangdFixSupport;
+ if (auto CodeActions = Diagnostics->getBoolean("codeActionsInline"))
+R.DiagnosticFixes = *CodeActions;
}
if (auto *Completion =
[PATCH] D53391: [clangd] Embed fixes as CodeAction, instead of clangd_fixes. Clean up serialization.
sammccall added a comment. Thanks! @arphaman, any concerns about the change to extension format? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53391 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53391: [clangd] Embed fixes as CodeAction, instead of clangd_fixes. Clean up serialization.
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
LGTM.
Comment at: clangd/ClangdLSPServer.cpp:558
json::Object{
{"uri", URIForFile{File}},
+{"diagnostics", std::move(LSPDiagnostics)},
nit: we can reuse `URI` insteading of calling `URIForFile` again.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53391
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53391: [clangd] Embed fixes as CodeAction, instead of clangd_fixes. Clean up serialization.
sammccall created this revision.
sammccall added reviewers: hokein, arphaman.
Herald added subscribers: cfe-commits, kadircet, jkorous, MaskRay, ioeric,
ilya-biryukov.
CodeAction provides us with a standard way of representing fixes inline, so
use it, replacing our existing ad-hoc extension.
After this, it's easy to serialize diagnostics using the structured
toJSON/Protocol.h mechanism rather than assembling JSON ad-hoc.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53391
Files:
clangd/ClangdLSPServer.cpp
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/fixits-embed-in-diagnostic.test
unittests/clangd/ClangdUnitTests.cpp
Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -199,11 +199,13 @@
// Transform dianostics and check the results.
std::vector>> LSPDiags;
- toLSPDiags(D, [&](clangd::Diagnostic LSPDiag,
-llvm::ArrayRef Fixes) {
-LSPDiags.push_back({std::move(LSPDiag),
-std::vector(Fixes.begin(), Fixes.end())});
- });
+ toLSPDiags(
+ D, URIForFile("/path/to/foo/bar/main.cpp"), ClangdDiagnosticOptions(),
+ [&](clangd::Diagnostic LSPDiag, llvm::ArrayRef Fixes) {
+LSPDiags.push_back(
+{std::move(LSPDiag),
+ std::vector(Fixes.begin(), Fixes.end())});
+ });
EXPECT_THAT(
LSPDiags,
Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- test/clangd/fixits-embed-in-diagnostic.test
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -1,12 +1,12 @@
# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"clangdFixSupport":true}}},"trace":"off"}}
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"codeActionsInline":true}}},"trace":"off"}}
---
{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"struct Point {}; union Point p;"}}}
-# CHECK:"method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:"params": {
-# CHECK-NEXT: "diagnostics": [
+# CHECK: "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT:"diagnostics": [
# CHECK-NEXT: {
-# CHECK-NEXT:"clangd_fixes": [
+# CHECK-NEXT:"codeActions": [
# CHECK-NEXT: {
# CHECK-NEXT:"edit": {
# CHECK-NEXT: "changes": {
@@ -27,6 +27,7 @@
# CHECK-NEXT:]
# CHECK-NEXT: }
# CHECK-NEXT:},
+# CHECK-NEXT:"kind": "quickfix",
# CHECK-NEXT:"title": "change 'union' to 'struct'"
# CHECK-NEXT: }
# CHECK-NEXT:],
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -323,9 +323,8 @@
/// workspace.symbol.symbolKind.valueSet
llvm::Optional WorkspaceSymbolKinds;
- /// Whether the client accepts diagnostics with fixes attached using the
- /// "clangd_fixes" extension.
- /// textDocument.publishDiagnostics.clangdFixSupport
+ /// Whether the client accepts diagnostics with codeActions attached inline.
+ /// textDocument.publishDiagnostics.codeActionsInline.
bool DiagnosticFixes = false;
/// Whether the client accepts diagnostics with category attached to it
@@ -536,6 +535,7 @@
};
bool fromJSON(const llvm::json::Value &, DocumentSymbolParams &);
+struct CodeAction;
struct Diagnostic {
/// The range at which the message applies.
Range range;
@@ -560,7 +560,12 @@
/// An LSP extension that's used to send the name of the category over to the
/// client. The category typically describes the compilation stage during
/// which the issue was produced, e.g. "Semantic Issue" or "Parse Issue".
- std::string category;
+ llvm::Optional category;
+
+ /// Clangd extension: code actions related to this diagnostic.
+ /// Only with capability textDocument.publishDiagnostics.codeActionsInline.
+ /// (These actions can also be obtained using textDocument/codeAction).
+ llvm::Optional> codeActions;
};
llvm::json::Value toJSON(const Diagnostic &);
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -210,8 +210,8 @@
if (auto *Diagnostics = TextDocument->getObject("publishDiagnostics")) {
if (auto CategorySupport = Diagnostics->getBoolean("categorySupport"))
R.DiagnosticCategory = *CategorySupport;
- if (auto ClangdFixSupport =
