[PATCH] D146916: [include-cleaner] Fix crash on unresolved headers

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
Herald added a project: All.
kadircet requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Make sure unresolved headers are not analyzed as part of unused
includes.

Also introduces a testing fixture for analyze tests


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146916

Files:
  clang-tools-extra/include-cleaner/lib/Analysis.cpp
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp


Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
===================================================================
--- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -24,6 +24,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cstddef>
+#include <memory>
 #include <vector>
 
 namespace clang::include_cleaner {
@@ -178,8 +179,31 @@
           Pair(Code.point("2"), UnorderedElementsAre(HdrFile))));
 }
 
-TEST(Analyze, Basic) {
+class AnalyzeTest : public testing::Test {
+protected:
   TestInputs Inputs;
+  PragmaIncludes PI;
+  RecordedPP PP;
+  AnalyzeTest() {
+    Inputs.MakeAction = [this] {
+      struct Hook : public SyntaxOnlyAction {
+      public:
+        Hook(RecordedPP &PP, PragmaIncludes &PI) : PP(PP), PI(PI) {}
+        bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
+          CI.getPreprocessor().addPPCallbacks(PP.record(CI.getPreprocessor()));
+          PI.record(CI);
+          return true;
+        }
+
+        RecordedPP &PP;
+        PragmaIncludes &PI;
+      };
+      return std::make_unique<Hook>(PP, PI);
+    };
+  }
+};
+
+TEST_F(AnalyzeTest, Basic) {
   Inputs.Code = R"cpp(
 #include "a.h"
 #include "b.h"
@@ -194,53 +218,41 @@
   )cpp");
   Inputs.ExtraFiles["c.h"] = guard("int c;");
   Inputs.ExtraFiles["keep.h"] = guard("");
+  TestAST AST(Inputs);
+  auto Decls = AST.context().getTranslationUnitDecl()->decls();
+  auto Results =
+      analyze(std::vector<Decl *>{Decls.begin(), Decls.end()},
+              PP.MacroReferences, PP.Includes, &PI, AST.sourceManager(),
+              AST.preprocessor().getHeaderSearchInfo());
 
-  RecordedPP PP;
-  PragmaIncludes PI;
-  Inputs.MakeAction = [&PP, &PI] {
-    struct Hook : public SyntaxOnlyAction {
-    public:
-      Hook(RecordedPP &PP, PragmaIncludes &PI) : PP(PP), PI(PI) {}
-      bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
-        CI.getPreprocessor().addPPCallbacks(PP.record(CI.getPreprocessor()));
-        PI.record(CI);
-        return true;
-      }
-
-      RecordedPP &PP;
-      PragmaIncludes &PI;
-    };
-    return std::make_unique<Hook>(PP, PI);
-  };
-
-  {
-    TestAST AST(Inputs);
-    auto Decls = AST.context().getTranslationUnitDecl()->decls();
-    auto Results =
-        analyze(std::vector<Decl *>{Decls.begin(), Decls.end()},
-                PP.MacroReferences, PP.Includes, &PI, AST.sourceManager(),
-                AST.preprocessor().getHeaderSearchInfo());
+  const Include *B = PP.Includes.atLine(3);
+  ASSERT_EQ(B->Spelled, "b.h");
+  EXPECT_THAT(Results.Missing, ElementsAre("\"c.h\""));
+  EXPECT_THAT(Results.Unused, ElementsAre(B));
+}
 
-    const Include *B = PP.Includes.atLine(3);
-    ASSERT_EQ(B->Spelled, "b.h");
-    EXPECT_THAT(Results.Missing, ElementsAre("\"c.h\""));
-    EXPECT_THAT(Results.Unused, ElementsAre(B));
-  }
+TEST_F(AnalyzeTest, PrivateUsedInPublic) {
+  // Check that umbrella header uses private include.
+  Inputs.Code = R"cpp(#include "private.h")cpp";
+  Inputs.ExtraFiles["private.h"] =
+      guard("// IWYU pragma: private, include \"public.h\"");
+  Inputs.FileName = "public.h";
+  TestAST AST(Inputs);
+  EXPECT_FALSE(PP.Includes.all().empty());
+  auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
+                         AST.preprocessor().getHeaderSearchInfo());
+  EXPECT_THAT(Results.Unused, testing::IsEmpty());
+}
 
+TEST_F(AnalyzeTest, NoCrashWhenUnresolved) {
   // Check that umbrella header uses private include.
-  {
-    Inputs.Code = R"cpp(#include "private.h")cpp";
-    Inputs.ExtraFiles["private.h"] =
-        guard("// IWYU pragma: private, include \"public.h\"");
-    Inputs.FileName = "public.h";
-    PP.Includes = {};
-    PI = {};
-    TestAST AST(Inputs);
-    EXPECT_FALSE(PP.Includes.all().empty());
-    auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
-                           AST.preprocessor().getHeaderSearchInfo());
-    EXPECT_THAT(Results.Unused, testing::IsEmpty());
-  }
+  Inputs.Code = R"cpp(#include "not_found.h")cpp";
+  Inputs.ErrorOK = true;
+  TestAST AST(Inputs);
+  EXPECT_FALSE(PP.Includes.all().empty());
+  auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
+                         AST.preprocessor().getHeaderSearchInfo());
+  EXPECT_THAT(Results.Unused, testing::IsEmpty());
 }
 
 TEST(FixIncludes, Basic) {
Index: clang-tools-extra/include-cleaner/lib/Analysis.cpp
===================================================================
--- clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -91,7 +91,7 @@
 
   AnalysisResults Results;
   for (const Include &I : Inc.all()) {
-    if (Used.contains(&I))
+    if (Used.contains(&I) || !I.Resolved)
       continue;
     if (PI) {
       if (PI->shouldKeep(I.Line))

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits