[clangd] IncludeCleaner: Add filtering mechanism

[kbobyrev]

Summary

This introduces filtering out inclusions based on the resolved path. This
mechanism will be important for disabling warnings for headers that we can not
diagnose correctly yet.

[sammccall]

clang-tools-extra/clangd/Config.h

106

Config is supposed to be cheap to create.
The regex should be created during compilation and shared across all usages.

Maybe store criteria here as vector<function<bool(StringRef)>> here and have each function capture a shared_ptr<vector<Regex>> by value?

clang-tools-extra/clangd/ConfigCompile.cpp

526

nit: invalid, not incorrect (incorrect suggests it's a regular expression that expresses the wrong condition)

531

you're capturing a reference to a local variable

533

Each time the resulting fragment is evaluated to create a config, we're going to move out of CompiledRegex. After the first time, there's going to be nothing there to move

(apart from the local-variable thing)

clang-tools-extra/clangd/ConfigFragment.h

237

... as unused or missing.

Second sentence is a bit technical.
"These can match any suffix of the header file in question."?

clang-tools-extra/clangd/IncludeCleaner.cpp

267

what are we doing about slashes?
As written, I think we're matching the regex against native paths, which doesn't really make sense for checked-in configs.

268

filterred -> filtered

not enough context in this dlog to be useful I think: filtered out from what?

[dsanders11]

Cool stuff! Chiming in randomly here, but I've been keeping a loose eye on the progress of IncludeCleaner.

I've been using IncludeCleaner to power include cleanup in the Chromium code base for the past few months, and have built Python scripts around it so I can more efficiently use it at a code base level. You can find all that fun stuff on my repo: https://github.com/dsanders11/chromium-include-cleanup

In my usages, I've found there are three categories of ignores I need (you can see my ongoing list of them in the config file: https://github.com/dsanders11/chromium-include-cleanup/blob/main/configs/chromium.json):

• Skip over a file entirely and don't provide any diagnostics for it. This is useful to ignore umbrella headers, without needing to ignore the headers included in that umbrella header.
• Ignore diagnostics for a particular header, which is what is implemented here so far.
• Ignore an include edge (filename, header). This is useful for cases where it's a spurious diagnostic, like when the usage is inside of a macro definition, so IncludeCleaner thinks it is unused but it is not.

This isn't entirely applicable here, but could be insightful: I initially I had my filtering mechanism in the script which ran clangd over the source files, but I found that it was better for my needs to decouple the filtering from the generating the diagnostics. Main reason being that generating the diagnostics for all 80k files in the Chromium code base takes hours to run, so if I found some new ignores I wanted to add, I'd have to do another 7 hour run (on the hardware I'm using), which wasn't efficient. So I let the many script output all diagnostics, and then I filter after the fact, letting me tweak the filtering without needing to re-run the whole thing. So for my own needs I'll probably stick with doing the filtering after the fact, even if it's built-in. Not too applicable for IncludeCleaner usage through say VS Code, but there are different challenges when doing it at code base level.

[kbobyrev]

Resolve comments.

[kbobyrev]

Fix the comment.

[kbobyrev]

In D123488#3444900, @dsanders11 wrote:

Cool stuff! Chiming in randomly here, but I've been keeping a loose eye on the progress of IncludeCleaner.

I've been using IncludeCleaner to power include cleanup in the Chromium code base for the past few months, and have built Python scripts around it so I can more efficiently use it at a code base level. You can find all that fun stuff on my repo: https://github.com/dsanders11/chromium-include-cleanup

In my usages, I've found there are three categories of ignores I need (you can see my ongoing list of them in the config file: https://github.com/dsanders11/chromium-include-cleanup/blob/main/configs/chromium.json):

• Skip over a file entirely and don't provide any diagnostics for it. This is useful to ignore umbrella headers, without needing to ignore the headers included in that umbrella header.
• Ignore diagnostics for a particular header, which is what is implemented here so far.
• Ignore an include edge (filename, header). This is useful for cases where it's a spurious diagnostic, like when the usage is inside of a macro definition, so IncludeCleaner thinks it is unused but it is not.

This isn't entirely applicable here, but could be insightful: I initially I had my filtering mechanism in the script which ran clangd over the source files, but I found that it was better for my needs to decouple the filtering from the generating the diagnostics. Main reason being that generating the diagnostics for all 80k files in the Chromium code base takes hours to run, so if I found some new ignores I wanted to add, I'd have to do another 7 hour run (on the hardware I'm using), which wasn't efficient. So I let the many script output all diagnostics, and then I filter after the fact, letting me tweak the filtering without needing to re-run the whole thing. So for my own needs I'll probably stick with doing the filtering after the fact, even if it's built-in. Not too applicable for IncludeCleaner usage through say VS Code, but there are different challenges when doing it at code base level.

Hi David! This is really interesting, thank you very much for sharing! This is a really exciting use case and I'll look into the code that you have!

I think you might be interested in a more generic effort we're currently investing into: we want to make Include Cleaner a library and make it possible for the users to run it from Clang-Tidy and standalone tools (https://reviews.llvm.org/D121593). Is that something that would be potentially interesting to you?

[kbobyrev]

Oops, sorry, I linked the wrong revision; here's the prototype: we plan to start rolling it out gradually https://reviews.llvm.org/D122677 and then have a common library that we could share between both clangd and Clang-Tidy (and potentially other tools).

[kbobyrev]

@sammccall ping

[sammccall]

Not to fix in this patch, but the move constructor of llvm::Regex is suspicious - there's no move assignment operator.
This putting them in a vector seems likely to copy them when the vector reallocates. Think you could fix this? :-)

clang-tools-extra/clangd/ConfigCompile.cpp

527

missing the error message

clang-tools-extra/clangd/IncludeCleaner.cpp

242

maybe just pass in config?

[sammccall]

Oops, LG with comments

[kbobyrev]

Resolve comments.

clang-tools-extra/clangd/Config.h

 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
 
 #include "support/Context.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Regex.h"
+#include <functional>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 /// Settings that express user/project preferences and control clangd behavior.
 ///
struct {
     /// Configures what clang-tidy checks to run and options to use with them.
     struct {
       // A comma-seperated list of globs specify which clang-tidy checks to run.
       std::string Checks;
       llvm::StringMap<std::string> CheckOptions;
     } ClangTidy;
 
     UnusedIncludesPolicy UnusedIncludes = None;
+
+    /// IncludeCleaner will not diagnose usages of these headers matched by
+    /// these regexes.
+    struct {
+      std::vector<std::function<bool(llvm::StringRef)>> IgnoreHeader;
+    } Includes;
   } Diagnostics;
 
   /// Style of the codebase.
   struct {
     // Namespaces that should always be fully qualified, meaning no "using"
     // declarations, always spell out the whole name (with or without leading
     // ::). All nested namespaces are affected as well.
     std::vector<std::string> FullyQualifiedNamespaces;

clang-tools-extra/clangd/ConfigCompile.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
 #include <algorithm>
+#include <memory>
 #include <string>
+#include <vector>
 
 namespace clang {
 namespace clangd {
 namespace config {
 namespace {
 
 // Returns an empty stringref if Path is not under FragmentDir. Returns Path
 // as-is when FragmentDir is empty.
if (F.UnusedIncludes)
       if (auto Val = compileEnum<Config::UnusedIncludesPolicy>(
                          "UnusedIncludes", **F.UnusedIncludes)
                          .map("Strict", Config::UnusedIncludesPolicy::Strict)
                          .map("None", Config::UnusedIncludesPolicy::None)
                          .value())
         Out.Apply.push_back([Val](const Params &, Config &C) {
           C.Diagnostics.UnusedIncludes = *Val;
         });
+    compile(std::move(F.Includes));
 
     compile(std::move(F.ClangTidy));
   }
 
   void compile(Fragment::StyleBlock &&F) {
     if (!F.FullyQualifiedNamespaces.empty()) {
       std::vector<std::string> FullyQualifiedNamespaces;
       for (auto &N : F.FullyQualifiedNamespaces) {
if (!F.CheckOptions.empty()) {
           [CheckOptions = std::move(CheckOptions)](const Params &, Config &C) {
             for (auto &StringPair : CheckOptions)
               C.Diagnostics.ClangTidy.CheckOptions.insert_or_assign(
                   StringPair.first, StringPair.second);
           });
     }
   }
 
+  void compile(Fragment::DiagnosticsBlock::IncludesBlock &&F) {
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+    static llvm::Regex::RegexFlags Flags = llvm::Regex::IgnoreCase;
+#else
+    static llvm::Regex::RegexFlags Flags = llvm::Regex::NoFlags;
+#endif
+    auto Filters = std::make_shared<std::vector<llvm::Regex>>();
+    for (auto &HeaderPattern : F.IgnoreHeader) {
+      // Anchor on the right.
+      std::string AnchoredPattern = "(" + *HeaderPattern + ")$";
+      llvm::Regex CompiledRegex(AnchoredPattern, Flags);
+      std::string RegexError;
+      if (!CompiledRegex.isValid(RegexError)) {
+        diag(Warning,
+             llvm::formatv("Invalid regular expression '{0}': {1}",
+                           *HeaderPattern, RegexError)
+                 .str(),
+             HeaderPattern.Range);
+        continue;
+      }
+      Filters->push_back(std::move(CompiledRegex));
+    }
+    if (Filters->empty())
+      return;
+    auto Filter = [Filters](llvm::StringRef Path) {
+      for (auto &Regex : *Filters)
+        if (Regex.match(Path))
+          return true;
+      return false;
+    };
+    Out.Apply.push_back([Filter](const Params &, Config &C) {
+      C.Diagnostics.Includes.IgnoreHeader.emplace_back(Filter);
+    });
+  }
+
   void compile(Fragment::CompletionBlock &&F) {
     if (F.AllScopes) {
       Out.Apply.push_back(
           [AllScopes(**F.AllScopes)](const Params &, Config &C) {
             C.Completion.AllScopes = AllScopes;
           });
     }
   }

clang-tools-extra/clangd/ConfigFragment.h

struct DiagnosticsBlock {
     /// transitively includes headers that are used. This should be fixed by
     /// including those headers directly.
     ///
     /// Valid values are:
     /// - Strict
     /// - None
     llvm::Optional<Located<std::string>> UnusedIncludes;
 
+    /// Controls IncludeCleaner diagnostics.
+    struct IncludesBlock {
+      /// Regexes that will be used to avoid diagnosing certain includes as
+      /// unused or missing. These can match any suffix of the header file in
+      /// question.
+      std::vector<Located<std::string>> IgnoreHeader;
+    };
+    IncludesBlock Includes;
+
     /// Controls how clang-tidy will run over the code base.
     ///
     /// The settings are merged with any settings found in .clang-tidy
     /// configuration files with these ones taking precedence.
     struct ClangTidyBlock {
       std::vector<Located<std::string>> Add;
       /// List of checks to disable.
       /// Takes precedence over Add. To enable all llvm checks except include

clang-tools-extra/clangd/IncludeCleaner.cpp

 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLFunctionalExtras.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Regex.h"
+#include <functional>
 
 namespace clang {
 namespace clangd {
 
 static bool AnalyzeStdlib = false;
 void setIncludeCleanerAnalyzesStdlib(bool B) { AnalyzeStdlib = B; }
 
 namespace {
if (!Macro)
       continue;
     auto Loc = Macro->Info->getDefinitionLoc();
     if (Loc.isValid())
       Result.User.insert(Loc);
     // FIXME: support stdlib macros
   }
 }
 
-static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST) {
+static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
+                              const Config &Cfg) {
   if (Inc.BehindPragmaKeep)
     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() == '<') {
     if (AnalyzeStdlib && tooling::stdlib::Header::named(Inc.Written))
       return true;
     return false;
   }
   // Headers without include guards have side effects and are not
   // self-contained, skip them.
   assert(Inc.HeaderID);
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
       AST.getIncludeStructure().getRealPath(
           static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID)));
   assert(FE);
   if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
           &FE->getFileEntry())) {
     dlog("{0} doesn't have header guard and will not be considered unused",
          FE->getName());
     return false;
   }
+  for (auto &Filter : Cfg.Diagnostics.Includes.IgnoreHeader) {
+    // Convert the path to Unix slashes and try to match aginast the fiilter.
+    llvm::SmallString<64> Path(Inc.Resolved);
+    llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
+    if (Filter(Inc.Resolved)) {
+      dlog("{0} header is filtered out by the configuration", FE->getName());
+      return false;
+    }
+  }
   return true;
 }
 
 // In case symbols are coming from non self-contained header, we need to find
 // its first includer that is self-contained. This is the header users can
 // include, so it will be responsible for bringing the symbols from given
 // header into the scope.
 FileID headerResponsible(FileID ID, const SourceManager &SM,
return findReferencedFiles(
       });
 }
 
 std::vector<const Inclusion *>
 getUnused(ParsedAST &AST,
           const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles,
           const llvm::StringSet<> &ReferencedPublicHeaders) {
   trace::Span Tracer("IncludeCleaner::getUnused");
+  const Config &Cfg = Config::current();
   std::vector<const Inclusion *> Unused;
   for (const Inclusion &MFI : AST.getIncludeStructure().MainFileIncludes) {
     if (!MFI.HeaderID)
       continue;
     if (ReferencedPublicHeaders.contains(MFI.Written))
       continue;
     auto IncludeID = static_cast<IncludeStructure::HeaderID>(*MFI.HeaderID);
     bool Used = ReferencedFiles.contains(IncludeID);
-    if (!Used && !mayConsiderUnused(MFI, AST)) {
+    if (!Used && !mayConsiderUnused(MFI, AST, Cfg)) {
       dlog("{0} was not used, but is not eligible to be diagnosed as unused",
            MFI.Written);
       continue;
     }
     if (!Used)
       Unused.push_back(&MFI);
     dlog("{0} is {1}", MFI.Written, Used ? "USED" : "UNUSED");
   }

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

TEST_F(ConfigCompileTests, DiagnosticsIncludeCleaner) {
   EXPECT_EQ(Conf.Diagnostics.UnusedIncludes,
             Config::UnusedIncludesPolicy::None);
 
   Frag = {};
   Frag.Diagnostics.UnusedIncludes.emplace("Strict");
   EXPECT_TRUE(compileAndApply());
   EXPECT_EQ(Conf.Diagnostics.UnusedIncludes,
             Config::UnusedIncludesPolicy::Strict);
+
+  Frag = {};
+  EXPECT_TRUE(Conf.Diagnostics.Includes.IgnoreHeader.empty())
+      << Conf.Diagnostics.Includes.IgnoreHeader.size();
+  Frag.Diagnostics.Includes.IgnoreHeader.push_back(
+      Located<std::string>("foo.h"));
+  Frag.Diagnostics.Includes.IgnoreHeader.push_back(
+      Located<std::string>(".*inc"));
+  EXPECT_TRUE(compileAndApply());
+  auto HeaderFilter = [this](llvm::StringRef Path) {
+    for (auto &Filter : Conf.Diagnostics.Includes.IgnoreHeader) {
+      if (Filter(Path))
+        return true;
+    }
+    return false;
+  };
+  EXPECT_TRUE(HeaderFilter("foo.h"));
+  EXPECT_FALSE(HeaderFilter("bar.h"));
 }
 
 TEST_F(ConfigCompileTests, DiagnosticSuppression) {
   Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move");
   Frag.Diagnostics.Suppress.emplace_back("unreachable-code");
   Frag.Diagnostics.Suppress.emplace_back("-Wunused-variable");
   Frag.Diagnostics.Suppress.emplace_back("typecheck_bool_condition");
   Frag.Diagnostics.Suppress.emplace_back("err_unexpected_friend");

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

 }
 
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
 ]]
   #include "used.h"
 
+  #include "ignore.h"
+
   #include <system_header.h>
 
   void foo() {
     used();
   }
   )cpp");
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
     #pragma once
     void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
     #pragma once
     void used() {}
   )cpp";
+  TU.AdditionalFiles["ignore.h"] = R"cpp(
+    #pragma once
+    void ignore() {}
+  )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
   TU.ExtraArgs = {"-isystem" + testPath("system")};
   // Off by default.
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
   Config Cfg;
   Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
+  // Set filtering.
+  Cfg.Diagnostics.Includes.IgnoreHeader.emplace_back(
+      [](llvm::StringRef Header) { return Header.endswith("ignore.h"); });
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(
       *TU.build().getDiagnostics(),
       UnorderedElementsAre(AllOf(
           Diag(Test.range("diag"), "included header unused.h is not used"),
           withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
           withFix(Fix(Test.range("fix"), "", "remove #include directive")))));
   Cfg.Diagnostics.SuppressAll = true;