Skip to content

Commit

Permalink
[clang][ASTMatchers] Fix forEachArgumentWithParam* for deducing "this…
Browse files Browse the repository at this point in the history 
…" operator calls (llvm#84887)

This is a follow-up commit of llvm#84446.
In this patch, I demonstrate that `forEachArgumentWithParam` and
`forEachArgumentWithParamType` did not correctly handle the presence of
the explicit object parameter for operator calls.

Prior to this patch, the matcher would skip the first (and only)
argument of the operator call if the explicit object param was used.

Note that I had to move the definition of
`isExplicitObjectMemberFunction`, to be declared before the matcher I
fix to be visible.

I also had to do some gymnastics for passing the language standard
version command-line flags to the invocation as
`matchAndVerifyResultTrue` wasn't really considered for non-c++11 code.
See the that it always prepends `-std=gnu++11` to the command-line
arguments. I workarounded it by accepting extra args, which get
appended, thus possibly overriding the hardcoded arguments.

I'm not sure if this qualifies for backporting to clang-18 (probably not
because its not a crash, but a semantic problem), but I figure it might
be useful for some vendors (like us).
But we are also happy to cherry-pick this fix to downstream. Let me know
if you want this to be backported or not.

CPP-5074
  • Loading branch information
@steakhal
steakhal authored Mar 12, 2024
1 parent af21659 commit fe8cf2f
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 41 deletions.
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,8 @@ AST Matchers

- ``isInStdNamespace`` now supports Decl declared with ``extern "C++"``.
- Add ``isExplicitObjectMemberFunction``.
- Fixed ``forEachArgumentWithParam`` and ``forEachArgumentWithParamType`` to
not skip the explicit object parameter for operator calls.

clang-format
------------
Expand Down
59 changes: 31 additions & 28 deletions clang/include/clang/ASTMatchers/ASTMatchers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5032,6 +5032,25 @@ AST_POLYMORPHIC_MATCHER_P2(hasParameter,
&& InnerMatcher.matches(*Node.parameters()[N], Finder, Builder));
}

/// Matches if the given method declaration declares a member function with an
/// explicit object parameter.
///
/// Given
/// \code
/// struct A {
/// int operator-(this A, int);
/// void fun(this A &&self);
/// static int operator()(int);
/// int operator+(int);
/// };
/// \endcode
///
/// cxxMethodDecl(isExplicitObjectMemberFunction()) matches the first two
/// methods but not the last two.
AST_MATCHER(CXXMethodDecl, isExplicitObjectMemberFunction) {
return Node.isExplicitObjectMemberFunction();
}

