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

[kadircet]

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

123

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

123

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

123

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

100

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

286

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/ADT/StringSet.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;
+  const DirectoryEntry *ResourceDir =
+      PP->getHeaderSearchInfo().getModuleMap().getBuiltinDir();
   // FIXME: Find a way to have less code duplication between include-cleaner
   // analysis implementation and the below code.
   walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
            *PP,
            [&](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)
+                   (H.physical() == MainFile ||
+                    H.physical()->getDir() == ResourceDir)) {
                  Satisfied = true;
+                 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() == 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(), AST.getPreprocessor(),
       [&](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"] = guard("");
+  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

 //===--- Analysis.cpp -----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang-include-cleaner/Analysis.h"
 #include "AnalysisInternal.h"
 #include "clang-include-cleaner/IncludeSpeller.h"
 #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/DirectoryEntry.h"
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
 #include "llvm/ADT/ArrayRef.h"
analyze(llvm::ArrayRef<Decl *> ASTRoots,
         const PragmaIncludes *PI, const Preprocessor &PP,
         llvm::function_ref<bool(llvm::StringRef)> HeaderFilter) {
   auto &SM = PP.getSourceManager();
   const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
   llvm::DenseSet<const Include *> Used;
   llvm::StringSet<> Missing;
   if (!HeaderFilter)
     HeaderFilter = [](llvm::StringRef) { return false; };
+  const DirectoryEntry *ResourceDir =
+      PP.getHeaderSearchInfo().getModuleMap().getBuiltinDir();
   walkUsed(ASTRoots, MacroRefs, PI, PP,
            [&](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()->getDir() == ResourceDir)) {
                  Satisfied = true;
+               }
                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(), PP.getHeaderSearchInfo(), 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->getFileEntry().getDir() == 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.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.preprocessor());
   EXPECT_THAT(Results.Unused, testing::IsEmpty());
 }
 
+TEST_F(AnalyzeTest, ResourceDirIsIgnored) {
+  Inputs.ExtraArgs.push_back("-resource-dir");
+  Inputs.ExtraArgs.push_back("resources");
+  Inputs.ExtraArgs.push_back("-internal-isystem");
+  Inputs.ExtraArgs.push_back("resources/include");
+  Inputs.Code = R"cpp(
+    #include <amintrin.h>
+    #include <imintrin.h>
+    void baz() {
+      bar();
+    }
+  )cpp";
+  Inputs.ExtraFiles["resources/include/amintrin.h"] = guard("");
+  Inputs.ExtraFiles["resources/include/emintrin.h"] = guard(R"cpp(
+    void bar();
+  )cpp");
+  Inputs.ExtraFiles["resources/include/imintrin.h"] = guard(R"cpp(
+    #include <emintrin.h>
+  )cpp");
+  TestAST AST(Inputs);
+  auto Results = analyze({}, {}, PP.Includes, &PI, AST.preprocessor());
+  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;