[include-cleaner][clangd][clang-tidy] Ignore resource dir during include-cleaner analysis.

[kadircet]

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp

122

let's use HS->getModuleMap().getBuiltinDir() then we can get away with just comparing that pointer to H.physical()->getLastRef().getDir() (same applies to all the other places as well)

clang-tools-extra/clangd/IncludeCleaner.cpp

314

let's move this into mayConsiderUnused, we also convert this include into a FileEntry in there, so we can directly compare the directory agian.

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

579–580

can you rather include these as <amintrin.h>

587–588

we should put these under resources/include/xxxx.h

[VitaNuo]

Address review comments.

[VitaNuo]

Thanks for the comments!

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp

122

This only works in clangd code for me. I get nullptr for HS->getModuleMap().getBuiltinDir() in other places (Clang Tidy check and the library).

clang-tools-extra/clangd/IncludeCleaner.cpp

314

Ok moved to mayConsiderUnused and using the file entry now, but see above regarding comparing the directories.

[kadircet]

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp

122

hmm that's probably because you're not setting up the include structure correctly. resource-dir isn't the builtindir, it's resource-dir + "/include" (the comments i had in the clangd unittests applies in a similar manner to other tests as well).

[VitaNuo]

Fix analysis test to properly recognize the resource directory.
Use HeaderSearch->getModuleMap().BuiltinDir to find out the resource directory.

[VitaNuo]

Thanks for the help in resolving the resource dir issues!

[VitaNuo]

Rebase to current main.

[kadircet]

thanks!

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

587

nit: also wrap contents in guard() here to make sure we're not testing multiple things here (i.e. behavior for non-self-contained headers)

clang-tools-extra/include-cleaner/lib/Analysis.cpp

82

now we're not going to put matching includes into the Used set. i know we're going to filter them put from the final output in the next loop, but still better to keep normal track of Used i believe

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp

288

again lets make this self-contained

[VitaNuo]

Address review comments.

[VitaNuo]

Thanks!

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp

 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Format/Format.h"
+#include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Inclusions/HeaderIncludes.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorHandling.h"
void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
   llvm::SmallVector<Decl *> MainFileDecls;
   for (Decl *D : Result.Nodes.getNodeAs<TranslationUnitDecl>("top")->decls()) {
     if (!SM->isWrittenInMainFile(SM->getExpansionLoc(D->getLocation())))
       continue;
     // FIXME: Filter out implicit template specializations.
     MainFileDecls.push_back(D);
   }
   llvm::DenseSet<include_cleaner::Symbol> SeenSymbols;
+  std::string ResourceDir = HS->getHeaderSearchOpts().ResourceDir;
   // FIXME: Find a way to have less code duplication between include-cleaner
   // analysis implementation and the below code.
