[clang][ASTMatchers] Fix forEachArgumentWithParam* for deducing "this" operator calls #84887
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…" operator calls 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.
steakhal
added
clang
|
@llvm/pr-subscribers-clang Author: Balazs Benics (steakhal) ChangesThis is a follow-up commit of #84446. 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 I also had to do some gymnastics for passing the language standard version command-line flags to the invocation as I'm not sure if this qualifies for backporting to clang-18, but I figure it might be useful for some vendors (like us). Full diff: https://github.com/llvm/llvm-project/pull/84887.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 88e552d5c46113..dcc0c2f2cc40c6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -447,6 +447,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
------------
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 96dbcdc344e131..2f71053d030f68 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -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
@@ -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) {
@@ -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)) {
@@ -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.
///
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTest.h b/clang/unittests/ASTMatchers/ASTMatchersTest.h
index 1ed1b5958a8b3a..e9812995315741 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTest.h
+++ b/clang/unittests/ASTMatchers/ASTMatchersTest.h
@@ -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),
@@ -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) {
@@ -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";
@@ -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
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 6911d7600a7188..f198dc71eb8337 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -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");
@@ -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;"
|
PiotrZSL
approved these changes
Mar 12, 2024
Comment on lines
+5145
to
+5150
| unsigned ArgIndex = | ||
| cxxOperatorCallExpr( | ||
| callee(cxxMethodDecl(unless(isExplicitObjectMemberFunction())))) | ||
| .matches(Node, Finder, &Matches) | ||
| ? 1 | ||
| : 0; |
There was a problem hiding this comment.
Legacy: this will be slow, better would be to rewrite this using simple code like.
dyn_cast<CXXOperatorCallExpr>, getCallee ...
There was a problem hiding this comment.
I don't feel confident of making changes like that.
It appears to me that the callee matchers does a bit more than a couple dyn_casts, so unless strictly necessary, I'd opt out for doing that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up commit of #84446.
In this patch, I demonstrate that
forEachArgumentWithParamandforEachArgumentWithParamTypedid 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
matchAndVerifyResultTruewasn't really considered for non-c++11 code. See the that it always prepends-std=gnu++11to 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