Add ignoring paren imp casts in has any argument

[komalverma04]

Maintaining Consistency in hasAnyArgument() and hasArgument() Matchers in Clang AST Matchers

The hasArgument() matcher already ignores implicit casts and parentheses, but the hasAnyArgument() matcher does not. To ensure consistency, we need to explicitly use ignoringParenImpCasts() to handle cases where there might be implicit casts or parentheses around the argument in the Clang AST match.

The code changes made are as follows:

- hasAnyArgument(hasType(asString("S *")))
+ hasAnyArgument(ignoringParenImpCasts(hasType(asString("S *"))))

This change ensures that any implicit casts and parentheses around the argument type S * are ignored.

Fixes #75754

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: None (komalverma04)

Changes

Maintaining Consistency in hasAnyArgument() and hasArgument() Matchers in Clang AST Matchers

The hasArgument() matcher already ignores implicit casts and parentheses, but the hasAnyArgument() matcher does not. To ensure consistency, we need to explicitly use ignoringParenImpCasts() to handle cases where there might be implicit casts or parentheses around the argument in the Clang AST match.

The code changes made are as follows:

- hasAnyArgument(hasType(asString("S *")))
+ hasAnyArgument(ignoringParenImpCasts(hasType(asString("S *"))))

This change ensures that any implicit casts and parentheses around the argument type S * are ignored.

Fixes #75754

---

Full diff: https://github.com/llvm/llvm-project/pull/89509.diff

11 Files Affected:

