[clang] [clang-tools-extra] [clangd][clang-tidy] Factor out mergeFixits() logic and reuse it in clangd (PR #208682)
Peiqi Li via cfe-commits
cfe-commits at lists.llvm.org
Sat Jul 18 08:51:11 PDT 2026
https://github.com/voyager-jhk updated https://github.com/llvm/llvm-project/pull/208682
>From 7723513cdb686f880b32f1d040b666686f4f2b78 Mon Sep 17 00:00:00 2001
From: voyager-jhk <voyager.lpq at gmail.com>
Date: Fri, 10 Jul 2026 17:55:39 +0800
Subject: [PATCH] [clangd][clang-tidy] Factor out mergeFixits() logic and reuse
it in clangd
This patch extracts mergeFixits() from DiagnosticRenderer to clang::edit
and reuses it in clangd. This prevents syntax errors caused by token merging
when applying clang-tidy fixits via clangd.
Fixes #207618
---
clang-tools-extra/clangd/CMakeLists.txt | 1 +
clang-tools-extra/clangd/Diagnostics.cpp | 16 ++++-
.../clangd/unittests/DiagnosticsTests.cpp | 66 ++++++++++++++++---
clang-tools-extra/docs/ReleaseNotes.rst | 2 +
clang/include/clang/Edit/EditedSource.h | 9 +++
clang/lib/Edit/EditedSource.cpp | 56 ++++++++++++++++
clang/lib/Frontend/DiagnosticRenderer.cpp | 52 +--------------
7 files changed, 140 insertions(+), 62 deletions(-)
diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt
index 890562dde1792..6e8ffd80e965a 100644
--- a/clang-tools-extra/clangd/CMakeLists.txt
+++ b/clang-tools-extra/clangd/CMakeLists.txt
@@ -165,6 +165,7 @@ clang_target_link_libraries(clangDaemon
clangBasic
clangDependencyScanning
clangDriver
+ clangEdit
clangOptions
clangFormat
clangFrontend
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index d0baf0224a18e..9ac568f9a29df 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -20,6 +20,7 @@
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TokenKinds.h"
+#include "clang/Edit/EditedSource.h"
#include "clang/Lex/Lexer.h"
#include "clang/Lex/Token.h"
#include "llvm/ADT/ArrayRef.h"
@@ -787,7 +788,6 @@ 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;
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
@@ -807,13 +807,23 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
return false;
if (!isInsideMainFile(FixIt.RemoveRange.getBegin(), SM))
return false;
+ }
+ llvm::SmallVector<FixItHint> MergedFixIts;
+ clang::edit::mergeFixits(FixIts, SM, *LangOpts, MergedFixIts);
+ if (MergedFixIts.empty())
+ return false;
+ llvm::SmallVector<TextEdit> Edits;
+ Edits.reserve(MergedFixIts.size());
+ for (const auto &FixIt : MergedFixIts) {
+ if (!isInsideMainFile(FixIt.RemoveRange.getBegin(), SM))
+ return false;
Edits.push_back(toTextEdit(FixIt, SM, *LangOpts));
}
llvm::SmallString<64> Message;
// If requested and possible, create a message like "change 'foo' to 'bar'".
- if (SyntheticMessage && FixIts.size() == 1) {
- const auto &FixIt = FixIts.front();
+ if (SyntheticMessage && MergedFixIts.size() == 1) {
+ const auto &FixIt = MergedFixIts.front();
bool Invalid = false;
llvm::StringRef Remove =
Lexer::getSourceText(FixIt.RemoveRange, SM, *LangOpts, &Invalid);
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 4258f7faf34fd..cbc5c50887a75 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -14,6 +14,7 @@
#include "FeatureModule.h"
#include "ParsedAST.h"
#include "Protocol.h"
+#include "SourceCode.h"
#include "TestFS.h"
#include "TestIndex.h"
#include "TestTU.h"
@@ -28,9 +29,11 @@
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceManager.h"
#include "clang/Basic/Specifiers.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/JSON.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/TargetSelect.h"
@@ -911,17 +914,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(
@@ -956,14 +959,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
@@ -1977,10 +1980,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) {
@@ -2204,6 +2207,53 @@ TEST(DiagnosticsTest, DontSuppressSubcategories) {
ElementsAre(diagName("-Wunreachable-code-break")));
}
+// Simulate a diagnostic with two removal fixits.
+TEST(DiagnosticsTest, TokenMergeFixit) {
+ Annotations Test(R"cpp(
+ int test() {
+ return$lparen[[(]]0$rparen[[)]];
+ }
+ )cpp");
+
+ SourceManagerForFile SM("TestTU.cpp", Test.code());
+ DiagnosticsEngine &DE = SM.get().getDiagnostics();
+ StoreDiags DiagConsumer;
+ DE.setClient(&DiagConsumer, /*ShouldOwnClient=*/false);
+
+ LangOptions LangOpts;
+ DiagConsumer.BeginSourceFile(LangOpts, /*PP=*/nullptr);
+ {
+ auto Loc = [&](Position P) {
+ return llvm::cantFail(sourceLocationInMainFile(SM.get(), P));
+ };
+ SourceLocation LParenBegin = Loc(Test.range("lparen").start);
+ SourceLocation LParenEnd = Loc(Test.range("lparen").end);
+ SourceLocation RParenBegin = Loc(Test.range("rparen").start);
+ SourceLocation RParenEnd = Loc(Test.range("rparen").end);
+
+ unsigned ID = DE.getCustomDiagID(DiagnosticsEngine::Warning,
+ "redundant parentheses around expression");
+ DiagnosticBuilder DB = DE.Report(LParenBegin, ID);
+ DB.AddFixItHint(FixItHint::CreateRemoval(
+ CharSourceRange::getCharRange(LParenBegin, LParenEnd)));
+ DB.AddFixItHint(FixItHint::CreateRemoval(
+ CharSourceRange::getCharRange(RParenBegin, RParenEnd)));
+ }
+ DiagConsumer.EndSourceFile();
+
+ auto Diags = DiagConsumer.take();
+
+ ASSERT_EQ(Diags.size(), 1u);
+ ASSERT_EQ(Diags[0].Fixes.size(), 1u);
+ const auto &Fix = Diags[0].Fixes[0];
+
+ ASSERT_EQ(Fix.Edits.size(), 2u);
+ EXPECT_EQ(Fix.Edits[0].range, Test.range("lparen"));
+ EXPECT_EQ(Fix.Edits[0].newText, " ");
+ EXPECT_EQ(Fix.Edits[1].range, Test.range("rparen"));
+ EXPECT_EQ(Fix.Edits[1].newText, "");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 99fe37f4145dd..8b308bbe06f80 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -171,6 +171,8 @@ Code actions
- A new tweak "Create function body out-of-line" was added that creates
an implementation for a function declaration.
+- clangd now applies clang-tidy fix-it post-processing before exposing fixes.
+
Signature help
^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Edit/EditedSource.h b/clang/include/clang/Edit/EditedSource.h
index ab76b0d719b05..56aab76dd0932 100644
--- a/clang/include/clang/Edit/EditedSource.h
+++ b/clang/include/clang/Edit/EditedSource.h
@@ -13,6 +13,7 @@
#include "clang/Basic/LLVM.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Edit/FileOffset.h"
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
@@ -26,6 +27,7 @@ namespace clang {
class LangOptions;
class PPConditionalDirectiveRecord;
class SourceManager;
+class FixItHint;
namespace edit {
@@ -110,6 +112,13 @@ class EditedSource {
void finishedCommit();
};
+/// Merges \p FixItHints into a normalized set of file edits.
+///
+/// \p MergedFixits is cleared before use. Removals may be adjusted to avoid
+/// changing token boundaries.
+void mergeFixits(ArrayRef<FixItHint> FixItHints, const SourceManager &SM,
+ const LangOptions &LangOpts,
+ SmallVectorImpl<FixItHint> &MergedFixits);
} // namespace edit
} // namespace clang
diff --git a/clang/lib/Edit/EditedSource.cpp b/clang/lib/Edit/EditedSource.cpp
index 398cce71d5e27..832bf5e474c18 100644
--- a/clang/lib/Edit/EditedSource.cpp
+++ b/clang/lib/Edit/EditedSource.cpp
@@ -8,6 +8,7 @@
#include "clang/Edit/EditedSource.h"
#include "clang/Basic/CharInfo.h"
+#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
@@ -476,3 +477,58 @@ EditedSource::getActionForOffset(FileOffset Offs) {
return FileEdits.end();
}
+
+namespace clang {
+namespace edit {
+
+namespace {
+
+class FixitReceiver : public edit::EditsReceiver {
+ SmallVectorImpl<FixItHint> &MergedFixits;
+
+public:
+ FixitReceiver(SmallVectorImpl<FixItHint> &MergedFixits)
+ : MergedFixits(MergedFixits) {}
+
+ void insert(SourceLocation loc, StringRef text) override {
+ MergedFixits.push_back(FixItHint::CreateInsertion(loc, text));
+ }
+
+ void replace(CharSourceRange range, StringRef text) override {
+ MergedFixits.push_back(FixItHint::CreateReplacement(range, text));
+ }
+};
+
+} // namespace
+
+void mergeFixits(ArrayRef<FixItHint> FixItHints, const SourceManager &SM,
+ const LangOptions &LangOpts,
+ SmallVectorImpl<FixItHint> &MergedFixits) {
+ MergedFixits.clear();
+ edit::Commit commit(SM, LangOpts);
+ for (const auto &Hint : FixItHints)
+ if (Hint.CodeToInsert.empty()) {
+ if (Hint.InsertFromRange.isValid())
+ commit.insertFromRange(Hint.RemoveRange.getBegin(),
+ Hint.InsertFromRange, /*afterToken=*/false,
+ Hint.BeforePreviousInsertions);
+ else
+ commit.remove(Hint.RemoveRange);
+ } else {
+ if (Hint.RemoveRange.isTokenRange() ||
+ Hint.RemoveRange.getBegin() != Hint.RemoveRange.getEnd())
+ commit.replace(Hint.RemoveRange, Hint.CodeToInsert);
+ else
+ commit.insert(Hint.RemoveRange.getBegin(), Hint.CodeToInsert,
+ /*afterToken=*/false, Hint.BeforePreviousInsertions);
+ }
+
+ edit::EditedSource Editor(SM, LangOpts);
+ if (Editor.commit(commit)) {
+ FixitReceiver Rec(MergedFixits);
+ Editor.applyRewrites(Rec);
+ }
+}
+
+} // namespace edit
+} // namespace clang
diff --git a/clang/lib/Frontend/DiagnosticRenderer.cpp b/clang/lib/Frontend/DiagnosticRenderer.cpp
index 2b6fbc274e587..cb1c667ba3969 100644
--- a/clang/lib/Frontend/DiagnosticRenderer.cpp
+++ b/clang/lib/Frontend/DiagnosticRenderer.cpp
@@ -12,9 +12,7 @@
#include "clang/Basic/LLVM.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
-#include "clang/Edit/Commit.h"
#include "clang/Edit/EditedSource.h"
-#include "clang/Edit/EditsReceiver.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
@@ -34,54 +32,6 @@ DiagnosticRenderer::DiagnosticRenderer(const LangOptions &LangOpts,
DiagnosticRenderer::~DiagnosticRenderer() = default;
-namespace {
-
-class FixitReceiver : public edit::EditsReceiver {
- SmallVectorImpl<FixItHint> &MergedFixits;
-
-public:
- FixitReceiver(SmallVectorImpl<FixItHint> &MergedFixits)
- : MergedFixits(MergedFixits) {}
-
- void insert(SourceLocation loc, StringRef text) override {
- MergedFixits.push_back(FixItHint::CreateInsertion(loc, text));
- }
-
- void replace(CharSourceRange range, StringRef text) override {
- MergedFixits.push_back(FixItHint::CreateReplacement(range, text));
- }
-};
-
-} // namespace
-
-static void mergeFixits(ArrayRef<FixItHint> FixItHints,
- const SourceManager &SM, const LangOptions &LangOpts,
- SmallVectorImpl<FixItHint> &MergedFixits) {
- edit::Commit commit(SM, LangOpts);
- for (const auto &Hint : FixItHints)
- if (Hint.CodeToInsert.empty()) {
- if (Hint.InsertFromRange.isValid())
- commit.insertFromRange(Hint.RemoveRange.getBegin(),
- Hint.InsertFromRange, /*afterToken=*/false,
- Hint.BeforePreviousInsertions);
- else
- commit.remove(Hint.RemoveRange);
- } else {
- if (Hint.RemoveRange.isTokenRange() ||
- Hint.RemoveRange.getBegin() != Hint.RemoveRange.getEnd())
- commit.replace(Hint.RemoveRange, Hint.CodeToInsert);
- else
- commit.insert(Hint.RemoveRange.getBegin(), Hint.CodeToInsert,
- /*afterToken=*/false, Hint.BeforePreviousInsertions);
- }
-
- edit::EditedSource Editor(SM, LangOpts);
- if (Editor.commit(commit)) {
- FixitReceiver Rec(MergedFixits);
- Editor.applyRewrites(Rec);
- }
-}
-
void DiagnosticRenderer::emitDiagnostic(FullSourceLoc Loc,
DiagnosticsEngine::Level Level,
StringRef Message,
@@ -101,7 +51,7 @@ void DiagnosticRenderer::emitDiagnostic(FullSourceLoc Loc,
SmallVector<FixItHint, 8> MergedFixits;
if (!FixItHints.empty()) {
- mergeFixits(FixItHints, Loc.getManager(), LangOpts, MergedFixits);
+ edit::mergeFixits(FixItHints, Loc.getManager(), LangOpts, MergedFixits);
FixItHints = MergedFixits;
}
More information about the cfe-commits
mailing list