[clang-tidy] Add bugprone-std-forward-type-mismatch check

[PiotrZSL]

Summary

Detects instances where std::forward is called with a different type as an
argument compared to the type specified as the template parameter.

Extracted from D144347.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/bugprone/std-forward-type-mismatch.rst

28

Excessive newline.

[ccotter]

clang-tools-extra/docs/clang-tidy/checks/bugprone/std-forward-type-mismatch.rst

18

How confident are we to suggest a fixit to use static_cast? Although equivalent, the code could be incorrect to begin with and merit manual review to decide what the correct cast really should be.

clang-tools-extra/test/clang-tidy/checkers/bugprone/std-forward-type-mismatch.cpp

116

Does this check work if testPartialTemplateBad is not instantiated?

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "ReservedIdentifierCheck.h"
 #include "SharedPtrArrayMismatchCheck.h"
 #include "SignalHandlerCheck.h"
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "SpuriouslyWakeUpFunctionsCheck.h"
 #include "StandaloneEmptyCheck.h"
+#include "StdForwardTypeMismatchCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "StringviewNullptrCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousIncludeCheck.h"
 #include "SuspiciousMemoryComparisonCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
     CheckFactories.registerCheck<SpuriouslyWakeUpFunctionsCheck>(
         "bugprone-spuriously-wake-up-functions");
     CheckFactories.registerCheck<StandaloneEmptyCheck>(
         "bugprone-standalone-empty");
+    CheckFactories.registerCheck<StdForwardTypeMismatchCheck>(
+        "bugprone-std-forward-type-mismatch");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "bugprone-string-integer-assignment");
     CheckFactories.registerCheck<StringLiteralWithEmbeddedNulCheck>(
         "bugprone-string-literal-with-embedded-nul");
     CheckFactories.registerCheck<StringviewNullptrCheck>(
         "bugprone-stringview-nullptr");

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