-  walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
-           *SM,
-           [&](const include_cleaner::SymbolReference &Ref,
-               llvm::ArrayRef<include_cleaner::Header> Providers) {
-             // Process each symbol once to reduce noise in the findings.
-             // Tidy checks are used in two different workflows:
-             // - Ones that show all the findings for a given file. For such
-             // workflows there is not much point in showing all the occurences,
-             // as one is enough to indicate the issue.
-             // - Ones that show only the findings on changed pieces. For such
-             // workflows it's useful to show findings on every reference of a
-             // symbol as otherwise tools might give incosistent results
-             // depending on the parts of the file being edited. But it should
-             // still help surface findings for "new violations" (i.e.
-             // dependency did not exist in the code at all before).
-             if (DeduplicateFindings && !SeenSymbols.insert(Ref.Target).second)
-               return;
-             bool Satisfied = false;
-             for (const include_cleaner::Header &H : Providers) {
-               if (H.kind() == include_cleaner::Header::Physical &&
-                   H.physical() == MainFile)
-                 Satisfied = true;
+  walkUsed(
+      MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI, *SM,
+              (H.physical() == MainFile ||
+               H.physical()->getLastRef().getDir().getName() == ResourceDir)) {
+            continue;
+          }
 
-               for (const include_cleaner::Include *I :
-                    RecordedPreprocessor.Includes.match(H)) {
-                 Used.insert(I);
-                 Satisfied = true;
-               }
-             }
-             if (!Satisfied && !Providers.empty() &&
-                 Ref.RT == include_cleaner::RefType::Explicit &&
-                 !shouldIgnore(Providers.front()))
-               Missing.push_back({Ref, Providers.front()});
-           });
 
   std::vector<const include_cleaner::Include *> Unused;
   for (const include_cleaner::Include &I :
        RecordedPreprocessor.Includes.all()) {
-    if (Used.contains(&I) || !I.Resolved)
+    if (Used.contains(&I) || !I.Resolved ||
+        I.Resolved->getDir().getName() == ResourceDir)
       continue;
     if (RecordedPI.shouldKeep(*I.Resolved))
       continue;
     // Check if main file is the public interface for a private header. If so
     // we shouldn't diagnose it as unused.
     if (auto PHeader = RecordedPI.getPublic(*I.Resolved); !PHeader.empty()) {
       PHeader = PHeader.trim("<>\"");
       // Since most private -> public mappings happen in a verbatim way, we

clang-tools-extra/clangd/IncludeCleaner.cpp

 
 bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
                        const include_cleaner::PragmaIncludes *PI) {
   assert(Inc.HeaderID);
   auto HID = static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID);
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
       AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
+  if (FE->getDir() == AST.getPreprocessor()
+                  .getHeaderSearchInfo()
+                  .getModuleMap()
+                  .getBuiltinDir()) 
+    return false;
   if (PI && PI->shouldKeep(*FE))
     return false;
   // FIXME(kirillbobyrev): We currently do not support the umbrella headers.
   // System headers are likely to be standard library headers.
   // Until we have good support for umbrella headers, don't warn about them.
   if (Inc.Written.front() == '<')
     return tooling::stdlib::Header::named(Inc.Written).has_value();
   if (PI) {
getUnused(ParsedAST &AST,
   trace::Span Tracer("IncludeCleaner::getUnused");
   std::vector<const Inclusion *> Unused;
   for (const Inclusion &MFI : AST.getIncludeStructure().MainFileIncludes) {
     if (!MFI.HeaderID)
       continue;
     auto IncludeID = static_cast<IncludeStructure::HeaderID>(*MFI.HeaderID);
     if (ReferencedFiles.contains(IncludeID))
       continue;
     if (!mayConsiderUnused(MFI, AST, AST.getPragmaIncludes().get())) {
       dlog("{0} was not used, but is not eligible to be diagnosed as unused",
            MFI.Written);
       continue;
     }
     Unused.push_back(&MFI);
   }
   return Unused;
 }
IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
   const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
   auto *PreamblePatch = PreamblePatch::getPatchEntry(AST.tuPath(), SM);
 
   std::vector<include_cleaner::SymbolReference> Macros =
       collectMacroReferences(AST);
   std::vector<MissingIncludeDiagInfo> MissingIncludes;
   llvm::DenseSet<IncludeStructure::HeaderID> Used;
   trace::Span Tracer("include_cleaner::walkUsed");
+  const DirectoryEntry *ResourceDir = AST.getPreprocessor()
+                                .getHeaderSearchInfo()
+                                .getModuleMap()
+                                .getBuiltinDir();
   include_cleaner::walkUsed(
       AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
       AST.getPragmaIncludes().get(), SM,
       [&](const include_cleaner::SymbolReference &Ref,
           llvm::ArrayRef<include_cleaner::Header> Providers) {
         bool Satisfied = false;
         for (const auto &H : Providers) {
           if (H.kind() == include_cleaner::Header::Physical &&
-              (H.physical() == MainFile || H.physical() == PreamblePatch)) {
+              (H.physical() == MainFile || H.physical() == PreamblePatch ||
+               H.physical()->getLastRef().getDir() == ResourceDir)) {
             Satisfied = true;
             continue;
           }
           for (auto *Inc : ConvertedIncludes.match(H)) {
             Satisfied = true;
             auto HeaderID =
                 AST.getIncludeStructure().getID(&Inc->Resolved->getFileEntry());
             assert(HeaderID.has_value() &&

clang-tools-extra/clangd/test/include-cleaner-batch-fix.test

 # We specify a custom path in XDG_CONFIG_HOME, which only works on some systems.
 # UNSUPPORTED: system-windows
 # UNSUPPORTED: system-darwin
 
 # RUN: rm -rf %t
 # RUN: mkdir -p %t/clangd
 # RUN: cp -r %S/Inputs/include-cleaner %t/include
+# RUN: echo '-I%t/include' > %t/compile_flags.txt
 # Create a config file enabling include-cleaner features.
 # RUN: echo $'Diagnostics:\n  UnusedIncludes: Strict\n  MissingIncludes: Strict' >> %t/clangd/config.yaml
 
-# RUN: env XDG_CONFIG_HOME=%t clangd -lit-test -enable-config --resource-dir=%t < %s | FileCheck -strict-whitespace %s
+# RUN: env XDG_CONFIG_HOME=%t clangd -lit-test -enable-config --compile-commands-dir=%t < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"workspaceEdit":{"documentChanges":true, "changeAnnotationSupport":{"groupsOnLabel":true}}}},"trace":"off"}}
 ---
 {
   "jsonrpc": "2.0",
   "method": "textDocument/didOpen",
   "params": {
     "textDocument": {
       "uri": "test:///simple.cpp",

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

TEST(IncludeCleaner, VerbatimEquivalence) {
   TU.ExtraArgs.push_back("-Irepo/lib");
 
   auto AST = TU.build();
   auto Findings = computeIncludeCleanerFindings(AST);
   EXPECT_THAT(Findings.MissingIncludes, IsEmpty());
   EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
 }
 
+TEST(IncludeCleaner, ResourceDirIsIgnored) {
+  auto TU = TestTU::withCode(R"cpp(
+    #include <amintrin.h>
+    #include <imintrin.h>
+    void baz() {
+      bar();
+    }
+  )cpp");
+  TU.ExtraArgs.push_back("-resource-dir");
+  TU.ExtraArgs.push_back(testPath("resources"));
+  TU.AdditionalFiles["resources/include/amintrin.h"] = "";
+  TU.AdditionalFiles["resources/include/imintrin.h"] = guard(R"cpp(
+    #include <emintrin.h>
+  )cpp");
+  TU.AdditionalFiles["resources/include/emintrin.h"] = guard(R"cpp(
+    void bar();
+  )cpp");
+  auto AST = TU.build();
+  auto Findings = computeIncludeCleanerFindings(AST);
+  EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
+  EXPECT_THAT(Findings.MissingIncludes, IsEmpty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/include-cleaner/lib/Analysis.cpp

 #include "clang-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/STLFunctionalExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
analyze(llvm::ArrayRef<Decl *> ASTRoots,
         const PragmaIncludes *PI, const SourceManager &SM,
         const HeaderSearch &HS,
         llvm::function_ref<bool(llvm::StringRef)> HeaderFilter) {
   const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
   llvm::DenseSet<const Include *> Used;
   llvm::StringSet<> Missing;
   if (!HeaderFilter)
     HeaderFilter = [](llvm::StringRef) { return false; };
+  std::string ResourceDir = HS.getHeaderSearchOpts().ResourceDir;
   walkUsed(ASTRoots, MacroRefs, PI, SM,
            [&](const SymbolReference &Ref, llvm::ArrayRef<Header> Providers) {
              bool Satisfied = false;
              for (const Header &H : Providers) {
-               if (H.kind() == Header::Physical && H.physical() == MainFile)
+               if (H.kind() == Header::Physical &&
+                   (H.physical() == MainFile ||
+                    H.physical()->getLastRef().getDir().getName() ==
+                        ResourceDir)) {
                  Satisfied = true;
+                 continue;
+               }
                for (const Include *I : Inc.match(H)) {
                  Used.insert(I);
                  Satisfied = true;
                }
              }
              if (!Satisfied && !Providers.empty() &&
                  Ref.RT == RefType::Explicit &&
                  !HeaderFilter(Providers.front().resolvedPath()))
                Missing.insert(spellHeader({Providers.front(), HS, MainFile}));
            });
 
   AnalysisResults Results;
   for (const Include &I : Inc.all()) {
     if (Used.contains(&I) || !I.Resolved ||
-        HeaderFilter(I.Resolved->getFileEntry().tryGetRealPathName()))
+        HeaderFilter(I.Resolved->getFileEntry().tryGetRealPathName()) ||
+        I.Resolved->getDir().getName() == ResourceDir)
       continue;
     if (PI) {
       if (PI->shouldKeep(*I.Resolved))
         continue;
       // Check if main file is the public interface for a private header. If so
       // we shouldn't diagnose it as unused.
       if (auto PHeader = PI->getPublic(*I.Resolved); !PHeader.empty()) {
         PHeader = PHeader.trim("<>\"");

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp

TEST_F(AnalyzeTest, NoCrashWhenUnresolved) {
   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_F(AnalyzeTest, ResourceDirIsIgnored) {
+  Inputs.ExtraArgs.push_back("-resource-dir");
+  Inputs.ExtraArgs.push_back("./resources");
+  Inputs.Code = R"cpp(
+    #include "resources/amintrin.h"
+    #include "resources/imintrin.h"
+    void baz() {
+      bar();
+    }
+  )cpp";
+  Inputs.ExtraFiles["resources/amintrin.h"] = "";
+  Inputs.ExtraFiles["resources/emintrin.h"] = guard(R"cpp(
+    void bar();
+  )cpp");
+  Inputs.ExtraFiles["resources/imintrin.h"] = guard(R"cpp(
+    #include "emintrin.h"
+  )cpp");
+  TestAST AST(Inputs);
+  auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
+                         AST.preprocessor().getHeaderSearchInfo());
+  EXPECT_THAT(Results.Unused, testing::IsEmpty());
+  EXPECT_THAT(Results.Missing, testing::IsEmpty());
+}
+
 TEST(FixIncludes, Basic) {
   llvm::StringRef Code = R"cpp(#include "d.h"
 #include "a.h"
 #include "b.h"
 #include <c.h>
 )cpp";
 
   Includes Inc;