Skip to content

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

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][AST][clang-tidy] Do not set a reduced traversal scope in ASTM… #132725

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
32 changes: 5 additions & 27 deletions clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 0 additions & 2 deletions clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines -97 to -98
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

  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.

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


- Improved :program:`clang-tidy-diff.py` script. Add the `-warnings-as-errors`
argument to treat warnings as errors.
Expand Down
22 changes: 8 additions & 14 deletions ...tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down
5 changes: 2 additions & 3 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
------------
Expand Down
40 changes: 13 additions & 27 deletions clang/lib/ASTMatchers/ASTMatchFinder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
#include <deque>
#include <memory>
#include <set>
#include <vector>

namespace clang {
namespace ast_matchers {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down