[clang-tidy] Flag std::forward on non-forwarding references

[ccotter]

Summary

Implement the std::forward specific enforcement of ES.56.

[PiotrZSL]

First, https://reviews.llvm.org/D144347 :)

[ccotter]

Indeed - sorry i missed that

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

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   AvoidCaptureDefaultWhenCapturingThisCheck.cpp
   AvoidCapturingLambdaCoroutinesCheck.cpp
   AvoidConstOrRefDataMembersCheck.cpp
   AvoidDoWhileCheck.cpp
   AvoidGotoCheck.cpp
   AvoidNonConstGlobalVariablesCheck.cpp
   AvoidReferenceCoroutineParametersCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
+  ForwardNonForwardingParameterCheck.cpp
   InitVariablesCheck.cpp
   InterfacesGlobalInitCheck.cpp
   MacroUsageCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp
   PreferMemberInitializerCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp

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

 //===-- CppCoreGuidelinesTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidCaptureDefaultWhenCapturingThisCheck.h"
 #include "AvoidCapturingLambdaCoroutinesCheck.h"
 #include "AvoidConstOrRefDataMembersCheck.h"
 #include "AvoidDoWhileCheck.h"
 #include "AvoidGotoCheck.h"
 #include "AvoidNonConstGlobalVariablesCheck.h"
 #include "AvoidReferenceCoroutineParametersCheck.h"
+#include "ForwardNonForwardingParameterCheck.h"
 #include "InitVariablesCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "PreferMemberInitializerCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<AvoidNonConstGlobalVariablesCheck>(
         "cppcoreguidelines-avoid-non-const-global-variables");
     CheckFactories.registerCheck<AvoidReferenceCoroutineParametersCheck>(
         "cppcoreguidelines-avoid-reference-coroutine-parameters");
     CheckFactories.registerCheck<modernize::UseOverrideCheck>(
         "cppcoreguidelines-explicit-virtual-functions");
+    CheckFactories.registerCheck<ForwardNonForwardingParameterCheck>(
+        "cppcoreguidelines-forward-non-forwarding-parameter");
     CheckFactories.registerCheck<InitVariablesCheck>(
         "cppcoreguidelines-init-variables");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<MacroUsageCheck>(
         "cppcoreguidelines-macro-usage");
     CheckFactories.registerCheck<NarrowingConversionsCheck>(
         "cppcoreguidelines-narrowing-conversions");

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

+//===--- ForwardNonForwardingParameterCheck.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_FORWARDNONFORWARDINGPARAMETERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_FORWARDNONFORWARDINGPARAMETERCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::cppcoreguidelines {
+
+/// Warns when std::forward is called on a non-forwarding reference. This
+/// check implements the std::forward specific enforcement section of
+/// CppCoreGuideline ES.56.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.html
+class ForwardNonForwardingParameterCheck : public ClangTidyCheck {
+public:
+  ForwardNonForwardingParameterCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::cppcoreguidelines
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_FORWARDNONFORWARDINGPARAMETERCHECK_H

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

+//===--- ForwardNonForwardingParameterCheck.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 "ForwardNonForwardingParameterCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+namespace {
+
+AST_MATCHER_P(QualType, possiblyPackExpansionOf,
+              ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
+  return InnerMatcher.matches(Node.getNonPackExpansionType(), Finder, Builder);
+}
+
+AST_MATCHER(ParmVarDecl, isForwardingRefTypeOfFunction) {
+  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();
+}
+
+AST_MATCHER(ParmVarDecl, isInInstantiation) {
+  const auto *Function = dyn_cast<FunctionDecl>(Node.getDeclContext());
+  if (!Function)
+    return false;
+  return Function->isTemplateInstantiation();
+}
+
+} // namespace
+
+void ForwardNonForwardingParameterCheck::registerMatchers(MatchFinder *Finder) {
+  auto RvalueRefDecl =
+      varDecl(hasType(type(rValueReferenceType())),
+              unless(hasType(references(qualType(isConstQualified())))));
+  auto VarMatcher =
+      varDecl(unless(parmVarDecl(isForwardingRefTypeOfFunction())),
+              unless(parmVarDecl(isInInstantiation())),
+              optionally(RvalueRefDecl.bind("rvalue-var")))
+          .bind("var");
+
+  Finder->addMatcher(
+      callExpr(anyOf(callee(functionDecl(hasName("::std::forward"))),
+                     callee(unresolvedLookupExpr(hasAnyDeclaration(namedDecl(
+                         hasUnderlyingDecl(hasName("::std::forward"))))))),
+               argumentCountIs(1),
+               hasArgument(0, declRefExpr(to(VarMatcher)).bind("ref")))
+          .bind("call"),
+      this);
+}
+
+void ForwardNonForwardingParameterCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+  const auto *Ref = Result.Nodes.getNodeAs<DeclRefExpr>("ref");
+  const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var");
+  const auto *RvalueRefVar = Result.Nodes.getNodeAs<VarDecl>("rvalue-var");
+
+  if (!Call || !Var || !Ref)
+    return;
+
+  if (RvalueRefVar) {
+    ASTContext &Context = *Result.Context;
+
+    std::string MoveText = "std::move(";
+    MoveText += Lexer::getSourceText(
+        CharSourceRange::getTokenRange(Ref->getBeginLoc(), Ref->getEndLoc()),
+        Context.getSourceManager(), Context.getLangOpts());
+    MoveText += ")";
+
+    diag(Ref->getLocation(),
+         "calling std::forward on rvalue reference %0; use std::move instead")
+        << Var
+        << FixItHint::CreateReplacement(
+               CharSourceRange::getTokenRange(Call->getBeginLoc(),
+                                              Call->getEndLoc()),
+               MoveText);
+  } else {
+    diag(Ref->getLocation(),
+         "calling std::forward on non-forwarding reference %0")
+        << Var;
+  }
+}
+
+} // namespace clang::tidy::cppcoreguidelines

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cppcoreguidelines-rvalue-reference-param-not-moved
   the function body.
 
 - New :doc:`llvmlibc-inline-function-decl
   <clang-tidy/checks/llvmlibc/inline-function-decl>` check.
 
   Checks that all implicit and explicit inline functions in header files are
   tagged with the ``LIBC_INLINE`` macro.
 
