diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -12,6 +12,7 @@ AvoidNonConstGlobalVariablesCheck.cpp AvoidReferenceCoroutineParametersCheck.cpp CppCoreGuidelinesTidyModule.cpp + ForwardNonForwardingParameterCheck.cpp InitVariablesCheck.cpp InterfacesGlobalInitCheck.cpp MacroUsageCheck.cpp diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -21,6 +21,7 @@ #include "AvoidGotoCheck.h" #include "AvoidNonConstGlobalVariablesCheck.h" #include "AvoidReferenceCoroutineParametersCheck.h" +#include "ForwardNonForwardingParameterCheck.h" #include "InitVariablesCheck.h" #include "InterfacesGlobalInitCheck.h" #include "MacroUsageCheck.h" @@ -70,6 +71,8 @@ "cppcoreguidelines-avoid-reference-coroutine-parameters"); CheckFactories.registerCheck( "cppcoreguidelines-explicit-virtual-functions"); + CheckFactories.registerCheck( + "cppcoreguidelines-forward-non-forwarding-parameter"); CheckFactories.registerCheck( "cppcoreguidelines-init-variables"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.h @@ -0,0 +1,32 @@ +//===--- 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 diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.cpp @@ -0,0 +1,115 @@ +//===--- 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, InnerMatcher) { + return InnerMatcher.matches(Node.getNonPackExpansionType(), Finder, Builder); +} + +AST_MATCHER(ParmVarDecl, isForwardingRefTypeOfFunction) { + ast_matchers::internal::Matcher 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(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(); + if (!TemplateType) + return false; + + return TemplateType->getDepth() == + FuncTemplate->getTemplateParameters()->getDepth(); +} + +AST_MATCHER(ParmVarDecl, isInInstantiation) { + const auto *Function = dyn_cast(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("call"); + const auto *Ref = Result.Nodes.getNodeAs("ref"); + const auto *Var = Result.Nodes.getNodeAs("var"); + const auto *RvalueRefVar = Result.Nodes.getNodeAs("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 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 @@ -133,6 +133,11 @@ 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 + ` check. + + Warns when ``std::forward`` is used on a non-forwarding reference. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.rst @@ -0,0 +1,35 @@ +.. 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(Input)); + } + + void move_param(Object&& Input) { + Object Copy(std::move(Input)); // Ok + Object Copy2(std::forward(Input)); // Use std::move instead + } + +This check implements the ``std::forward`` specific enforcements of +`CppCoreGuideline ES.56 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 @@ -186,6 +186,7 @@ `cppcoreguidelines-avoid-goto `_, `cppcoreguidelines-avoid-non-const-global-variables `_, `cppcoreguidelines-avoid-reference-coroutine-parameters `_, + `cppcoreguidelines-forward-non-forwarding-parameter `_, "Yes" `cppcoreguidelines-init-variables `_, "Yes" `cppcoreguidelines-interfaces-global-init `_, `cppcoreguidelines-macro-usage `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forward-non-forwarding-parameter.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forward-non-forwarding-parameter.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forward-non-forwarding-parameter.cpp @@ -0,0 +1,163 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-forward-non-forwarding-parameter %t + +// NOLINTBEGIN +namespace std { + +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(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; + +} // namespace std +// NOLINTEND + +struct Obj { + Obj(); + Obj(const Obj&); + Obj(Obj&&) noexcept; + Obj& operator=(const Obj&); + Obj& operator=(Obj&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +void forward_local_object() { + Obj obj; + Obj& obj_ref = obj; + + Obj obj2 = std::forward(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_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); + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter] +} + +template +void forward_pack_of_values(Ts... ts) { + consumes_all(std::forward(ts)...); + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: calling std::forward on non-forwarding reference 'ts' [cppcoreguidelines-forward-non-forwarding-parameter] +} + +template +void forward_pack_of_lvalue_refs(Ts&... ts) { + consumes_all(std::forward(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); + // 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(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); + // 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(obj); + // CHECK-MESSAGES: :[[@LINE-1]]:38: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter] +} + +template +void forward_value_t(T t) { + T other = std::forward(t); + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter] +} + +template +void forward_lvalue_ref_t(T& t) { + T other = std::forward(t); + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter] +} + +template +void forward_const_rvalue_ref_t(const T&& t) { + T other = std::forward(t); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter] +} + +template +struct SomeClass +{ + void forwards_lvalue_ref(T& t) { + T other = std::forward(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); + // 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 +void forwards_param(T&& t) { + T other = std::forward(t); +} + +template +void forwards_param_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +template +struct SomeClass { + template + void forwards_param(T&& t) { + T other = std::forward(t); + } + + template + void forwards_param(T&& t) { + T other = std::forward(t); + } + + template + void forwards_param_pack(Ts&&... ts) { + consumes_all(1, std::forward(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 someObj; + someObj.forwards_param(0); + someObj.forwards_param(Obj{}); + someObj.forwards_param(o); + someObj.forwards_param_pack(0, Obj{}, o); +} + +} // namespace negative_cases