[clang-tidy] Avoid matching nodes in system headers (#151035)

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: #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` - 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:
#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 75413 warnings (75413 in non-user code).

real	0m12.418s
user	0m12.270s
sys	0m0.129s
```

With this patch:

```
Suppressed 11448 warnings (11448 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

real	0m1.666s
user	0m1.538s
sys	0m0.129s
```

With the original patch that got reverted:

```
Suppressed 11428 warnings (11428 in non-user code).

real	0m1.193s
user	0m1.096s
sys	0m0.096s
```

We therefore get a dramatic reduction in number of warnings and runtime,
with no change in functionality.

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, so it's probably not worth the effort.

Fixes #52959

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

Author: carlosgalvezp

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

@@ -424,6 +424,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

@@ -104,6 +104,10 @@ Improvements to clang-query
 Improvements to clang-tidy
 --------------------------
 
+- :program:`clang-tidy` no longer attemps to analyze code from system headers
+  by default, greatly improving performance. This behavior is disabled if the
+  `SystemHeaders` option is enabled.
+
 - The :program:`run-clang-tidy.py` and :program:`clang-tidy-diff.py` scripts
   now run checks in parallel by default using all available hardware threads.
   Both scripts display the number of threads being used in their output.

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

@@ -308,6 +308,9 @@ AST Matchers
 - Ensure ``hasBitWidth`` doesn't crash on bit widths that are dependent on template
   parameters.
 
+- Add a boolean member ``IgnoreSystemHeaders`` to ``MatchFinderOptions``. This
+  allows it to ignore 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

@@ -1344,6 +1344,41 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
     return false;
   }
 
+  template <typename T> static SourceLocation getNodeLocation(const T &Node) {
+    return Node.getBeginLoc();
+  }
+
+  static SourceLocation getNodeLocation(const CXXCtorInitializer &Node) {
+    return Node.getSourceLocation();
+  }
+
+  static SourceLocation getNodeLocation(const TemplateArgumentLoc &Node) {
+    return Node.getLocation();
+  }
+
+  static SourceLocation getNodeLocation(const Attr &Node) {
+    return Node.getLocation();
+  }
+
+  bool isInSystemHeader(SourceLocation Loc) {
+    const SourceManager &SM = getASTContext().getSourceManager();
+    return SM.isInSystemHeader(Loc);
+  }
+
+  template <typename T> bool shouldSkipNode(T &Node) {
+    if (Options.IgnoreSystemHeaders && isInSystemHeader(getNodeLocation(Node)))
+      return true;
+    return false;
+  }
+
+  template <typename T> bool shouldSkipNode(T *Node) {
+    return (Node == nullptr) || shouldSkipNode(*Node);
+  }
+
+  bool shouldSkipNode(QualType &) { return false; }
+
+  bool shouldSkipNode(NestedNameSpecifier &) { return false; }
+
   /// Bucket to record map.
   ///
   /// Used to get the appropriate bucket for each matcher.
@@ -1473,9 +1508,8 @@ bool MatchASTVisitor::objcClassIsDerivedFrom(
 }
 
 bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
-  if (!DeclNode) {
+  if (shouldSkipNode(DeclNode))
     return true;
-  }
 
   bool ScopedTraversal =
       TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
@@ -1503,9 +1537,9 @@ bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
 }
 
 bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) {
-  if (!StmtNode) {
+  if (shouldSkipNode(StmtNode))
     return true;
-  }
+
   bool ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
                          TraversingASTChildrenNotSpelledInSource;
 
@@ -1515,13 +1549,18 @@ bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) {
 }
 
 bool MatchASTVisitor::TraverseType(QualType TypeNode, bool TraverseQualifier) {
+  if (shouldSkipNode(TypeNode))
+    return true;
+
   match(TypeNode);
   return RecursiveASTVisitor<MatchASTVisitor>::TraverseType(TypeNode,
                                                             TraverseQualifier);
 }
 
 bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode,
                                       bool TraverseQualifier) {
+  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
@@ -1534,6 +1573,9 @@ bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode,
 }
 
 bool MatchASTVisitor::TraverseNestedNameSpecifier(NestedNameSpecifier NNS) {
+  if (shouldSkipNode(NNS))
+    return true;
+
   match(NNS);
   return RecursiveASTVisitor<MatchASTVisitor>::TraverseNestedNameSpecifier(NNS);
 }
@@ -1543,6 +1585,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)
@@ -1555,7 +1600,7 @@ bool MatchASTVisitor::TraverseNestedNameSpecifierLoc(
 
 bool MatchASTVisitor::TraverseConstructorInitializer(
     CXXCtorInitializer *CtorInit) {
-  if (!CtorInit)
+  if (shouldSkipNode(CtorInit))
     return true;
 
   bool ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
@@ -1573,11 +1618,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);
 }