+- New :doc:`cppcoreguidelines-forward-non-forwarding-parameter
+  <clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter>` check.
+
+  Warns when ``std::forward`` is used on a non-forwarding reference.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc24-c
   <clang-tidy/checks/cert/msc24-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 - New alias :doc:`cert-msc33-c

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.rst

+.. title:: clang-tidy - cppcoreguidelines-forward-non-forwarding-parameter
+
+cppcoreguidelines-forward-non-forwarding-parameter
+==================================================
+
+Warns when ``std::forward`` is used on a non-forwarding reference.
+``std::forward`` is a named cast that preserves the value category of
+the input parameter. It is used in templated functions when the
+parameter should be "forwarded" to another function or constructor
+within the function template body. Using ``std::forward`` on
+non-forwarding references can lead to unexpected or buggy behavior.
+
+Example:
+
+.. code-block:: c++
+
+  void by_lvalue_ref(Object& Input) {
+    // This will actually move Input, always. This is confusing
+    // since the caller of by_lvalue_ref did not std::move, and
+    // could lead to use after move bugs.
+
+    // If a copy was intended, remove std::forward.
+    // If a move is intended, change Input to 'Object&&' or 'Object'
+    // and update the caller.
+    If that is what is intended, use std::move.
+    Object Copy(std::forward<Object>(Input));
+  }
+
+  void move_param(Object&& Input) {
+    Object Copy(std::move(Input)); // Ok
+    Object Copy2(std::forward<Object>(Input)); // Use std::move instead
+  }
+
+This check implements the ``std::forward`` specific enforcements of
+`CppCoreGuideline ES.56 <http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es56-write-stdmove-only-when-you-need-to-explicitly-move-an-object-to-another-scope>

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

.. csv-table::
    `concurrency-thread-canceltype-asynchronous <concurrency/thread-canceltype-asynchronous.html>`_,
    `cppcoreguidelines-avoid-capture-default-when-capturing-this <cppcoreguidelines/avoid-capture-default-when-capturing-this.html>`_, "Yes"
    `cppcoreguidelines-avoid-capturing-lambda-coroutines <cppcoreguidelines/avoid-capturing-lambda-coroutines.html>`_,
    `cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines/avoid-const-or-ref-data-members.html>`_,
    `cppcoreguidelines-avoid-do-while <cppcoreguidelines/avoid-do-while.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines/avoid-goto.html>`_,
    `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines/avoid-non-const-global-variables.html>`_,
    `cppcoreguidelines-avoid-reference-coroutine-parameters <cppcoreguidelines/avoid-reference-coroutine-parameters.html>`_,
