[clang][AST][clang-tidy] Do not set a reduced traversal scope in ASTM…

[carlosgalvezp]

…atchFinder

This was introduced here with the purpose of speed up clang-tidy, making it not process system headers:

e4a8969

However it was later realized that this change is too aggressive: the reduced traversal scope also impacts the ParentMapContext, which in turn means that matchers like "hasParent" or "hasAncestor" are not able to find parents in system headers, even if the declaration at hand is in a user header. This causes regressions for downstream users writing custom clang-tidy checks:
#128150 (comment)

This patch fixes this problem, as follows:

• Revert the changes to the clang-tidy unit tests.
• Revert setting the TraversalScope in MatchASTConsumer.
• Move the logic to MatchASTVisitor::TraverseDecl instead.

Pros:

• Leaves the ASTContext and ParentMapContext untouched, so hasParent and similar matchers should work again.
• Keeps avoiding matching and checking decls outside of system headers.
• The broken unit test in readability now works, because we are no longer processing only TopLevelDecls.
• The changes to the CERT test can be reverted.
• Most likely we don't lose any functionality or introduce false negatives.

Cons:

• We still have to traverse decls in system headers. I did try to return false; instead of return true; in the proposed code but that made around 60 clang-tidy tests to fail.
• We still process many nodes (not Decls) that are in system headers.

As a benchmark, I tried running all clang-tidy checks on a .cpp file that includes all the standard C++ headers. This leads to:

• Baseline (without any optimizations). Suppressed 196093 warnings (196093 in non-user code) 10 seconds to run.

• Trunk (aggressive optimizations). Suppressed 8050 warnings (8050 in non-user code). 1 second to run.

• This patch (conservative optimizations). Suppressed 141779 warnings (141779 in non-user code). 3 seconds to run.

This patch thus gives some performance improvement while keeping backwards compatibility. There's still room for improvement which shall be tackled in a follow-up patch after unbreaking clang-tidy.

Fixes #130618

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang

Author: Carlos Galvez (carlosgalvezp)

Changes

…atchFinder

This was introduced here with the purpose of speed up clang-tidy, making it not process system headers:

e4a8969

However it was later realized that this change is too aggressive: the reduced traversal scope also impacts the ParentMapContext, which in turn means that matchers like "hasParent" or "hasAncestor" are not able to find parents in system headers, even if the declaration at hand is in a user header. This causes regressions for downstream users writing custom clang-tidy checks:
#128150 (comment)

This patch fixes this problem, as follows:

• Revert the changes to the clang-tidy unit tests.
• Revert setting the TraversalScope in MatchASTConsumer.
• Move the logic to MatchASTVisitor::TraverseDecl instead.

Pros:

• Leaves the ASTContext and ParentMapContext untouched, so hasParent and similar matchers should work again.
• Keeps avoiding matching and checking decls outside of system headers.
• The broken unit test in readability now works, because we are no longer processing only TopLevelDecls.
• The changes to the CERT test can be reverted.
• Most likely we don't lose any functionality or introduce false negatives.

Cons:

• We still have to traverse decls in system headers. I did try to return false; instead of return true; in the proposed code but that made around 60 clang-tidy tests to fail.
• We still process many nodes (not Decls) that are in system headers.

As a benchmark, I tried running all clang-tidy checks on a .cpp file that includes all the standard C++ headers. This leads to:

• Baseline (without any optimizations). Suppressed 196093 warnings (196093 in non-user code) 10 seconds to run.

• Trunk (aggressive optimizations). Suppressed 8050 warnings (8050 in non-user code). 1 second to run.

• This patch (conservative optimizations). Suppressed 141779 warnings (141779 in non-user code). 3 seconds to run.

This patch thus gives some performance improvement while keeping backwards compatibility. There's still room for improvement which shall be tackled in a follow-up patch after unbreaking clang-tidy.

Fixes #130618

---

Full diff: https://github.com/llvm/llvm-project/pull/132725.diff

5 Files Affected:

• (modified) clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp (+5-27)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (-2)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp (+8-14)
• (modified) clang/docs/ReleaseNotes.rst (+2-3)
• (modified) clang/lib/ASTMatchers/ASTMatchFinder.cpp (+13-27)

