https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/79867
>From 281cb038ef4e8951f93f8c2b68aad4e3e7994e52 Mon Sep 17 00:00:00 2001 From: Tor Shepherd <[email protected]> Date: Mon, 29 Jan 2024 11:44:25 -0500 Subject: [PATCH 1/3] [clangd] Add fix-all CodeActions --- clang-tools-extra/clangd/Diagnostics.cpp | 72 ++++++- clang-tools-extra/clangd/Protocol.h | 4 + .../clangd/unittests/DiagnosticsTests.cpp | 202 +++++++++++++----- 3 files changed, 219 insertions(+), 59 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 7dfc6ebb3fe0e..36185b27e4b4f 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -339,6 +339,72 @@ std::string noteMessage(const Diag &Main, const DiagBase &Note, return capitalize(std::move(Result)); } +std::optional<Fix> +generateApplyAllFromOption(const llvm::StringRef Name, + llvm::ArrayRef<Diag *> AllDiagnostics) { + Fix ApplyAll; + for (auto *const Diag : AllDiagnostics) { + for (const auto &Fix : Diag->Fixes) + ApplyAll.Edits.insert(ApplyAll.Edits.end(), Fix.Edits.begin(), + Fix.Edits.end()); + } + llvm::sort(ApplyAll.Edits); + ApplyAll.Edits.erase( + std::unique(ApplyAll.Edits.begin(), ApplyAll.Edits.end()), + ApplyAll.Edits.end()); + // Skip diagnostic categories that don't have multiple fixes to apply + if (ApplyAll.Edits.size() < 2U) { + return std::nullopt; + } + ApplyAll.Message = llvm::formatv("apply all '{0}' fixes", Name); + return ApplyAll; +} + +std::optional<Fix> +generateApplyAllFixesOption(llvm::ArrayRef<Diag> AllDiagnostics) { + Fix ApplyAll; + for (auto const &Diag : AllDiagnostics) { + for (const auto &Fix : Diag.Fixes) + ApplyAll.Edits.insert(ApplyAll.Edits.end(), Fix.Edits.begin(), + Fix.Edits.end()); + } + llvm::sort(ApplyAll.Edits); + ApplyAll.Edits.erase( + std::unique(ApplyAll.Edits.begin(), ApplyAll.Edits.end()), + ApplyAll.Edits.end()); + if (ApplyAll.Edits.size() < 2U) { + return std::nullopt; + } + ApplyAll.Message = "apply all clangd fixes"; + return ApplyAll; +} + +void appendApplyAlls(std::vector<Diag> &AllDiagnostics) { + llvm::DenseMap<llvm::StringRef, std::vector<Diag *>> CategorizedFixes; + + for (auto &Diag : AllDiagnostics) { + // Keep track of fixable diagnostics for generating "apply all fixes" + if (!Diag.Fixes.empty()) { + if (auto [It, DidEmplace] = CategorizedFixes.try_emplace( + Diag.Name, std::vector<struct Diag *>{&Diag}); + !DidEmplace) + It->second.emplace_back(&Diag); + } + } + + auto FixAllClangd = generateApplyAllFixesOption(AllDiagnostics); + for (const auto &[Name, DiagsForThisCategory] : CategorizedFixes) { + auto FixAllForCategory = + generateApplyAllFromOption(Name, DiagsForThisCategory); + for (auto *Diag : DiagsForThisCategory) { + if (DiagsForThisCategory.size() >= 2U && FixAllForCategory.has_value()) + Diag->Fixes.emplace_back(*FixAllForCategory); + if (CategorizedFixes.size() >= 2U && FixAllClangd.has_value()) + Diag->Fixes.emplace_back(*FixAllClangd); + } + } +} + void setTags(clangd::Diag &D) { static const auto *DeprecatedDiags = new llvm::DenseSet<unsigned>{ diag::warn_access_decl_deprecated, @@ -575,7 +641,8 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) { // Do not forget to emit a pending diagnostic if there is one. flushLastDiag(); - // Fill in name/source now that we have all the context needed to map them. + // Fill in name/source now that we have all the context needed to map + // them. for (auto &Diag : Output) { if (const char *ClangDiag = getDiagnosticCode(Diag.ID)) { // Warnings controlled by -Wfoo are better recognized by that name. @@ -629,6 +696,9 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) { llvm::erase_if(Output, [&](const Diag &D) { return !SeenDiags.emplace(D.Range, D.Message).second; }); + + appendApplyAlls(Output); + return std::move(Output); } diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index e81603a7b1a35..63547f9844269 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -258,6 +258,10 @@ inline bool operator==(const TextEdit &L, const TextEdit &R) { return std::tie(L.newText, L.range, L.annotationId) == std::tie(R.newText, R.range, L.annotationId); } +inline bool operator<(const TextEdit &L, const TextEdit &R) { + return std::tie(L.newText, L.range, L.annotationId) < + std::tie(R.newText, R.range, L.annotationId); +} bool fromJSON(const llvm::json::Value &, TextEdit &, llvm::json::Path); llvm::json::Value toJSON(const TextEdit &); llvm::raw_ostream &operator<<(llvm::raw_ostream &, const TextEdit &); diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index e7950526a3ec3..52b7699f2b999 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -32,6 +32,7 @@ #include "clang/Basic/SourceManager.h" #include "clang/Basic/Specifiers.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include "llvm/Support/JSON.h" @@ -44,6 +45,7 @@ #include <memory> #include <optional> #include <string> +#include <type_traits> #include <utility> #include <vector> @@ -63,13 +65,10 @@ using ::testing::Pair; using ::testing::SizeIs; using ::testing::UnorderedElementsAre; -::testing::Matcher<const Diag &> withFix(::testing::Matcher<Fix> FixMatcher) { - return Field(&Diag::Fixes, ElementsAre(FixMatcher)); -} - +template <typename... T> ::testing::Matcher<const Diag &> withFix(::testing::Matcher<Fix> FixMatcher1, - ::testing::Matcher<Fix> FixMatcher2) { - return Field(&Diag::Fixes, UnorderedElementsAre(FixMatcher1, FixMatcher2)); + T... Rest) { + return Field(&Diag::Fixes, UnorderedElementsAre(FixMatcher1, Rest...)); } ::testing::Matcher<const Diag &> withID(unsigned ID) { @@ -127,9 +126,13 @@ MATCHER_P(equalToFix, Fix, "LSP fix " + llvm::to_string(Fix)) { return false; if (arg.Edits.size() != Fix.Edits.size()) return false; + auto LHSEdits = arg.Edits; + auto RHSEdits = Fix.Edits; + llvm::sort(LHSEdits); + llvm::sort(RHSEdits); for (std::size_t I = 0; I < arg.Edits.size(); ++I) { - if (arg.Edits[I].range != Fix.Edits[I].range || - arg.Edits[I].newText != Fix.Edits[I].newText) + if (LHSEdits[I].range != RHSEdits[I].range || + LHSEdits[I].newText != RHSEdits[I].newText) return false; } return true; @@ -178,6 +181,22 @@ o]](); $macro[[ID($macroarg[[fod]])]](); } )cpp"); + + clangd::Fix ExpectedUndeclaredVar; + ExpectedUndeclaredVar.Message = + "apply all 'undeclared_var_use_suggest' fixes"; + ExpectedUndeclaredVar.Edits.push_back(TextEdit{Test.range("typo"), "foo"}); + ExpectedUndeclaredVar.Edits.push_back( + TextEdit{Test.range("macroarg"), "foo"}); + + clangd::Fix ExpectedFixAll; + ExpectedFixAll.Message = "apply all clangd fixes"; + ExpectedFixAll.Edits.push_back(TextEdit{Test.range("insertstar"), "*"}); + ExpectedFixAll.Edits.push_back(TextEdit{Test.range("semicolon"), ";"}); + ExpectedFixAll.Edits.insert(ExpectedFixAll.Edits.end(), + ExpectedUndeclaredVar.Edits.begin(), + ExpectedUndeclaredVar.Edits.end()); + auto TU = TestTU::withCode(Test.code()); EXPECT_THAT( TU.build().getDiagnostics(), @@ -186,20 +205,24 @@ o]](); AllOf(Diag(Test.range("range"), "invalid range expression of type 'struct Container *'; " "did you mean to dereference it with '*'?"), - withFix(Fix(Test.range("insertstar"), "*", "insert '*'"))), + withFix(Fix(Test.range("insertstar"), "*", "insert '*'"), + equalToFix(ExpectedFixAll))), // This range spans lines. - AllOf(Diag(Test.range("typo"), - "use of undeclared identifier 'goo'; did you mean 'foo'?"), - diagSource(Diag::Clang), diagName("undeclared_var_use_suggest"), - withFix( - Fix(Test.range("typo"), "foo", "change 'go\\…' to 'foo'")), - // This is a pretty normal range. - withNote(Diag(Test.range("decl"), "'foo' declared here"))), + AllOf( + Diag(Test.range("typo"), + "use of undeclared identifier 'goo'; did you mean 'foo'?"), + diagSource(Diag::Clang), diagName("undeclared_var_use_suggest"), + withFix(Fix(Test.range("typo"), "foo", "change 'go\\…' to 'foo'"), + equalToFix(ExpectedUndeclaredVar), + equalToFix(ExpectedFixAll)), + // This is a pretty normal range. + withNote(Diag(Test.range("decl"), "'foo' declared here"))), // This range is zero-width and insertion. Therefore make sure we are // not expanding it into other tokens. Since we are not going to // replace those. AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"), - withFix(Fix(Test.range("semicolon"), ";", "insert ';'"))), + withFix(Fix(Test.range("semicolon"), ";", "insert ';'"), + equalToFix(ExpectedFixAll))), // This range isn't provided by clang, we expand to the token. Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"), Diag(Test.range("type"), @@ -210,8 +233,10 @@ o]](); "no member named 'test' in namespace 'test'"), AllOf(Diag(Test.range("macro"), "use of undeclared identifier 'fod'; did you mean 'foo'?"), - withFix(Fix(Test.range("macroarg"), "foo", - "change 'fod' to 'foo'"))))); + withFix( + Fix(Test.range("macroarg"), "foo", "change 'fod' to 'foo'"), + equalToFix(ExpectedUndeclaredVar), + equalToFix(ExpectedFixAll))))); } // Verify that the -Wswitch case-not-covered diagnostic range covers the @@ -338,7 +363,8 @@ TEST(DiagnosticsTest, ClangTidy) { diagSource(Diag::ClangTidy), diagName("modernize-deprecated-headers"), withFix(Fix(Test.range("deprecated"), "<cassert>", - "change '\"assert.h\"' to '<cassert>'"))), + "change '\"assert.h\"' to '<cassert>'"), + fixMessage("apply all clangd fixes"))), Diag(Test.range("doubled"), "suspicious usage of 'sizeof(sizeof(...))'"), AllOf(Diag(Test.range("macroarg"), @@ -354,8 +380,9 @@ TEST(DiagnosticsTest, ClangTidy) { diagSource(Diag::ClangTidy), diagName("modernize-use-trailing-return-type"), // Verify there's no "[check-name]" suffix in the message. - withFix(fixMessage( - "use a trailing return type for this function"))), + withFix( + fixMessage("use a trailing return type for this function"), + fixMessage("apply all clangd fixes"))), Diag(Test.range("foo"), "function 'foo' is within a recursive call chain"), Diag(Test.range("bar"), @@ -945,22 +972,63 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiags) { // 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"}); + + clangd::Fix ExpectedMemberInitializerFix; + ExpectedMemberInitializerFix.Message = + "apply all 'cppcoreguidelines-prefer-member-initializer' fixes"; + ExpectedMemberInitializerFix.Edits.insert( + ExpectedMemberInitializerFix.Edits.end(), ExpectedAFix.Edits.begin(), + ExpectedAFix.Edits.end()); + ExpectedMemberInitializerFix.Edits.insert( + ExpectedMemberInitializerFix.Edits.end(), ExpectedBFix.Edits.begin(), + ExpectedBFix.Edits.end()); + + clangd::Fix ExpectedInitVariablesFix; + ExpectedInitVariablesFix.Message = + "apply all 'cppcoreguidelines-init-variables' fixes"; + ExpectedInitVariablesFix.Edits.insert(ExpectedInitVariablesFix.Edits.end(), + ExpectedCFix.Edits.begin(), + ExpectedCFix.Edits.end()); + ExpectedInitVariablesFix.Edits.insert(ExpectedInitVariablesFix.Edits.end(), + ExpectedDFix.Edits.begin(), + ExpectedDFix.Edits.end()); + + clangd::Fix ExpectedFixAll; + ExpectedFixAll.Message = "apply all clangd fixes"; + ExpectedFixAll.Edits.insert(ExpectedFixAll.Edits.end(), + ExpectedMemberInitializerFix.Edits.begin(), + ExpectedMemberInitializerFix.Edits.end()); + ExpectedFixAll.Edits.insert(ExpectedFixAll.Edits.end(), + ExpectedInitVariablesFix.Edits.begin(), + ExpectedInitVariablesFix.Edits.end()); + + // This edit is duplicated in C, so add it after inserting all of the "fix + // all" edits 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( AllOf(Diag(Main.range("A"), "'A' should be initialized in a member " "initializer of the constructor"), - withFix(equalToFix(ExpectedAFix))), + withFix(equalToFix(ExpectedAFix), + equalToFix(ExpectedMemberInitializerFix), + equalToFix(ExpectedFixAll))), AllOf(Diag(Main.range("B"), "'B' should be initialized in a member " "initializer of the constructor"), - withFix(equalToFix(ExpectedBFix))), + withFix(equalToFix(ExpectedBFix), + equalToFix(ExpectedMemberInitializerFix), + equalToFix(ExpectedFixAll))), AllOf(Diag(Main.range("C"), "variable 'C' is not initialized"), - withFix(equalToFix(ExpectedCFix))), + withFix(equalToFix(ExpectedCFix), + equalToFix(ExpectedInitVariablesFix), + equalToFix(ExpectedFixAll))), AllOf(Diag(Main.range("D"), "variable 'D' is not initialized"), - withFix(equalToFix(ExpectedDFix)))))); + withFix(equalToFix(ExpectedDFix), + equalToFix(ExpectedInitVariablesFix), + equalToFix(ExpectedFixAll)))))); } TEST(DiagnosticTest, ClangTidySelfContainedDiagsFormatting) { @@ -1473,32 +1541,42 @@ using Type = ns::$template[[Foo]]<int>; AllOf(Diag(Test.range("unqualified1"), "unknown type name 'X'"), diagName("unknown_typename"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"))), + "Include \"x.h\" for symbol ns::X"), + fixMessage("apply all clangd fixes"))), Diag(Test.range("unqualified2"), "use of undeclared identifier 'X'"), - AllOf(Diag(Test.range("qualified1"), - "no type named 'X' in namespace 'ns'"), - diagName("typename_nested_not_found"), - withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"))), + AllOf( + Diag(Test.range("qualified1"), + "no type named 'X' in namespace 'ns'"), + diagName("typename_nested_not_found"), + withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", + "Include \"x.h\" for symbol ns::X"), + fixMessage("apply all 'typename_nested_not_found' fixes"), + fixMessage("apply all clangd fixes"))), AllOf(Diag(Test.range("qualified2"), "no member named 'X' in namespace 'ns'"), diagName("no_member"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"))), - AllOf(Diag(Test.range("global"), - "no type named 'Global' in the global namespace"), - diagName("typename_nested_not_found"), - withFix(Fix(Test.range("insert"), "#include \"global.h\"\n", - "Include \"global.h\" for symbol Global"))), + "Include \"x.h\" for symbol ns::X"), + fixMessage("apply all clangd fixes"))), + AllOf( + Diag(Test.range("global"), + "no type named 'Global' in the global namespace"), + diagName("typename_nested_not_found"), + withFix(Fix(Test.range("insert"), "#include \"global.h\"\n", + "Include \"global.h\" for symbol Global"), + fixMessage("apply all 'typename_nested_not_found' fixes"), + fixMessage("apply all clangd fixes"))), AllOf(Diag(Test.range("template"), "no template named 'Foo' in namespace 'ns'"), diagName("no_member_template"), withFix(Fix(Test.range("insert"), "#include \"foo.h\"\n", - "Include \"foo.h\" for symbol ns::Foo"))), + "Include \"foo.h\" for symbol ns::Foo"), + fixMessage("apply all clangd fixes"))), AllOf(Diag(Test.range("base"), "expected class name"), diagName("expected_class_name"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"))))); + "Include \"x.h\" for symbol ns::X"), + fixMessage("apply all clangd fixes"))))); } TEST(IncludeFixerTest, TypoInMacro) { @@ -1519,7 +1597,9 @@ ID(ns::X a6); // FIXME: -fms-compatibility (which is default on windows) breaks the // ns::X cases when the namespace is undeclared. Find out why! TU.ExtraArgs = {"-fno-ms-compatibility"}; - EXPECT_THAT(TU.build().getDiagnostics(), Each(withFix(_))); + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre(withFix(_), withFix(_), withFix(_), + withFix(_), withFix(_), withFix(_))); } TEST(IncludeFixerTest, MultipleMatchedSymbols) { @@ -1651,26 +1731,32 @@ void f() { AllOf(Diag(Test.range("q1"), "use of undeclared identifier 'clangd'; " "did you mean 'clang'?"), diagName("undeclared_var_use_suggest"), - withFix(_, // change clangd to clang - Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol clang::clangd::X"))), + withFix( + _, // change clangd to clang + Fix(Test.range("insert"), "#include \"x.h\"\n", + "Include \"x.h\" for symbol clang::clangd::X"), + fixMessage("apply all 'undeclared_var_use_suggest' fixes"), + fixMessage("apply all clangd fixes"))), AllOf(Diag(Test.range("x"), "no type named 'X' in namespace 'clang'"), diagName("typename_nested_not_found"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol clang::clangd::X"))), - AllOf( - Diag(Test.range("q2"), "use of undeclared identifier 'clangd'; " - "did you mean 'clang'?"), - diagName("undeclared_var_use_suggest"), - withFix(_, // change clangd to clang - Fix(Test.range("insert"), "#include \"y.h\"\n", - "Include \"y.h\" for symbol clang::clangd::ns::Y"))), + "Include \"x.h\" for symbol clang::clangd::X"), + fixMessage("apply all clangd fixes"))), + AllOf(Diag(Test.range("q2"), "use of undeclared identifier 'clangd'; " + "did you mean 'clang'?"), + diagName("undeclared_var_use_suggest"), + withFix( + _, // change clangd to clang + Fix(Test.range("insert"), "#include \"y.h\"\n", + "Include \"y.h\" for symbol clang::clangd::ns::Y"), + fixMessage("apply all 'undeclared_var_use_suggest' fixes"), + fixMessage("apply all clangd fixes"))), AllOf(Diag(Test.range("ns"), "no member named 'ns' in namespace 'clang'"), diagName("no_member"), - withFix( - Fix(Test.range("insert"), "#include \"y.h\"\n", - "Include \"y.h\" for symbol clang::clangd::ns::Y"))))); + withFix(Fix(Test.range("insert"), "#include \"y.h\"\n", + "Include \"y.h\" for symbol clang::clangd::ns::Y"), + fixMessage("apply all clangd fixes"))))); } TEST(IncludeFixerTest, SpecifiedScopeIsNamespaceAlias) { @@ -2003,10 +2089,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 ac7017fdb9a9292ab1d7af7764cb1d33fa9657d3 Mon Sep 17 00:00:00 2001 From: Tor Shepherd <[email protected]> Date: Fri, 14 Jun 2024 18:03:35 -0400 Subject: [PATCH 2/3] Fix conflicts --- clang-tools-extra/clangd/Diagnostics.cpp | 38 ++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 36185b27e4b4f..6798d0b620fba 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -339,6 +339,35 @@ std::string noteMessage(const Diag &Main, const DiagBase &Note, return capitalize(std::move(Result)); } +// Tests if any two `TextEdit`s in `Edits` conflict. Two `TextEdit`s +// conflict if they have overlapping source ranges. +// NOTE: This function is inspired by clang::internal::anyConflict +bool anyConflict(const llvm::SmallVector<TextEdit, 1> &Edits) { + // A simple interval overlap detection algorithm. Sorts all ranges by their + // begin location then finds the first overlap in one pass. + llvm::SmallVector<const TextEdit *, 1> All; // a copy of `Edits` + + for (const TextEdit &E : Edits) + All.push_back(&E); + std::sort(All.begin(), All.end(), [](const TextEdit *H1, const TextEdit *H2) { + return H1->range.start < H2->range.start; + }); + + const TextEdit *CurrHint = nullptr; + + for (const TextEdit *Hint : All) { + if (!CurrHint || CurrHint->range.end < Hint->range.start) { + // Either to initialize `CurrHint` or `CurrHint` does not + // overlap with `Hint`: + CurrHint = Hint; + } else + // In case `Hint` overlaps the `CurrHint`, we found at least one + // conflict: + return true; + } + return false; +} + std::optional<Fix> generateApplyAllFromOption(const llvm::StringRef Name, llvm::ArrayRef<Diag *> AllDiagnostics) { @@ -352,8 +381,9 @@ generateApplyAllFromOption(const llvm::StringRef Name, ApplyAll.Edits.erase( std::unique(ApplyAll.Edits.begin(), ApplyAll.Edits.end()), ApplyAll.Edits.end()); - // Skip diagnostic categories that don't have multiple fixes to apply - if (ApplyAll.Edits.size() < 2U) { + // Skip diagnostic categories that don't have multiple fixes to apply or that + // have conflicting fixes to apply + if (ApplyAll.Edits.size() < 2U || anyConflict(ApplyAll.Edits)) { return std::nullopt; } ApplyAll.Message = llvm::formatv("apply all '{0}' fixes", Name); @@ -372,7 +402,9 @@ generateApplyAllFixesOption(llvm::ArrayRef<Diag> AllDiagnostics) { ApplyAll.Edits.erase( std::unique(ApplyAll.Edits.begin(), ApplyAll.Edits.end()), ApplyAll.Edits.end()); - if (ApplyAll.Edits.size() < 2U) { + // Skip diagnostics that don't have multiple fixes to apply or that have + // conflicting fixes to apply + if (ApplyAll.Edits.size() < 2U || anyConflict(ApplyAll.Edits)) { return std::nullopt; } ApplyAll.Message = "apply all clangd fixes"; >From c0d55d8fd13b6458541700700935f3077c9f17b3 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Mon, 31 Aug 2026 16:53:19 +0200 Subject: [PATCH 3/3] [clangd] Update tests for the fix-all code actions The conflict check added to "apply all" fix generation suppresses the aggregate fix whenever two of its edits share a source range, including insertions at the same position (i.e. edits with a zero-width range, so nothing gets replaced) but with different text. Adapt all test expectations according to this conservative approach and add a TODO comment for the cases where we would like to have a smarter approach. Assisted-by: Claude --- .../clangd/unittests/DiagnosticsTests.cpp | 111 +++++++++--------- 1 file changed, 56 insertions(+), 55 deletions(-) diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 52b7699f2b999..637b56cee8fbc 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -974,16 +974,12 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiags) { ExpectedDFix.Message = "variable 'D' is not initialized"; ExpectedDFix.Edits.push_back(TextEdit{Main.range("DFix"), " = NAN"}); - clangd::Fix ExpectedMemberInitializerFix; - ExpectedMemberInitializerFix.Message = - "apply all 'cppcoreguidelines-prefer-member-initializer' fixes"; - ExpectedMemberInitializerFix.Edits.insert( - ExpectedMemberInitializerFix.Edits.end(), ExpectedAFix.Edits.begin(), - ExpectedAFix.Edits.end()); - ExpectedMemberInitializerFix.Edits.insert( - ExpectedMemberInitializerFix.Edits.end(), ExpectedBFix.Edits.begin(), - ExpectedBFix.Edits.end()); - + // The two member-initializer fixes both insert at the same point (the + // constructor's brace), so combining them verbatim would produce invalid + // C++ (" : A(1) : B(1)" instead of ", "-joined). The conflict check + // correctly suppresses any "apply all" fix for this diagnostic, and since + // the same edits would also be pulled into the file-wide "apply all clangd + // fixes", that one is suppressed too. clangd::Fix ExpectedInitVariablesFix; ExpectedInitVariablesFix.Message = "apply all 'cppcoreguidelines-init-variables' fixes"; @@ -994,15 +990,6 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiags) { ExpectedDFix.Edits.begin(), ExpectedDFix.Edits.end()); - clangd::Fix ExpectedFixAll; - ExpectedFixAll.Message = "apply all clangd fixes"; - ExpectedFixAll.Edits.insert(ExpectedFixAll.Edits.end(), - ExpectedMemberInitializerFix.Edits.begin(), - ExpectedMemberInitializerFix.Edits.end()); - ExpectedFixAll.Edits.insert(ExpectedFixAll.Edits.end(), - ExpectedInitVariablesFix.Edits.begin(), - ExpectedInitVariablesFix.Edits.end()); - // This edit is duplicated in C, so add it after inserting all of the "fix // all" edits ExpectedDFix.Edits.push_back( @@ -1013,22 +1000,16 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiags) { ifTidyChecks(UnorderedElementsAre( AllOf(Diag(Main.range("A"), "'A' should be initialized in a member " "initializer of the constructor"), - withFix(equalToFix(ExpectedAFix), - equalToFix(ExpectedMemberInitializerFix), - equalToFix(ExpectedFixAll))), + withFix(equalToFix(ExpectedAFix))), AllOf(Diag(Main.range("B"), "'B' should be initialized in a member " "initializer of the constructor"), - withFix(equalToFix(ExpectedBFix), - equalToFix(ExpectedMemberInitializerFix), - equalToFix(ExpectedFixAll))), + withFix(equalToFix(ExpectedBFix))), AllOf(Diag(Main.range("C"), "variable 'C' is not initialized"), withFix(equalToFix(ExpectedCFix), - equalToFix(ExpectedInitVariablesFix), - equalToFix(ExpectedFixAll))), + equalToFix(ExpectedInitVariablesFix))), AllOf(Diag(Main.range("D"), "variable 'D' is not initialized"), withFix(equalToFix(ExpectedDFix), - equalToFix(ExpectedInitVariablesFix), - equalToFix(ExpectedFixAll)))))); + equalToFix(ExpectedInitVariablesFix)))))); } TEST(DiagnosticTest, ClangTidySelfContainedDiagsFormatting) { @@ -1059,6 +1040,19 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiagsFormatting) { {TextEdit{Main.range("virtual2"), ""}, TextEdit{Main.range("override2"), " override"}}, {}}; + // The two fixes don't conflict (they touch disjoint ranges), and this is + // the only fixable diagnostic category in the file, so we expect a + // category-wide "apply all" fix but no file-wide "apply all clangd fixes" + // (that one only appears when there are multiple distinct categories). + clangd::Fix ExpectedCategoryFix; + ExpectedCategoryFix.Message = + "apply all 'cppcoreguidelines-explicit-virtual-functions' fixes"; + ExpectedCategoryFix.Edits.insert(ExpectedCategoryFix.Edits.end(), + ExpectedFix1.Edits.begin(), + ExpectedFix1.Edits.end()); + ExpectedCategoryFix.Edits.insert(ExpectedCategoryFix.Edits.end(), + ExpectedFix2.Edits.begin(), + ExpectedFix2.Edits.end()); // Note that in the Fix we expect the "virtual" keyword and the following // whitespace to be deleted EXPECT_THAT(TU.build().getDiagnostics(), @@ -1066,11 +1060,13 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiagsFormatting) { AllOf(Diag(Main.range("Reset1"), "prefer using 'override' or (rarely) 'final' " "instead of 'virtual'"), - withFix(equalToFix(ExpectedFix1))), + withFix(equalToFix(ExpectedFix1), + equalToFix(ExpectedCategoryFix))), AllOf(Diag(Main.range("Reset2"), "prefer using 'override' or (rarely) 'final' " "instead of 'virtual'"), - withFix(equalToFix(ExpectedFix2)))))); + withFix(equalToFix(ExpectedFix2), + equalToFix(ExpectedCategoryFix)))))); } TEST(DiagnosticsTest, ClangTidyCallingIntoPreprocessor) { @@ -1535,48 +1531,52 @@ using Type = ns::$template[[Foo]]<int>; SymbolWithHeader{"ns::Foo", "unittest:///foo.h", "\"foo.h\""}}); TU.ExternalIndex = Index.get(); + // All of these fixes insert at the same point ("insert"), i.e. they all + // use the same zero-width range (start == end, so nothing gets replaced), + // but with different headers to include. The conflict check treats any + // two edits sharing a range as unsafe to combine, so every "apply all" + // fix that would pull in more than one of them (per-category or + // file-wide) is suppressed, leaving only the original per-diagnostic + // fixes. + // TODO: This is overly conservative: independent #include insertions at + // the same point are actually safe to combine. Relax this once we have a + // heuristic for recognizing edits that are safe to concatenate (e.g. + // self-contained, newline-terminated line insertions) rather than treating + // every same-range edit as an unresolvable conflict. EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre( AllOf(Diag(Test.range("unqualified1"), "unknown type name 'X'"), diagName("unknown_typename"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"), - fixMessage("apply all clangd fixes"))), + "Include \"x.h\" for symbol ns::X"))), Diag(Test.range("unqualified2"), "use of undeclared identifier 'X'"), AllOf( Diag(Test.range("qualified1"), "no type named 'X' in namespace 'ns'"), diagName("typename_nested_not_found"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"), - fixMessage("apply all 'typename_nested_not_found' fixes"), - fixMessage("apply all clangd fixes"))), + "Include \"x.h\" for symbol ns::X"))), AllOf(Diag(Test.range("qualified2"), "no member named 'X' in namespace 'ns'"), diagName("no_member"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"), - fixMessage("apply all clangd fixes"))), + "Include \"x.h\" for symbol ns::X"))), AllOf( Diag(Test.range("global"), "no type named 'Global' in the global namespace"), diagName("typename_nested_not_found"), withFix(Fix(Test.range("insert"), "#include \"global.h\"\n", - "Include \"global.h\" for symbol Global"), - fixMessage("apply all 'typename_nested_not_found' fixes"), - fixMessage("apply all clangd fixes"))), + "Include \"global.h\" for symbol Global"))), AllOf(Diag(Test.range("template"), "no template named 'Foo' in namespace 'ns'"), diagName("no_member_template"), withFix(Fix(Test.range("insert"), "#include \"foo.h\"\n", - "Include \"foo.h\" for symbol ns::Foo"), - fixMessage("apply all clangd fixes"))), + "Include \"foo.h\" for symbol ns::Foo"))), AllOf(Diag(Test.range("base"), "expected class name"), diagName("expected_class_name"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"), - fixMessage("apply all clangd fixes"))))); + "Include \"x.h\" for symbol ns::X"))))); } TEST(IncludeFixerTest, TypoInMacro) { @@ -1725,6 +1725,13 @@ void f() { SymbolWithHeader{"clang::clangd::ns::Y", "unittest:///y.h", "\"y.h\""}}); TU.ExternalIndex = Index.get(); + // As in IncludeFixerTest.Typo, all these fixes insert at the same point, + // i.e. their ranges are all the same zero-width range, so every "apply + // all" fix that would combine more than one of them is suppressed by the + // conflict check; only the original per-diagnostic fixes remain. + // TODO: see the TODO in IncludeFixerTest.Typo above; this is the same + // overly conservative case (independent #include insertions) and should + // be relaxed by the same future heuristic. EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre( @@ -1734,29 +1741,23 @@ void f() { withFix( _, // change clangd to clang Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol clang::clangd::X"), - fixMessage("apply all 'undeclared_var_use_suggest' fixes"), - fixMessage("apply all clangd fixes"))), + "Include \"x.h\" for symbol clang::clangd::X"))), AllOf(Diag(Test.range("x"), "no type named 'X' in namespace 'clang'"), diagName("typename_nested_not_found"), withFix(Fix(Test.range("insert"), "#include \"x.h\"\n", - "Include \"x.h\" for symbol clang::clangd::X"), - fixMessage("apply all clangd fixes"))), + "Include \"x.h\" for symbol clang::clangd::X"))), AllOf(Diag(Test.range("q2"), "use of undeclared identifier 'clangd'; " "did you mean 'clang'?"), diagName("undeclared_var_use_suggest"), withFix( _, // change clangd to clang Fix(Test.range("insert"), "#include \"y.h\"\n", - "Include \"y.h\" for symbol clang::clangd::ns::Y"), - fixMessage("apply all 'undeclared_var_use_suggest' fixes"), - fixMessage("apply all clangd fixes"))), + "Include \"y.h\" for symbol clang::clangd::ns::Y"))), AllOf(Diag(Test.range("ns"), "no member named 'ns' in namespace 'clang'"), diagName("no_member"), withFix(Fix(Test.range("insert"), "#include \"y.h\"\n", - "Include \"y.h\" for symbol clang::clangd::ns::Y"), - fixMessage("apply all clangd fixes"))))); + "Include \"y.h\" for symbol clang::clangd::ns::Y"))))); } TEST(IncludeFixerTest, SpecifiedScopeIsNamespaceAlias) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
