Skip to content

Commit

Permalink
[clangd] Add batch fixes for include-cleaner diagnostics
Browse files Browse the repository at this point in the history
For each unused-include/missing-include diagnostic, we provide fix-all
alternative to them.

This patch also adds LSP ChangeAnnotation support.

Differential Revision: https://reviews.llvm.org/D147684
  • Loading branch information
hokein committed Apr 27, 2023
1 parent fdc0d5f commit 4b1cec0
Show file tree
Hide file tree
Showing 15 changed files with 737 additions and 43 deletions.
22 changes: 17 additions & 5 deletions clang-tools-extra/clangd/ClangdLSPServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,19 +99,29 @@ CodeAction toCodeAction(const ClangdServer::TweakRef &T, const URIForFile &File,
/// Convert from Fix to LSP CodeAction.
CodeAction toCodeAction(const Fix &F, const URIForFile &File,
const std::optional<int64_t> &Version,
bool SupportsDocumentChanges) {
bool SupportsDocumentChanges,
bool SupportChangeAnnotation) {
CodeAction Action;
Action.title = F.Message;
Action.kind = std::string(CodeAction::QUICKFIX_KIND);
Action.edit.emplace();
if (!SupportsDocumentChanges) {
Action.edit->changes.emplace();
(*Action.edit->changes)[File.uri()] = {F.Edits.begin(), F.Edits.end()};
auto &Changes = (*Action.edit->changes)[File.uri()];
for (const auto &E : F.Edits)
Changes.push_back({E.range, E.newText, /*annotationId=*/""});
} else {
Action.edit->documentChanges.emplace();
TextDocumentEdit& Edit = Action.edit->documentChanges->emplace_back();
TextDocumentEdit &Edit = Action.edit->documentChanges->emplace_back();
Edit.textDocument = VersionedTextDocumentIdentifier{{File}, Version};
Edit.edits = {F.Edits.begin(), F.Edits.end()};
for (const auto &E : F.Edits)
Edit.edits.push_back(
{E.range, E.newText,
SupportChangeAnnotation ? E.annotationId : ""});
if (SupportChangeAnnotation) {
for (const auto &[AID, Annotation]: F.Annotations)
Action.edit->changeAnnotations[AID] = Annotation;
}
}
return Action;
}
Expand Down Expand Up @@ -509,6 +519,7 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params,
SupportsReferenceContainer = Params.capabilities.ReferenceContainer;
SupportFileStatus = Params.initializationOptions.FileStatus;
SupportsDocumentChanges = Params.capabilities.DocumentChanges;
SupportsChangeAnnotation = Params.capabilities.ChangeAnnotation;
HoverContentFormat = Params.capabilities.HoverContentFormat;
Opts.LineFoldingOnly = Params.capabilities.LineFoldingOnly;
SupportsOffsetsInSignatureHelp = Params.capabilities.OffsetsInSignatureHelp;
Expand Down Expand Up @@ -1742,7 +1753,8 @@ void ClangdLSPServer::onDiagnosticsReady(PathRef File, llvm::StringRef Version,
for (const auto &Fix : Fixes)
CodeActions.push_back(toCodeAction(Fix, Notification.uri,
Notification.version,
SupportsDocumentChanges));
SupportsDocumentChanges,
SupportsChangeAnnotation));

if (DiagOpts.EmbedFixesInDiagnostics) {
Diag.codeActions.emplace(CodeActions);
Expand Down
3 changes: 3 additions & 0 deletions clang-tools-extra/clangd/ClangdLSPServer.h
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,9 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
bool SupportsOffsetsInSignatureHelp = false;
/// Whether the client supports the versioned document changes.
bool SupportsDocumentChanges = false;
/// Whether the client supports change annotations on text edits.
bool SupportsChangeAnnotation = false;

std::mutex BackgroundIndexProgressMutex;
enum class BackgroundIndexProgress {
// Client doesn't support reporting progress. No transitions possible.
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/clangd/CodeComplete.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2242,7 +2242,7 @@ CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const {
}
LSP.sortText = sortText(Score.Total, FilterText);
LSP.filterText = FilterText;
LSP.textEdit = {CompletionTokenRange, RequiredQualifier + Name};
LSP.textEdit = {CompletionTokenRange, RequiredQualifier + Name, ""};
// Merge continuous additionalTextEdits into main edit. The main motivation
// behind this is to help LSP clients, it seems most of them are confused when
// they are provided with additionalTextEdits that are consecutive to main
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/clangd/Diagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
if (Message.empty()) // either !SyntheticMessage, or we failed to make one.
Info.FormatDiagnostic(Message);
LastDiag->Fixes.push_back(
Fix{std::string(Message.str()), std::move(Edits)});
Fix{std::string(Message.str()), std::move(Edits), {}});
return true;
};

Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/clangd/Diagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ struct Fix {
std::string Message;
/// TextEdits from clang's fix-its. Must be non-empty.
llvm::SmallVector<TextEdit, 1> Edits;

/// Annotations for the Edits.
llvm::SmallVector<std::pair<ChangeAnnotationIdentifier, ChangeAnnotation>>
Annotations;
};
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Fix &F);

