diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -52,6 +52,7 @@ #include "SizeofExpressionCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" +#include "StdForwardTypeMismatchCheck.h" #include "StringConstructorCheck.h" #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" @@ -166,6 +167,8 @@ "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( "bugprone-standalone-empty"); + CheckFactories.registerCheck( + "bugprone-std-forward-type-mismatch"); CheckFactories.registerCheck( "bugprone-string-constructor"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -48,6 +48,7 @@ SmartPtrArrayMismatchCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp + StdForwardTypeMismatchCheck.cpp StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/StdForwardTypeMismatchCheck.h b/clang-tools-extra/clang-tidy/bugprone/StdForwardTypeMismatchCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/StdForwardTypeMismatchCheck.h @@ -0,0 +1,33 @@ +//===--- 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 getCheckTraversalKind() const override; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STDFORWARDTYPEMISMATCHCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/StdForwardTypeMismatchCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StdForwardTypeMismatchCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/StdForwardTypeMismatchCheck.cpp @@ -0,0 +1,111 @@ +//===--- 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, + 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(MatchedCallee)) + return SourceRange(DeclRefExprCallee->getLAngleLoc(), + DeclRefExprCallee->getRAngleLoc()); + + if (const auto *UnresolvedLookupExprCallee = + dyn_cast(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 +StdForwardTypeMismatchCheck::getCheckTraversalKind() const { + return TK_IgnoreUnlessSpelledInSource; +} + +void StdForwardTypeMismatchCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedTemplateType = + Result.Nodes.getNodeAs("template"); + const auto *MatchedArgumentType = + Result.Nodes.getNodeAs("argument"); + + const QualType CleanMatchedTemplateType = + MatchedTemplateType->getCanonicalType().getNonReferenceType(); + const QualType CleanMatchedArgumentType = + MatchedArgumentType->getCanonicalType().getNonReferenceType(); + if (CleanMatchedTemplateType == CleanMatchedArgumentType) + return; + + const auto *MatchedExpr = Result.Nodes.getNodeAs("call"); + const auto *MatchedCallee = Result.Nodes.getNodeAs("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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -121,6 +121,12 @@ 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 + ` 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 ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/std-forward-type-mismatch.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/std-forward-type-mismatch.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/std-forward-type-mismatch.rst @@ -0,0 +1,28 @@ +.. 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(base)); // Incorrect usage + doSomething(static_cast(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. + diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -118,6 +118,7 @@ `bugprone-sizeof-expression `_, `bugprone-spuriously-wake-up-functions `_, `bugprone-standalone-empty `_, "Yes" + `bugprone-std-forward-type-mismatch `_, `bugprone-string-constructor `_, "Yes" `bugprone-string-integer-assignment `_, "Yes" `bugprone-string-literal-with-embedded-nul `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/std-forward-type-mismatch.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/std-forward-type-mismatch.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/std-forward-type-mismatch.cpp @@ -0,0 +1,144 @@ +// 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 +struct is_lvalue_reference : false_type {}; + +template +struct is_lvalue_reference : true_type {}; + +template +struct remove_reference { + using type = T; +}; + +template +struct remove_reference { + using type = T; +}; + +template +struct remove_reference { + using type = T; +}; + +template +using remove_reference_t = typename remove_reference::type; + +template +constexpr T&& forward(typename std::remove_reference::type& t) noexcept { + return static_cast(t); +} + +template +constexpr T&& forward(typename std::remove_reference::type&& t) noexcept { + static_assert(!std::is_lvalue_reference::value, "Can't forward an rvalue as an lvalue."); + return static_cast(t); +} + +template +constexpr typename std::remove_reference::type&& move(T&& t) noexcept { + return static_cast::type&&>(t); +} + +} + +struct TestType { + int value = 0U; +}; + +struct TestTypeEx : TestType { +}; + +template +void test(Args&&...) {} + + +template +void testCorrectForward(T&& value) { + test(std::forward(value)); +} + +template +void testCorrectForwardVariadicTemplate(Args&& ...args) { + test(std::forward(args)...); +} + +void testExplicit1(TestTypeEx value) { + test(std::forward(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(value));{{$}} +} + +void testExplicit2(TestTypeEx value) { + test(std::forward(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(value));{{$}} +} + +void testExplicit3(TestTypeEx value) { + test(std::forward(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(value));{{$}} +} + +#define FORWARD(x,y) std::forward(y) +#define FORWARD_FUNC std::forward +#define TEMPLATE_ARG(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(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(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 +struct Wrapper {}; + +template +struct WrapperEx : Wrapper {}; + +template +void testPartialTemplateBad(WrapperEx value) { + test(std::forward>(value)); +// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: using 'std::forward' for type conversions from 'WrapperEx' to 'Wrapper' is not recommended here, use 'static_cast' instead +// CHECK-FIXES: {{^ }}test(static_cast &&>(value));{{$}} +} + +template +void testPartialTemplateCorrect(WrapperEx value) { + test(std::forward>(value)); +} + +template +void testTemplateBad(T2&& value) { + test(std::forward(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(value));{{$}} +} + +void callAll() { + TestType type1; + const TestType type2; + testCorrectForward(type1); + testCorrectForward(type2); + testCorrectForwardVariadicTemplate(type1, type2, TestType()); + testCorrectForward(TestType()); + testPartialTemplateBad(WrapperEx()); + testPartialTemplateCorrect(WrapperEx()); + testTemplateBad(TestType()); +} +