[clang] [clang-tools-extra] [clangd][clang-tidy] Make clangd run `format::cleanupAroundReplacements()` for all code actions just as clang-tidy does (PR #118569)
Richard Li via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 4 06:24:48 PST 2024
https://github.com/chomosuke updated https://github.com/llvm/llvm-project/pull/118569
>From d2257eb43bbd9ce2dfd8a13123c2048e1cf8b439 Mon Sep 17 00:00:00 2001
From: chomosuke <a13323600 at gmail.com>
Date: Wed, 4 Dec 2024 00:07:31 +0000
Subject: [PATCH 1/4] clangd cleanupAroundReplacements just as clang-tidy does
---
clang-tools-extra/clangd/Diagnostics.cpp | 48 +++++++++++++++++++++++-
1 file changed, 46 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index a59d1e7ac84096..af8d0d534af689 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,50 @@ 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->add(R);
+ if (Err) {
+ // FIXME: Remove duplicated code as in ClangTidy.cpp
+ unsigned NewOffset =
+ Replacements->getShiftedCodePosition(R.getOffset());
+ unsigned NewLength = Replacements->getShiftedCodePosition(
+ R.getOffset() + R.getLength()) -
+ NewOffset;
+ if (NewLength == R.getLength()) {
+ llvm::consumeError(std::move(Err));
+ R = tooling::Replacement(R.getFilePath(), NewOffset, NewLength,
+ R.getReplacementText());
+ Replacements = Replacements->merge(tooling::Replacements(R));
+ } else {
+ 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;
>From cbdc1e25e3a39472c068a742b0f40e9c71221521 Mon Sep 17 00:00:00 2001
From: chomosuke <a13323600 at gmail.com>
Date: Wed, 4 Dec 2024 01:15:57 +0000
Subject: [PATCH 2/4] fixed tests
---
.../clangd/unittests/DiagnosticsTests.cpp | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 7a47d6ebebf3b2..94765b48099eda 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) {
>From 0735f2821c36827c1b123d63d4ca9e716cc790dd Mon Sep 17 00:00:00 2001
From: chomosuke <a13323600 at gmail.com>
Date: Wed, 4 Dec 2024 13:16:31 +0000
Subject: [PATCH 3/4] Added test
---
.../clangd/unittests/DiagnosticsTests.cpp | 37 +++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 94765b48099eda..6d9f8d5931a0e5 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -2136,6 +2136,43 @@ 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
>From f0331d1bc663dfc24df7832798cb794e87e01efa Mon Sep 17 00:00:00 2001
From: chomosuke <a13323600 at gmail.com>
Date: Wed, 4 Dec 2024 14:24:25 +0000
Subject: [PATCH 4/4] removed duplicated code
---
clang-tools-extra/clang-tidy/ClangTidy.cpp | 34 ++++++-------------
clang-tools-extra/clangd/Diagnostics.cpp | 23 +++----------
.../include/clang/Tooling/Core/Replacement.h | 6 +++-
clang/lib/Tooling/Core/Replacement.cpp | 33 ++++++++++++------
4 files changed, 42 insertions(+), 54 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 9c8c93c5d16c72..fc9926e2105a9a 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;
@@ -424,8 +410,8 @@ ClangTidyASTConsumerFactory::createASTConsumer(
std::unique_ptr<ClangTidyProfiling> Profiling;
if (Context.getEnableProfiling()) {
- Profiling = std::make_unique<ClangTidyProfiling>(
- Context.getProfileStorageParams());
+ Profiling =
+ std::make_unique<ClangTidyProfiling>(Context.getProfileStorageParams());
FinderOptions.CheckProfiling.emplace(Profiling->Records);
}
@@ -437,8 +423,8 @@ ClangTidyASTConsumerFactory::createASTConsumer(
if (Context.canEnableModuleHeadersParsing() &&
Context.getLangOpts().Modules && OverlayFS != nullptr) {
- auto ModuleExpander = std::make_unique<ExpandModularHeadersPPCallbacks>(
- &Compiler, OverlayFS);
+ auto ModuleExpander =
+ std::make_unique<ExpandModularHeadersPPCallbacks>(&Compiler, OverlayFS);
ModuleExpanderPP = ModuleExpander->getPreprocessor();
PP->addPPCallbacks(std::move(ModuleExpander));
}
@@ -502,7 +488,7 @@ getCheckNames(const ClangTidyOptions &Options,
bool AllowEnablingAnalyzerAlphaCheckers) {
clang::tidy::ClangTidyContext Context(
std::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
- Options),
+ Options),
AllowEnablingAnalyzerAlphaCheckers);
ClangTidyASTConsumerFactory Factory(Context);
return Factory.getCheckNames();
@@ -513,7 +499,7 @@ getCheckOptions(const ClangTidyOptions &Options,
bool AllowEnablingAnalyzerAlphaCheckers) {
clang::tidy::ClangTidyContext Context(
std::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
- Options),
+ Options),
AllowEnablingAnalyzerAlphaCheckers);
ClangTidyDiagnosticConsumer DiagConsumer(Context);
DiagnosticsEngine DE(llvm::makeIntrusiveRefCnt<DiagnosticIDs>(),
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index af8d0d534af689..9f270af506fce1 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -765,25 +765,12 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
auto R = tooling::Replacement(SM, FixIt.RemoveRange, FixIt.CodeToInsert,
*LangOpts);
- auto Err = Replacements->add(R);
+ auto Err = Replacements->addOrMerge(R);
if (Err) {
- // FIXME: Remove duplicated code as in ClangTidy.cpp
- unsigned NewOffset =
- Replacements->getShiftedCodePosition(R.getOffset());
- unsigned NewLength = Replacements->getShiftedCodePosition(
- R.getOffset() + R.getLength()) -
- NewOffset;
- if (NewLength == R.getLength()) {
- llvm::consumeError(std::move(Err));
- R = tooling::Replacement(R.getFilePath(), NewOffset, NewLength,
- R.getReplacementText());
- Replacements = Replacements->merge(tooling::Replacements(R));
- } else {
- log("Skipping formatting the replacement due to conflict: {0}",
- llvm::toString(std::move(Err)));
- Replacements = std::nullopt;
- break;
- }
+ log("Skipping formatting the replacement due to conflict: {0}",
+ llvm::toString(std::move(Err)));
+ Replacements = std::nullopt;
+ break;
}
}
diff --git a/clang/include/clang/Tooling/Core/Replacement.h b/clang/include/clang/Tooling/Core/Replacement.h
index f9452111e147f1..835a68847592ba 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;
@@ -282,7 +286,7 @@ class Replacements {
const_iterator end() const { return Replaces.end(); }
- const_reverse_iterator rbegin() const { return Replaces.rbegin(); }
+ const_reverse_iterator rbegin() const { return Replaces.rbegin(); }
const_reverse_iterator rend() const { return Replaces.rend(); }
diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp
index 92e9859ca206e5..4586fbd7a03223 100644
--- a/clang/lib/Tooling/Core/Replacement.cpp
+++ b/clang/lib/Tooling/Core/Replacement.cpp
@@ -40,7 +40,7 @@
using namespace clang;
using namespace tooling;
-static const char * const InvalidLocation = "";
+static const char *const InvalidLocation = "";
Replacement::Replacement() : FilePath(InvalidLocation) {}
@@ -61,9 +61,7 @@ Replacement::Replacement(const SourceManager &Sources,
setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
}
-bool Replacement::isApplicable() const {
- return FilePath != InvalidLocation;
-}
+bool Replacement::isApplicable() const { return FilePath != InvalidLocation; }
bool Replacement::apply(Rewriter &Rewrite) const {
SourceManager &SM = Rewrite.getSourceMgr();
@@ -72,9 +70,8 @@ bool Replacement::apply(Rewriter &Rewrite) const {
return false;
FileID ID = SM.getOrCreateFileID(*Entry, SrcMgr::C_User);
- const SourceLocation Start =
- SM.getLocForStartOfFile(ID).
- getLocWithOffset(ReplacementRange.getOffset());
+ const SourceLocation Start = SM.getLocForStartOfFile(ID).getLocWithOffset(
+ ReplacementRange.getOffset());
// ReplaceText returns false on success.
// ReplaceText only fails if the source location is not a file location, in
// which case we already returned false earlier.
@@ -139,7 +136,8 @@ static int getRangeSize(const SourceManager &Sources,
SourceLocation SpellingEnd = Sources.getSpellingLoc(Range.getEnd());
std::pair<FileID, unsigned> Start = Sources.getDecomposedLoc(SpellingBegin);
std::pair<FileID, unsigned> End = Sources.getDecomposedLoc(SpellingEnd);
- if (Start.first != End.first) return -1;
+ if (Start.first != End.first)
+ return -1;
if (Range.isTokenRange())
End.second += Lexer::MeasureTokenLength(SpellingEnd, Sources, LangOpts);
return End.second - Start.second;
@@ -342,6 +340,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 +590,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();
@@ -593,8 +605,7 @@ llvm::Expected<std::string> applyAllReplacements(StringRef Code,
InMemoryFileSystem->addFile(
"<stdin>", 0, llvm::MemoryBuffer::getMemBuffer(Code, "<stdin>"));
FileID ID = SourceMgr.createFileID(*Files.getOptionalFileRef("<stdin>"),
- SourceLocation(),
- clang::SrcMgr::C_User);
+ SourceLocation(), clang::SrcMgr::C_User);
for (auto I = Replaces.rbegin(), E = Replaces.rend(); I != E; ++I) {
Replacement Replace("<stdin>", I->getOffset(), I->getLength(),
I->getReplacementText());
More information about the cfe-commits
mailing list