Expand Down
118 changes: 111 additions & 7 deletions clang-tools-extra/clangd/IncludeCleaner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Casting.h"
Expand Down Expand Up @@ -435,6 +436,115 @@ IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
return {std::move(UnusedIncludes), std::move(MissingIncludes)};
}

Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
assert(!UnusedIncludes.empty());

Fix RemoveAll;
RemoveAll.Message = "remove all unused includes";
for (const auto &Diag : UnusedIncludes) {
assert(Diag.Fixes.size() == 1 && "Expected exactly one fix.");
RemoveAll.Edits.insert(RemoveAll.Edits.end(),
Diag.Fixes.front().Edits.begin(),
Diag.Fixes.front().Edits.end());
}

// TODO(hokein): emit a suitable text for the label.
ChangeAnnotation Annotation = {/*label=*/"",
/*needsConfirmation=*/true,
/*description=*/""};
static const ChangeAnnotationIdentifier RemoveAllUnusedID =
"RemoveAllUnusedIncludes";
for (unsigned I = 0; I < RemoveAll.Edits.size(); ++I) {
ChangeAnnotationIdentifier ID = RemoveAllUnusedID + std::to_string(I);
RemoveAll.Edits[I].annotationId = ID;
RemoveAll.Annotations.push_back({ID, Annotation});
}
return RemoveAll;
}
Fix addAllMissingIncludes(llvm::ArrayRef<Diag> MissingIncludeDiags) {
assert(!MissingIncludeDiags.empty());

Fix AddAllMissing;
AddAllMissing.Message = "add all missing includes";
// A map to deduplicate the edits with the same new text.
// newText (#include "my_missing_header.h") -> TextEdit.
llvm::StringMap<TextEdit> Edits;
for (const auto &Diag : MissingIncludeDiags) {
assert(Diag.Fixes.size() == 1 && "Expected exactly one fix.");
for (const auto& Edit : Diag.Fixes.front().Edits) {
Edits.try_emplace(Edit.newText, Edit);
}
}
// FIXME(hokein): emit used symbol reference in the annotation.
ChangeAnnotation Annotation = {/*label=*/"",
/*needsConfirmation=*/true,
/*description=*/""};
static const ChangeAnnotationIdentifier AddAllMissingID =
"AddAllMissingIncludes";
unsigned I = 0;
for (auto &It : Edits) {
ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
AddAllMissing.Edits.push_back(std::move(It.getValue()));
AddAllMissing.Edits.back().annotationId = ID;

AddAllMissing.Annotations.push_back({ID, Annotation});
}
return AddAllMissing;
}
Fix fixAll(const Fix& RemoveAllUnused, const Fix& AddAllMissing) {
Fix FixAll;
FixAll.Message = "fix all includes";

for (const auto &F : RemoveAllUnused.Edits)
FixAll.Edits.push_back(F);
for (const auto &F : AddAllMissing.Edits)
FixAll.Edits.push_back(F);

for (const auto& A : RemoveAllUnused.Annotations)
FixAll.Annotations.push_back(A);
for (const auto& A : AddAllMissing.Annotations)
FixAll.Annotations.push_back(A);
return FixAll;
}

std::vector<Diag> generateIncludeCleanerDiagnostic(
ParsedAST &AST, const IncludeCleanerFindings &Findings,
llvm::StringRef Code) {
std::vector<Diag> UnusedIncludes = generateUnusedIncludeDiagnostics(
AST.tuPath(), Findings.UnusedIncludes, Code);
std::optional<Fix> RemoveAllUnused;;
if (UnusedIncludes.size() > 1)
RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);