add_clang_library(clangTidyBugproneModule
   SharedPtrArrayMismatchCheck.cpp
   SignalHandlerCheck.cpp
   SignedCharMisuseCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   SmartPtrArrayMismatchCheck.cpp
   SpuriouslyWakeUpFunctionsCheck.cpp
   StandaloneEmptyCheck.cpp
+  StdForwardTypeMismatchCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   StringviewNullptrCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousIncludeCheck.cpp
   SuspiciousMemoryComparisonCheck.cpp
   SuspiciousMemsetUsageCheck.cpp

clang-tools-extra/clang-tidy/bugprone/StdForwardTypeMismatchCheck.h

+//===--- StdForwardTypeMismatchCheck.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_BUGPRONE_STDFORWARDTYPEMISMATCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STDFORWARDTYPEMISMATCHCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Detects instances where ``std::forward`` is called with a different type as
+/// an argument compared to the type specified as the template parameter.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/std-forward-type-mismatch.html
+class StdForwardTypeMismatchCheck : public ClangTidyCheck {
+public:
+  StdForwardTypeMismatchCheck(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;
+  std::optional<TraversalKind> getCheckTraversalKind() const override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STDFORWARDTYPEMISMATCHCHECK_H

clang-tools-extra/clang-tidy/bugprone/StdForwardTypeMismatchCheck.cpp

+//===--- StdForwardTypeMismatchCheck.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 "StdForwardTypeMismatchCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+AST_MATCHER_P(UnresolvedLookupExpr, hasOnyTemplateArgumentLoc,
+              ast_matchers::internal::Matcher<TemplateArgumentLoc>,
+              InnerMatcher) {
+  return Node.getNumTemplateArgs() == 1U &&
+         InnerMatcher.matches(Node.getTemplateArgs()[0], Finder, Builder);
+}
+
+} // namespace
+
+static SourceRange getAnglesLoc(const Expr *MatchedCallee) {
+  if (const auto *DeclRefExprCallee = dyn_cast<DeclRefExpr>(MatchedCallee))
+    return SourceRange(DeclRefExprCallee->getLAngleLoc(),
+                       DeclRefExprCallee->getRAngleLoc());
+
+  if (const auto *UnresolvedLookupExprCallee =
+          dyn_cast<UnresolvedLookupExpr>(MatchedCallee))
+    return SourceRange(UnresolvedLookupExprCallee->getLAngleLoc(),
+                       UnresolvedLookupExprCallee->getRAngleLoc());
+  return {};
+}
+
+void StdForwardTypeMismatchCheck::registerMatchers(MatchFinder *Finder) {
+  auto IsNamedStdForwardMatcher = hasName("::std::forward");
+  auto TemplateArgumentTypeMatcher =
+      templateArgumentLoc(hasTypeLoc(loc(qualType().bind("template"))));
+
+  Finder->addMatcher(
+      callExpr(
+          argumentCountIs(1U), unless(isExpansionInSystemHeader()),
+          callee(
+              expr(anyOf(declRefExpr(hasDeclaration(functionDecl(
+                                         IsNamedStdForwardMatcher)),
+                                     hasTemplateArgumentLoc(
+                                         0U, templateArgumentLoc(
+                                                 TemplateArgumentTypeMatcher))),
+                         unresolvedLookupExpr(
+                             hasAnyDeclaration(
+                                 namedDecl(IsNamedStdForwardMatcher)),
+                             hasOnyTemplateArgumentLoc(templateArgumentLoc(
+                                 TemplateArgumentTypeMatcher)))))
+                  .bind("callee")),
+          hasArgument(0U, expr(hasType(qualType().bind("argument")))))
+          .bind("call"),
+      this);
+}
+
+bool StdForwardTypeMismatchCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus11;
+}
+
+std::optional<TraversalKind>
+StdForwardTypeMismatchCheck::getCheckTraversalKind() const {
+  return TK_IgnoreUnlessSpelledInSource;
+}
+
+void StdForwardTypeMismatchCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedTemplateType =
+      Result.Nodes.getNodeAs<QualType>("template");
+  const auto *MatchedArgumentType =
+      Result.Nodes.getNodeAs<QualType>("argument");
+
+  const QualType CleanMatchedTemplateType =
+      MatchedTemplateType->getCanonicalType().getNonReferenceType();
+  const QualType CleanMatchedArgumentType =
+      MatchedArgumentType->getCanonicalType().getNonReferenceType();
+  if (CleanMatchedTemplateType == CleanMatchedArgumentType)
+    return;
+
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("call");
+  const auto *MatchedCallee = Result.Nodes.getNodeAs<Expr>("callee");
+
+  auto Diagnostic =
+      diag(MatchedExpr->getBeginLoc(),
+           "using 'std::forward' for type conversions from %0 to %1 is not "
+           "recommended here, use 'static_cast' instead");
+  Diagnostic << *MatchedArgumentType << *MatchedTemplateType;
+
+  const SourceRange AngleSourceRange = getAnglesLoc(MatchedCallee);
+  if (AngleSourceRange.isInvalid())
+    return;
+
+  if (!MatchedTemplateType->getCanonicalType()->isReferenceType())
+    Diagnostic << FixItHint::CreateReplacement(
+        SourceRange(AngleSourceRange.getEnd(), AngleSourceRange.getEnd()),
+        " &&>");
+
+  Diagnostic << FixItHint::CreateReplacement(
+      SourceRange(MatchedCallee->getBeginLoc(), AngleSourceRange.getBegin()),
+      "static_cast<");
+}
+
+} // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-multiple-new-in-one-expression
   memory by the first ``new`` may leak if the second allocation fails and throws exception.
 
 - New :doc:`bugprone-non-zero-enum-to-bool-conversion
   <clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion>` check.
 
   Detect implicit and explicit casts of ``enum`` type into ``bool`` where ``enum`` type
   doesn't have a zero-value enumerator.
 
+- New :doc:`bugprone-std-forward-type-mismatch
+  <clang-tidy/checks/bugprone/std-forward-type-mismatch>` check.
+
+  Detects instances where ``std::forward`` is called with a different type as an
+  argument compared to the type specified as the template parameter.
+
 - New :doc:`bugprone-unique-ptr-array-mismatch
   <clang-tidy/checks/bugprone/unique-ptr-array-mismatch>` check.
 
   Finds initializations of C++ unique pointers to non-array type that are
   initialized with an array.
 
 - New :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check.

clang-tools-extra/docs/clang-tidy/checks/bugprone/std-forward-type-mismatch.rst

+.. title:: clang-tidy - bugprone-std-forward-type-mismatch
+
+bugprone-std-forward-type-mismatch
+==================================
+
+Detects instances where ``std::forward`` is called with a different type as an
+argument compared to the type specified as the template parameter. It takes
+into consideration unwinding type aliases and excludes references or qualifiers
+when comparing the types.
+
+.. code-block:: c++
+
+  struct Base {};
+  struct Derived : Base {};
+
+  void func(Base& base) {
+    doSomething(std::forward<Derived&>(base)); // Incorrect usage
+    doSomething(static_cast<Derived&>(base)); // Suggested usage
+  }
+
+The ``std::forward`` function is primarily intended to forward the value
+category of an argument while preserving its constness and reference qualifiers.
+Using it with different types can lead to issues like object slicing or
+incorrect behavior.
+
+It is recommended to use ``static_cast`` instead of ``std::forward`` when there
+is a mismatch between the type passed as an argument and the template parameter.
+

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

.. csv-table::
    `bugprone-reserved-identifier <bugprone/reserved-identifier.html>`_, "Yes"
    `bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch.html>`_, "Yes"
    `bugprone-signal-handler <bugprone/signal-handler.html>`_,
    `bugprone-signed-char-misuse <bugprone/signed-char-misuse.html>`_,
    `bugprone-sizeof-container <bugprone/sizeof-container.html>`_,
    `bugprone-sizeof-expression <bugprone/sizeof-expression.html>`_,
    `bugprone-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions.html>`_,
    `bugprone-standalone-empty <bugprone/standalone-empty.html>`_, "Yes"
+   `bugprone-std-forward-type-mismatch <bugprone/std-forward-type-mismatch.html>`_,
    `bugprone-string-constructor <bugprone/string-constructor.html>`_, "Yes"
    `bugprone-string-integer-assignment <bugprone/string-integer-assignment.html>`_, "Yes"
    `bugprone-string-literal-with-embedded-nul <bugprone/string-literal-with-embedded-nul.html>`_,
    `bugprone-stringview-nullptr <bugprone/stringview-nullptr.html>`_, "Yes"
    `bugprone-suspicious-enum-usage <bugprone/suspicious-enum-usage.html>`_,
    `bugprone-suspicious-include <bugprone/suspicious-include.html>`_,
    `bugprone-suspicious-memory-comparison <bugprone/suspicious-memory-comparison.html>`_,
    `bugprone-suspicious-memset-usage <bugprone/suspicious-memset-usage.html>`_, "Yes"

clang-tools-extra/test/clang-tidy/checkers/bugprone/std-forward-type-mismatch.cpp

+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-std-forward-type-mismatch %t -- -- -fno-delayed-template-parsing
+
+namespace std {
+
+struct false_type {
+  static constexpr bool value = false;
+};
+
+struct true_type {
+  static constexpr bool value = true;
+};
+
+template <class T>
+struct is_lvalue_reference : false_type {};
+
+template <class T>
+struct is_lvalue_reference<T&> : true_type {};
+
+template <class T>
+struct remove_reference {
+  using type = T;
+};
+
+template <class T>
+struct remove_reference<T&> {
+  using type = T;
+};
+
+template <class T>
+struct remove_reference<T&&> {
+  using type = T;
+};
+
+template <class T>
+using remove_reference_t = typename remove_reference<T>::type;
+
+template <class T>
+constexpr T&& forward(typename std::remove_reference<T>::type& t) noexcept {
+  return static_cast<T&&>(t);
+}
+
+template <class T>
+constexpr T&& forward(typename std::remove_reference<T>::type&& t) noexcept {
+  static_assert(!std::is_lvalue_reference<T>::value, "Can't forward an rvalue as an lvalue.");
+  return static_cast<T&&>(t);
+}
+
+template <class T>
+constexpr typename std::remove_reference<T>::type&& move(T&& t) noexcept {
+  return static_cast<typename std::remove_reference<T>::type&&>(t);
+}
+
+}
+
+struct TestType {
+  int value = 0U;
+};
+
+struct TestTypeEx : TestType {
+};
+
+template<typename ...Args>
+void test(Args&&...) {}
+
+
+template<typename T>
+void testCorrectForward(T&& value) {
+  test(std::forward<T>(value));
+}
+
+template<typename ...Args>
+void testCorrectForwardVariadicTemplate(Args&& ...args) {
+  test(std::forward<Args>(args)...);
+}
+
+void testExplicit1(TestTypeEx value) {
+  test(std::forward<TestType>(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'TestTypeEx' to 'TestType' is not recommended here, use 'static_cast' instead
+// CHECK-FIXES: {{^  }}test(static_cast<TestType &&>(value));{{$}}
+}
+
+void testExplicit2(TestTypeEx value) {
+  test(std::forward<TestType&>(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'TestTypeEx' to 'TestType &' is not recommended here, use 'static_cast' instead
+// CHECK-FIXES: {{^  }}test(static_cast<TestType&>(value));{{$}}
+}
+
+void testExplicit3(TestTypeEx value) {
+  test(std::forward<TestType&&>(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'TestTypeEx' to 'TestType &&' is not recommended here, use 'static_cast' instead
+// CHECK-FIXES: {{^  }}test(static_cast<TestType&&>(value));{{$}}
+}
+
+#define FORWARD(x,y) std::forward<x>(y)
+#define FORWARD_FUNC std::forward
+#define TEMPLATE_ARG(x) <x>
+
+void testMacro(TestTypeEx value) {
+  test(FORWARD(TestType, value));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'TestTypeEx' to 'TestType' is not recommended here, use 'static_cast' instead
+  test(FORWARD_FUNC<TestType>(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'TestTypeEx' to 'TestType' is not recommended here, use 'static_cast' instead
+// CHECK-FIXES: {{^  }}test(static_cast<TestType &&>(value));{{$}}
+  test(std::forward TEMPLATE_ARG(TestType)(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'TestTypeEx' to 'TestType' is not recommended here, use 'static_cast' instead
+}
+
+template<typename T>
+struct Wrapper {};
+
+template<typename T>
+struct WrapperEx : Wrapper<T> {};
+
+template<typename T>
+void testPartialTemplateBad(WrapperEx<T> value) {
+  test(std::forward<Wrapper<T>>(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'WrapperEx<T>' to 'Wrapper<T>' is not recommended here, use 'static_cast' instead
+// CHECK-FIXES: {{^  }}test(static_cast<Wrapper<T> &&>(value));{{$}}
+}
+
+template<typename T>
+void testPartialTemplateCorrect(WrapperEx<T> value) {
+  test(std::forward<WrapperEx<T>>(value));
+}
+
+template<typename T1, typename T2>
+void testTemplateBad(T2&& value) {
+  test(std::forward<T1>(value));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'T2' to 'T1' is not recommended here, use 'static_cast' instead
+// CHECK-FIXES: {{^  }}test(static_cast<T1 &&>(value));{{$}}
+}
+
+void callAll() {
+  TestType type1;
+  const TestType type2;
+  testCorrectForward(type1);
+  testCorrectForward(type2);
+  testCorrectForwardVariadicTemplate(type1, type2, TestType());
+  testCorrectForward(TestType());
+  testPartialTemplateBad(WrapperEx<TestType>());
+  testPartialTemplateCorrect(WrapperEx<TestType>());
+  testTemplateBad<TestType>(TestType());
+}
+