[clang] [clang-tools-extra] [clangd][clang-tidy] Factor out mergeFixits() logic and reuse it in clangd (PR #208682)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 10 03:06:03 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Peiqi Li (voyager-jhk)
<details>
<summary>Changes</summary>
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
---
Full diff: https://github.com/llvm/llvm-project/pull/208682.diff
7 Files Affected:
- (modified) clang-tools-extra/clangd/CMakeLists.txt (+1)
- (modified) clang-tools-extra/clangd/Diagnostics.cpp (+13-4)
- (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+36-6)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2)
- (modified) clang/include/clang/Edit/EditedSource.h (+9)
- (modified) clang/lib/Edit/EditedSource.cpp (+56)
- (modified) clang/lib/Frontend/DiagnosticRenderer.cpp (+1-51)
``````````diff
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..27a57235799c4 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
@@ -803,8 +803,17 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
}
// Otherwise, follow clang's behavior: no fixits in macros.
if (FixIt.RemoveRange.getBegin().isMacroID() ||
- FixIt.RemoveRange.getEnd().isMacroID())
+ FixIt.RemoveRange.getEnd().isMacroID() ||
+ !isInsideMainFile(FixIt.RemoveRange.getBegin(), SM))
return false;
+ }
+ llvm::SmallVector<FixItHint, 4> MergedFixIts;
+ clang::edit::mergeFixits(FixIts, SM, *LangOpts, MergedFixIts);
+ if (MergedFixIts.empty())
+ return false;
+ llvm::SmallVector<TextEdit, 1> 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));
@@ -812,8 +821,8 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
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..bff8b2f61aa66 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -911,17 +911,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 +956,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
@@ -2204,6 +2204,36 @@ TEST(DiagnosticsTest, DontSuppressSubcategories) {
ElementsAre(diagName("-Wunreachable-code-break")));
}
+TEST(DiagnosticsTest, TokenMergeFixit) {
+ Annotations Test(R"cpp(
+ int test() {
+ return$lparen[[(]]0$rparen[[)]];
+ }
+ )cpp");
+
+ auto TU = TestTU::withCode(Test.code());
+ TU.ClangTidyProvider = addTidyChecks("readability-redundant-parentheses");
+ auto AST = TU.build();
+ auto Diags = AST.getDiagnostics();
+ if (!CLANGD_TIDY_CHECKS) {
+ EXPECT_THAT(Diags, IsEmpty());
+ return;
+ }
+
+ ASSERT_EQ(Diags.size(), 1u);
+ EXPECT_EQ(Diags[0].Message, "redundant parentheses around expression");
+
+ ASSERT_EQ(Diags[0].Fixes.size(), 1u);
+ const auto &Fix = Diags[0].Fixes[0];
+ EXPECT_EQ(Fix.Message, "redundant parentheses around expression");
+
+ 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;
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/208682
More information about the cfe-commits
mailing list