std::vector<Diag> MissingIncludeDiags = generateMissingIncludeDiagnostics(
AST, Findings.MissingIncludes, Code);
std::optional<Fix> AddAllMissing;
if (MissingIncludeDiags.size() > 1)
AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);

std::optional<Fix> FixAll;
if (RemoveAllUnused && AddAllMissing)
FixAll = fixAll(*RemoveAllUnused, *AddAllMissing);

auto AddBatchFix = [](const std::optional<Fix> &F, clang::clangd::Diag *Out) {
if (!F) return;
Out->Fixes.push_back(*F);
};
for (auto &Diag : MissingIncludeDiags) {
AddBatchFix(AddAllMissing, &Diag);
AddBatchFix(FixAll, &Diag);
}
for (auto &Diag : UnusedIncludes) {
AddBatchFix(RemoveAllUnused, &Diag);
AddBatchFix(FixAll, &Diag);
}

auto Result = std::move(MissingIncludeDiags);
llvm::move(UnusedIncludes,
std::back_inserter(Result));
return Result;
}

std::vector<Diag> issueIncludeCleanerDiagnostics(ParsedAST &AST,
llvm::StringRef Code) {
// Interaction is only polished for C/CPP.
Expand All @@ -450,13 +560,7 @@ std::vector<Diag> issueIncludeCleanerDiagnostics(ParsedAST &AST,
// will need include-cleaner results, call it once
Findings = computeIncludeCleanerFindings(AST);
}

std::vector<Diag> Result = generateUnusedIncludeDiagnostics(
AST.tuPath(), Findings.UnusedIncludes, Code);
llvm::move(
generateMissingIncludeDiagnostics(AST, Findings.MissingIncludes, Code),
std::back_inserter(Result));
return Result;
return generateIncludeCleanerDiagnostic(AST, Findings, Code);
}

