[clang-tidy] Add check 'misc-move-forwarding-reference'

[mboehme]

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.

[mgehre]

Nice check. This should be mentioned in docs/ReleaseNotes.rst

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[Prazek]

Very usefull check!
I don't have enough time right now to check everything, so better wait for review of Alexfh or someone else. I just wanted to leave some thoughts.

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

34 ↗

(On Diff #63513)

you could probably use llvm::StringRef here, but I am not sure about it - ask Alex.

92 ↗

(On Diff #63513)

maybe use ignoringImplicit or if you won't need that use ignoringParenImpCasts (or something similar).

I am not sure if it is needed it, but it might help in some cases.

test/clang-tidy/misc-move-forwarding-reference.cpp

68 ↗

(On Diff #63513)

Please add some tests that checks if fixes doesn't happen inside macro.

[Prazek]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

71 ↗

(On Diff #63513)

Use CPlusPlus11 here

clang-tidy/misc/MoveForwardingReferenceCheck.h

19 ↗

(On Diff #63513)

Please add doc comment.

docs/clang-tidy/checks/misc-move-forwarding-reference.rst

29 ↗

(On Diff #63513)

Very nice section! good idea.

So I have a thoughts about multiple sections in documentation (which is not a issue for you).
I think the check lists doc should not include sections - it doesn't look good and it also prevents people from using sections in docs.

[mboehme]

Addressed Prazek's review comments.

[mboehme]

In D22220#480415, @mgehre wrote:

Nice check. This should be mentioned in docs/ReleaseNotes.rst

Done.

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

34 ↗

(On Diff #63513)

I've talked to Alex -- using a Twine to avoid multiple allocations is the best I can do here.

docs/clang-tidy/checks/misc-move-forwarding-reference.rst

29 ↗

(On Diff #63513)

Do I understand correctly that you're not expecting me to change anything here?

[Prazek]

docs/clang-tidy/checks/misc-move-forwarding-reference.rst

30 ↗

(On Diff #63663)

Nothing from me. It's just general comment

[aaron.ballman]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

20 ↗

(On Diff #63663)

Might as well pass Context by const reference rather than pointer.

43–53 ↗

(On Diff #63663)

I'm not certain I understand the benefit to this. We know it's a call to std::move() from the standard namespace already, so why do we care about the original text? I think it's reasonable to replace any call to move() from the standard namespace with ::std::forward(), so we should be able to remove the if statements and not have to go read the original source text from the source manager (which could involve, for instance, a query for that text over a slow network).

92 ↗

(On Diff #63663)

It might be a bit more clear if you made isStdMove() into a local AST matcher and called it from here.

119 ↗

(On Diff #63663)

Is there a reason why you can't just use CallMove->getExprLoc() instead of toying with character ranges?

[alexfh]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

36 ↗

(On Diff #63663)

nit: The second llvm::Twine is not necessary.

43–53 ↗

(On Diff #63663)

I think, the value of this is to maintain consistency with the existing code in terms of whether the std namespace should be globally qualified or not. Changing std::move to ::std::forward would sometimes be unwelcome, if the code doesn't use ::std otherwise.

60 ↗

(On Diff #63663)

The variable name is rather uncommon for LLVM/Clang code (in particular, the The prefix). Maybe FuncTemplateDecl? Equally clear and somewhat shorter.

119 ↗

(On Diff #63663)

I think, makeFileCharRange is needed. It helps finding the correct macro expansion level to insert the replacement on. In case the range is not on the same macro expansion level, the condition on the line 116 will stop the check from introducing changes to the code that are more likely to break it.

The only change I would suggest is to only disable the fix-it and not the whole diagnostic in case unsupported macro usage is detected.

[etienneb]

thx for the check

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

25 ↗

(On Diff #63663)

nit: reference?

43–53 ↗

(On Diff #63663)

I agree with alex, it's better to keep code consistency (programmer intend).
But, at the same time, the check should be bomb proof for ugly cases like:

"std:: move"
 ":: std    ::    move",

For the moment, the code seems to propose a fix only when it's a known case,
and a warning is emitted in every cases.

92 ↗

(On Diff #63663)

+1 I agree with Aaron here.

The match will also be more precise.

[sbenza]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

20 ↗

(On Diff #63663)

Function names start in lower case.

30 ↗

(On Diff #63663)

Isn't this just CharSourceRange::getTokenRange(Callee->getLocStart(),Callee->getLocEnd()) ?

83 ↗

(On Diff #63663)

Will this trigger on code like this:

template <typename T>
void F(T);

void G() { F<string&&>(std::move(str)); }

[mboehme]

Changes in response to reviewer comments

[mboehme]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

20 ↗

(On Diff #63663)

Sorry -- still getting used to LLVM naming conventions.

30 ↗

(On Diff #63663)

Thanks -- done.

43–53 ↗

(On Diff #63663)

why do we care about the original text?

See alexfh's comment -- my intent is to perserve the existing style in terms of whether "std" is globally qualified or not.

83 ↗

(On Diff #63663)

No, it won't -- I've added a test for this.

The reason it doesn't trigger is because the check only looks at the template itself, but not any instantiations (this is because the main matcher below matches for UnresolvedLookupExpr, which only exists in the template).

92 ↗

(On Diff #63663)

That was my original plan, but there doesn't seem to be a way to do this with an AST matcher, and I'm not sure whether it's desired that this should change or what would be the best way to change it.

The main issue is that the decls() contained in a UnresolvedLookupExpr aren't children of the UnresolvedLookupExpr, and RecursiveASTVisitor doesn't contain any special-case logic to visit them either. (A consequence of this is that they don't show up when dumping the AST.) This means that it isn't possible to match for them using unresolvedLookupExpr(has(...)), for example.

This is the reason I went with an explicit test inside the check(). But if you have a suggestion on how this could be changed into an AST matcher in a way that's consistent with the overall design of AST matchers, I'd be open to that!

119 ↗

(On Diff #63663)

Multiple changes here:

• Following alexfh's suggestion, I only disable the fix-it and not the whole diagnostic for the macro constructs in question (see corresponding change in the test).
• It turns out that once I do this, the isValid() check in replaceMoveWithForward() already does the job of avoiding making invalid replacements on macros, so I can do without it here and indeed simply use CallMove->getExprLoc(). (I've also added another macro test case for additional coverage.)

test/clang-tidy/misc-move-forwarding-reference.cpp

18 ↗

(On Diff #65484)

Added numbering for consistency.

[aaron.ballman]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

44–54 ↗

(On Diff #65484)

I kind of thought that was going to be the case, but wanted to double-check. I agree with the notion, but disagree with the approach -- as @etienneb mentions, this code is not resilient enough to actually do the right thing. I don't think we should be going back to the original source text, but should instead be using the information the AST already tracks. This will perform better and do the right thing in all cases.

93 ↗

(On Diff #65484)

I wonder if there's a reason for this behavior, or if it's simple a matter of not being needed previously and so it was never implemented. @sbenza or @klimek may know. I think we should be fixing the RecursiveASTVisitor, unless there is a valid reason not to (which there may be), though that would be a separate patch (and can happen after we land this one).

[sbenza]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

93 ↗

(On Diff #65484)

Even if the nodes are not visited through the recursive visitor, you can always have a matcher for it.
Eg: hasAnyConstructorInitializer / cxxCtorInitializer.

But what node are you trying to visit here?
The only node I see is NamingClass, which is not really a child node.
Like the referred Decl in a DeclRefExpr is not a child either. You can't use has() there, you have to use to().

[mboehme]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

93 ↗

(On Diff #65484)

I've now reworked this to use new matchers (in review in D23004). Thanks for the comments, which finally got me on the right track!

But what node are you trying to visit here?

I'm trying to visit the decls() in an OverloadExpr (this happened in isStdMove(), which is now deleted).

As you note, these intentionally aren't children of the OverloadExpr (so it doesn't make sense for the RecursiveASTMatcher to visit them), but it still makes sense to have a matchers for them -- which I've now added.

45–55 ↗

(On Diff #66284)

I've changed the code to look at the NestedNameSpecifiers instead of looking at the original source text.

But, at the same time, the check should be bomb proof for ugly cases like [snip]

Apologies, I misunderstood -- I thought you were saying it should be bomb proof in the sense that it shouldn't suggest any replacement (and that's why I marked your comment done). Having read aaron.ballman's comment, I now realize what you meant is that the check should be bomb proof in the sense that it should create a correct fix even for such "ugly" cases.

[mboehme]

Renamed canReferToDecl to hasAnyDeclaration.

This is for consistency with the corresponding change in D23004.

[mboehme]

Now that D23004 is submitted, can you take another look?

[sbenza]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

64 ↗

(On Diff #66914)

I'm guessing this is checking Lang==C++11, but we actually want Lang>=C++11, right?
I'm not sure if there is a way to check that.

test/clang-tidy/misc-move-forwarding-reference.cpp

49 ↗

(On Diff #66914)

I don't understand what this is trying to test.
What is the overload resolution operator?
Also, these two cases look exactly the same as the previous two.

117 ↗

(On Diff #66914)

This is missing one case for C++14's generic lambdas.

[&] (auto&& x) { y = std::move(x); }

It might already detect correctly, but it will need a special case for generating the std::forward
It might need to be something like:

[&] (auto&& x) { y = std::forward<decltype(x)>(x); }

We can also just leave a TODO for later.

[aaron.ballman]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

64 ↗

(On Diff #66914)

CPlusPlus11 will be true for C++ >= 11.

[mboehme]

Handle case where the forwarding reference is a parameter of a generic lambda.

[mboehme]

Fix typo in comment

[mboehme]

Restore spaces around scope resolution operator in test case

[mboehme]

test/clang-tidy/misc-move-forwarding-reference.cpp

50 ↗

(On Diff #67830)

Sorry, that should have been "scope resolution operator". Comment changed.

What this is trying to test is that the replacements still happen as desired even if there are spaces around the scope resolution operator. Unfortunately, I inadvertently deleted those spaces when I clang-formatted this entire file... I've now restored the spaces.

118 ↗

(On Diff #67830)

Good point. Done.

As it turns out, the detection didn't yet work quite correctly. This was due to the special format of LambdaExprs for generic lambdas. In particular, what tripped my test up is that the same TemplateTypeParmDecl is contained in two different FunctionTemplateDecls -- one for operator() and one for __invoke.

As a result, I've needed to rewrite the detection to do a little more work outside of the matcher. (There was no way to do it inside the matcher as the FunctionTemplateDecl for operator() is actually not visited by the RecursiveASTVisitor.)

[aaron.ballman]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

46–56 ↗

(On Diff #67830)

This code can be combined with the Global check below with something like:

Diag << ..., Twine(NNS->getPrefix()->getKind() == NestedNameSpecifier::Global ? "::" : "") + "std::" + ForwardName);

124–125 ↗

(On Diff #67830)

Given that the user is possibly in a confused state when they wrote std::move() rather than std::forward(), I wonder if it makes more sense to word the diagnostic as an imperative rather than a question? The diagnostic as it is doesn't really explain why std::forward() would be right while std::move() could be wrong.

Perhaps: "forwarding reference passed to std::move() may unexpectedly leave lvalue references in an indeterminate state; use std::forward() instead for perfect forwarding"

clang-tidy/misc/MoveForwardingReferenceCheck.h

27 ↗

(On Diff #67830)

"If" should not be capitalized here.

docs/ReleaseNotes.rst

78 ↗

(On Diff #67830)

Uncertain whether we're doing this or not, but should we keep this list alphabetized?

docs/clang-tidy/checks/misc-move-forwarding-reference.rst

32 ↗

(On Diff #67830)

"is often written in the" -> "is sometimes written with the"

[mboehme]

Reponses to reviewer comments

[mboehme]

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

46–56 ↗

(On Diff #67830)

It could -- but note that there is a third case that also needs to be handled here, namely that NNS has a prefix, but it's something other than "::". In other words, the code would need to go something like this:

if (!NNS->getPrefix() || NNS->getPrefix()->getKind() == NestedNameSpecifier::Global) {
  Diag << ..., Twine(NNS->getPrefix() ? "::" : "") + "std::" + ForwardName);
}

This repeats part of the check, and I feel it's more opaque than explicitly spelling out the two cases I'm interested in, at the cost of a little duplication.

124–125 ↗

(On Diff #67830)

I wonder if it makes more sense to word the diagnostic as an imperative rather than a question?

Good point -- done.

I've reworded your suggestion a bit:

• I've phrased this simply as "may unexpectedly cause lvalues to be moved". That's shorter and, I'm hoping, more accessible than talking about "indeterminate state".

• I've left out the reference to "perfect forwarding". If someone triggers this warning, chances are they won't know what it is anyway. They still have everything they need to find out about perfect forwarding; if they do a search for std::forward(), they should find relevant material.

What do you think?

docs/ReleaseNotes.rst

78 ↗

(On Diff #67830)

I think it's a good idea -- done!

[mboehme]

Any more comments here?

[aaron.ballman]

One minor typo fix, but otherwise LGTM. Thank you for the check!

clang-tidy/misc/MoveForwardingReferenceCheck.cpp

125–126 ↗

(On Diff #68322)

Anyone understanding what an lvalue is will probably also understand indeterminate state. ;-) However, I think the new wording is fine.

clang-tidy/misc/MoveForwardingReferenceCheck.h

31 ↗

(On Diff #68322)

s/callsite/call site

[mboehme]

Fix typo

[mboehme]

Update to current head

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MisplacedWideningCastCheck.cpp
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
+  MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   PointerAndIntegralOperationCheck.cpp
   RedundantExpressionCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
+#include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "PointerAndIntegralOperationCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "misc-macro-repeated-side-effects");
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "misc-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveConstantArgumentCheck>(
         "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>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");

clang-tools-extra/trunk/clang-tidy/misc/MoveForwardingReferenceCheck.h

+//===--- 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

clang-tools-extra/trunk/clang-tidy/misc/MoveForwardingReferenceCheck.cpp

+//===--- 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

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `cppcoreguidelines-slicing
 
   Flags slicing of member variables or vtable.
 
 - New `cppcoreguidelines-special-member-functions
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html>`_ check
 
   Flags classes where some, but not all, special member functions are user-defined.
 
+- New `misc-move-forwarding-reference
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html>`_ check
+
+  Warns when `std::move` is applied to a forwarding reference instead of
+  `std::forward`.
+
 - New `mpi-buffer-deref
   <http://clang.llvm.org/extra/clang-tidy/checks/mpi-buffer-deref.html>`_ check
 
   Flags buffers which are insufficiently dereferenced when passed to an MPI function call.
 
 - New `mpi-type-mismatch
   <http://clang.llvm.org/extra/clang-tidy/checks/mpi-type-mismatch.html>`_ check
 

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

.. toctree::
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-const
    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
    misc-non-copyable-objects
    misc-pointer-and-integral-operation
    misc-redundant-expression
    misc-sizeof-container
    misc-sizeof-expression

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-forwarding-reference.rst

+.. 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 <typename T>
+    void foo(T&& t) {
+      bar(std::move(t));
+    }
+
+`Forwarding references
+<http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4164.pdf>`_ 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>(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.

clang-tools-extra/trunk/test/clang-tidy/misc-move-forwarding-reference.cpp

+// RUN: %check_clang_tidy %s misc-move-forwarding-reference %t -- -- -std=c++14
+
+namespace std {
+template <typename> struct remove_reference;
+
+template <typename _Tp> struct remove_reference { typedef _Tp type; };
+
+template <typename _Tp> struct remove_reference<_Tp &> { typedef _Tp type; };
+
+template <typename _Tp> struct remove_reference<_Tp &&> { typedef _Tp type; };
+
+template <typename _Tp>
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t);
+
+} // namespace std
+
+// Standard case.
+template <typename T, typename U> 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<U>(SomeU));
+}
+
+// Ignore parentheses around the argument to std::move().
+template <typename T, typename U> 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<U>((SomeU)));
+}
+
+// Handle the case correctly where std::move() is being used through a using
+// declaration.
+template <typename T, typename U> 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<U>(SomeU));
+}
+
+// Handle the case correctly where a global specifier is prepended to
+// std::move().
+template <typename T, typename U> 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<U>(SomeU));
+}
+
+// Create a correct fix if there are spaces around the scope resolution
+// operator.
+template <typename T, typename U> 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<U>(SomeU));
+  }
+  {
+    T SomeT(std  ::  move(SomeU));
+    // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: forwarding reference passed to
+    // CHECK-FIXES: T SomeT(std::forward<U>(SomeU));
+  }
+}
+
+// Ignore const rvalue reference parameters.
+template <typename T, typename U> 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 <typename T, typename U> void f7() {
+  [](U &&SomeU) { T SomeT(std::move(SomeU)); };
+}
+
+// Ignore the case where the argument is a lvalue reference.
+template <typename T, typename U> 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 <typename T, typename U> 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 <typename T> void foo(T);
+void f8() {
+  A a;
+  foo<A &&>(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 <typename T, typename U> 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 <typename T, typename U> 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 <typename T, typename U> 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 <typename T> 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<decltype(x)>(x)); }
+}