[clang] [clang-tools-extra] [clangd][clang-tidy] Make clangd run `format::cleanupAroundReplacements()` for all code actions just as clang-tidy does (PR #118569)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 4 06:44:52 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Richard Li (chomosuke)
<details>
<summary>Changes</summary>
The problem:
When running the code action for some clang-tidy checks with clangd, there will be commas left over after the code action is completed, causing a compile error and requiring the user to manually fix the error. An example is #<!-- -->118568.
However, this isn't a problem when running `clang-tidy --fix`. This is because `clang-tidy --fix` runs `format::cleanupAroundReplacements()` before committing the fixes where as clangd does not.
This PR fixes the problem by running `format::cleanupAroundReplacements()` in clangd for every quickfix.
---
Full diff: https://github.com/llvm/llvm-project/pull/118569.diff
5 Files Affected:
- (modified) clang-tools-extra/clang-tidy/ClangTidy.cpp (+4-18)
- (modified) clang-tools-extra/clangd/Diagnostics.cpp (+33-2)
- (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+43-8)
- (modified) clang/include/clang/Tooling/Core/Replacement.h (+4)
- (modified) clang/lib/Tooling/Core/Replacement.cpp (+15-1)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 9c8c93c5d16c72..82331c724eaaf2 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;
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index a59d1e7ac84096..60c6ac7256b58c 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,37 @@ 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->addOrMerge(R);
+ if (Err) {
+ 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;
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 7a47d6ebebf3b2..b4d365a1fabf2d 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) {
@@ -2136,6 +2136,41 @@ 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
diff --git a/clang/include/clang/Tooling/Core/Replacement.h b/clang/include/clang/Tooling/Core/Replacement.h
index f9452111e147f1..341ef4bf1c1ce0 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;
diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp
index 92e9859ca206e5..b989b865f63106 100644
--- a/clang/lib/Tooling/Core/Replacement.cpp
+++ b/clang/lib/Tooling/Core/Replacement.cpp
@@ -342,6 +342,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 +592,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();
``````````
</details>
https://github.com/llvm/llvm-project/pull/118569
More information about the cfe-commits
mailing list