Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clangd] Add 'apply all clangd fixes' and 'apply all '_' fixes' QuickFixes #79867

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

torshepherd
Copy link
Contributor

@torshepherd torshepherd commented Jan 29, 2024

This PR adds the following features

  1. If there are at least 2 distinct fixable diagnostics stemming from different clang or clang-tidy warnings overall and their edits are distinct as well, we add a Fix to each one that applies all edits.
  2. If there are at least 2 distinct fixable diagnostics stemming from the same code (either clang error source or clang-tidy check name) and their edits are distinct as well, we add a Fix to each one that applies all edits from that category.

For example when there are only readability-identifier-naming fixes:

image

When there are multiple categories of fixes, the hovered one being readability-identifier-naming,

image

This addresses clangd/clangd#830 (the server-side approach). In a follow-up, I plan to address clangd/clangd#1446 and listen for client-side fix all commands to do the same effect as 'apply all clangd fixes'. That only leaves clangd/clangd#1536 as the last open thread on auto-fixes, and I think we can close that one as "won't do unless we get more interest".

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 29, 2024

@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: Tor Shepherd (torshepherd)

Changes

This PR adds the following features

  1. If there are at least 2 distinct fixable diagnostics stemming from either clang or clang-tidy overall and their edits are distinct as well, we add a Fix to each one that applies all edits.
  2. If there are at least 2 distinct fixable diagnostics stemming from the same code (either clang error source or clang-tidy check name) and their edits are distinct as well, we add a Fix to each one that applies all edits from that category.

For example,

image

This addresses clangd/clangd#830 (the server-side approach). In a follow-up, I plan to address clangd/clangd#1446 and listen for client-side fix all commands to do the same effect as 'apply all clangd fixes'. That only leaves clangd/clangd#1536 as the last open thread on auto-fixes, and I think we can close that one as "won't do unless we get more interest".


Patch is 23.48 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/79867.diff

3 Files Affected:

  • (modified) clang-tools-extra/clangd/Diagnostics.cpp (+73-1)
  • (modified) clang-tools-extra/clangd/Protocol.h (+13-9)
  • (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+143-57)
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index 704e61b1e4dd792..cf6d87cd4518145 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -339,6 +339,74 @@ std::string noteMessage(const Diag &Main, const DiagBase &Note,
   return capitalize(std::move(Result));
 }
 
