From efc17a803c9c22543de7d5f9e960a7267ade1f2e Mon Sep 17 00:00:00 2001 From: chomosuke Date: Wed, 4 Dec 2024 14:42:24 +0000 Subject: [PATCH 1/3] [clangd][clang-tidy] Make clangd run `format::cleanupAroundReplacements()` for all code actions just as clang-tidy does --- clang-tools-extra/clang-tidy/ClangTidy.cpp | 22 ++------ clang-tools-extra/clangd/Diagnostics.cpp | 35 ++++++++++++- .../clangd/unittests/DiagnosticsTests.cpp | 51 ++++++++++++++++--- .../include/clang/Tooling/Core/Replacement.h | 4 ++ clang/lib/Tooling/Core/Replacement.cpp | 16 +++++- 5 files changed, 99 insertions(+), 29 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp index 9c8c93c5d16c72d..82331c724eaaf21 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 a59d1e7ac84096a..60c6ac7256b58c3 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 Edits; + auto Replacements = std::make_optional(); 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 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 7a47d6ebebf3b25..b4d365a1fabf2dc 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 \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 \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 f9452111e147f17..341ef4bf1c1ce0f 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 92e9859ca206e51..b989b865f63106c 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( + 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 applyAllReplacements(StringRef Code, - const Replacements &Replaces) { + const Replacements &Replaces) { if (Replaces.empty()) return Code.str(); From cea03670d965abc8f9899097d3a02654dd1e3419 Mon Sep 17 00:00:00 2001 From: chomosuke Date: Thu, 5 Dec 2024 07:23:37 +1100 Subject: [PATCH 2/3] mentioned 'Improved diagnostics code actions to better remove left over commas and colons.' in ReleaseNotes.txt --- clang-tools-extra/docs/ReleaseNotes.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e00f86f7d01447f..7604c54ae5d3851 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -83,6 +83,8 @@ Code actions - Improved the extract-to-function code action to allow extracting statements with overloaded operators like ``<<`` of ``std::ostream``. +- Improved diagnostics code actions to better remove leftover commas and colons. + Signature help ^^^^^^^^^^^^^^ From aa49041e77118b16615e8125e297f84030d3f2d3 Mon Sep 17 00:00:00 2001 From: chomosuke Date: Tue, 24 Dec 2024 01:23:41 +1100 Subject: [PATCH 3/3] Fixed review suggestion --- clang-tools-extra/clang-tidy/ClangTidy.cpp | 3 +-- clang-tools-extra/clangd/Diagnostics.cpp | 14 ++++++-------- clang/lib/Tooling/Core/Replacement.cpp | 3 +-- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp index 4dd7181c5167596..71eef1668127700 100644 --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -149,8 +149,7 @@ class ErrorReporter { Repl.getLength(), Repl.getReplacementText()); auto &Entry = FileReplacements[R.getFilePath()]; Replacements &Replacements = Entry.Replaces; - llvm::Error Err = Replacements.addOrMerge(R); - if (Err) { + if (llvm::Error Err = Replacements.addOrMerge(R)) { // FIXME: Implement better conflict handling. llvm::errs() << "Can't resolve conflict, skipping the replacement: " diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 60c6ac7256b58c3..b699360b95ab2f2 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -765,8 +765,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, auto R = tooling::Replacement(SM, FixIt.RemoveRange, FixIt.CodeToInsert, *LangOpts); - auto Err = Replacements->addOrMerge(R); - if (Err) { + if (llvm::Error Err = Replacements->addOrMerge(R)) { log("Skipping formatting the replacement due to conflict: {0}", llvm::toString(std::move(Err))); Replacements = std::nullopt; @@ -778,21 +777,20 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, if (Replacements) { StringRef Code = SM.getBufferData(SM.getMainFileID()); - auto Repl = format::cleanupAroundReplacements(Code, *Replacements, - format::getNoStyle()); + Expected Repl = format::cleanupAroundReplacements( + Code, *Replacements, format::getNoStyle()); if (!Repl) { log("Skipping formatting the replacement due to conflict: {0}", - llvm::toString(std::move(Repl.takeError()))); + llvm::toString(Repl.takeError())); Replacements = std::nullopt; } else { - auto Es = replacementsToEdits(Code, *Repl); + std::vector Es = replacementsToEdits(Code, *Repl); Edits.append(Es.begin(), Es.end()); } } if (!Replacements) { - for (auto &FixIt : FixIts) { + for (auto &FixIt : FixIts) Edits.push_back(toTextEdit(FixIt, SM, *LangOpts)); - } } llvm::SmallString<64> Message; diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp index b989b865f63106c..ee9efc4e847f08e 100644 --- a/clang/lib/Tooling/Core/Replacement.cpp +++ b/clang/lib/Tooling/Core/Replacement.cpp @@ -343,8 +343,7 @@ llvm::Error Replacements::add(const Replacement &R) { } llvm::Error Replacements::addOrMerge(const Replacement &R) { - auto Err = add(R); - if (Err) { + if (llvm::Error Err = add(R)) { llvm::consumeError(std::move(Err)); auto Replace = getReplacementInChangedCode(R); if (Replace.getLength() != R.getLength()) {