Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clang-tools-extra/clang-tidy/ClangTidy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)));

Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,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.
Expand Down
5 changes: 0 additions & 5 deletions clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,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
------------

Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/ASTMatchers/ASTMatchFinder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,15 @@ class MatchFinder {
llvm::StringMap<llvm::TimeRecord> &Records;
};

MatchFinderOptions() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MatchFinderOptions() = default; (why add it in the first place? Having no ctor should be enough)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, it bothers me a lot as well. I am not sure if this is a compiler bug or expected, but the moment I add a non-static member initializer (NSDMI) to the struct, I get the following compiler error:

https://godbolt.org/z/Mb3bnxjjM

Using = default surprisingly does not work :(

The solutions are:

  • Move MatchFinderOptions outside the MatchFinder class (like I did on the original patch). I didn't like this because it introduced unrelated noise to the patch, but I'm happy to do it on a separate patch if we feel it's the cleanest.
  • Not use NSDMI, and instead create a constructor with member initializer list.
  • Add a non-default user-defined constructor (this patch).


/// 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());
Expand Down
62 changes: 57 additions & 5 deletions clang/lib/ASTMatchers/ASTMatchFinder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1336,6 +1336,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.
Expand Down Expand Up @@ -1465,9 +1500,8 @@ bool MatchASTVisitor::objcClassIsDerivedFrom(
}

bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
if (!DeclNode) {
if (shouldSkipNode(DeclNode))
return true;
}

bool ScopedTraversal =
TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
Expand Down Expand Up @@ -1495,9 +1529,9 @@ bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
}

bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) {
if (!StmtNode) {
if (shouldSkipNode(StmtNode))
return true;
}

bool ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
TraversingASTChildrenNotSpelledInSource;

Expand All @@ -1507,11 +1541,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
Expand All @@ -1523,6 +1563,9 @@ bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode) {
}

bool MatchASTVisitor::TraverseNestedNameSpecifier(NestedNameSpecifier *NNS) {
if (shouldSkipNode(NNS))
return true;

match(*NNS);
return RecursiveASTVisitor<MatchASTVisitor>::TraverseNestedNameSpecifier(NNS);
}
Expand All @@ -1532,6 +1575,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)
Expand All @@ -1544,7 +1590,7 @@ bool MatchASTVisitor::TraverseNestedNameSpecifierLoc(

bool MatchASTVisitor::TraverseConstructorInitializer(
CXXCtorInitializer *CtorInit) {
if (!CtorInit)
if (shouldSkipNode(CtorInit))
return true;

bool ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
Expand All @@ -1562,11 +1608,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);
}
Expand Down