diff --git a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
index 2dff4c0e53b8c..bc4970825b4ca 100644
--- a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
@@ -35,30 +35,6 @@ 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 {
@@ -66,10 +42,12 @@ 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(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index ed7da975f3de7..a05be39c8dce5 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,8 +94,6 @@ 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.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
index ad6525276ff8a..1b4d4e924a721 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
+++ b/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 {
 
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8182bccdd2da8..f4698b6c70c81 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -460,9 +460,8 @@ AST Matchers
   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.
+- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``. This
+  allows it to avoid matching declarations in system headers .
 
 clang-format
 ------------
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index e347d0c54d9b0..995d9986b1ba3 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -28,7 +28,6 @@
 #include <deque>
 #include <memory>
 #include <set>
-#include <vector>
 
 namespace clang {
 namespace ast_matchers {
@@ -1464,11 +1463,21 @@ bool MatchASTVisitor::objcClassIsDerivedFrom(
   return false;
 }
 
+static bool isInSystemHeader(Decl *D) {
+  const SourceManager &SM = D->getASTContext().getSourceManager();
+  const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
+  return SM.isInSystemHeader(Loc);
+}
+
 bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
   if (!DeclNode) {
     return true;
   }
 
+  // Skip declarations in system headers if requested
+  if (Options.SkipSystemHeaders && isInSystemHeader(DeclNode))
+    return true;
+
   bool ScopedTraversal =
       TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
   bool ScopedChildren = TraversingASTChildrenNotSpelledInSource;
@@ -1574,41 +1583,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 +1714,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) {

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: Carlos Galvez (carlosgalvezp)

Changes

…atchFinder

This was introduced here with the purpose of speed up clang-tidy, making it not process system headers:

e4a8969

However it was later realized that this change is too aggressive: the reduced traversal scope also impacts the ParentMapContext, which in turn means that matchers like "hasParent" or "hasAncestor" are not able to find parents in system headers, even if the declaration at hand is in a user header. This causes regressions for downstream users writing custom clang-tidy checks:
#128150 (comment)

This patch fixes this problem, as follows:

• Revert the changes to the clang-tidy unit tests.
• Revert setting the TraversalScope in MatchASTConsumer.
• Move the logic to MatchASTVisitor::TraverseDecl instead.

Pros:

• Leaves the ASTContext and ParentMapContext untouched, so hasParent and similar matchers should work again.
• Keeps avoiding matching and checking decls outside of system headers.
• The broken unit test in readability now works, because we are no longer processing only TopLevelDecls.
• The changes to the CERT test can be reverted.
• Most likely we don't lose any functionality or introduce false negatives.

Cons:

• We still have to traverse decls in system headers. I did try to return false; instead of return true; in the proposed code but that made around 60 clang-tidy tests to fail.
• We still process many nodes (not Decls) that are in system headers.

As a benchmark, I tried running all clang-tidy checks on a .cpp file that includes all the standard C++ headers. This leads to:

• Baseline (without any optimizations). Suppressed 196093 warnings (196093 in non-user code) 10 seconds to run.

• Trunk (aggressive optimizations). Suppressed 8050 warnings (8050 in non-user code). 1 second to run.

• This patch (conservative optimizations). Suppressed 141779 warnings (141779 in non-user code). 3 seconds to run.

This patch thus gives some performance improvement while keeping backwards compatibility. There's still room for improvement which shall be tackled in a follow-up patch after unbreaking clang-tidy.

Fixes #130618

---

Full diff: https://github.com/llvm/llvm-project/pull/132725.diff

5 Files Affected:

• (modified) clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp (+5-27)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (-2)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp (+8-14)
• (modified) clang/docs/ReleaseNotes.rst (+2-3)
• (modified) clang/lib/ASTMatchers/ASTMatchFinder.cpp (+13-27)

diff --git a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
index 2dff4c0e53b8c..bc4970825b4ca 100644
--- a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
@@ -35,30 +35,6 @@ 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 {
@@ -66,10 +42,12 @@ 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(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index ed7da975f3de7..a05be39c8dce5 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,8 +94,6 @@ 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.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
index ad6525276ff8a..1b4d4e924a721 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
+++ b/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 {
 
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8182bccdd2da8..f4698b6c70c81 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -460,9 +460,8 @@ AST Matchers
   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.
+- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``. This
+  allows it to avoid matching declarations in system headers .
 
 clang-format
 ------------
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index e347d0c54d9b0..995d9986b1ba3 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -28,7 +28,6 @@
 #include <deque>
 #include <memory>
 #include <set>
-#include <vector>
 
 namespace clang {
 namespace ast_matchers {
@@ -1464,11 +1463,21 @@ bool MatchASTVisitor::objcClassIsDerivedFrom(
   return false;
 }
 
+static bool isInSystemHeader(Decl *D) {
+  const SourceManager &SM = D->getASTContext().getSourceManager();
+  const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
+  return SM.isInSystemHeader(Loc);
+}
+
 bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
   if (!DeclNode) {
     return true;
   }
 
+  // Skip declarations in system headers if requested
+  if (Options.SkipSystemHeaders && isInSystemHeader(DeclNode))
+    return true;
+
   bool ScopedTraversal =
       TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
   bool ScopedChildren = TraversingASTChildrenNotSpelledInSource;
@@ -1574,41 +1583,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 +1714,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) {

[carlosgalvezp]

@kadircet Are you able to test this PR and confirm it fixes the regressions you encountered?

[Xazax-hun]

I think it might be cleaner if reverting the previous solution and introducing this new one would be two separate commits or maybe even separate PRs.

[kadircet]

Are you able to test this PR and confirm it fixes the regressions you encountered?

hi @carlosgalvezp , yes this fixes the particular issues we had, thanks!

[kadircet]

i think this still holds. the matching logic is still different than before. despite this version being more conservative, i think there is still value in having an explicit SkipSystemHeaders flag so that affected users can run with old behavior until code fixes are available for affected clang-tidy checks.

[carlosgalvezp]

explicit SkipSystemHeaders flag

The problem I have with such a flag is that 1) users will not report issues, just silence them with the flag, therefore 2) people will rely on the flag, so it will need to live forever and 3) we won't be able to unit test all checks with and without the flag.

[kadircet]

1. users will not report issues, just silence them with the flag

I agree that this is a risk, but many people use major releases of clang-tidy, hence by the time we get feedback, it'll be impossible for those folks to pick up the fix. I am not sure if it's worth leaving it in broken state for these people until next release. But I am not really a decision maker here, so it's up to you folks.

3. we won't be able to unit test all checks with and without the flag

Well, we still have two modes today, it's just controlled by a different flag (warn in system headers on vs off) and we're not testing both. I think this is fine, as we know that SkipSystemHeaders = True is the only risky configuration. It should be fine to keep testing scenario the same, by default test with SkipSystemHeaders = True and only test for SkipSystemHeaders = false when we want to prevent regressions in the future.

[carlosgalvezp]

reverting the previous solution

I thought about that, yes that might be cleaner. Will do!

[vbvictor]

Since patch #151035 is merged, do we still need this one, or it could be closed?

[carlosgalvezp]

Yes, thank you, closing!