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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
