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

[chomosuke]

The problem:
When running the code action for some clang-tidy checks with clangd, there will be commas left over after the code action is completed, causing a compile error and requiring the user to manually fix the error. An example is #118568.

However, this isn't a problem when running clang-tidy --fix. This is because clang-tidy --fix runs format::cleanupAroundReplacements() before committing the fixes where as clangd does not.

This PR fixes the problem by running format::cleanupAroundReplacements() in clangd for every quickfix.

[github-actions]

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]

@llvm/pr-subscribers-clangd
@llvm/pr-subscribers-clang

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

Author: Richard Li (chomosuke)

Changes

The problem:
When running the code action for some clang-tidy checks with clangd, there will be commas left over after the code action is completed, causing a compile error and requiring the user to manually fix the error. An example is #118568.

However, this isn't a problem when running clang-tidy --fix. This is because clang-tidy --fix runs format::cleanupAroundReplacements() before committing the fixes where as clangd does not.

This PR fixes the problem by running format::cleanupAroundReplacements() in clangd for every quickfix.

---

Full diff: https://github.com/llvm/llvm-project/pull/118569.diff

5 Files Affected:

• (modified) clang-tools-extra/clang-tidy/ClangTidy.cpp (+4-18)
• (modified) clang-tools-extra/clangd/Diagnostics.cpp (+33-2)
• (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+43-8)
• (modified) clang/include/clang/Tooling/Core/Replacement.h (+4)
• (modified) clang/lib/Tooling/Core/Replacement.cpp (+15-1)

diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 9c8c93c5d16c72..82331c724eaaf2 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -149,26 +149,12 @@ class ErrorReporter {
                                    Repl.getLength(), Repl.getReplacementText());
             auto &Entry = FileReplacements[R.getFilePath()];
             Replacements &Replacements = Entry.Replaces;
-            llvm::Error Err = Replacements.add(R);
+            llvm::Error Err = Replacements.addOrMerge(R);
             if (Err) {
               // 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;
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index a59d1e7ac84096..60c6ac7256b58c 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/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"
@@ -741,7 +742,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
@@ -761,7 +762,37 @@ 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);
+      auto Err = Replacements->addOrMerge(R);
+      if (Err) {
+        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());
+      auto Repl = format::cleanupAroundReplacements(Code, *Replacements,
+                                                    format::getNoStyle());
+      if (!Repl) {
+        log("Skipping formatting the replacement due to conflict: {0}",
+            llvm::toString(std::move(Repl.takeError())));
+        Replacements = std::nullopt;
+      } else {
+        auto 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;
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 7a47d6ebebf3b2..b4d365a1fabf2d 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/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
diff --git a/clang/include/clang/Tooling/Core/Replacement.h b/clang/include/clang/Tooling/Core/Replacement.h
index f9452111e147f1..341ef4bf1c1ce0 100644
--- a/clang/include/clang/Tooling/Core/Replacement.h
+++ b/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;
diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp
index 92e9859ca206e5..b989b865f63106 100644
--- a/clang/lib/Tooling/Core/Replacement.cpp
+++ b/clang/lib/Tooling/Core/Replacement.cpp
@@ -342,6 +342,20 @@ llvm::Error Replacements::add(const Replacement &R) {
   return llvm::Error::success();
 }
 
+llvm::Error Replacements::addOrMerge(const Replacement &R) {
+  auto Err = add(R);
+  if (Err) {
+    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 +592,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();
 

[chomosuke]

Ping

[HerrCai0907]

clang-tidy part is fine for me. If it's only a refactor for clang-tidy, you could remove the [clang-tidy] in title.

[llvm-beanz]

This mostly looks good to me. I have a few style nits to use more idiomatic patterns and conform better to LLVM coding standards.

[chomosuke]

Thank you for the review @llvm-beanz, I've fixed the suggestions :).

[kadircet]

we deliberately do not apply formatting here due to latency concerns. there can be many fix-its available in a file and eagerly formatting all of them is likely to affect diagnostics latencies (as clangd potentially re-evaluates them at every keystroke).

can we rather perform this when the user asks for a code-action to be applied?