+   `cppcoreguidelines-forward-non-forwarding-parameter <cppcoreguidelines/forward-non-forwarding-parameter.html>`_, "Yes"
    `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-narrowing-conversions <cppcoreguidelines/narrowing-conversions.html>`_,
    `cppcoreguidelines-no-malloc <cppcoreguidelines/no-malloc.html>`_,
    `cppcoreguidelines-owning-memory <cppcoreguidelines/owning-memory.html>`_,
    `cppcoreguidelines-prefer-member-initializer <cppcoreguidelines/prefer-member-initializer.html>`_, "Yes"
    `cppcoreguidelines-pro-bounds-array-to-pointer-decay <cppcoreguidelines/pro-bounds-array-to-pointer-decay.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forward-non-forwarding-parameter.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-forward-non-forwarding-parameter %t
+
+// 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;
+
+} // namespace std
+// NOLINTEND
+
+struct Obj {
+  Obj();
+  Obj(const Obj&);
+  Obj(Obj&&) noexcept;
+  Obj& operator=(const Obj&);
+  Obj& operator=(Obj&&) noexcept;
+};
+
+template <class... Ts>
+void consumes_all(Ts&&...);
+
+namespace positive_cases {
+
+void forward_local_object() {
+  Obj obj;
+  Obj& obj_ref = obj;
+
+  Obj obj2 = std::forward<Obj>(obj);
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+
+  Obj obj3 = std::forward<Obj>(obj_ref);
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj_ref' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+void forward_value_param(Obj obj) {
+  Obj obj2 = std::forward<Obj>(obj);
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class... Ts>
+void forward_pack_of_values(Ts... ts) {
+  consumes_all(std::forward<Ts>(ts)...);
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: calling std::forward on non-forwarding reference 'ts' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class... Ts>
+void forward_pack_of_lvalue_refs(Ts&... ts) {
+  consumes_all(std::forward<Ts>(ts)...);
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: calling std::forward on non-forwarding reference 'ts' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+void forward_lvalue_ref(Obj& obj) {
+  Obj obj2 = std::forward<Obj>(obj);
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+void forward_const_lvalue_ref(const Obj& obj) {
+  Obj obj2 = std::forward<const Obj>(obj);
+  // CHECK-MESSAGES: :[[@LINE-1]]:38: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+void forward_rvalue_ref(Obj&& obj) {
+  Obj obj2 = std::forward<Obj>(obj);
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on rvalue reference 'obj'; use std::move instead [cppcoreguidelines-forward-non-forwarding-parameter]
+  // CHECK-FIXES: Obj obj2 = std::move(obj);
+}
+
+void forward_const_rvalue_ref(const Obj&& obj) {
+  Obj obj2 = std::forward<const Obj>(obj);
+  // CHECK-MESSAGES: :[[@LINE-1]]:38: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class T>
+void forward_value_t(T t) {
+  T other = std::forward<T>(t);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class T>
+void forward_lvalue_ref_t(T& t) {
+  T other = std::forward<T>(t);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class T>
+void forward_const_rvalue_ref_t(const T&& t) {
+  T other = std::forward<const T>(t);
+  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class T>
+struct SomeClass
+{
+  void forwards_lvalue_ref(T& t) {
+    T other = std::forward<T>(t);
+    // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter]
+  }
+
+  void forwards_rvalue_ref(T&& t) {
+    T other = std::forward<T>(t);
+    // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: calling std::forward on rvalue reference 't'; use std::move instead [cppcoreguidelines-forward-non-forwarding-parameter]
+    // CHECK-FIXES: T other = std::move(t);
+  }
+};
+
+} // namespace positive_cases
+
+namespace negative_cases {
+
+template <class T>
+void forwards_param(T&& t) {
+  T other = std::forward<T>(t);
+}
+
+template <class... Ts>
+void forwards_param_pack(Ts&&... ts) {
+  consumes_all(std::forward<Ts>(ts)...);
+}
+
+template <class A>
+struct SomeClass {
+  template <class T>
+  void forwards_param(T&& t) {
+    T other = std::forward<T>(t);
+  }
+
+  template <class T, class U>
+  void forwards_param(T&& t) {
+    T other = std::forward<T>(t);
+  }
+
+  template <class... Ts>
+  void forwards_param_pack(Ts&&... ts) {
+    consumes_all(1, std::forward<Ts>(ts)...);
+  }
+};
+
+void instantiates_calls() {
+  Obj o;
+  forwards_param(0);
+  forwards_param(Obj{});
+  forwards_param(o);
+
+  forwards_param_pack(0);
+  forwards_param_pack(Obj{});
+  forwards_param_pack(o);
+  forwards_param_pack(0, Obj{}, o);
+
+  SomeClass<Obj> someObj;
+  someObj.forwards_param(0);
+  someObj.forwards_param(Obj{});
+  someObj.forwards_param(o);
+  someObj.forwards_param_pack(0, Obj{}, o);
+}
+
+} // namespace negative_cases