[clangd] Make clangd run format::cleanupAroundReplacements() for all code actions just as clang-tidy does

clang-tools-extra/clang-tidy/ClangTidy.cpp

@@ -149,26 +149,11 @@ class ErrorReporter {
                                    Repl.getLength(), Repl.getReplacementText());
             auto &Entry = FileReplacements[R.getFilePath()];
             Replacements &Replacements = Entry.Replaces;
-            llvm::Error Err = Replacements.add(R);
-            if (Err) {
+            if (llvm::Error Err = Replacements.addOrMerge(R)) {
               // FIXME: Implement better conflict handling.
-              llvm::errs() << "Trying to resolve conflict: "
-                           << llvm::toString(std::move(Err)) << "\n";
-              unsigned NewOffset =
-                  Replacements.getShiftedCodePosition(R.getOffset());
-              unsigned NewLength = Replacements.getShiftedCodePosition(
-                                       R.getOffset() + R.getLength()) -
-                                   NewOffset;
-              if (NewLength == R.getLength()) {
-                R = Replacement(R.getFilePath(), NewOffset, NewLength,
-                                R.getReplacementText());
-                Replacements = Replacements.merge(tooling::Replacements(R));
-                CanBeApplied = true;
-                ++AppliedFixes;
-              } else {
-                llvm::errs()
-                    << "Can't resolve conflict, skipping the replacement.\n";
-              }
+              llvm::errs()
+                  << "Can't resolve conflict, skipping the replacement: "
+                  << llvm::toString(std::move(Err)) << '\n';
             } else {
               CanBeApplied = true;
               ++AppliedFixes;

clang-tools-extra/clangd/Diagnostics.cpp

@@ -19,6 +19,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
+#include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Token.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -751,7 +752,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
       return false;
     // Copy as we may modify the ranges.
     auto FixIts = Info.getFixItHints().vec();
-    llvm::SmallVector<TextEdit, 1> Edits;
+    auto Replacements = std::make_optional<tooling::Replacements>();
     for (auto &FixIt : FixIts) {
       // Allow fixits within a single macro-arg expansion to be applied.
       // This can be incorrect if the argument is expanded multiple times in
@@ -771,7 +772,35 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
         return false;
       if (!isInsideMainFile(FixIt.RemoveRange.getBegin(), SM))
         return false;
-      Edits.push_back(toTextEdit(FixIt, SM, *LangOpts));
+
+      auto R = tooling::Replacement(SM, FixIt.RemoveRange, FixIt.CodeToInsert,
+                                    *LangOpts);
+      if (llvm::Error Err = Replacements->addOrMerge(R)) {
+        log("Skipping formatting the replacement due to conflict: {0}",
+            llvm::toString(std::move(Err)));
+        Replacements = std::nullopt;
+        break;
+      }
+    }
+
+    llvm::SmallVector<TextEdit, 1> Edits;
+
+    if (Replacements) {
+      StringRef Code = SM.getBufferData(SM.getMainFileID());
+      Expected<tooling::Replacements> Repl = format::cleanupAroundReplacements(
+          Code, *Replacements, format::getNoStyle());
+      if (!Repl) {
+        log("Skipping formatting the replacement due to conflict: {0}",
+            llvm::toString(Repl.takeError()));
+        Replacements = std::nullopt;
+      } else {
+        std::vector<TextEdit> Es = replacementsToEdits(Code, *Repl);
+        Edits.append(Es.begin(), Es.end());
+      }
+    }
+    if (!Replacements) {
+      for (auto &FixIt : FixIts)
+        Edits.push_back(toTextEdit(FixIt, SM, *LangOpts));
     }
 
     llvm::SmallString<64> Message;

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

@@ -876,17 +876,17 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiags) {
 
   clangd::Fix ExpectedCFix;
   ExpectedCFix.Message = "variable 'C' is not initialized";
-  ExpectedCFix.Edits.push_back(TextEdit{Main.range("CFix"), " = NAN"});
   ExpectedCFix.Edits.push_back(
       TextEdit{Main.range("MathHeader"), "#include <math.h>\n\n"});
+  ExpectedCFix.Edits.push_back(TextEdit{Main.range("CFix"), " = NAN"});
 
   // Again in clang-tidy only the include directive would be emitted for the
   // first warning. However we need the include attaching for both warnings.
   clangd::Fix ExpectedDFix;
   ExpectedDFix.Message = "variable 'D' is not initialized";
-  ExpectedDFix.Edits.push_back(TextEdit{Main.range("DFix"), " = NAN"});
   ExpectedDFix.Edits.push_back(
       TextEdit{Main.range("MathHeader"), "#include <math.h>\n\n"});
+  ExpectedDFix.Edits.push_back(TextEdit{Main.range("DFix"), " = NAN"});
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       ifTidyChecks(UnorderedElementsAre(
@@ -921,14 +921,14 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiagsFormatting) {
   clangd::Fix const ExpectedFix1{
       "prefer using 'override' or (rarely) 'final' "
       "instead of 'virtual'",
-      {TextEdit{Main.range("override1"), " override"},
-       TextEdit{Main.range("virtual1"), ""}},
+      {TextEdit{Main.range("virtual1"), ""},
+       TextEdit{Main.range("override1"), " override"}},
       {}};
   clangd::Fix const ExpectedFix2{
       "prefer using 'override' or (rarely) 'final' "
       "instead of 'virtual'",
-      {TextEdit{Main.range("override2"), " override"},
-       TextEdit{Main.range("virtual2"), ""}},
+      {TextEdit{Main.range("virtual2"), ""},
+       TextEdit{Main.range("override2"), " override"}},
       {}};
   // Note that in the Fix we expect the "virtual" keyword and the following
   // whitespace to be deleted
@@ -1930,10 +1930,10 @@ TEST(ParsedASTTest, ModuleSawDiag) {
   TestTU TU;
 
   auto AST = TU.build();
-        #if 0
+#if 0
   EXPECT_THAT(AST.getDiagnostics(),
               testing::Contains(Diag(Code.range(), KDiagMsg.str())));
-        #endif
+#endif
 }
 
 TEST(Preamble, EndsOnNonEmptyLine) {
@@ -2136,6 +2136,41 @@ TEST(DiagnosticsTest, UnusedInHeader) {
   EXPECT_THAT(TU.build().getDiagnostics(), IsEmpty());
 }
 
+TEST(DiagnosticsTest, CleanupAroundReplacements) {
+  Annotations Test(R"cpp(
+    struct PositiveValueChar {
+      PositiveValueChar() : $c0fix[[c0()]]$comma[[,]] $c1fix[[c1()]] {}
+      const char $c0[[c0]];
+      wchar_t $c1[[c1]];
+    };
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  TU.ClangTidyProvider = addTidyChecks("modernize-use-default-member-init");
+
+  clangd::Fix C0Fix;
+  C0Fix.Message = "use default member initializer for 'c0'";
+  C0Fix.Edits.push_back(
+      TextEdit{{Test.range("c0fix").start, Test.range("comma").end}, ""});
+  C0Fix.Edits.push_back(
+      TextEdit{{Test.range("c0").end, Test.range("c0").end}, "{}"});
+
+  clangd::Fix C1Fix;
+  C1Fix.Message = "use default member initializer for 'c1'";
+  C1Fix.Edits.push_back(TextEdit{Test.range("comma"), ""});
+  C1Fix.Edits.push_back(TextEdit{Test.range("c1fix"), ""});
+  C1Fix.Edits.push_back(
+      TextEdit{{Test.range("c1").end, Test.range("c1").end}, "{}"});
+
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              ifTidyChecks(UnorderedElementsAre(
+                  AllOf(Diag(Test.range("c0"),
+                             "use default member initializer for 'c0'"),
+                        withFix(equalToFix(C0Fix))),
+                  AllOf(Diag(Test.range("c1"),
+                             "use default member initializer for 'c1'"),
+                        withFix(equalToFix(C1Fix))))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

@@ -67,6 +67,8 @@ Code completion
 Code actions
 ^^^^^^^^^^^^
 
+- Improved diagnostics code actions to better remove leftover commas and colons.
+
 Signature help
 ^^^^^^^^^^^^^^
 

clang/include/clang/Tooling/Core/Replacement.h

@@ -260,6 +260,10 @@ class Replacements {
   /// category of replacements.
   llvm::Error add(const Replacement &R);
 
+  /// Adds a replacement `R` into `Replaces` or merges it into `Replaces` by
+  /// applying all existing Replaces first if there is conflict.
+  llvm::Error addOrMerge(const Replacement &R);
+
   /// Merges \p Replaces into the current replacements. \p Replaces
   /// refers to code after applying the current replacements.
   [[nodiscard]] Replacements merge(const Replacements &Replaces) const;

clang/lib/Tooling/Core/Replacement.cpp

@@ -342,6 +342,19 @@ llvm::Error Replacements::add(const Replacement &R) {
   return llvm::Error::success();
 }
 
+llvm::Error Replacements::addOrMerge(const Replacement &R) {
+  if (llvm::Error Err = add(R)) {
+    llvm::consumeError(std::move(Err));
+    auto Replace = getReplacementInChangedCode(R);
+    if (Replace.getLength() != R.getLength()) {
+      return llvm::make_error<ReplacementError>(
+          replacement_error::overlap_conflict, R);
+    }
+    *this = merge(tooling::Replacements(Replace));
+  }
+  return llvm::Error::success();
+}
+
 namespace {
 
 // Represents a merged replacement, i.e. a replacement consisting of multiple
@@ -578,7 +591,7 @@ bool applyAllReplacements(const Replacements &Replaces, Rewriter &Rewrite) {
 }
 
 llvm::Expected<std::string> applyAllReplacements(StringRef Code,
-                                                const Replacements &Replaces) {
+                                                 const Replacements &Replaces) {
   if (Replaces.empty())
     return Code.str();