std::optional<include_cleaner::Header>
Expand Down
37 changes: 34 additions & 3 deletions clang-tools-extra/clangd/Protocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,34 @@ bool fromJSON(const llvm::json::Value &Params, TextDocumentItem &R,
bool fromJSON(const llvm::json::Value &Params, TextEdit &R,
llvm::json::Path P) {
llvm::json::ObjectMapper O(Params, P);
return O && O.map("range", R.range) && O.map("newText", R.newText);
return O && O.map("range", R.range) && O.map("newText", R.newText) &&
O.mapOptional("annotationId", R.annotationId);
}

llvm::json::Value toJSON(const TextEdit &P) {
return llvm::json::Object{
llvm::json::Object Result{
{"range", P.range},
{"newText", P.newText},
};
if (!P.annotationId.empty())
Result["annotationId"] = P.annotationId;
return Result;
}

bool fromJSON(const llvm::json::Value &Params, ChangeAnnotation &R,
llvm::json::Path P) {
llvm::json::ObjectMapper O(Params, P);
return O && O.map("label", R.label) &&
O.map("needsConfirmation", R.needsConfirmation) &&
O.mapOptional("description", R.description);
}
llvm::json::Value toJSON(const ChangeAnnotation & CA) {
llvm::json::Object Result{{"label", CA.label}};
if (CA.needsConfirmation)
Result["needsConfirmation"] = *CA.needsConfirmation;
if (!CA.description.empty())
Result["description"] = CA.description;
return Result;
}

bool fromJSON(const llvm::json::Value &Params, TextDocumentEdit &R,
Expand Down Expand Up @@ -458,6 +478,10 @@ bool fromJSON(const llvm::json::Value &Params, ClientCapabilities &R,
if (auto *WorkspaceEdit = Workspace->getObject("workspaceEdit")) {
if (auto DocumentChanges = WorkspaceEdit->getBoolean("documentChanges"))
R.DocumentChanges = *DocumentChanges;
if (const auto& ChangeAnnotation =
WorkspaceEdit->getObject("changeAnnotationSupport")) {
R.ChangeAnnotation = true;
}
}
}
if (auto *Window = O->getObject("window")) {
Expand Down Expand Up @@ -733,7 +757,8 @@ bool fromJSON(const llvm::json::Value &Params, WorkspaceEdit &R,
llvm::json::Path P) {
llvm::json::ObjectMapper O(Params, P);
return O && O.map("changes", R.changes) &&
O.map("documentChanges", R.documentChanges);
O.map("documentChanges", R.documentChanges) &&
O.mapOptional("changeAnnotations", R.changeAnnotations);
}

bool fromJSON(const llvm::json::Value &Params, ExecuteCommandParams &R,
Expand Down Expand Up @@ -888,6 +913,12 @@ llvm::json::Value toJSON(const WorkspaceEdit &WE) {
}
if (WE.documentChanges)
Result["documentChanges"] = *WE.documentChanges;
if (!WE.changeAnnotations.empty()) {
llvm::json::Object ChangeAnnotations;
for (auto &Annotation : WE.changeAnnotations)
ChangeAnnotations[Annotation.first] = Annotation.second;
Result["changeAnnotations"] = std::move(ChangeAnnotations);
}
return Result;
}

Expand Down
32 changes: 31 additions & 1 deletion clang-tools-extra/clangd/Protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,8 @@ struct ReferenceLocation : Location {
llvm::json::Value toJSON(const ReferenceLocation &);
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const ReferenceLocation &);

using ChangeAnnotationIdentifier = std::string;
// A combination of a LSP standard TextEdit and AnnotatedTextEdit.
struct TextEdit {
/// The range of the text document to be manipulated. To insert
/// text into a document create a range where start === end.
Expand All @@ -246,14 +248,35 @@ struct TextEdit {
/// The string to be inserted. For delete operations use an
/// empty string.
std::string newText;

/// The actual annotation identifier (optional)
/// If empty, then this field is nullopt.
ChangeAnnotationIdentifier annotationId = "";
};
inline bool operator==(const TextEdit &L, const TextEdit &R) {
return std::tie(L.newText, L.range) == std::tie(R.newText, R.range);
return std::tie(L.newText, L.range, L.annotationId) ==
std::tie(R.newText, R.range, L.annotationId);
}
bool fromJSON(const llvm::json::Value &, TextEdit &, llvm::json::Path);
llvm::json::Value toJSON(const TextEdit &);
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const TextEdit &);

struct ChangeAnnotation {
/// A human-readable string describing the actual change. The string
/// is rendered prominent in the user interface.
std::string label;

/// A flag which indicates that user confirmation is needed
/// before applying the change.
std::optional<bool> needsConfirmation;

/// A human-readable string which is rendered less prominent in
/// the user interface.
std::string description;
};
bool fromJSON(const llvm::json::Value &, ChangeAnnotation &, llvm::json::Path);
llvm::json::Value toJSON(const ChangeAnnotation &);

struct TextDocumentEdit {
/// The text document to change.
VersionedTextDocumentIdentifier textDocument;
Expand Down Expand Up @@ -530,6 +553,9 @@ struct ClientCapabilities {

/// The client supports versioned document changes for WorkspaceEdit.
bool DocumentChanges = false;

/// The client supports change annotations on text edits,
bool ChangeAnnotation = false;

/// Whether the client supports the textDocument/inactiveRegions
/// notification. This is a clangd extension.
Expand Down Expand Up @@ -996,6 +1022,10 @@ struct WorkspaceEdit {
/// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s
/// using the `changes` property are supported.
std::optional<std::vector<TextDocumentEdit>> documentChanges;

/// A map of change annotations that can be referenced in
/// AnnotatedTextEdit.
std::map<std::string, ChangeAnnotation> changeAnnotations;
};
bool fromJSON(const llvm::json::Value &, WorkspaceEdit &, llvm::json::Path);
llvm::json::Value toJSON(const WorkspaceEdit &WE);
Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/clangd/test/Inputs/include-cleaner/all1.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#pragma once

#include "bar.h"
#include "foo.h"
4 changes: 4 additions & 0 deletions clang-tools-extra/clangd/test/Inputs/include-cleaner/all2.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#pragma once

#include "bar.h"
#include "foo.h"
2 changes: 2 additions & 0 deletions clang-tools-extra/clangd/test/Inputs/include-cleaner/bar.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#pragma once
class Bar {};
2 changes: 2 additions & 0 deletions clang-tools-extra/clangd/test/Inputs/include-cleaner/foo.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#pragma once
class Foo {};

0 comments on commit 4b1cec0

Please sign in to comment.