[clang-tidy] Implement cppcoreguidelines F.19

[ccotter]

Summary

Warns when a function accepting a forwarding reference does anything besides
forwarding (with std::forward) the parameter in the body of the function.

[PiotrZSL]

maybe some other name for check, like missing-std-forward.

clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp

62 ↗

(On Diff #508432)

move this up to be checkered first

67 ↗

(On Diff #508432)

anyway invert this logic.
would be nice to invert this to start matching with functionDecl, but we don't have forEachParameter

69–72 ↗

(On Diff #508432)

use isDefinition also for functionDecl,
anyway maybe

hasAncestor(functionnDecl(isDefinition(),
                                            ToParam,
                                            unless(hasDescendant(ForwardCallMatcher)))))))

I don't see reason for eplicit handling of cxxConstructorDecl, hasDescendant will match also initialization list.
Exclude code that is used in decltype, like:

template<typename T>
void test(T&& t) {
   using Type = decltype(callSomething(std::forward<T>(t)));
   callOther<Type>(t);
}

83–84 ↗

(On Diff #508432)

I thing this can never happen

clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h

32 ↗

(On Diff #508432)

add here that thing about UnlessSpelledInSource....

[ccotter]

• add tests, simplify expr, handle unevaluated exprs

clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp

83–84 ↗

(On Diff #508432)

Another review suggested I check the matched node for nullness, even though it was the only possible match (https://reviews.llvm.org/D140793). I'll keep it as is since in the spirit of defensive programming and to assist future developers who may add more matches that might invalidate the invariant that Param is always non-null.

clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h

32 ↗

(On Diff #508432)

Sorry, I think I'm missing some context here. Would you mind clarifying your comment?

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

127

Please follow 80 characters limit for text.

[ccotter]

• add tests, simplify expr, handle unevaluated exprs
• formatting

[PiotrZSL]

clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp

20 ↗

(On Diff #508450)

move this matcher to some utils/...
It may be needed by other checks.

clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h

32 ↗

(On Diff #508432)

I mean:

std::optional<TraversalKind> getCheckTraversalKind() const override {
return TK_IgnoreUnlessSpelledInSource;
}

just to specify this explicitly..

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst

7 ↗

(On Diff #508450)

Add link to cpp guidelines, mention rule name like in other checks.

clang-tools-extra/docs/clang-tidy/checks/list.rst

189

no fixes

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp

138 ↗

(On Diff #508450)

what about when someone uses std::move instead of std::format ?
maybe some "note" for such issue ?

[ccotter]

• fix docs, fix test

clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp

20 ↗

(On Diff #508450)

Will be done in https://reviews.llvm.org/D146929

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp

138 ↗

(On Diff #508450)

Are you suggesting to have the tool add a special note in something like

template <class T>
void foo(T&& t) { T other = std::move(t); }

I'm not sure I completely followed what you were saying. Or perhaps a fixit for this specific case of using move on a forwarding reference (fixit to replace move with forward).

[ccotter]

• Use common function

[ccotter]

• Rename

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp

138 ↗

(On Diff #508450)

@PiotrZSL - I wasn't sure what you meant here.

[ccotter]

• format

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp

138 ↗

(On Diff #508450)

Yes, I meant that situation. But only because "forwarding reference parameter 't' is never forwarded inside the function body" could be confused in such case, where there is std::move instead of std::forward.
But that could be ignored, or covered by other check that would simply detect that you doing move when you should do forward.

[ccotter]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp

138 ↗

(On Diff #508450)

Ya, https://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-forwarding-reference.html should cover this. This new check missing-std-forward only knows how to find code that is missing forward

[ccotter]

Fix tests

[ccotter]

bump?

[PiotrZSL]

Overall looks fine. My main concern are lambdas (and maybe functions/classes in functions, but that should only hit performance).
Please close all comments before pushing this.

clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp

61

probably auto would be sufficient.

70

maybe think like: "functionDecl(forEachDescendant(parmVarDecl" could work.

71

probably this could be named like isTemplateTypeParameter, current name suggest more that it's a FunctionDecl matcher, not ParmVarDecl.

71

consider excluding system code, or code inside std namespace.

72

maybe we should skip also template instances.

73

consider std::move those matchers, always check construction could be faster...

clang-tools-extra/docs/clang-tidy/checks/list.rst

192

no fixes provided, remove

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp

137

Add test with std::forward happen in lambda, like:

template <class T>
void does_something(T&& t) {
   [=] { call_other(std::forward<T>(t)); }
}

this may not be detected.
I usually solve those things like this:

  auto ForwardCallMatcher = callExpr(forCallable(equalsBoundNode("first")));

...
  hasAncestor(functionDecl().bind("first")),
  hasAncestor(functionDecl(isDefinition(), equalsBoundNode("first"), ToParam, unless(hasDescendant(ForwardCallMatcher))))),

Problem with hasAncestor, is that if first ancestor does not match then it's trying parent one. equalsBoundNode can be used to limit this only to first hasAncestor of specific type (unless there is other matcher for that).

[ccotter]

• feedback

[ccotter]

clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp

70

Not sure if I see any advantages of forEachDescendant over what I have here - I think I'll leave it as is.

71

Why specifically do this in this check, but not others (very few seem to do this)?

72

Not sure I follow. Can you give an example?

[ccotter]

rebase

[ccotter]

Note, @PiotrZSL I don't have commit access, so if you're happy with the updates, could you please land this for me?

[ccotter]

• fix merge

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

136

Should be after cppcoreguidelines-misleading-capture-default-by-value.

[PiotrZSL]

forwarding reference parameter '' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
consider ignoring unnamed parameters, they unused, so nothing to forward.

[PiotrZSL]

Add delayed template parsing in tests (to fix windows tests)
Reorder release notes

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

   InitVariablesCheck.cpp
   InterfacesGlobalInitCheck.cpp
   MacroUsageCheck.cpp
+  MissingStdForwardCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #include "InitVariablesCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
+#include "MissingStdForwardCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"

         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<MacroUsageCheck>(
         "cppcoreguidelines-macro-usage");
+    CheckFactories.registerCheck<MissingStdForwardCheck>(
+        "cppcoreguidelines-missing-std-forward");
     CheckFactories.registerCheck<NarrowingConversionsCheck>(
         "cppcoreguidelines-narrowing-conversions");
     CheckFactories.registerCheck<NoMallocCheck>("cppcoreguidelines-no-malloc");

clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h

+//===--- MissingStdForwardCheck.h - clang-tidy ------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MISSINGSTDFORWARDCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MISSINGSTDFORWARDCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::cppcoreguidelines {
+
+/// Warns when a function accepting a forwarding reference does anything besides
+/// forwarding (with std::forward) the parameter in the body of the function.
+/// This check implement CppCoreGuideline F.19.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/missing-std-forward.html
+class MissingStdForwardCheck : public ClangTidyCheck {
+public:
+  MissingStdForwardCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus11;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+
+} // namespace clang::tidy::cppcoreguidelines
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MISSINGSTDFORWARDCHECK_H

clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp

+//===--- MissingStdForwardCheck.cpp - clang-tidy --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "MissingStdForwardCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+namespace {
+
+using matchers::hasUnevaluatedContext;
+
+AST_MATCHER_P(QualType, possiblyPackExpansionOf,
+              ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
+  return InnerMatcher.matches(Node.getNonPackExpansionType(), Finder, Builder);
+}
+
+AST_MATCHER(ParmVarDecl, isTemplateTypeOfFunction) {
+  ast_matchers::internal::Matcher<QualType> Inner = possiblyPackExpansionOf(
+      qualType(rValueReferenceType(),
+               references(templateTypeParmType(
+                   hasDeclaration(templateTypeParmDecl()))),
+               unless(references(qualType(isConstQualified())))));
+  if (!Inner.matches(Node.getType(), Finder, Builder))
+    return false;
+
+  const auto *Function = dyn_cast<FunctionDecl>(Node.getDeclContext());
+  if (!Function)
+    return false;
+
+  const FunctionTemplateDecl *FuncTemplate =
+      Function->getDescribedFunctionTemplate();
+  if (!FuncTemplate)
+    return false;
+
+  QualType ParamType =
+      Node.getType().getNonPackExpansionType()->getPointeeType();
+  const auto *TemplateType = ParamType->getAs<TemplateTypeParmType>();
+  if (!TemplateType)
+    return false;
+
+  return TemplateType->getDepth() ==
+         FuncTemplate->getTemplateParameters()->getDepth();
+}
+
+} // namespace
+
+void MissingStdForwardCheck::registerMatchers(MatchFinder *Finder) {
+  auto ToParam = hasAnyParameter(parmVarDecl(equalsBoundNode("param")));
+
+  StatementMatcher ForwardCallMatcher = callExpr(
+      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())))));
+
+  Finder->addMatcher(
+      parmVarDecl(
+          parmVarDecl().bind("param"), isTemplateTypeOfFunction(),
+          hasAncestor(functionDecl(isDefinition(), ToParam,
+                                   unless(hasDescendant(ForwardCallMatcher))))),
+      this);
+}
+
+void MissingStdForwardCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
+
+  if (!Param)
+    return;
+
+  diag(Param->getLocation(),
+       "forwarding reference parameter %0 is never forwarded "
+       "inside the function body")
+      << Param;
+}
+
+} // namespace clang::tidy::cppcoreguidelines

clang-tools-extra/docs/ReleaseNotes.rst

   use-after-free errors and suggests avoiding captures or ensuring the lambda
   closure object has a guaranteed lifetime.
 
+- New :doc:`cppcoreguidelines-missing-std-forward
+  <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check.
+
+  Warns when a forwarding reference parameter is not forwarded within the
+  function body.
+
 - New :doc:`cppcoreguidelines-rvalue-reference-param-not-moved
   <clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved>` check.
 

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst

+.. title:: clang-tidy - cppcoreguidelines-missing-std-forward
+
+cppcoreguidelines-missing-std-forward
+=====================================
+
+Warns when a forwarding reference parameter is not forwarded inside the
+function body.
+
+Example:
+
+.. code-block:: c++
+
+  template <class T>
+  void wrapper(T&& t) {
+    impl(std::forward<T>(t), 1, 2); // Correct
+  }
+
+  template <class T>
+  void wrapper2(T&& t) {
+    impl(t, 1, 2); // Oops - should use std::forward<T>(t)
+  }
+
+  template <class T>
+  void wrapper3(T&& t) {
+    impl(std::move(t), 1, 2); // Also buggy - should use std::forward<T>(t)
+  }
+
+  template <class F>
+  void wrapper_function(F&& f) {
+    std::forward<F>(f)(1, 2); // Correct
+  }
+
+  template <class F>
+  void wrapper_function2(F&& f) {
+    f(1, 2); // Incorrect - may not invoke the desired qualified function operator
+  }
+
+This check implements
+`CppCoreGuideline F.19 <http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-forward>`_.

clang-tools-extra/docs/clang-tidy/checks/list.rst

    `cppcoreguidelines-init-variables <cppcoreguidelines/init-variables.html>`_, "Yes"
    `cppcoreguidelines-interfaces-global-init <cppcoreguidelines/interfaces-global-init.html>`_,
    `cppcoreguidelines-macro-usage <cppcoreguidelines/macro-usage.html>`_,
+   `cppcoreguidelines-missing-std-forward <cppcoreguidelines/missing-std-forward.html>`_, "Yes"
    `cppcoreguidelines-narrowing-conversions <cppcoreguidelines/narrowing-conversions.html>`_,
    `cppcoreguidelines-no-malloc <cppcoreguidelines/no-malloc.html>`_,
    `cppcoreguidelines-owning-memory <cppcoreguidelines/owning-memory.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t -- -- -fno-delayed-template-parsing
+
+// NOLINTBEGIN
+namespace std {
+
+template <typename T> struct remove_reference      { using type = T; };
+template <typename T> struct remove_reference<T&>  { using type = T; };
+template <typename T> struct remove_reference<T&&> { using type = T; };
+
+template <typename T> using remove_reference_t = typename remove_reference<T>::type;
+
+template <typename T> constexpr T &&forward(remove_reference_t<T> &t) noexcept;
+template <typename T> constexpr T &&forward(remove_reference_t<T> &&t) noexcept;
+template <typename T> constexpr remove_reference_t<T> &&move(T &&x);
+
+} // namespace std
+// NOLINTEND
+
+struct S {
+  S();
+  S(const S&);
+  S(S&&) noexcept;
+  S& operator=(const S&);
+  S& operator=(S&&) noexcept;
+};
+
+template <class... Ts>
+void consumes_all(Ts&&...);
+
+namespace positive_cases {
+
+template <class T>
+void does_not_forward(T&& t) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+  T other = t;
+}
+
+template <class T>
+void does_not_forward_invoked(T&& t) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+  T other = t();
+}
+
+template <class T>
+void forwards_pairwise(T&& t) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+  auto first = std::forward<T>(t.first);
+  auto second = std::forward<T>(t.second);
+}
+
+template <class... Ts>
+void does_not_forward_pack(Ts&&... ts) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+  consumes_all(ts...);
+}
+
+template <class T>
+class AClass {
+
+  template <class U>
+  AClass(U&& u) : data(u) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+
+  template <class U>
+  AClass& operator=(U&& u) { }
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+
+  template <class U>
+  void mixed_params(T&& t, U&& u) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+    T other1 = std::move(t);
+    U other2 = std::move(u);
+  }
+
+  T data;
+};
+
+template <class T>
+void does_not_forward_in_evaluated_code(T&& t) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
+  using result_t = decltype(std::forward<T>(t));
+  unsigned len = sizeof(std::forward<T>(t));
+  T other = t;
+}
+
+} // namespace positive_cases
+
+namespace negative_cases {
+
+template <class T>
+void just_a_decl(T&&t);
+
+template <class T>
+void does_forward(T&& t) {
+  T other = std::forward<T>(t);
+}
+
+template <class... Ts>
+void does_forward_pack(Ts&&... ts) {
+  consumes_all(std::forward<Ts>(ts)...);
+}
+
+void pass_by_value(S s) {
+  S other = std::move(s);
+}
+
+void lvalue_ref(S& s) {
+  S other = std::move(s);
+}
+
+void rvalue_ref(S&& s) {
+  S other = std::move(s);
+}
+
+template <class T>
+void templated_rvalue_ref(std::remove_reference_t<T>&& t) {
+  T other = std::move(t);
+}
+
+template <class T>
+class AClass {
+
+  template <class U>
+  AClass(U&& u) : data(std::forward<U>(u)) {}
+
+  template <class U>
+  AClass& operator=(U&& u) {
+    data = std::forward<U>(u);
+  }
+
+  void rvalue_ref(T&& t) {
+    T other = std::move(t);
+  }
+
+  T data;
+};
+
+} // namespace negative_cases