Revert "[clang-tidy] Avoid processing declarations in system headers … (#132743)

…(#128150)"

This was too aggressive, and leads to problems for downstream users:
#128150 (comment)

Let's revert and reland it once we have addressed the problems.

This reverts commit e4a8969.

Co-authored-by: Carlos Gálvez <[email protected]>

Author: carlosgalvezp

clang-tools-extra/clang-query/Query.cpp

@@ -114,7 +114,7 @@ bool MatchQuery::run(llvm::raw_ostream &OS, QuerySession &QS) const {
     Profiler.emplace();
 
   for (auto &AST : QS.ASTs) {
-    ast_matchers::MatchFinderOptions FinderOptions;
+    ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
     std::optional<llvm::StringMap<llvm::TimeRecord>> Records;
     if (QS.EnableProfile) {
       Records.emplace();

clang-tools-extra/clang-tidy/ClangTidy.cpp

@@ -420,7 +420,7 @@ ClangTidyASTConsumerFactory::createASTConsumer(
   std::vector<std::unique_ptr<ClangTidyCheck>> Checks =
       CheckFactories->createChecksForLanguage(&Context);
 
-  ast_matchers::MatchFinderOptions FinderOptions;
+  ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
 
   std::unique_ptr<ClangTidyProfiling> Profiling;
   if (Context.getEnableProfiling()) {
@@ -429,10 +429,6 @@ ClangTidyASTConsumerFactory::createASTConsumer(
     FinderOptions.CheckProfiling.emplace(Profiling->Records);
   }
 
-  // Avoid processing system headers, unless the user explicitly requests it
-  if (!Context.getOptions().SystemHeaders.value_or(false))
-    FinderOptions.SkipSystemHeaders = true;
-
   std::unique_ptr<ast_matchers::MatchFinder> Finder(
       new ast_matchers::MatchFinder(std::move(FinderOptions)));
 

clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

@@ -35,41 +35,19 @@ AST_POLYMORPHIC_MATCHER_P(
                              Builder) != Args.end();
 }
 
-bool isStdOrPosixImpl(const DeclContext *Ctx) {
-  if (!Ctx->isNamespace())
-    return false;
-
-  const auto *ND = cast<NamespaceDecl>(Ctx);
-  if (ND->isInline()) {
-    return isStdOrPosixImpl(ND->getParent());
-  }
-
-  if (!ND->getParent()->getRedeclContext()->isTranslationUnit())
-    return false;
-
-  const IdentifierInfo *II = ND->getIdentifier();
-  return II && (II->isStr("std") || II->isStr("posix"));
-}
-
-AST_MATCHER(Decl, isInStdOrPosixNS) {
-  for (const auto *Ctx = Node.getDeclContext(); Ctx; Ctx = Ctx->getParent()) {
-    if (isStdOrPosixImpl(Ctx))
-      return true;
-  }
-  return false;
-}
-
 } // namespace
 
 namespace clang::tidy::cert {
 
 void DontModifyStdNamespaceCheck::registerMatchers(MatchFinder *Finder) {
   auto HasStdParent =
       hasDeclContext(namespaceDecl(hasAnyName("std", "posix"),
-                                   unless(hasDeclContext(namespaceDecl())))
+                                   unless(hasParent(namespaceDecl())))
                          .bind("nmspc"));
-  auto UserDefinedType = qualType(hasUnqualifiedDesugaredType(
-      tagType(unless(hasDeclaration(tagDecl(isInStdOrPosixNS()))))));
+  auto UserDefinedType = qualType(
+      hasUnqualifiedDesugaredType(tagType(unless(hasDeclaration(tagDecl(
+          hasAncestor(namespaceDecl(hasAnyName("std", "posix"),
+                                    unless(hasParent(namespaceDecl()))))))))));
   auto HasNoProgramDefinedTemplateArgument = unless(
       hasAnyTemplateArgumentIncludingPack(refersToType(UserDefinedType)));
   auto InsideStdClassOrClassTemplateSpecialization = hasDeclContext(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -91,12 +91,6 @@ Improvements to clang-query
 Improvements to clang-tidy
 --------------------------
 
-- :program:`clang-tidy` no longer processes declarations from system headers
-  by default, greatly improving performance. This behavior is disabled if the
-  `SystemHeaders` option is enabled.
-  Note: this may lead to false negatives; downstream users may need to adjust
-  their checks to preserve existing behavior.
-
 - Improved :program:`clang-tidy-diff.py` script. Add the `-warnings-as-errors`
   argument to treat warnings as errors.
 

clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp

@@ -33,29 +33,23 @@
 // RUN:     readability-identifier-naming.LocalConstantPointerPrefix: 'lc_', \
 // RUN:   }}'
 
-// FIXME: make this test case pass.
-// Currently not working because the CXXRecordDecl for the global anonymous
-// union is *not* collected as a top-level declaration.
-// https://github.com/llvm/llvm-project/issues/130618
-#if 0
 static union {
   int global;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
-// FIXME-CHECK-FIXES: {{^}}  int g_global;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
+// CHECK-FIXES: {{^}}  int g_global;{{$}}
 
   const int global_const;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for global constant 'global_const'
-// FIXME-CHECK-FIXES: {{^}}  const int GLOBAL_CONST;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for global constant 'global_const'
+// CHECK-FIXES: {{^}}  const int GLOBAL_CONST;{{$}}
 
   int *global_ptr;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for global pointer 'global_ptr'
-// FIXME-CHECK-FIXES: {{^}}  int *GlobalPtr_Ptr;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for global pointer 'global_ptr'
+// CHECK-FIXES: {{^}}  int *GlobalPtr_Ptr;{{$}}
 
   int *const global_const_ptr;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global constant pointer 'global_const_ptr'
-// FIXME-CHECK-FIXES: {{^}}  int *const GLOBAL_CONST_PTR_Ptr;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global constant pointer 'global_const_ptr'
+// CHECK-FIXES: {{^}}  int *const GLOBAL_CONST_PTR_Ptr;{{$}}
 };
-#endif
 
 namespace ns {
 

clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp

@@ -66,12 +66,19 @@ class A { A(int); };
 // CHECK4-NOT: warning:
 // CHECK4-QUIET-NOT: warning:
 
+// CHECK: Suppressed 3 warnings (3 in non-user code)
 // CHECK: Use -header-filter=.* to display errors from all non-system headers.
 // CHECK-QUIET-NOT: Suppressed
+// CHECK2: Suppressed 1 warnings (1 in non-user code)
+// CHECK2: Use -header-filter=.* {{.*}}
 // CHECK2-QUIET-NOT: Suppressed
+// CHECK3: Suppressed 2 warnings (2 in non-user code)
+// CHECK3: Use -header-filter=.* {{.*}}
 // CHECK3-QUIET-NOT: Suppressed
 // CHECK4-NOT: Suppressed {{.*}} warnings
+// CHECK4-NOT: Use -header-filter=.* {{.*}}
 // CHECK4-QUIET-NOT: Suppressed
+// CHECK6: Suppressed 2 warnings (2 in non-user code)
 // CHECK6: Use -header-filter=.* {{.*}}
 
 int x = 123;

clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp

@@ -11,9 +11,9 @@
 // RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
 
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS --allow-empty %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS --allow-empty %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
 
 #include <system_header.h>
 // CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit

clang/docs/ReleaseNotes.rst

@@ -458,11 +458,6 @@ AST Matchers
 - Ensure ``isDerivedFrom`` matches the correct base in case more than one alias exists.
 - Extend ``templateArgumentCountIs`` to support function and variable template
   specialization.
-- Move ``ast_matchers::MatchFinder::MatchFinderOptions`` to
-  ``ast_matchers::MatchFinderOptions``.
-- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``, and make
-  ``MatchASTConsumer`` receive a reference to ``MatchFinderOptions`` in the
-  constructor. This allows it to skip system headers when traversing the AST.
 
 clang-format
 ------------

clang/include/clang/ASTMatchers/ASTMatchFinder.h

@@ -50,24 +50,6 @@ namespace clang {
 
 namespace ast_matchers {
 
-/// A struct defining options for configuring the MatchFinder.
-struct MatchFinderOptions {
-  struct Profiling {
-    Profiling(llvm::StringMap<llvm::TimeRecord> &Records) : Records(Records) {}
-
-    /// Per bucket timing information.
-    llvm::StringMap<llvm::TimeRecord> &Records;
-  };
-
-  /// Enables per-check timers.
-  ///
-  /// It prints a report after match.
-  std::optional<Profiling> CheckProfiling;
-
-  /// Avoids matching declarations in system headers.
-  bool SkipSystemHeaders = false;
-};
-
 /// A class to allow finding matches over the Clang AST.
 ///
 /// After creation, you can add multiple matchers to the MatchFinder via
@@ -144,6 +126,21 @@ class MatchFinder {
     virtual void run() = 0;
   };
 
+  struct MatchFinderOptions {
+    struct Profiling {
+      Profiling(llvm::StringMap<llvm::TimeRecord> &Records)
+          : Records(Records) {}
+
+      /// Per bucket timing information.
+      llvm::StringMap<llvm::TimeRecord> &Records;
+    };
+
+    /// Enables per-check timers.
+    ///
+    /// It prints a report after match.
+    std::optional<Profiling> CheckProfiling;
+  };
+
   MatchFinder(MatchFinderOptions Options = MatchFinderOptions());
   ~MatchFinder();
 

clang/lib/ASTMatchers/ASTMatchFinder.cpp

@@ -28,7 +28,6 @@
 #include <deque>
 #include <memory>
 #include <set>
-#include <vector>
 
 namespace clang {
 namespace ast_matchers {
@@ -423,7 +422,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
                         public ASTMatchFinder {
 public:
   MatchASTVisitor(const MatchFinder::MatchersByType *Matchers,
-                  const MatchFinderOptions &Options)
+                  const MatchFinder::MatchFinderOptions &Options)
       : Matchers(Matchers), Options(Options), ActiveASTContext(nullptr) {}
 
   ~MatchASTVisitor() override {
@@ -1351,7 +1350,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
   /// We precalculate a list of matchers that pass the toplevel restrict check.
   llvm::DenseMap<ASTNodeKind, std::vector<unsigned short>> MatcherFiltersMap;
 
-  const MatchFinderOptions &Options;
+  const MatchFinder::MatchFinderOptions &Options;
   ASTContext *ActiveASTContext;
 
   // Maps a canonical type to its TypedefDecls.
@@ -1574,41 +1573,19 @@ bool MatchASTVisitor::TraverseAttr(Attr *AttrNode) {
 class MatchASTConsumer : public ASTConsumer {
 public:
   MatchASTConsumer(MatchFinder *Finder,
-                   MatchFinder::ParsingDoneTestCallback *ParsingDone,
-                   const MatchFinderOptions &Options)
-      : Finder(Finder), ParsingDone(ParsingDone), Options(Options) {}
+                   MatchFinder::ParsingDoneTestCallback *ParsingDone)
+      : Finder(Finder), ParsingDone(ParsingDone) {}
 
 private:
-  bool HandleTopLevelDecl(DeclGroupRef DG) override {
-    if (Options.SkipSystemHeaders) {
-      for (Decl *D : DG) {
-        if (!isInSystemHeader(D))
-          TraversalScope.push_back(D);
-      }
-    }
-    return true;
-  }
-
   void HandleTranslationUnit(ASTContext &Context) override {
-    if (!TraversalScope.empty())
-      Context.setTraversalScope(TraversalScope);
-
     if (ParsingDone != nullptr) {
       ParsingDone->run();
     }
     Finder->matchAST(Context);
   }
 
-  bool isInSystemHeader(Decl *D) {
-    const SourceManager &SM = D->getASTContext().getSourceManager();
-    const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
-    return SM.isInSystemHeader(Loc);
-  }
-
   MatchFinder *Finder;
   MatchFinder::ParsingDoneTestCallback *ParsingDone;
-  const MatchFinderOptions &Options;
-  std::vector<Decl *> TraversalScope;
 };
 
 } // end namespace
@@ -1727,8 +1704,7 @@ bool MatchFinder::addDynamicMatcher(const internal::DynTypedMatcher &NodeMatch,
 }
 
 std::unique_ptr<ASTConsumer> MatchFinder::newASTConsumer() {
-  return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone,
-                                                      Options);
+  return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone);
 }
 
 void MatchFinder::match(const clang::DynTypedNode &Node, ASTContext &Context) {