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

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

Author: Carlos Gálvez

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

@@ -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.

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/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
 ------------

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) {