+std::optional<Fix>
+generateApplyAllFromOption(const llvm::StringRef Name,
+                           llvm::ArrayRef<Diag *> AllDiagnostics) {
+  Fix ApplyAll;
+  for (auto *const Diag : AllDiagnostics) {
+    for (const auto &Fix : Diag->Fixes)
+      ApplyAll.Edits.insert(ApplyAll.Edits.end(), Fix.Edits.begin(),
+                            Fix.Edits.end());
+  }
+  llvm::sort(ApplyAll.Edits);
+  ApplyAll.Edits.erase(
+      std::unique(ApplyAll.Edits.begin(), ApplyAll.Edits.end()),
+      ApplyAll.Edits.end());
+  // Skip diagnostic categories that don't have multiple fixes to apply
+  if (ApplyAll.Edits.size() < 2U) {
+    return std::nullopt;
+  }
+  ApplyAll.Message = llvm::formatv("apply all '{0}' fixes", Name);
+  return ApplyAll;
+}
+
+std::optional<Fix>
+generateApplyAllFixesOption(llvm::ArrayRef<Diag> AllDiagnostics) {
+  Fix ApplyAll;
+  for (auto const &Diag : AllDiagnostics) {
+    for (const auto &Fix : Diag.Fixes)
+      ApplyAll.Edits.insert(ApplyAll.Edits.end(), Fix.Edits.begin(),
+                            Fix.Edits.end());
+  }
+  llvm::sort(ApplyAll.Edits);
+  ApplyAll.Edits.erase(
+      std::unique(ApplyAll.Edits.begin(), ApplyAll.Edits.end()),
+      ApplyAll.Edits.end());
+  if (ApplyAll.Edits.size() < 2U) {
+    return std::nullopt;
+  }
+  ApplyAll.Message = "apply all clangd fixes";
+  return ApplyAll;
+}
+
+void appendApplyAlls(std::vector<Diag> &AllDiagnostics) {
+  llvm::DenseMap<llvm::StringRef, std::vector<Diag *>> CategorizedFixes;
+  size_t FixableOverall = 0U;
+
+  for (auto &Diag : AllDiagnostics) {
+    // Keep track of fixable diagnostics for generating "apply all fixes"
+    if (!Diag.Fixes.empty()) {
+      ++FixableOverall;
+      if (auto [It, DidEmplace] = CategorizedFixes.try_emplace(
+              Diag.Name, std::vector<struct Diag *>{&Diag});
+          !DidEmplace)
+        It->second.emplace_back(&Diag);
+    }
+  }
+
+  auto FixAllClangd = generateApplyAllFixesOption(AllDiagnostics);
+  for (const auto &[Name, DiagsForThisCategory] : CategorizedFixes) {
+    auto FixAllForCategory =
+        generateApplyAllFromOption(Name, DiagsForThisCategory);
+    for (auto *Diag : DiagsForThisCategory) {
+      if (DiagsForThisCategory.size() >= 2U && FixAllForCategory.has_value())
+        Diag->Fixes.emplace_back(*FixAllForCategory);
+      if (FixableOverall >= 2U && FixAllClangd.has_value())
+        Diag->Fixes.emplace_back(*FixAllClangd);
+    }
+  }
+}
+
 void setTags(clangd::Diag &D) {
   static const auto *DeprecatedDiags = new llvm::DenseSet<unsigned>{
       diag::warn_access_decl_deprecated,
@@ -575,7 +643,8 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
   // Do not forget to emit a pending diagnostic if there is one.
   flushLastDiag();
 
-  // Fill in name/source now that we have all the context needed to map them.
+  // Fill in name/source now that we have all the context needed to map
+  // them.
   for (auto &Diag : Output) {
     if (const char *ClangDiag = getDiagnosticCode(Diag.ID)) {
       // Warnings controlled by -Wfoo are better recognized by that name.
@@ -619,6 +688,9 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
   llvm::erase_if(Output, [&](const Diag &D) {
     return !SeenDiags.emplace(D.Range, D.Message).second;
   });
+
+  appendApplyAlls(Output);
+
   return std::move(Output);
 }
 
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index e88c804692568f5..d1e6cf35f9d9de0 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -257,6 +257,10 @@ inline bool operator==(const TextEdit &L, const TextEdit &R) {
   return std::tie(L.newText, L.range, L.annotationId) ==
          std::tie(R.newText, R.range, L.annotationId);
 }
+inline bool operator<(const TextEdit &L, const TextEdit &R) {
+  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 &);
@@ -281,7 +285,7 @@ struct TextDocumentEdit {
   /// The text document to change.
   VersionedTextDocumentIdentifier textDocument;
 
-	/// The edits to be applied.
+  /// The edits to be applied.
   /// FIXME: support the AnnotatedTextEdit variant.
   std::vector<TextEdit> edits;
 };
@@ -557,7 +561,7 @@ struct ClientCapabilities {
 
   /// The client supports versioned document changes for WorkspaceEdit.
   bool DocumentChanges = false;
-  
+
   /// The client supports change annotations on text edits,
   bool ChangeAnnotation = false;
 
@@ -1013,12 +1017,12 @@ struct WorkspaceEdit {
   /// Versioned document edits.
   ///
   /// If a client neither supports `documentChanges` nor
-	/// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s
-	/// using the `changes` property are supported.
+  /// `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.
+  /// AnnotatedTextEdit.
   std::map<std::string, ChangeAnnotation> changeAnnotations;
 };
 bool fromJSON(const llvm::json::Value &, WorkspaceEdit &, llvm::json::Path);
@@ -1274,13 +1278,13 @@ enum class InsertTextFormat {
 /// Additional details for a completion item label.
 struct CompletionItemLabelDetails {
   /// An optional string which is rendered less prominently directly after label
-	/// without any spacing. Should be used for function signatures or type
+  /// without any spacing. Should be used for function signatures or type
   /// annotations.
   std::string detail;
 
   /// An optional string which is rendered less prominently after
-	/// CompletionItemLabelDetails.detail. Should be used for fully qualified
-	/// names or file path.
+  /// CompletionItemLabelDetails.detail. Should be used for fully qualified
+  /// names or file path.
   std::string description;
 };
 llvm::json::Value toJSON(const CompletionItemLabelDetails &);
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index a52b647b0029b4f..80d2d70db7b92b4 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -30,6 +30,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -41,6 +42,7 @@
 #include <memory>
 #include <optional>
 #include <string>
+#include <type_traits>
 #include <utility>
 #include <vector>
 
@@ -60,13 +62,10 @@ using ::testing::Pair;
 using ::testing::SizeIs;
 using ::testing::UnorderedElementsAre;
 
-::testing::Matcher<const Diag &> withFix(::testing::Matcher<Fix> FixMatcher) {
-  return Field(&Diag::Fixes, ElementsAre(FixMatcher));
-}
-
+template <typename... T>
 ::testing::Matcher<const Diag &> withFix(::testing::Matcher<Fix> FixMatcher1,
-                                         ::testing::Matcher<Fix> FixMatcher2) {
-  return Field(&Diag::Fixes, UnorderedElementsAre(FixMatcher1, FixMatcher2));
+                                         T... Rest) {
+  return Field(&Diag::Fixes, UnorderedElementsAre(FixMatcher1, Rest...));
 }
 
 ::testing::Matcher<const Diag &> withID(unsigned ID) {
@@ -124,9 +123,13 @@ MATCHER_P(equalToFix, Fix, "LSP fix " + llvm::to_string(Fix)) {
     return false;
   if (arg.Edits.size() != Fix.Edits.size())
     return false;
+  auto LHSEdits = arg.Edits;
+  auto RHSEdits = Fix.Edits;
+  llvm::sort(LHSEdits);
+  llvm::sort(RHSEdits);
   for (std::size_t I = 0; I < arg.Edits.size(); ++I) {
-    if (arg.Edits[I].range != Fix.Edits[I].range ||
-        arg.Edits[I].newText != Fix.Edits[I].newText)
+    if (LHSEdits[I].range != RHSEdits[I].range ||
+        LHSEdits[I].newText != RHSEdits[I].newText)
       return false;
   }
   return true;
@@ -175,6 +178,22 @@ o]]();
       $macro[[ID($macroarg[[fod]])]]();
     }
   )cpp");
+
+  clangd::Fix ExpectedUndeclaredVar;
+  ExpectedUndeclaredVar.Message =
+      "apply all 'undeclared_var_use_suggest' fixes";
+  ExpectedUndeclaredVar.Edits.push_back(TextEdit{Test.range("typo"), "foo"});
+  ExpectedUndeclaredVar.Edits.push_back(
+      TextEdit{Test.range("macroarg"), "foo"});
+
+  clangd::Fix ExpectedFixAll;
+  ExpectedFixAll.Message = "apply all clangd fixes";
+  ExpectedFixAll.Edits.push_back(TextEdit{Test.range("insertstar"), "*"});
+  ExpectedFixAll.Edits.push_back(TextEdit{Test.range("semicolon"), ";"});
+  ExpectedFixAll.Edits.insert(ExpectedFixAll.Edits.end(),
+                              ExpectedUndeclaredVar.Edits.begin(),
+                              ExpectedUndeclaredVar.Edits.end());
+
   auto TU = TestTU::withCode(Test.code());
   EXPECT_THAT(
       TU.build().getDiagnostics(),
@@ -183,20 +202,24 @@ o]]();
           AllOf(Diag(Test.range("range"),
                      "invalid range expression of type 'struct Container *'; "
                      "did you mean to dereference it with '*'?"),
-                withFix(Fix(Test.range("insertstar"), "*", "insert '*'"))),
+                withFix(Fix(Test.range("insertstar"), "*", "insert '*'"),
+                        equalToFix(ExpectedFixAll))),
           // This range spans lines.
-          AllOf(Diag(Test.range("typo"),
-                     "use of undeclared identifier 'goo'; did you mean 'foo'?"),
-                diagSource(Diag::Clang), diagName("undeclared_var_use_suggest"),
-                withFix(
-                    Fix(Test.range("typo"), "foo", "change 'go\\…' to 'foo'")),
-                // This is a pretty normal range.
-                withNote(Diag(Test.range("decl"), "'foo' declared here"))),
+          AllOf(
+              Diag(Test.range("typo"),
+                   "use of undeclared identifier 'goo'; did you mean 'foo'?"),
+              diagSource(Diag::Clang), diagName("undeclared_var_use_suggest"),
+              withFix(Fix(Test.range("typo"), "foo", "change 'go\\…' to 'foo'"),
+                      equalToFix(ExpectedUndeclaredVar),
+                      equalToFix(ExpectedFixAll)),
+              // This is a pretty normal range.
+              withNote(Diag(Test.range("decl"), "'foo' declared here"))),
           // This range is zero-width and insertion. Therefore make sure we are
           // not expanding it into other tokens. Since we are not going to
           // replace those.
           AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"),
-                withFix(Fix(Test.range("semicolon"), ";", "insert ';'"))),
+                withFix(Fix(Test.range("semicolon"), ";", "insert ';'"),
+                        equalToFix(ExpectedFixAll))),
           // This range isn't provided by clang, we expand to the token.
           Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"),
           Diag(Test.range("type"),
@@ -207,8 +230,10 @@ o]]();
                "no member named 'test' in namespace 'test'"),
           AllOf(Diag(Test.range("macro"),
                      "use of undeclared identifier 'fod'; did you mean 'foo'?"),
-                withFix(Fix(Test.range("macroarg"), "foo",
-                            "change 'fod' to 'foo'")))));
+                withFix(
+                    Fix(Test.range("macroarg"), "foo", "change 'fod' to 'foo'"),
+                    equalToFix(ExpectedUndeclaredVar),
+                    equalToFix(ExpectedFixAll)))));
 }
 
 // Verify that the -Wswitch case-not-covered diagnostic range covers the
