https://github.com/voyager-jhk created https://github.com/llvm/llvm-project/pull/208682
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 >From 69ff8476905b06044475004614b8ba5d1748126e Mon Sep 17 00:00:00 2001 From: voyager-jhk <[email protected]> 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 | 17 ++++-- .../clangd/unittests/DiagnosticsTests.cpp | 42 ++++++++++++-- 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, 118 insertions(+), 61 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..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; } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
