[clang-tools-extra] d0e8911 - [clangd] Fix fixAll not shown when there is only one unused-include and missing-include diagnostics.

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2023-05-15T14:02:20+02:00
New Revision: d0e89116aff224ac2d8d3f88029ae44e12c9b6cc

URL: https://github.com/llvm/llvm-project/commit/d0e89116aff224ac2d8d3f88029ae44e12c9b6cc
DIFF: https://github.com/llvm/llvm-project/commit/d0e89116aff224ac2d8d3f88029ae44e12c9b6cc.diff

LOG: [clangd] Fix fixAll not shown when there is only one unused-include and missing-include diagnostics.

Discovered during the review in https://reviews.llvm.org/D149437#inline-1444851.

Differential Revision: https://reviews.llvm.org/D149822

Added: 
    

Modified: 
    clang-tools-extra/clangd/IncludeCleaner.cpp
    clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 3b913d851abe2..22d0808235f7f 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -440,8 +440,9 @@ IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
   return {std::move(UnusedIncludes), std::move(MissingIncludes)};
 }
 
-Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
-  assert(!UnusedIncludes.empty());
+std::optional<Fix> removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
+  if (UnusedIncludes.empty())
+    return std::nullopt;
 
   Fix RemoveAll;
   RemoveAll.Message = "remove all unused includes";
@@ -465,8 +466,10 @@ Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
   }
   return RemoveAll;
 }
-Fix addAllMissingIncludes(llvm::ArrayRef<Diag> MissingIncludeDiags) {
-  assert(!MissingIncludeDiags.empty());
+std::optional<Fix>
+addAllMissingIncludes(llvm::ArrayRef<Diag> MissingIncludeDiags) {
+  if (MissingIncludeDiags.empty())
+    return std::nullopt;
 
   Fix AddAllMissing;
   AddAllMissing.Message = "add all missing includes";
@@ -516,15 +519,11 @@ std::vector<Diag> generateIncludeCleanerDiagnostic(
     llvm::StringRef Code) {
   std::vector<Diag> UnusedIncludes = generateUnusedIncludeDiagnostics(
       AST.tuPath(), Findings.UnusedIncludes, Code);
-  std::optional<Fix> RemoveAllUnused;;
-  if (UnusedIncludes.size() > 1)
-    RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);
+  std::optional<Fix> RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);
 
   std::vector<Diag> MissingIncludeDiags = generateMissingIncludeDiagnostics(
       AST, Findings.MissingIncludes, Code);
-  std::optional<Fix> AddAllMissing;
-  if (MissingIncludeDiags.size() > 1)
-    AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);
+  std::optional<Fix> AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);
 
   std::optional<Fix> FixAll;
   if (RemoveAllUnused && AddAllMissing)
@@ -535,11 +534,16 @@ std::vector<Diag> generateIncludeCleanerDiagnostic(
     Out->Fixes.push_back(*F);
   };
   for (auto &Diag : MissingIncludeDiags) {
-    AddBatchFix(AddAllMissing, &Diag);
+    AddBatchFix(MissingIncludeDiags.size() > 1
+                    ? AddAllMissing
+                    : std::nullopt,
+                &Diag);
     AddBatchFix(FixAll, &Diag);
   }
   for (auto &Diag : UnusedIncludes) {
-    AddBatchFix(RemoveAllUnused, &Diag);
+    AddBatchFix(UnusedIncludes.size() > 1 ? RemoveAllUnused
+                                          : std::nullopt,
+                &Diag);
     AddBatchFix(FixAll, &Diag);
   }
 

diff  --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index a38c01b43270f..c0831c2693c97 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -197,10 +197,10 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
 
     ns::$bar[[Bar]] bar;
     bar.d();
-    $f[[f]](); 
+    $f[[f]]();
 
     // this should not be diagnosed, because it's ignored in the config
-    buzz(); 
+    buzz();
 
     $foobar[[foobar]]();
 
@@ -244,7 +244,7 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
   TU.AdditionalFiles["foo.h"] = guard(R"cpp(
     #define BAR(x) Foo *x
     #define FOO 1
-    struct Foo{}; 
+    struct Foo{};
   )cpp");
 
   TU.Code = MainFile.code();
@@ -510,6 +510,71 @@ TEST(IncludeCleaner, FirstMatchedProvider) {
   }
 }
 
+TEST(IncludeCleaner, BatchFix) {
+  Config Cfg;
+  Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict;
+  Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict;
+  WithContextValue Ctx(Config::Key, std::move(Cfg));
+
+  TestTU TU;
+  TU.Filename = "main.cpp";
+  TU.AdditionalFiles["foo.h"] = guard("class Foo;");
+  TU.AdditionalFiles["bar.h"] = guard("class Bar;");
+  TU.AdditionalFiles["all.h"] = guard(R"cpp(
+    #include "foo.h"
+    #include "bar.h"
+  )cpp");
+
+  TU.Code = R"cpp(
+  #include "all.h"
+
+  Foo* foo;
+  )cpp";
+  auto AST = TU.build();
+  EXPECT_THAT(
+      issueIncludeCleanerDiagnostics(AST, TU.Code),
+      UnorderedElementsAre(withFix({FixMessage("#include \"foo.h\""),
+                                    FixMessage("fix all includes")}),
+                           withFix({FixMessage("remove #include directive"),
+                                    FixMessage("fix all includes")})));
+
+  TU.Code = R"cpp(
+  #include "all.h"
+  #include "bar.h"
+
+  Foo* foo;
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(
+      issueIncludeCleanerDiagnostics(AST, TU.Code),
+      UnorderedElementsAre(withFix({FixMessage("#include \"foo.h\""),
+                                    FixMessage("fix all includes")}),
+                           withFix({FixMessage("remove #include directive"),
+                                    FixMessage("remove all unused includes"),
+                                    FixMessage("fix all includes")}),
+                           withFix({FixMessage("remove #include directive"),
+                                   FixMessage("remove all unused includes"),
+                                    FixMessage("fix all includes")})));
+
+  TU.Code = R"cpp(
+  #include "all.h"
+
+  Foo* foo;
+  Bar* bar;
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(
+      issueIncludeCleanerDiagnostics(AST, TU.Code),
+      UnorderedElementsAre(withFix({FixMessage("#include \"foo.h\""),
+                                    FixMessage("add all missing includes"),
+                                    FixMessage("fix all includes")}),
+                           withFix({FixMessage("#include \"bar.h\""),
+                                    FixMessage("add all missing includes"),
+                                    FixMessage("fix all includes")}),
+                           withFix({FixMessage("remove #include directive"),
+                                    FixMessage("fix all includes")})));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang