[clang-tidy] Avoid matching nodes in system headers

This commit is a re-do of e4a8969,
which got reverted, with the same goal: dramatically speed-up
clang-tidy by avoiding doing work in system headers (which is wasteful
as warnings are later discarded). This proposal was already discussed
here with favorable feedback:
llvm#132725

The novelty of this patch is:

- It's less aggressive: it does not fiddle with AST traversal. This
  solves the issue with the previous patch, which impacted the ability
  to inspect parents of a given node.

- Instead, what we optimize for is exitting early in each Traverse*
  function of MatchASTVisitor if the node is in a system header, thus
  avoiding calling the match() function with its corresponding callback
  (when there is a match).

- It does not cause any failing tests.

- It does not move MatchFinderOptions outside - instead we add
  a user-defined default constructor which solves the same problem.

- It introduces a function "shouldSkipNode" which can be extended
  for adding more conditions. For example there's a PR open about
  skipping modules in clang-tidy where this could come handy:
  llvm#145630

As a benchmark, I ran clang-tidy with all checks activated, on a single
.cpp file which #includes all the standard C++ headers, then measure
the time as well as found warnings.

On trunk:

Suppressed 213314 warnings (213314 in non-user code).

real	0m14.311s
user	0m14.126s
sys	0m0.185s

With this patch:

Suppressed 149399 warnings (149399 in non-user code).
real	0m3.583s
user	0m3.454s
sys	0m0.128s

With the original patch that got reverted:

Suppressed 8050 warnings (8050 in non-user code).
Runtime: around 1 second.

A lot of warnings remain and the runtime is sligthly higher, but we
still got a dramatic reduction with no change in functionality.

Further investigation has shown that all of the remaining warnings are
due to PPCallbacks - implementing a similar system-header exclusion
mechanism there can lead to almost no warnings left in system headers.
This does not bring the runtime down as much, though.

However this may not be as straightforward or wanted, it may even
need to be done on a per-check basis (there's about 10 checks or so
that would need to explicitly ignore system headers). I will leave that
for another patch, it's low priority and does not improve the runtime
much (it just prints better statistics).

Fixes llvm#52959

Author: Carlos Gálvez

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

@@ -425,6 +425,10 @@ 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.IgnoreSystemHeaders = true;
+
   std::unique_ptr<ast_matchers::MatchFinder> Finder(
       new ast_matchers::MatchFinder(std::move(FinderOptions)));
 

clang-tools-extra/docs/ReleaseNotes.rst

@@ -97,6 +97,10 @@ Improvements to clang-tidy
   now run checks in parallel by default using all available hardware threads.
   Both scripts display the number of threads being used in their output.
 
+- :program:`clang-tidy` no longer attemps to match AST nodes from system headers
+  by default, greatly improving performance. This behavior is disabled if the
+  `SystemHeaders` option is enabled.
+
 New checks
 ^^^^^^^^^^
 

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

@@ -66,19 +66,14 @@ 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 %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='.*' -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 %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
 
 #include <system_header.h>
 // CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit

clang/docs/ReleaseNotes.rst

@@ -207,6 +207,9 @@ AST Matchers
 - Ensure ``hasBitWidth`` doesn't crash on bit widths that are dependent on template
   parameters.
 
+- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``. This
+  allows it to skip nodes in system headers when traversing the AST.
+
 clang-format
 ------------
 

clang/include/clang/ASTMatchers/ASTMatchFinder.h

@@ -135,10 +135,15 @@ class MatchFinder {
       llvm::StringMap<llvm::TimeRecord> &Records;
     };
 
+    MatchFinderOptions() {}
+
     /// Enables per-check timers.
     ///
     /// It prints a report after match.
     std::optional<Profiling> CheckProfiling;
+
+    /// Avoids matching declarations in system headers.
+    bool IgnoreSystemHeaders{false};
   };
 
   MatchFinder(MatchFinderOptions Options = MatchFinderOptions());

clang/lib/ASTMatchers/ASTMatchFinder.cpp

@@ -152,7 +152,6 @@ class MatchChildASTVisitor
   // They are public only to allow CRTP to work. They are *not *part
   // of the public API of this class.
   bool TraverseDecl(Decl *DeclNode) {
-
     if (DeclNode && DeclNode->isImplicit() &&
         Finder->isTraversalIgnoringImplicitNodes())
       return baseTraverse(*DeclNode);
@@ -1336,6 +1335,45 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
     return false;
   }
 
+  bool isInSystemHeader(const SourceLocation &Loc) {
+    const SourceManager &SM = getASTContext().getSourceManager();
+    return SM.isInSystemHeader(Loc);
+  }
+
+  template <typename T> SourceLocation getNodeLocation(T const &Node) {
+    return Node.getBeginLoc();
+  }
+
+  SourceLocation getNodeLocation(QualType const &Node) { return {}; }
+
+  SourceLocation getNodeLocation(NestedNameSpecifier const &Node) { return {}; }
+
+  SourceLocation getNodeLocation(CXXCtorInitializer const &Node) {
+    return Node.getSourceLocation();
+  }
+
+  SourceLocation getNodeLocation(TemplateArgumentLoc const &Node) {
+    return Node.getLocation();
+  }
+
+  SourceLocation getNodeLocation(Attr const &Node) {
+    return Node.getLocation();
+  }
+
+  template <typename T>
+  auto shouldSkipNode(T const &Node)
+      -> std::enable_if_t<std::is_pointer_v<T>, bool> {
+    return (Node == nullptr) || shouldSkipNode(*Node);
+  }
+
+  template <typename T>
+  auto shouldSkipNode(T const &Node)
+      -> std::enable_if_t<!std::is_pointer_v<T>, bool> {
+    if (Options.IgnoreSystemHeaders && isInSystemHeader(getNodeLocation(Node)))
+      return true;
+    return false;
+  }
+
   /// Bucket to record map.
   ///
   /// Used to get the appropriate bucket for each matcher.
@@ -1465,9 +1503,8 @@ bool MatchASTVisitor::objcClassIsDerivedFrom(
 }
 
 bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
-  if (!DeclNode) {
+  if (shouldSkipNode(DeclNode))
     return true;
-  }
 
   bool ScopedTraversal =
       TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
@@ -1495,9 +1532,9 @@ bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
 }
 
 bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) {
-  if (!StmtNode) {
+  if (shouldSkipNode(StmtNode))
     return true;
-  }
+
   bool ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
                          TraversingASTChildrenNotSpelledInSource;
 
@@ -1507,11 +1544,17 @@ bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) {
 }
 
 bool MatchASTVisitor::TraverseType(QualType TypeNode) {
+  if (shouldSkipNode(TypeNode))
+    return true;
+
   match(TypeNode);
   return RecursiveASTVisitor<MatchASTVisitor>::TraverseType(TypeNode);
 }
 
 bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode) {
+  if (shouldSkipNode(TypeLocNode))
+    return true;
+
   // The RecursiveASTVisitor only visits types if they're not within TypeLocs.
   // We still want to find those types via matchers, so we match them here. Note
   // that the TypeLocs are structurally a shadow-hierarchy to the expressed
@@ -1523,6 +1566,9 @@ bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode) {
 }
 
 bool MatchASTVisitor::TraverseNestedNameSpecifier(NestedNameSpecifier *NNS) {
+  if (shouldSkipNode(NNS))
+    return true;
+
   match(*NNS);
   return RecursiveASTVisitor<MatchASTVisitor>::TraverseNestedNameSpecifier(NNS);
 }
@@ -1532,6 +1578,9 @@ bool MatchASTVisitor::TraverseNestedNameSpecifierLoc(
   if (!NNS)
     return true;
 
+  if (shouldSkipNode(NNS))
+    return true;
+
   match(NNS);
 
   // We only match the nested name specifier here (as opposed to traversing it)
@@ -1544,7 +1593,7 @@ bool MatchASTVisitor::TraverseNestedNameSpecifierLoc(
 
 bool MatchASTVisitor::TraverseConstructorInitializer(
     CXXCtorInitializer *CtorInit) {
-  if (!CtorInit)
+  if (shouldSkipNode(CtorInit))
     return true;
 
   bool ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
@@ -1562,11 +1611,17 @@ bool MatchASTVisitor::TraverseConstructorInitializer(
 }
 
 bool MatchASTVisitor::TraverseTemplateArgumentLoc(TemplateArgumentLoc Loc) {
+  if (shouldSkipNode(Loc))
+    return true;
+
   match(Loc);
   return RecursiveASTVisitor<MatchASTVisitor>::TraverseTemplateArgumentLoc(Loc);
 }
 
 bool MatchASTVisitor::TraverseAttr(Attr *AttrNode) {
+  if (shouldSkipNode(AttrNode))
+    return true;
+
   match(*AttrNode);
   return RecursiveASTVisitor<MatchASTVisitor>::TraverseAttr(AttrNode);
 }