/// Matches all arguments and their respective ParmVarDecl.
///
/// Given
Expand Down Expand Up @@ -5060,10 +5079,12 @@ AST_POLYMORPHIC_MATCHER_P2(forEachArgumentWithParam,
// argument of the method which should not be matched against a parameter, so
// we skip over it here.
BoundNodesTreeBuilder Matches;
unsigned ArgIndex = cxxOperatorCallExpr(callee(cxxMethodDecl()))
.matches(Node, Finder, &Matches)
? 1
: 0;
unsigned ArgIndex =
cxxOperatorCallExpr(
callee(cxxMethodDecl(unless(isExplicitObjectMemberFunction()))))
.matches(Node, Finder, &Matches)
? 1
: 0;
int ParamIndex = 0;
bool Matched = false;
for (; ArgIndex < Node.getNumArgs(); ++ArgIndex) {
Expand Down Expand Up @@ -5121,11 +5142,12 @@ AST_POLYMORPHIC_MATCHER_P2(forEachArgumentWithParamType,
// argument of the method which should not be matched against a parameter, so
// we skip over it here.
BoundNodesTreeBuilder Matches;
unsigned ArgIndex = cxxOperatorCallExpr(callee(cxxMethodDecl()))
.matches(Node, Finder, &Matches)
? 1
: 0;

unsigned ArgIndex =
cxxOperatorCallExpr(
callee(cxxMethodDecl(unless(isExplicitObjectMemberFunction()))))
.matches(Node, Finder, &Matches)
? 1
: 0;
const FunctionProtoType *FProto = nullptr;

if (const auto *Call = dyn_cast<CallExpr>(&Node)) {
Expand Down Expand Up @@ -6366,25 +6388,6 @@ AST_MATCHER(CXXMethodDecl, isConst) {
return Node.isConst();
}

/// Matches if the given method declaration declares a member function with an
/// explicit object parameter.
///
/// Given
/// \code
/// struct A {
/// int operator-(this A, int);
/// void fun(this A &&self);
/// static int operator()(int);
/// int operator+(int);
/// };
/// \endcode
///
/// cxxMethodDecl(isExplicitObjectMemberFunction()) matches the first two
/// methods but not the last two.
AST_MATCHER(CXXMethodDecl, isExplicitObjectMemberFunction) {
return Node.isExplicitObjectMemberFunction();
}

/// Matches if the given method declaration declares a copy assignment
/// operator.
///
Expand Down
36 changes: 23 additions & 13 deletions clang/unittests/ASTMatchers/ASTMatchersTest.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,8 @@ testing::AssertionResult notMatchesWithOpenMP51(const Twine &Code,
template <typename T>
testing::AssertionResult matchAndVerifyResultConditionally(
const Twine &Code, const T &AMatcher,
std::unique_ptr<BoundNodesCallback> FindResultVerifier, bool ExpectResult) {
std::unique_ptr<BoundNodesCallback> FindResultVerifier, bool ExpectResult,
ArrayRef<std::string> Args = {}, StringRef Filename = "input.cc") {
bool VerifiedResult = false;
MatchFinder Finder;
VerifyMatch VerifyVerifiedResult(std::move(FindResultVerifier),
Expand All @@ -304,9 +305,13 @@ testing::AssertionResult matchAndVerifyResultConditionally(
// Some tests use typeof, which is a gnu extension. Using an explicit
// unknown-unknown triple is good for a large speedup, because it lets us
// avoid constructing a full system triple.
std::vector<std::string> Args = {"-std=gnu++11", "-target",
"i386-unknown-unknown"};
if (!runToolOnCodeWithArgs(Factory->create(), Code, Args)) {
std::vector<std::string> CompileArgs = {"-std=gnu++11", "-target",
"i386-unknown-unknown"};
// Append additional arguments at the end to allow overriding the default
// choices that we made above.
llvm::copy(Args, std::back_inserter(CompileArgs));

if (!runToolOnCodeWithArgs(Factory->create(), Code, CompileArgs, Filename)) {
return testing::AssertionFailure() << "Parsing error in \"" << Code << "\"";
}
if (!VerifiedResult && ExpectResult) {
Expand All @@ -319,8 +324,8 @@ testing::AssertionResult matchAndVerifyResultConditionally(

VerifiedResult = false;
SmallString<256> Buffer;
std::unique_ptr<ASTUnit> AST(
buildASTFromCodeWithArgs(Code.toStringRef(Buffer), Args));
std::unique_ptr<ASTUnit> AST(buildASTFromCodeWithArgs(
Code.toStringRef(Buffer), CompileArgs, Filename));
if (!AST.get())
return testing::AssertionFailure()
<< "Parsing error in \"" << Code << "\" while building AST";
Expand All @@ -339,19 +344,24 @@ testing::AssertionResult matchAndVerifyResultConditionally(
// FIXME: Find better names for these functions (or document what they
// do more precisely).
template <typename T>
testing::AssertionResult matchAndVerifyResultTrue(
const Twine &Code, const T &AMatcher,
std::unique_ptr<BoundNodesCallback> FindResultVerifier) {
return matchAndVerifyResultConditionally(Code, AMatcher,
std::move(FindResultVerifier), true);
testing::AssertionResult
matchAndVerifyResultTrue(const Twine &Code, const T &AMatcher,
std::unique_ptr<BoundNodesCallback> FindResultVerifier,
ArrayRef<std::string> Args = {},
StringRef Filename = "input.cc") {
return matchAndVerifyResultConditionally(
Code, AMatcher, std::move(FindResultVerifier),
/*ExpectResult=*/true, Args, Filename);
}

template <typename T>
testing::AssertionResult matchAndVerifyResultFalse(
const Twine &Code, const T &AMatcher,
std::unique_ptr<BoundNodesCallback> FindResultVerifier) {
std::unique_ptr<BoundNodesCallback> FindResultVerifier,
ArrayRef<std::string> Args = {}, StringRef Filename = "input.cc") {
return matchAndVerifyResultConditionally(
Code, AMatcher, std::move(FindResultVerifier), false);
Code, AMatcher, std::move(FindResultVerifier),
/*ExpectResult=*/false, Args, Filename);
}

// Implements a run method that returns whether BoundNodes contains a
Expand Down
63 changes: 63 additions & 0 deletions clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,38 @@ TEST(ForEachArgumentWithParam, HandlesBoundNodesForNonMatches) {
std::make_unique<VerifyIdIsBoundTo<VarDecl>>("v", 4)));
}

TEST_P(ASTMatchersTest,
ForEachArgumentWithParamMatchesExplicitObjectParamOnOperatorCalls) {
if (!GetParam().isCXX23OrLater()) {
return;
}

auto DeclRef = declRefExpr(to(varDecl().bind("declOfArg"))).bind("arg");
auto SelfParam = parmVarDecl().bind("param");
StatementMatcher CallExpr =
callExpr(forEachArgumentWithParam(DeclRef, SelfParam));

StringRef S = R"cpp(
struct A {
int operator()(this const A &self);
};
A obj;
int global = obj();
)cpp";

auto Args = GetParam().getCommandLineArgs();
auto Filename = getFilenameForTesting(GetParam().Language);

EXPECT_TRUE(matchAndVerifyResultTrue(
S, CallExpr,
std::make_unique<VerifyIdIsBoundTo<ParmVarDecl>>("param", "self"), Args,
Filename));
EXPECT_TRUE(matchAndVerifyResultTrue(
S, CallExpr,
std::make_unique<VerifyIdIsBoundTo<VarDecl>>("declOfArg", "obj"), Args,
Filename));
}

TEST(ForEachArgumentWithParamType, ReportsNoFalsePositives) {
StatementMatcher ArgumentY =
declRefExpr(to(varDecl(hasName("y")))).bind("arg");
Expand Down Expand Up @@ -1168,6 +1200,37 @@ TEST(ForEachArgumentWithParamType, MatchesVariadicFunctionPtrCalls) {
S, CallExpr, std::make_unique<VerifyIdIsBoundTo<DeclRefExpr>>("arg")));
}

TEST_P(ASTMatchersTest,
ForEachArgumentWithParamTypeMatchesExplicitObjectParamOnOperatorCalls) {
if (!GetParam().isCXX23OrLater()) {
return;
}

auto DeclRef = declRefExpr(to(varDecl().bind("declOfArg"))).bind("arg");
auto SelfTy = qualType(asString("const A &")).bind("selfType");
StatementMatcher CallExpr =
callExpr(forEachArgumentWithParamType(DeclRef, SelfTy));

StringRef S = R"cpp(
struct A {
int operator()(this const A &self);
};
A obj;
int global = obj();
)cpp";

auto Args = GetParam().getCommandLineArgs();
auto Filename = getFilenameForTesting(GetParam().Language);

EXPECT_TRUE(matchAndVerifyResultTrue(
S, CallExpr, std::make_unique<VerifyIdIsBoundTo<QualType>>("selfType"),
Args, Filename));
EXPECT_TRUE(matchAndVerifyResultTrue(
S, CallExpr,
std::make_unique<VerifyIdIsBoundTo<VarDecl>>("declOfArg", "obj"), Args,
Filename));
}

TEST(QualType, hasCanonicalType) {
EXPECT_TRUE(notMatches("typedef int &int_ref;"
"int a;"
Expand Down

0 comments on commit fe8cf2f

Please sign in to comment.