[clang-tools-extra] [clang-tidy] fix false positive in cppcoreguidelines-missing-std-forward (PR #77056)

[Qizhi Hu via cfe-commits]

https://github.com/jcsxky created https://github.com/llvm/llvm-project/pull/77056

Parameter variable which is forwarded in lambda capture list or in body by reference is reasonable and current version of this check produces false positive on these cases. This patch try to fix the [issue](https://github.com/llvm/llvm-project/issues/68105)

>From 799efba6c8fd3acfc96db8b5b2185bcd93aecb84 Mon Sep 17 00:00:00 2001
From: huqizhi <huqizhi at feysh.com>
Date: Wed, 3 Jan 2024 09:44:26 +0800
Subject: [PATCH] [clang-tidy] fix false positive in
 cppcoreguidelines-missing-std-forward

---
 .../MissingStdForwardCheck.cpp                | 73 +++++++++++++++++--
 .../cppcoreguidelines/missing-std-forward.cpp | 26 ++++++-
 2 files changed, 89 insertions(+), 10 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
index 0b85ea19735eef..f0e021039f094d 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
@@ -53,18 +53,76 @@ AST_MATCHER(ParmVarDecl, isTemplateTypeParameter) {
          FuncTemplate->getTemplateParameters()->getDepth();
 }
 
+AST_MATCHER_P(NamedDecl, hasSameNameAsBoundNode, std::string, BindingID) {
+  const auto &Name = Node.getNameAsString();
+
+  return Builder->removeBindings(
+      [this, Name](const ast_matchers::internal::BoundNodesMap &Nodes) {
+        const auto &BN = Nodes.getNode(this->BindingID);
+        if (const auto *ND = BN.get<NamedDecl>()) {
+          if (!isa<FieldDecl, CXXMethodDecl, VarDecl>(ND))
+            return true;
+          return ND->getName() != Name;
+        }
+        return true;
+      });
+}
+
+AST_MATCHER_P(LambdaCapture, hasCaptureKind, LambdaCaptureKind, Kind) {
+  return Node.getCaptureKind() == Kind;
+}
+
+AST_MATCHER_P(LambdaExpr, hasCaptureDefaultKind, LambdaCaptureDefault, Kind) {
+  return Node.getCaptureDefault() == Kind;
+}
+
+AST_MATCHER(LambdaExpr, hasCaptureToParm) {
+  auto RefToParm = capturesVar(varDecl(hasSameNameAsBoundNode("param")));
+  auto HasRefToParm = hasAnyCapture(RefToParm);
+
+  auto CaptureInRef =
+      allOf(hasCaptureDefaultKind(LambdaCaptureDefault::LCD_ByRef),
+            unless(HasRefToParm));
+  auto CaptureInCopy = allOf(
+      hasCaptureDefaultKind(LambdaCaptureDefault::LCD_ByCopy), HasRefToParm);
+  auto CaptureByRefExplicit = hasAnyCapture(
+      allOf(hasCaptureKind(LambdaCaptureKind::LCK_ByRef), RefToParm));
+
+  auto Captured =
+      lambdaExpr(anyOf(CaptureInRef, CaptureInCopy, CaptureByRefExplicit));
+  if (Captured.matches(Node, Finder, Builder))
+    return true;
+
+  return false;
+}
+
+AST_MATCHER(CallExpr, forCallableNode) {
+  auto InvokeInCaptureList = hasAnyCapture(capturesVar(
+      varDecl(hasInitializer(ignoringParenImpCasts(equalsNode(&Node))))));
+  auto InvokeInLambda = hasDeclContext(cxxRecordDecl(
+      isLambda(),
+      hasParent(lambdaExpr(forCallable(equalsBoundNode("func")),
+                           anyOf(InvokeInCaptureList, hasCaptureToParm())))));
+
+  if (forCallable(anyOf(equalsBoundNode("func"), InvokeInLambda))
+          .matches(Node, Finder, Builder))
+    return true;
+
+  return false;
+}
+
 } // namespace
 
 void MissingStdForwardCheck::registerMatchers(MatchFinder *Finder) {
   auto ToParam = hasAnyParameter(parmVarDecl(equalsBoundNode("param")));
 
-  auto ForwardCallMatcher = callExpr(
-      forCallable(equalsBoundNode("func")), argumentCountIs(1),
-      callee(unresolvedLookupExpr(hasAnyDeclaration(
-          namedDecl(hasUnderlyingDecl(hasName("::std::forward")))))),
-      hasArgument(0, declRefExpr(to(equalsBoundNode("param"))).bind("ref")),
-      unless(anyOf(hasAncestor(typeLoc()),
-                   hasAncestor(expr(hasUnevaluatedContext())))));
+  auto ForwardCallMatcher =
+      callExpr(forCallableNode(), argumentCountIs(1),
+               callee(unresolvedLookupExpr(hasAnyDeclaration(
+                   namedDecl(hasUnderlyingDecl(hasName("::std::forward")))))),
+               hasArgument(0, declRefExpr(to(equalsBoundNode("param")))),
+               unless(anyOf(hasAncestor(typeLoc()),
+                            hasAncestor(expr(hasUnevaluatedContext())))));
 
   Finder->addMatcher(
       parmVarDecl(parmVarDecl().bind("param"), isTemplateTypeParameter(),
@@ -76,6 +134,7 @@ void MissingStdForwardCheck::registerMatchers(MatchFinder *Finder) {
 }
 
 void MissingStdForwardCheck::check(const MatchFinder::MatchResult &Result) {
+
   const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
 
   if (!Param)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp
index b9720db272e406..55d6be743c22ab 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp
@@ -90,9 +90,9 @@ void lambda_value_capture(T&& t) {
 }
 
 template <class T>
-void lambda_value_reference(T&& t) {
-  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
-  [&]() { T other = std::forward<T>(t); };
+void lambda_value_capture_copy(T&& t) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+  [&,t]() { T other = std::forward<T>(t); };
 }
 
 } // namespace positive_cases
@@ -147,4 +147,24 @@ class AClass {
   T data;
 };
 
+template <class T>
+void lambda_value_reference(T&& t) {
+  [&]() { T other = std::forward<T>(t); };
+}
+
+template<typename T>
+void lambda_value_reference_capture_list_ref_1(T&& t) {
+    [=, &t] { T other = std::forward<T>(t); };
+}
+
+template<typename T>
+void lambda_value_reference_capture_list_ref_2(T&& t) {
+    [&t] { T other = std::forward<T>(t); };
+}
+
+template<typename T>
+void lambda_value_reference_capture_list(T&& t) {
+    [t = std::forward<T>(t)] { t(); };
+}
+
 } // namespace negative_cases