[clang-tidy] fix false positive in cppcoreguidelines-missing-std-forward

[jcsxky]

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

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Qizhi Hu (jcsxky)

Changes

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

---

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

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp (+66-7)
• (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp (+23-3)

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

[PiotrZSL]

Missing release notes & some mention about this in documentation.

[PiotrZSL]

LGTM.

[HerrCai0907]

I agree to fix false positive reported in #68105. But for capturing in lambda be reference and do forward in lambda, It is just like create a record by ref instead of using forward and do forward in some member function. I don't think it is an expected false positive.

template<typename T>
void lambda_value_reference_capture_list_ref_2(T&& t) {
    [&t] { T other = std::forward<T>(t); };
}

@jcsxky @PiotrZSL Could you think again about this PR?

[jcsxky]

I agree to fix false positive reported in #68105. But for capturing in lambda be reference and do forward in lambda, It is just like create a record by ref instead of using forward and do forward in some member function. I don't think it is an expected false positive.

template<typename T>
void lambda_value_reference_capture_list_ref_2(T&& t) {
    [&t] { T other = std::forward<T>(t); };
}

@jcsxky @PiotrZSL Could you think again about this PR?

Not really. Capture by reference makes t in parameter same with it in lambda body. Maybe you can check #68105 (comment) .

[HerrCai0907]

Capture by reference makes t in parameter same with it in lambda body

That is definitely wrong.
It will get stack-use-after-scope error if address sanitize is enabled.

#include <iostream>
#include <utility>

template <class T>
auto f(T &&t) {
  return [&]() {
    return std::forward<T>(t);
  };
}

struct S {
  int v;
};

int main() {
  auto fn = f(S{.v = 1000});

  std::cout << fn().v << "\n";
}