https://github.com/voyager-jhk updated https://github.com/llvm/llvm-project/pull/208682
>From 8ac7de61efe6ad9fb21c2f157ca5942fe4f49eff 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 | 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 9fa7d92d8374c..151f4ee028b87 100644 --- a/clang-tools-extra/clangd/CMakeLists.txt +++ b/clang-tools-extra/clangd/CMakeLists.txt @@ -166,6 +166,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 6d91ac1ef1e8e..e53731c2df35a 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" @@ -934,17 +937,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( @@ -979,14 +982,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 @@ -2000,10 +2003,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) { @@ -2227,6 +2230,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 69c3bcf67b8db..83bafdec8e419 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -73,6 +73,8 @@ Code completion Code actions ^^^^^^^^^^^^ +- 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