• (modified) clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.cpp (+8-7)
• (modified) clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp (+2-2)
• (modified) clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp (+2-1)
• (modified) clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp (+6-8)
• (modified) clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp (+5-4)
• (modified) clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp (+6-3)
• (modified) clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp (+7-5)
• (modified) clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp (+2-1)
• (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp (+2-2)
• (modified) clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp (+7-7)
• (modified) clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp (+2-2)

diff --git a/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.cpp b/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.cpp
index ab6ed701e59fee..f6b64a2dc15ede 100644
--- a/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/StrCatAppendCheck.cpp
@@ -34,23 +34,23 @@ AST_MATCHER_P(Stmt, IgnoringTemporaries, ast_matchers::internal::Matcher<Stmt>,
   return InnerMatcher.matches(*E, Finder, Builder);
 }
 
-}  // namespace
+} // namespace
 
 // TODO: str += StrCat(...)
 //       str.append(StrCat(...))
 
 void StrCatAppendCheck::registerMatchers(MatchFinder *Finder) {
   const auto StrCat = functionDecl(hasName("::absl::StrCat"));
-  // The arguments of absl::StrCat are implicitly converted to AlphaNum. This 
-  // matches to the arguments because of that behavior. 
+  // The arguments of absl::StrCat are implicitly converted to AlphaNum. This
+  // matches to the arguments because of that behavior.
   const auto AlphaNum = IgnoringTemporaries(cxxConstructExpr(
       argumentCountIs(1), hasType(cxxRecordDecl(hasName("::absl::AlphaNum"))),
       hasArgument(0, ignoringImpCasts(declRefExpr(to(equalsBoundNode("LHS")),
                                                   expr().bind("Arg0"))))));
 
-  const auto HasAnotherReferenceToLhs =
-      callExpr(hasAnyArgument(expr(hasDescendant(declRefExpr(
-          to(equalsBoundNode("LHS")), unless(equalsBoundNode("Arg0")))))));
+  const auto HasAnotherReferenceToLhs = callExpr(
+      hasAnyArgument(ignoringParenImpCasts(expr(hasDescendant(declRefExpr(
+          to(equalsBoundNode("LHS")), unless(equalsBoundNode("Arg0"))))))));
 
   // Now look for calls to operator= with an object on the LHS and a call to
   // StrCat on the RHS. The first argument of the StrCat call should be the same
@@ -73,7 +73,8 @@ void StrCatAppendCheck::registerMatchers(MatchFinder *Finder) {
 void StrCatAppendCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Op = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("Op");
   const auto *Call = Result.Nodes.getNodeAs<CallExpr>("Call");
-  assert(Op != nullptr && Call != nullptr && "Matcher does not work as expected");
+  assert(Op != nullptr && Call != nullptr &&
+         "Matcher does not work as expected");
 
   // Handles the case 'x = absl::StrCat(x)', which has no effect.
   if (Call->getNumArgs() == 1) {
diff --git a/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp
index 40e4ab6c8b12af..cf7f06e06b26a0 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp
@@ -24,7 +24,7 @@ void MisplacedOperatorInStrlenInAllocCheck::registerMatchers(
 
   const auto BadUse =
       callExpr(callee(StrLenFunc),
-               hasAnyArgument(ignoringImpCasts(
+               hasAnyArgument(ignoringParenImpCasts(
                    binaryOperator(
                        hasOperatorName("+"),
                        hasRHS(ignoringParenImpCasts(integerLiteral(equals(1)))))
@@ -74,7 +74,7 @@ void MisplacedOperatorInStrlenInAllocCheck::check(
   if (!Alloc)
     Alloc = Result.Nodes.getNodeAs<CXXNewExpr>("Alloc");
   assert(Alloc && "Matched node bound by 'Alloc' should be either 'CallExpr'"
-         " or 'CXXNewExpr'");
+                  " or 'CXXNewExpr'");
 
   const auto *StrLen = Result.Nodes.getNodeAs<CallExpr>("StrLen");
   const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("BinOp");
diff --git a/clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp
index a1f92aae55448c..f989e927871ace 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp
@@ -42,7 +42,8 @@ void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) {
 
   Finder->addMatcher(varDecl(hasInitializer(Cast)), this);
   Finder->addMatcher(returnStmt(hasReturnValue(Cast)), this);
-  Finder->addMatcher(callExpr(hasAnyArgument(Cast)), this);
+  Finder->addMatcher(callExpr(hasAnyArgument(ignoringParenImpCasts(Cast))),
+                     this);
   Finder->addMatcher(binaryOperator(hasOperatorName("="), hasRHS(Cast)), this);
   Finder->addMatcher(
       binaryOperator(isComparisonOperator(), hasEitherOperand(Cast)), this);
diff --git a/clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
index 41191a3cfed23a..d304ac0bd681a7 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
@@ -95,18 +95,16 @@ void MultipleNewInOneExpressionCheck::registerMatchers(MatchFinder *Finder) {
 
   Finder->addMatcher(
       callExpr(
-          hasAnyArgument(
-              expr(HasNewExpr1).bind("arg1")),
-          hasAnyArgument(
-              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+          hasAnyArgument(ignoringParenImpCasts(expr(HasNewExpr1).bind("arg1"))),
+          hasAnyArgument(ignoringParenImpCasts(
+              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2"))),
           hasAncestor(BadAllocCatchingTryBlock)),
       this);
   Finder->addMatcher(
       cxxConstructExpr(
-          hasAnyArgument(
-              expr(HasNewExpr1).bind("arg1")),
-          hasAnyArgument(
-              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+          hasAnyArgument(ignoringParenImpCasts(expr(HasNewExpr1).bind("arg1"))),
+          hasAnyArgument(ignoringParenImpCasts(
+              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2"))),
           unless(isListInitialization()),
           hasAncestor(BadAllocCatchingTryBlock)),
       this);
diff --git a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
index 977241e91b9a93..264926de70353e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
@@ -618,10 +618,11 @@ void NotNullTerminatedResultCheck::registerMatchers(MatchFinder *Finder) {
   // Note: Sometimes the size of char is explicitly written out.
   auto SizeExpr = anyOf(SizeOfCharExpr, integerLiteral(equals(1)));
 
-  auto MallocLengthExpr = allOf(
-      callee(functionDecl(
-          hasAnyName("::alloca", "::calloc", "malloc", "realloc"))),
-      hasAnyArgument(allOf(unless(SizeExpr), expr().bind(DestMallocExprName))));
+  auto MallocLengthExpr =
+      allOf(callee(functionDecl(
+                hasAnyName("::alloca", "::calloc", "malloc", "realloc"))),
+            hasAnyArgument(ignoringParenImpCasts(
+                allOf(unless(SizeExpr), expr().bind(DestMallocExprName)))));
 
   // - Example:  (char *)malloc(length);
   auto DestMalloc = anyOf(callExpr(MallocLengthExpr),
diff --git a/clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp
index 72e680d25cb846..8e5a7a4462179d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp
@@ -47,12 +47,15 @@ void StringLiteralWithEmbeddedNulCheck::registerMatchers(MatchFinder *Finder) {
 
   // Detect passing a suspicious string literal to a string constructor.
   // example: std::string str = "abc\0def";
-  Finder->addMatcher(traverse(TK_AsIs,
-      cxxConstructExpr(StringConstructorExpr, hasArgument(0, StrLitWithNul))),
+  Finder->addMatcher(
+      traverse(TK_AsIs, cxxConstructExpr(StringConstructorExpr,
+                                         hasArgument(0, StrLitWithNul))),
       this);
 
   // Detect passing a suspicious string literal through an overloaded operator.
-  Finder->addMatcher(cxxOperatorCallExpr(hasAnyArgument(StrLitWithNul)), this);
+  Finder->addMatcher(
+      cxxOperatorCallExpr(hasAnyArgument(ignoringParenImpCasts(StrLitWithNul))),
+      this);
 }
 
 void StringLiteralWithEmbeddedNulCheck::check(
diff --git a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
index ea50250f829f02..fc7123a2caf881 100644
--- a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
@@ -74,8 +74,9 @@ RewriteRuleWith<std::string> StringviewNullptrCheckImpl() {
   auto BasicStringViewConstructingFromNullExpr =
       cxxConstructExpr(
           HasBasicStringViewType, argumentCountIs(1),
-          hasAnyArgument(/* `hasArgument` would skip over parens */ anyOf(
-              NullLiteral, NullInitList, EmptyInitList)),
+          hasAnyArgument(
+              /* `hasArgument` would skip over parens */ ignoringParenImpCasts(
+                  anyOf(NullLiteral, NullInitList, EmptyInitList))),
           unless(cxxTemporaryObjectExpr(/* filters out type spellings */)),
           has(expr().bind("null_arg_expr")))
           .bind("construct_expr");
@@ -90,8 +91,9 @@ RewriteRuleWith<std::string> StringviewNullptrCheckImpl() {
   auto HandleTemporaryCXXTemporaryObjectExprAndCompoundLiteralExpr = makeRule(
       cxxTemporaryObjectExpr(cxxConstructExpr(
           HasBasicStringViewType, argumentCountIs(1),
-          hasAnyArgument(/* `hasArgument` would skip over parens */ anyOf(
-              NullLiteral, NullInitList, EmptyInitList)),
+          hasAnyArgument(
+              /* `hasArgument` would skip over parens */ ignoringParenImpCasts(
+                  anyOf(NullLiteral, NullInitList, EmptyInitList))),
           has(expr().bind("null_arg_expr")))),
       remove(node("null_arg_expr")), construction_warning);
 
@@ -263,7 +265,7 @@ RewriteRuleWith<std::string> StringviewNullptrCheckImpl() {
   auto HandleConstructorInvocation =
       makeRule(cxxConstructExpr(
                    hasAnyArgument(/* `hasArgument` would skip over parens */
-                                  ignoringImpCasts(
+                                  ignoringParenImpCasts(
                                       BasicStringViewConstructingFromNullExpr)),
                    unless(HasBasicStringViewType)),
                changeTo(node("construct_expr"), cat("\"\"")),
diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp
index 8f4b0c5e0dceda..f71bc6df200caf 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp
@@ -73,7 +73,8 @@ void SuspiciousStringviewDataUsageCheck::registerMatchers(MatchFinder *Finder) {
                   hasAnyArgument(
                       ignoringParenImpCasts(equalsBoundNode("data-call"))),
                   unless(hasAnyArgument(ignoringParenImpCasts(SizeCall))),
-                  unless(hasAnyArgument(DescendantSizeCall)),
+                  unless(hasAnyArgument(
+                      ignoringParenImpCasts(DescendantSizeCall))),
                   hasDeclaration(namedDecl(
                       unless(matchers::matchesAnyListedName(AllowedCallees))))),
               initListExpr(expr().bind("parent"),
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
index 9b4d2ef99e5bf1..4b26949d2ca899 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
@@ -86,9 +86,9 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) {
   // functions shall be 'gsl::owner<>'.
   Finder->addMatcher(
       traverse(TK_AsIs, callExpr(callee(LegacyOwnerConsumers),
-                                 hasAnyArgument(expr(
+                                 hasAnyArgument(ignoringParenImpCasts(expr(
                                      unless(ignoringImpCasts(ConsideredOwner)),
-                                     hasType(pointerType()))))
+                                     hasType(pointerType())))))
                             .bind("legacy_consumer")),
       this);
 
diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
index 430455a38f395e..0c061fc0eac6f7 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -98,8 +98,8 @@ auto hasWantedType(llvm::ArrayRef<StringRef> TypeNames) {
 
 // Matches member call expressions of the named method on the listed container
 // types.
-auto cxxMemberCallExprOnContainer(
-    StringRef MethodName, llvm::ArrayRef<StringRef> ContainerNames) {
+auto cxxMemberCallExprOnContainer(StringRef MethodName,
+                                  llvm::ArrayRef<StringRef> ContainerNames) {
   return cxxMemberCallExpr(
       hasDeclaration(functionDecl(hasName(MethodName))),
       on(hasTypeOrPointeeType(hasWantedType(ContainerNames))));
@@ -177,16 +177,16 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
       hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(SmartPointers))));
 
   // Bitfields binds only to consts and emplace_back take it by universal ref.
-  auto BitFieldAsArgument = hasAnyArgument(
-      ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField())))));
+  auto BitFieldAsArgument = hasAnyArgument(ignoringParenImpCasts(
+      memberExpr(hasDeclaration(fieldDecl(isBitField())))));
 
   // Initializer list can't be passed to universal reference.
   auto InitializerListAsArgument = hasAnyArgument(
-      ignoringImplicit(allOf(cxxConstructExpr(isListInitialization()),
-                             unless(cxxTemporaryObjectExpr()))));
+      ignoringParenImpCasts(allOf(cxxConstructExpr(isListInitialization()),
+                                  unless(cxxTemporaryObjectExpr()))));
 
   // We could have leak of resource.
-  auto NewExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
+  auto NewExprAsArgument = hasAnyArgument(ignoringParenImpCasts(cxxNewExpr()));
   // We would call another constructor.
   auto ConstructingDerived =
       hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
diff --git a/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
index 9e4e3f63e19cfe..498fbecd2baa59 100644
--- a/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
@@ -32,12 +32,12 @@ void InefficientStringConcatenationCheck::registerMatchers(
 
   const auto BasicStringPlusOperator = cxxOperatorCallExpr(
       hasOverloadedOperatorName("+"),
-      hasAnyArgument(ignoringImpCasts(declRefExpr(BasicStringType))));
+      hasAnyArgument(ignoringParenImpCasts(declRefExpr(BasicStringType))));
 
   const auto PlusOperator =
       cxxOperatorCallExpr(
           hasOverloadedOperatorName("+"),
-          hasAnyArgument(ignoringImpCasts(declRefExpr(BasicStringType))),
+          hasAnyArgument(ignoringParenImpCasts(declRefExpr(BasicStringType))),
           hasDescendant(BasicStringPlusOperator))
           .bind("plusOperator");
 

[komalverma04]

@PiotrZSL Please guide me tackling the failing tests

[5chmidti]

I started taking a look at this and realized you switched up which argument matcher needs the extra ignoringParenImpCasts, so that it can be removed from the matcher definition (possibly in a different pr).

See

llvm-project/clang/include/clang/ASTMatchers/ASTMatchers.h

Lines 4891 to 4907 in d674f45

	AST_POLYMORPHIC_MATCHER_P(hasAnyArgument,
	AST_POLYMORPHIC_SUPPORTED_TYPES(
	CallExpr, CXXConstructExpr,
	CXXUnresolvedConstructExpr, ObjCMessageExpr),
	internal::Matcher<Expr>, InnerMatcher) {
	for (const Expr *Arg : Node.arguments()) {
	if (Finder->isTraversalIgnoringImplicitNodes() &&
	isa<CXXDefaultArgExpr>(Arg))
	break;
	BoundNodesTreeBuilder Result(*Builder);
	if (InnerMatcher.matches(*Arg, Finder, &Result)) {
	*Builder = std::move(Result);
	return true;
	}
	}
	return false;
	}

vs

llvm-project/clang/include/clang/ASTMatchers/ASTMatchers.h

Lines 4553 to 4564 in d674f45

	AST_POLYMORPHIC_MATCHER_P2(hasArgument,
	AST_POLYMORPHIC_SUPPORTED_TYPES(
	CallExpr, CXXConstructExpr,
	CXXUnresolvedConstructExpr, ObjCMessageExpr),
	unsigned, N, internal::Matcher<Expr>, InnerMatcher) {
	if (N >= Node.getNumArgs())
	return false;
	const Expr *Arg = Node.getArg(N);
	if (Finder->isTraversalIgnoringImplicitNodes() && isa<CXXDefaultArgExpr>(Arg))
	return false;
	return InnerMatcher.matches(*Arg->IgnoreParenImpCasts(), Finder, Builder);
	}

. We don't want the IgnoreParenImpCasts() inside the hasArgument matcher, because that is not it's job. So we want to a) add ignoringParenImpCasts to arguments of the hasArgument matcher, and b) remove the call to IgnoreParenImpCasts from the hasArgument matcher.
See this comment from the original issue: #75754 (comment)

This is also the reason why the tests are failing, you are actually changing the behavior of these checks.

[5chmidti]

Checkout this file later for this and the other section I highlighted, here it looks like the only reason that hasAnyArgument was chosen is because of this differing behavior, instead, this can be replaced with hasArgument(0, ...) after the IgnoreParenImpCasts is removed from the hasArgument matcher.

[5chmidti]

Same as above