@@ -334,7 +359,8 @@ TEST(DiagnosticsTest, ClangTidy) {
                 diagSource(Diag::ClangTidy),
                 diagName("modernize-deprecated-headers"),
                 withFix(Fix(Test.range("deprecated"), "<cassert>",
-                            "change '\"assert.h\"' to '<cassert>'"))),
+                            "change '\"assert.h\"' to '<cassert>'"),
+                        fixMessage("apply all clangd fixes"))),
           Diag(Test.range("doubled"),
                "suspicious usage of 'sizeof(sizeof(...))'"),
           AllOf(Diag(Test.range("macroarg"),
@@ -350,8 +376,9 @@ TEST(DiagnosticsTest, ClangTidy) {
                 diagSource(Diag::ClangTidy),
                 diagName("modernize-use-trailing-return-type"),
                 // Verify there's no "[check-name]" suffix in the message.
-                withFix(fixMessage(
-                    "use a trailing return type for this function"))),
+                withFix(
+                    fixMessage("use a trailing return type for this function"),
+                    fixMessage("apply all clangd fixes"))),
           Diag(Test.range("foo"),
                "function 'foo' is within a recursive call chain"),
           Diag(Test.range("bar"),
@@ -825,21 +852,62 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiags) {
   clangd::Fix ExpectedDFix;
   ExpectedDFix.Message = "variable 'D' is not initialized";
   ExpectedDFix.Edits.push_back(TextEdit{Main.range("DFix"), " = NAN"});
+
+  clangd::Fix ExpectedMemberInitializerFix;
+  ExpectedMemberInitializerFix.Message =
+      "apply all 'cppcoreguidelines-prefer-member-initializer' fixes";
+  ExpectedMemberInitializerFix.Edits.insert(
+      ExpectedMemberInitializerFix.Edits.end(), ExpectedAFix.Edits.begin(),
+      ExpectedAFix.Edits.end());
+  ExpectedMemberInitializerFix.Edits.insert(
+      ExpectedMemberInitializerFix.Edits.end(), ExpectedBFix.Edits.begin(),
+      ExpectedBFix.Edits.end());
+
+  clangd::Fix ExpectedInitVariablesFix;
+  ExpectedInitVariablesFix.Message =
+      "apply all 'cppcoreguidelines-init-variables' fixes";
+  ExpectedInitVariablesFix.Edits.insert(ExpectedInitVariablesFix.Edits.end(),
+                                        ExpectedCFix.Edits.begin(),
+                                        ExpectedCFix.Edits.end());
+  ExpectedInitVariablesFix.Edits.insert(ExpectedInitVariablesFix.Edits.end(),
+                                        ExpectedDFix.Edits.begin(),
+                                        ExpectedDFix.Edits.end());
+
+  clangd::Fix ExpectedFixAll;
+  ExpectedFixAll.Message = "apply all clangd fixes";
+  ExpectedFixAll.Edits.insert(ExpectedFixAll.Edits.end(),
+                              ExpectedMemberInitializerFix.Edits.begin(),
+                              ExpectedMemberInitializerFix.Edits.end());
+  ExpectedFixAll.Edits.insert(ExpectedFixAll.Edits.end(),
+                              ExpectedInitVariablesFix.Edits.begin(),
+                              ExpectedInitVariablesFix.Edits.end());
+
+  // This edit is duplicated in C, so add it after inserting all of the "fix
+  // all" edits
   ExpectedDFix.Edits.push_back(
       TextEdit{Main.range("MathHeader"), "#include <math.h>\n\n"});
+
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       ifTidyChecks(UnorderedElementsAre(
           AllOf(Diag(Main.range("A"), "'A' should be initialized in a member "
                                       "initializer of the constructor"),
-                withFix(equalToFix(ExpectedAFix))),
+                withFix(equalToFix(ExpectedAFix),
+                        equalToFix(ExpectedMemberInitializerFix),
+                        equalToFix(ExpectedFixAll))),
           AllOf(Diag(Main.range("B"), "'B' should be initialized in a member "
                                       "initializer of the constructor"),
-                withFix(equalToFix(ExpectedBFix))),
+                withFix(equalToFix(ExpectedBFix),
+                        equalToFix(ExpectedMemberInitializerFix),
+                        equalToFix(ExpectedFixAll))),
           AllOf(Diag(Main.range("C"), "variable 'C' is not initialized"),
-                withFix(equalToFix(ExpectedCFix))),
+                withFix(equalToFix(ExpectedCFix),
+                        equalToFix(ExpectedInitVariablesFix),
+                        equalToFix(ExpectedFixAll))),
           AllOf(Diag(Main.range("D"), "variable 'D' is not initialized"),
-                withFix(equalToFix(ExpectedDFix))))));
+                withFix(equalToFix(ExpectedDFix),
+                        equalToFix(ExpectedInitVariablesFix),
+                        equalToFix(ExpectedFixAll))))));
 }
 
 TEST(DiagnosticsTest, Preprocessor) {
@@ -1281,32 +1349,42 @@ using Type = ns::$template[[Foo]]<int>;
           AllOf(Diag(Test.range("unqualified1"), "unknown type name 'X'"),
                 diagName("unknown_typename"),
                 withFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-                            "Include \"x.h\" for symbol ns::X"))),
