[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
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 @@
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 @@
"misc-move-const-arg");
CheckFactories.registerCheck<MoveConstructorInitCheck>(
"misc-move-constructor-init");
+ CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
+ "misc-move-forwarding-reference");
CheckFactories.registerCheck<MultipleStatementMacroCheck>(
"misc-multiple-statement-macro");
CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
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 <algorithm>
+
+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<CallExpr>("call-move");
+ const auto *UnresolvedLookup =
+ Result.Nodes.getNodeAs<UnresolvedLookupExpr>("lookup");
+ const auto *ParmVar = Result.Nodes.getNodeAs<ParmVarDecl>("parm-var");
+ const auto *TypeParmDecl =
+ Result.Nodes.getNodeAs<TemplateTypeParmDecl>("type-parm-decl");
+
+ // Get the FunctionDecl and FunctionTemplateDecl containing the function
+ // parameter.
+ const FunctionDecl *FuncForParam =
+ dyn_cast<FunctionDecl>(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