Inconsistency between hasDescendant in clang-query and Clang/LibTooling matchers

Was using clang 17.0.6 and stumbled onto this inconsistency:

clang-query accepts the following and outputs correct expected match.

clang-query> match declaratorDecl(unless(isExpansionInSystemHeader()), hasType(qualType(hasDeclaration(classTemplateSpecializationDecl(hasDescendant(hasDeclaration(cxxRecordDecl(hasName(“::std::thread”)))))))))

Match #1:

…/clang-tidy-extensions/src/test/no-std-thread.cpp:41:1: note: “root” binds here
41 | std::variant< std::thread > VariantThread {};
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 match.

However, when compiled in a clang-tidy plugin, the outcome is an error.

/opt/clang170/include/clang/ASTMatchers/ASTMatchersInternal.h:1498:3: note: template argument deduction/substitution failed:
…/clang-tidy-extensions/src/clang-tidy/NoStdThreadCheck.cpp:75:194: note: ‘clang::ast_matchers::internal::PolymorphicMatcher<clang::ast_matchers::internal::HasDeclarationMatcher, void(clang::ast_matchers::internal::TypeList<clang::CallExpr, clang::CXXConstructExpr, clang::CXXNewExpr, clang::DeclRefExpr, clang::EnumType, clang::ElaboratedType, clang::InjectedClassNameType, clang::LabelStmt, clang::AddrLabelExpr, clang::MemberExpr, clang::QualType, clang::RecordType, clang::TagType, clang::TemplateSpecializationType, clang::TemplateTypeParmType, clang::TypedefType, clang::UnresolvedUsingType, clang::ObjCIvarRefExpr>), clang::ast_matchers::internal::Matcherclang::Decl >’ is not derived from ‘const clang::ast_matchers::internal::MapAnyOfHelper<T …>’
declaratorDecl(unless(isExpansionInSystemHeader()), hasType(qualType(hasDeclaration(classTemplateSpecializationDecl(hasDescendant(hasDeclaration(cxxRecordDecl(hasName(“::std::thread”))))))))).bind(“template-vars”),

As the Getting Involved — Extra Clang Tools 20.0.0git documentation guide suggests using clang-query to prototype matchers, this difference caught me offguard.

The equivalent that works in the plugin is actually

declaratorDecl(unless(isExpansionInSystemHeader()), hasType(qualType(hasDeclaration(classTemplateSpecializationDecl(hasDescendant(cxxRecordDecl(hasName(“::std::thread”))))))))

which removes the hasDeclaration after the hasDescendant.

Clang-query doesn’t match this one, though.

clang-query> match declaratorDecl(unless(isExpansionInSystemHeader()), hasType(qualType(hasDeclaration(classTemplateSpecializationDecl(hasDescendant(cxxRecordDecl(hasName(“::std::thread”))))))))
0 matches.

Can anything be done regarding this? Similar surprises occured here: Matcher working in clang-query, but not in LibTooling - #3 by Stephen_Kelly

Yeah, there are differences between the dynamic AST matchers used via clang-query and the C++ AST matchers used via clang-tidy. We try to keep the differences to a minimum, but they do still occur.

I think this is the same root cause, too. Unfortunately, doing something about this is likely to be pretty involved – the C++ matchers are a pile of template instantiation machinery, and the dynamic matchers use most of the same machinery but add their own layer of indirection. So it’s not a trivial matter to determine where we need more constraints on the dynamic matchers in order to match the C++ behavior.

If you’re interested in poking at the problem yourself, most of the dynamic matcher logic lives in llvm-project/clang/lib/ASTMatchers/Dynamic/Marshallers.cpp at main · llvm/llvm-project · GitHub and llvm-project/clang/lib/ASTMatchers/Dynamic/Parser.cpp at main · llvm/llvm-project · GitHub while most of the C++ matcher logic lives in llvm-project/clang/lib/ASTMatchers/ASTMatchersInternal.cpp at main · llvm/llvm-project · GitHub and llvm-project/clang/include/clang/ASTMatchers/ASTMatchersInternal.h at main · llvm/llvm-project · GitHub

2 Likes

Fair enough, I suspected that was the case. Thankfully here I got a similar-ish matcher to work, so the work wasn’t wasted — just a concern that in the future someone might have spent an hour or two getting the clang-query to work only to find out that they have to write yet another matcher from the ground up…but hopefully that won’t happen.

Regarding poking at the problem, I’m currently focused on other LLVM-contribution efforts ([clang-tidy] Create bugprone-incorrect-enable-shared-from-this check by MichelleCDjunaidi · Pull Request #102299 · llvm/llvm-project · GitHub) so I’ll see if I have the time and knowledge to do so after my PR is done. If nothing else, I’ll contribute to the Contributing documentation to disclaim the inconsistency on clang-query. (Side note, but what’s the usual pacing of a PR review? A few reviews per week?)

Usually it’s a matter of finding the correct casts or additional matcher to work around the laxity of clang-query, but yeah, I agree that it’d be lovely to not have that laxity in the first place when possible. Less surprises are better. :slight_smile:

Thank you! PR review pacing really depends on a lot of factors, so it’s not the same for every review or every part of the project. We usually try to turn around feedback as quickly as we can, so if you’ve not gotten feedback after a week, a good idea is to leave a “ping” comment to remind reviewers you’re still waiting. If you still can’t get anyone’s attention after that, feel free to msg me (here, email, Discord, IRC, carrier pigeon, whatever works for you) and I’ll try to scare up reviewers or review it myself.

1 Like

Raised the issue here so that there’s a centralized place to see what is being done regarding this matter. Will do the documentation change/update later, though no guarantees on the matcher update/changes (I don’t think I’m knowledgeable enough yet for that)

1 Like

Here’s the guide update to disclaim the thing. Also cleaned up some outdated references to Phabricator.

1 Like