Skip to content
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.

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

Add ignoringParenImpCasts in hasAnyArgument fix#75754 #87268

Conversation

komalverma04
Copy link
Contributor

@komalverma04 komalverma04 commented Apr 1, 2024

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 *"))))

To ignore any implicit casts and parenthesis around the argument type S *

Fixes #75754

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 1, 2024

@llvm/pr-subscribers-clang-tidy

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

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 *"))))

To ignore any implicit casts and parenthesis around the argument type S *


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

9 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp (+1-1)
  • (modified) clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp (+1-1)
  • (modified) clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp (+4-4)
  • (modified) clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp (+1-1)
  • (modified) clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp (+1-1)
  • (modified) clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp (+1-1)
  • (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp (+2-2)
  • (modified) clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp (+3-3)
  • (modified) clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp (+2-2)
diff --git a/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp
index 40e4ab6c8b12af..415183d5c57ba7 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)))))
diff --git a/clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp
index a1f92aae55448c..b62829a3776572 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MisplacedWideningCastCheck.cpp
@@ -42,7 +42,7 @@ 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..b8dbea600fd368 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp
@@ -96,17 +96,17 @@ void MultipleNewInOneExpressionCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       callExpr(
           hasAnyArgument(
-              expr(HasNewExpr1).bind("arg1")),
+              ignoringParenImpCasts(expr(HasNewExpr1).bind("arg1"))),
           hasAnyArgument(
-              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+              ignoringParenImpCasts(expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2"))),
           hasAncestor(BadAllocCatchingTryBlock)),
       this);
   Finder->addMatcher(
       cxxConstructExpr(
           hasAnyArgument(
-              expr(HasNewExpr1).bind("arg1")),
+              ignoringParenImpCasts(expr(HasNewExpr1).bind("arg1"))),
           hasAnyArgument(
-              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+              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..d322f2488f8082 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
@@ -621,7 +621,7 @@ void NotNullTerminatedResultCheck::registerMatchers(MatchFinder *Finder) {
   auto MallocLengthExpr = allOf(
       callee(functionDecl(
           hasAnyName("::alloca", "::calloc", "malloc", "realloc"))),
-      hasAnyArgument(allOf(unless(SizeExpr), expr().bind(DestMallocExprName))));
+      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..18a9dc6c430159 100644
--- a/clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp
@@ -52,7 +52,7 @@ void StringLiteralWithEmbeddedNulCheck::registerMatchers(MatchFinder *Finder) {
       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/SuspiciousStringviewDataUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp
index 8f4b0c5e0dceda..183d7c4bfb5b15 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousStringviewDataUsageCheck.cpp
@@ -73,7 +73,7 @@ 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..3055948fc3ad13 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -178,15 +178,15 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
 
   // Bitfields binds only to consts and emplace_back take it by universal ref.
   auto BitFieldAsArgument = hasAnyArgument(
-      ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField())))));
+      ignoringParenImpCasts(memberExpr(hasDeclaration(fieldDecl(isBitField())))));
 
   // Initializer list can't be passed to universal reference.
   auto InitializerListAsArgument = hasAnyArgument(
-      ignoringImplicit(allOf(cxxConstructExpr(isListInitialization()),
+      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");
 

Copy link

github-actions bot commented Apr 1, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

LGTM, run clang-format before commit,
and fix rest of tests

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

Fix tests & clang-format

@komalverma04
Copy link
Contributor Author

LGTM, run clang-format before commit, and fix rest of tests

Okay, will do that. Thank you.

@komalverma04 komalverma04 force-pushed the Add-ignoringParenImpCasts-in-hasAnyArgument branch from 4395604 to f72e3c4 Compare April 9, 2024 16:27
@komalverma04 komalverma04 force-pushed the Add-ignoringParenImpCasts-in-hasAnyArgument branch from 9da778e to d8503a3 Compare April 20, 2024 14:26
@komalverma04 komalverma04 deleted the Add-ignoringParenImpCasts-in-hasAnyArgument branch April 20, 2024 14:52
@komalverma04 komalverma04 restored the Add-ignoringParenImpCasts-in-hasAnyArgument branch April 20, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hasAnyArgument() spuriously ignores match
3 participants