+                            "Include \"x.h\" for symbol ns::X"),
+                        fixMessage("apply all clangd fixes"))),
           Diag(Test.range("unqualified2"), "use of undeclared identifier 'X'"),
-          AllOf(Diag(Test.range("qualified1"),
-                     "no type named 'X' in namespace 'ns'"),
-                diagName("typename_nested_not_found"),
-                withFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-                            "Include \"x.h\" for symbol ns::X"))),
+          AllOf(
+              Diag(Test.range("qualified1"),
+                   "no type named 'X' in namespace 'ns'"),
+              diagName("typename_nested_not_found"),
+              withFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+                          "Include \"x.h\" for symbol ns::X"),
+                      fixMessage("apply all 'typename_nested_not_found' fixes"),
+                      fixMessage("apply all clangd fixes"))),
           AllOf(Diag(Test.range("qualified2"),
                      "no member named 'X' in namespace 'ns'"),
                 diagName("no_member"),
                 withFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-                            "Include \"x.h\" for symbol ns::X"))),
-          AllOf(Diag(Test.range("global"),
-                     "no type named 'Global' in the global namespace"),
-                diagName("typename_nested_not_found"),
-                withFix(Fix(Test.range("insert"), "#include \"global.h\"\n",
-                            "Include \"global.h\" for symbol Global"))),
+                            "Include \"x.h\" for symbol ns::X"),
+                        fixMessage("apply all clangd fixes"))),
+          AllOf(
+              Diag(Test.range("global"),
+                   "no type named 'Global' in the global namespace"),
+              diagName("typename_nested_not_found"),
+              withFix(Fix(Test.range("insert"), "#include \"global.h\"\n",
+                          "Include \"global.h\" for symbol Global"),
+                      fixMessage("apply all 'typename_nested_not_found' fixes"),
+                      fixMessage("apply all clangd fixes"))),
           AllOf(Diag(Test.range("template"),
                      "no template named 'Foo' in namespace 'ns'"),
                 diagName("no_member_template"),
                 withFix(Fix(Test.range("insert"), "#include \"foo.h\"\n",
-                            "Include \"foo.h\" for symbol ns::Foo"))),
+                            "Include \"foo.h\" for symbol ns::Foo"),
+                        fixMessage("apply all clangd fixes"))),
           AllOf(Diag(Test.range("base"), "expected class name"),
                 diagName("expected_class_name"),
                 withFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-                            "Include \"x.h\" for symbol ns::X")))));
