From b1ce6c6d5713b8c39eadcddcd9839a8c61b0bd7b Mon Sep 17 00:00:00 2001 From: Martin Bohme Date: Tue, 30 Aug 2016 12:11:12 +0000 Subject: [PATCH] [clang-tidy] Add check 'misc-move-forwarding-reference' Summary: The check emits a warning if std::move() is applied to a forwarding reference, i.e. an rvalue reference of a function template argument type. If a developer is unaware of the special rules for template argument deduction on forwarding references, it will seem reasonable to apply std::move() to the forwarding reference, in the same way that this would be done for a "normal" rvalue reference. This has a consequence that is usually unwanted and possibly surprising: If the function that takes the forwarding reference as its parameter is called with an lvalue, that lvalue will be moved from (and hence placed into an indeterminate state) even though no std::move() was applied to the lvalue at the callsite. As a fix, the check will suggest replacing the std::move() with a std::forward(). This patch requires D23004 to be submitted before it. Reviewers: sbenza, aaron.ballman Subscribers: klimek, etienneb, alexfh, aaron.ballman, Prazek, Eugene.Zelenko, mgehre, cfe-commits Projects: #clang-tools-extra Differential Revision: https://reviews.llvm.org/D22220 llvm-svn: 280077 --- clang-tools-extra/clang-tidy/misc/CMakeLists.txt | 1 + .../clang-tidy/misc/MiscTidyModule.cpp | 3 + .../misc/MoveForwardingReferenceCheck.cpp | 134 +++++++++++++++++++++ .../clang-tidy/misc/MoveForwardingReferenceCheck.h | 49 ++++++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + clang-tools-extra/docs/clang-tidy/checks/list.rst | 1 + .../checks/misc-move-forwarding-reference.rst | 60 +++++++++ .../clang-tidy/misc-move-forwarding-reference.cpp | 125 +++++++++++++++++++ 8 files changed, 379 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/misc/MoveForwardingReferenceCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/misc/MoveForwardingReferenceCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/misc-move-forwarding-reference.rst create mode 100644 clang-tools-extra/test/clang-tidy/misc-move-forwarding-reference.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 4d6fb2c..7b15c32 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -19,6 +19,7 @@ add_clang_library(clangTidyMiscModule MisplacedWideningCastCheck.cpp MoveConstantArgumentCheck.cpp MoveConstructorInitCheck.cpp + MoveForwardingReferenceCheck.cpp MultipleStatementMacroCheck.cpp NewDeleteOverloadsCheck.cpp NoexceptMoveConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index abaf4b1..ac6e47e 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -27,6 +27,7 @@ #include "MisplacedWideningCastCheck.h" #include "MoveConstantArgumentCheck.h" #include "MoveConstructorInitCheck.h" +#include "MoveForwardingReferenceCheck.h" #include "MultipleStatementMacroCheck.h" #include "NewDeleteOverloadsCheck.h" #include "NoexceptMoveConstructorCheck.h" @@ -92,6 +93,8 @@ public: "misc-move-const-arg"); CheckFactories.registerCheck( "misc-move-constructor-init"); + CheckFactories.registerCheck( + "misc-move-forwarding-reference"); CheckFactories.registerCheck( "misc-multiple-statement-macro"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/misc/MoveForwardingReferenceCheck.cpp b/clang-tools-extra/clang-tidy/misc/MoveForwardingReferenceCheck.cpp new file mode 100644 index 0000000..986edea --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/MoveForwardingReferenceCheck.cpp @@ -0,0 +1,134 @@ +//===--- MoveForwardingReferenceCheck.cpp - clang-tidy --------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "MoveForwardingReferenceCheck.h" +#include "clang/Lex/Lexer.h" +#include "llvm/Support/raw_ostream.h" + +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +static void replaceMoveWithForward(const UnresolvedLookupExpr *Callee, + const ParmVarDecl *ParmVar, + const TemplateTypeParmDecl *TypeParmDecl, + DiagnosticBuilder &Diag, + const ASTContext &Context) { + const SourceManager &SM = Context.getSourceManager(); + const LangOptions &LangOpts = Context.getLangOpts(); + + CharSourceRange CallRange = + Lexer::makeFileCharRange(CharSourceRange::getTokenRange( + Callee->getLocStart(), Callee->getLocEnd()), + SM, LangOpts); + + if (CallRange.isValid()) { + const std::string TypeName = + TypeParmDecl->getIdentifier() + ? TypeParmDecl->getName().str() + : (llvm::Twine("decltype(") + ParmVar->getName() + ")").str(); + + const std::string ForwardName = + (llvm::Twine("forward<") + TypeName + ">").str(); + + // Create a replacement only if we see a "standard" way of calling + // std::move(). This will hopefully prevent erroneous replacements if the + // code does unusual things (e.g. create an alias for std::move() in + // another namespace). + NestedNameSpecifier *NNS = Callee->getQualifier(); + if (!NNS) { + // Called as "move" (i.e. presumably the code had a "using std::move;"). + // We still conservatively put a "std::" in front of the forward because + // we don't know whether the code also had a "using std::forward;". + Diag << FixItHint::CreateReplacement(CallRange, "std::" + ForwardName); + } else if (const NamespaceDecl *Namespace = NNS->getAsNamespace()) { + if (Namespace->getName() == "std") { + if (!NNS->getPrefix()) { + // Called as "std::move". + Diag << FixItHint::CreateReplacement(CallRange, + "std::" + ForwardName); + } else if (NNS->getPrefix()->getKind() == NestedNameSpecifier::Global) { + // Called as "::std::move". + Diag << FixItHint::CreateReplacement(CallRange, + "::std::" + ForwardName); + } + } + } + } +} + +void MoveForwardingReferenceCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus11) + return; + + // Matches a ParmVarDecl for a forwarding reference, i.e. a non-const rvalue + // reference of a function template parameter type. + auto ForwardingReferenceParmMatcher = + parmVarDecl( + hasType(qualType(rValueReferenceType(), + references(templateTypeParmType(hasDeclaration( + templateTypeParmDecl().bind("type-parm-decl")))), + unless(references(qualType(isConstQualified())))))) + .bind("parm-var"); + + Finder->addMatcher( + callExpr(callee(unresolvedLookupExpr( + hasAnyDeclaration(namedDecl( + hasUnderlyingDecl(hasName("::std::move"))))) + .bind("lookup")), + argumentCountIs(1), + hasArgument(0, ignoringParenImpCasts(declRefExpr( + to(ForwardingReferenceParmMatcher))))) + .bind("call-move"), + this); +} + +void MoveForwardingReferenceCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *CallMove = Result.Nodes.getNodeAs("call-move"); + const auto *UnresolvedLookup = + Result.Nodes.getNodeAs("lookup"); + const auto *ParmVar = Result.Nodes.getNodeAs("parm-var"); + const auto *TypeParmDecl = + Result.Nodes.getNodeAs("type-parm-decl"); + + // Get the FunctionDecl and FunctionTemplateDecl containing the function + // parameter. + const FunctionDecl *FuncForParam = + dyn_cast(ParmVar->getDeclContext()); + if (!FuncForParam) + return; + const FunctionTemplateDecl *FuncTemplate = + FuncForParam->getDescribedFunctionTemplate(); + if (!FuncTemplate) + return; + + // Check that the template type parameter belongs to the same function + // template as the function parameter of that type. (This implies that type + // deduction will happen on the type.) + const TemplateParameterList *Params = FuncTemplate->getTemplateParameters(); + if (!std::count(Params->begin(), Params->end(), TypeParmDecl)) + return; + + auto Diag = diag(CallMove->getExprLoc(), + "forwarding reference passed to std::move(), which may " + "unexpectedly cause lvalues to be moved; use " + "std::forward() instead"); + + replaceMoveWithForward(UnresolvedLookup, ParmVar, TypeParmDecl, Diag, + *Result.Context); +} + +} // namespace misc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/misc/MoveForwardingReferenceCheck.h b/clang-tools-extra/clang-tidy/misc/MoveForwardingReferenceCheck.h new file mode 100644 index 0000000..2e6ec36 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/MoveForwardingReferenceCheck.h @@ -0,0 +1,49 @@ +//===--- MoveForwardingReferenceCheck.h - clang-tidy ----------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVEFORWARDINGREFERENCECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVEFORWARDINGREFERENCECHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// The check warns if std::move is applied to a forwarding reference (i.e. an +/// rvalue reference of a function template argument type). +/// +/// If a developer is unaware of the special rules for template argument +/// deduction on forwarding references, it will seem reasonable to apply +/// std::move to the forwarding reference, in the same way that this would be +/// done for a "normal" rvalue reference. +/// +/// This has a consequence that is usually unwanted and possibly surprising: if +/// the function that takes the forwarding reference as its parameter is called +/// with an lvalue, that lvalue will be moved from (and hence placed into an +/// indeterminate state) even though no std::move was applied to the lvalue at +/// the call site. +// +/// The check suggests replacing the std::move with a std::forward. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html +class MoveForwardingReferenceCheck : public ClangTidyCheck { +public: + MoveForwardingReferenceCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVEFORWARDINGREFERENCECHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 51b8de7..72a183b 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -69,6 +69,12 @@ Improvements to clang-tidy Flags classes where some, but not all, special member functions are user-defined. +- New `misc-move-forwarding-reference + `_ check + + Warns when `std::move` is applied to a forwarding reference instead of + `std::forward`. + - New `mpi-buffer-deref `_ check diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 80ad81a..e6ba8ba 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -68,6 +68,7 @@ Clang-Tidy Checks misc-misplaced-widening-cast misc-move-const-arg misc-move-constructor-init + misc-move-forwarding-reference misc-multiple-statement-macro misc-new-delete-overloads misc-noexcept-move-constructor diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-move-forwarding-reference.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-move-forwarding-reference.rst new file mode 100644 index 0000000..327f720 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-move-forwarding-reference.rst @@ -0,0 +1,60 @@ +.. title:: clang-tidy - misc-move-forwarding-reference + +misc-move-forwarding-reference +============================== + +Warns if ``std::move`` is called on a forwarding reference, for example: + + .. code-block:: c++ + + template + void foo(T&& t) { + bar(std::move(t)); + } + +`Forwarding references +`_ should +typically be passed to ``std::forward`` instead of ``std::move``, and this is +the fix that will be suggested. + +(A forwarding reference is an rvalue reference of a type that is a deduced +function template argument.) + +In this example, the suggested fix would be + + .. code-block:: c++ + + bar(std::forward(t)); + +Background +---------- + +Code like the example above is sometimes written with the expectation that +``T&&`` will always end up being an rvalue reference, no matter what type is +deduced for ``T``, and that it is therefore not possible to pass an lvalue to +``foo()``. However, this is not true. Consider this example: + + .. code-block:: c++ + + std::string s = "Hello, world"; + foo(s); + +This code compiles and, after the call to ``foo()``, ``s`` is left in an +indeterminate state because it has been moved from. This may be surprising to +the caller of ``foo()`` because no ``std::move`` was used when calling +``foo()``. + +The reason for this behavior lies in the special rule for template argument +deduction on function templates like ``foo()`` -- i.e. on function templates +that take an rvalue reference argument of a type that is a deduced function +template argument. (See section [temp.deduct.call]/3 in the C++11 standard.) + +If ``foo()`` is called on an lvalue (as in the example above), then ``T`` is +deduced to be an lvalue reference. In the example, ``T`` is deduced to be +``std::string &``. The type of the argument ``t`` therefore becomes +``std::string& &&``; by the reference collapsing rules, this collapses to +``std::string&``. + +This means that the ``foo(s)`` call passes ``s`` as an lvalue reference, and +``foo()`` ends up moving ``s`` and thereby placing it into an indeterminate +state. diff --git a/clang-tools-extra/test/clang-tidy/misc-move-forwarding-reference.cpp b/clang-tools-extra/test/clang-tidy/misc-move-forwarding-reference.cpp new file mode 100644 index 0000000..6a20fe6 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-move-forwarding-reference.cpp @@ -0,0 +1,125 @@ +// RUN: %check_clang_tidy %s misc-move-forwarding-reference %t -- -- -std=c++14 + +namespace std { +template struct remove_reference; + +template struct remove_reference { typedef _Tp type; }; + +template struct remove_reference<_Tp &> { typedef _Tp type; }; + +template struct remove_reference<_Tp &&> { typedef _Tp type; }; + +template +constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t); + +} // namespace std + +// Standard case. +template void f1(U &&SomeU) { + T SomeT(std::move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(std::forward(SomeU)); +} + +// Ignore parentheses around the argument to std::move(). +template void f2(U &&SomeU) { + T SomeT(std::move((SomeU))); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(std::forward((SomeU))); +} + +// Handle the case correctly where std::move() is being used through a using +// declaration. +template void f3(U &&SomeU) { + using std::move; + T SomeT(move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(std::forward(SomeU)); +} + +// Handle the case correctly where a global specifier is prepended to +// std::move(). +template void f4(U &&SomeU) { + T SomeT(::std::move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(::std::forward(SomeU)); +} + +// Create a correct fix if there are spaces around the scope resolution +// operator. +template void f5(U &&SomeU) { + { + T SomeT(:: std :: move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(::std::forward(SomeU)); + } + { + T SomeT(std :: move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(std::forward(SomeU)); + } +} + +// Ignore const rvalue reference parameters. +template void f6(const U &&SomeU) { + T SomeT(std::move(SomeU)); +} + +// Ignore the case where the argument to std::move() is a lambda parameter (and +// thus not actually a parameter of the function template). +template void f7() { + [](U &&SomeU) { T SomeT(std::move(SomeU)); }; +} + +// Ignore the case where the argument is a lvalue reference. +template void f8(U &SomeU) { + T SomeT(std::move(SomeU)); +} + +// Ignore the case where the template parameter is a class template parameter +// (i.e. no template argument deduction is taking place). +template class SomeClass { + void f(U &&SomeU) { T SomeT(std::move(SomeU)); } +}; + +// Ignore the case where the function parameter in the template isn't an rvalue +// reference but the template argument is explicitly set to be an rvalue +// reference. +class A {}; +template void foo(T); +void f8() { + A a; + foo(std::move(a)); +} + +// A warning is output, but no fix is suggested, if a macro is used to rename +// std::move. +#define MOVE(x) std::move((x)) +template void f9(U &&SomeU) { + T SomeT(MOVE(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to +} + +// Same result if the argument is passed outside of the macro. +#undef MOVE +#define MOVE std::move +template void f10(U &&SomeU) { + T SomeT(MOVE(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to +} + +// Same result if the macro does not include the "std" namespace. +#undef MOVE +#define MOVE move +template void f11(U &&SomeU) { + T SomeT(std::MOVE(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to +} + +// Handle the case correctly where the forwarding reference is a parameter of a +// generic lambda. +template void f12() { + [] (auto&& x) { T SomeT(std::move(x)); }; + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference passed to + // CHECK-FIXES: [] (auto&& x) { T SomeT(std::forward(x)); } +} -- 2.7.4