+                            "Include \"x.h\" for symbol ns::X"),
+                        fixMessage("apply all clangd fixes")))));
 }
 
 TEST(IncludeFixerTest, TypoInMacro) {
@@ -1327,7 +1405,9 @@ ID(ns::X a6);
   // FIXME: -fms-compatibility (which is default on windows) bre...
[truncated]

@torshepherd
Copy link
Contributor Author

Another thing that would be nice to solve in a follow-up (applies to unused includes as well btw) is to clean up the list of fixes when someone selects all and presses quick fix:

image

Not sure if this is possible server-side though

@torshepherd
Copy link
Contributor Author

Also, I noticed that the conversion function toCodeAction(const Fix& F, ...) specifically hardcodes all Fix objects to QUICKFIX_KIND. When we do clangd/clangd#1446, it would be good to add a string for SOURCE_FIX_ALL_KIND and set these fix-alls to that kind

@torshepherd
Copy link
Contributor Author

torshepherd commented Jan 30, 2024

Hmm I noticed another mini-issue. Ideally we shouldn't have a 'fix all clangd' quick fix if all of the issues come from the same source, only the 'fix all _' option

Edit: Fixed this

@torshepherd
Copy link
Contributor Author

Also pinging @kadircet who recently worked on Include Cleaner's similar batch fixes

@torshepherd
Copy link
Contributor Author

Ping

@torshepherd
Copy link
Contributor Author

Bump @HighCommander4 - did you get a chance to review this?

I've been using this with great success in my local build for several months now, and the feature is extremely handy.

There is a slight bug that overlapping fixes are troublesome if you choose "clangd apply all", but it's minor. I can pretty easily push a fix to this, but I don't want to work more on this unless it has a chance of being reviewed/accepted

@HighCommander4
Copy link
Collaborator

Bump @HighCommander4 - did you get a chance to review this?

Hi @torshepherd! Sorry for not being more responsive on this. I haven't had as much time as I'd like to spend on clangd reviews recently, and certainly not enough to keep up with the volume of review requests that I get.

That said, I haven't forgotten about this patch, and it's definitely on a shortlist that I'm planning to make time to look at over the next couple of weeks.

@torshepherd
Copy link
Contributor Author

Ah, awesome. In that case note that I'll make the option not show up when changes would have conflicting overlap ranges

@torshepherd
Copy link
Contributor Author

@HighCommander4 I've pushed up the change that doesn't allow conflicting fixits. I'll now be smoke-testing this over the coming weeks while I work, but consider it ready for review again whenever you have a chance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants