[clang-tidy] misc-move-const-arg: Detect if result of std::move() is being passed as a const ref argument

[mboehme]

Summary

Conceptually, this is very close to the existing functionality of misc-move-const-arg, which is why I'm adding it here and not creating a new check. For example, for a type A that is both movable and copyable, this

const A a1;
A a2(std::move(a1));

is not only a case where a const argument is being passed to std::move(), but the result of std::move() is also being passed as a const reference (due to overload resolution).

The new check typically triggers (exclusively) in cases where people think they're dealing with a movable type, but in fact the type is not movable.

[aaron.ballman]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

20

Instead of an unnamed namespace, you should use static functions per our coding standards (http://llvm.org/docs/CodingStandards.html#anonymous-namespaces).

22

Should pass Diag by reference rather than by pointer.

26

Should only use auto when the type is explicitly spelled out in the initialization (which this is not). Same elsewhere.

[alexfh]

Thank you for working on this! A few more comments in addition to what Aaron has written.

clang-tidy/misc/MoveConstantArgumentCheck.cpp

22

I'd prefer the parameter to be named Call instead of TheCallExpr. Would make it more consistent with the rest of clang-tidy code.

37

Alternatively, this function could return a llvm::SmallVector<FixItHint, 2> (either empty or with the fixes) and the caller could just feed it to the result of diag(). Not much difference though, just pointing to this possibility.

42

With two lines of implementation and just one caller, this function doesn't seem to add any value.

116

Is this equivalent to just ignoring all cases where CallMove->getLocStart().isMacroID()? If not, what's the subset of the cases that would be ignored by isMacroID(), but not with the current condition?

[mboehme]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

37

Thanks for the suggestion! If it's OK with you, though, I'd like to keep this as-is -- I feel it makes the call-site clearer.

42

I've inlined the function. (I think I originally had two places where it got called, so I factored it out, but then one of those places went away.)

116

Actually, having looked into this some more, it seems that the macro cases are already handled by the check "if (!FileMoveRange.isValid())" above -- so I'm deleting this entire block of code.

(I think I added the isMacroBodyExpansion() and isMacroArgExpansion() checks first before I moved the FileMoveRange.isValid() check, which then made them unnecessary.)

[aaron.ballman]

test/clang-tidy/misc-move-const-arg.cpp

75–76

This type isn't non-moveable. For that, you need to explicitly delete the move constructor. Perhaps a better name is NonMoveConstructible?

Also, no need for the public access specifier.

116

No need for the public access specifier.

[mboehme]

test/clang-tidy/misc-move-const-arg.cpp

75–76

This type isn't non-moveable. For that, you need to explicitly delete the move constructor.

Can you expand on this?

The standard says: "If the definition of a class X does not explicitly declare a move constructor, one will be implicitly declared as defaulted if and only if - X does not have a user-declared copy constructor [...]" (12.8/9).

Because I'm declaring a copy constructor, I would thus have expected not to get an implicitly-declared move constructor. Where am I going wrong here?

Perhaps a better name is NonMoveConstructible?

But the type _is_ move-constructible (in the sense of std::is_move_constructible<>).

Also, no need for the public access specifier.

Oops -- thanks for catching this.

Actually, I meant to make this a class -- not sure how the "struct" slipped in there.

116

Thanks for cathching this -- as above, I meant to make this a class.

[aaron.ballman]

test/clang-tidy/misc-move-const-arg.cpp

75–76

Can you expand on this?

The standard says: "If the definition of a class X does not explicitly declare a move constructor, one will be implicitly declared as defaulted if and only if - X does not have a user-declared copy constructor [...]" (12.8/9).

Because I'm declaring a copy constructor, I would thus have expected not to get an implicitly-declared move constructor. Where am I going wrong here?

Move operations are optimized versions of a copy operation, so a failed move operation can fall back to perform a copy operation instead and achieve the same operational semantics but with slightly worse performance. Because you have a copy constructor, you do not get an implicitly-declared move constructor (so that's correct), but that doesn't mean the type cannot be used in a context where the user expects a move (i.e., calling std::move() on it) -- the operation just falls back on the copy constructor.

Basically, there's an operational difference between not having the implicitly-declared move constructor and having a deleted move constructor. In the former, fallback to copy happens and in the latter you get a diagnostic. So when I hear "non-moveable type", my brain assumes you mean "get a diagnostic when you try to move."

[mboehme]

test/clang-tidy/misc-move-const-arg.cpp

75–76

Ah, now I see what you mean.

I've renamed "NonMoveable" to "NoMoveSemantics" (and "Moveable" to "MoveSemantics" for consistency). What do you think?

(I don't like NonMoveConstructible because it's prone to the same ambiguity -- or even more so, because std::is_move_constructible<> uses exactly the same term and the type is move constructible in the sense of that trait.)

[aaron.ballman]

LGTM, but you should wait for @alexfh to sign off as well since he had some comments.

[alexfh]

LG. Thanks!

[alexfh]

BTW, I'll commit the patch for you.

clang-tidy/misc/MoveConstantArgumentCheck.cpp

 namespace tidy {
 namespace misc {
 
+static void ReplaceCallWithArg(const CallExpr *Call, DiagnosticBuilder &Diag,
+                               const SourceManager &SM,
+                               const LangOptions &LangOpts) {
+  const Expr *Arg = Call->getArg(0);
+
+  CharSourceRange BeforeArgumentsRange = Lexer::makeFileCharRange(
+      CharSourceRange::getCharRange(Call->getLocStart(), Arg->getLocStart()),
+      SM, LangOpts);
+  CharSourceRange AfterArgumentsRange = Lexer::makeFileCharRange(
+      CharSourceRange::getCharRange(Call->getLocEnd(),
+                                    Call->getLocEnd().getLocWithOffset(1)),
+      SM, LangOpts);
+
+  if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) {
+    Diag << FixItHint::CreateRemoval(BeforeArgumentsRange)
+         << FixItHint::CreateRemoval(AfterArgumentsRange);
+  }
+}
+
 void MoveConstantArgumentCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
-  Finder->addMatcher(callExpr(callee(functionDecl(hasName("::std::move"))),
-                              argumentCountIs(1),
-                              unless(isInTemplateInstantiation()))
-                         .bind("call-move"),
+
+  auto MoveCallMatcher =
+      callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
+               unless(isInTemplateInstantiation()))
+          .bind("call-move");
+
+  Finder->addMatcher(MoveCallMatcher, this);
+
+  auto ConstParamMatcher = forEachArgumentWithParam(
+      MoveCallMatcher, parmVarDecl(hasType(references(isConstQualified()))));
+
+  Finder->addMatcher(callExpr(ConstParamMatcher).bind("receiving-expr"), this);
+  Finder->addMatcher(cxxConstructExpr(ConstParamMatcher).bind("receiving-expr"),
                      this);
 }
 
 void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
+  const auto *ReceivingExpr = Result.Nodes.getNodeAs<Expr>("receiving-expr");
   const Expr *Arg = CallMove->getArg(0);
   SourceManager &SM = Result.Context->getSourceManager();
 
+  CharSourceRange MoveRange =
+      CharSourceRange::getCharRange(CallMove->getSourceRange());
+  CharSourceRange FileMoveRange =
+      Lexer::makeFileCharRange(MoveRange, SM, getLangOpts());
+  if (!FileMoveRange.isValid())
+    return;
+
   bool IsConstArg = Arg->getType().isConstQualified();
   bool IsTriviallyCopyable =
       Arg->getType().isTriviallyCopyableType(*Result.Context);
 
   if (IsConstArg || IsTriviallyCopyable) {
-    auto MoveRange = CharSourceRange::getCharRange(CallMove->getSourceRange());
-    auto FileMoveRange = Lexer::makeFileCharRange(MoveRange, SM, getLangOpts());
-    if (!FileMoveRange.isValid())
-      return;
     bool IsVariable = isa<DeclRefExpr>(Arg);
     auto Diag = diag(FileMoveRange.getBegin(),
                      "std::move of the %select{|const }0"

                      "has no effect; remove std::move()")
                 << IsConstArg << IsVariable << IsTriviallyCopyable;
 
-    auto BeforeArgumentsRange = Lexer::makeFileCharRange(
-        CharSourceRange::getCharRange(CallMove->getLocStart(),
-                                      Arg->getLocStart()),
-        SM, getLangOpts());
-    auto AfterArgumentsRange = Lexer::makeFileCharRange(
-        CharSourceRange::getCharRange(
-            CallMove->getLocEnd(), CallMove->getLocEnd().getLocWithOffset(1)),
-        SM, getLangOpts());
-
-    if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) {
-      Diag << FixItHint::CreateRemoval(BeforeArgumentsRange)
-           << FixItHint::CreateRemoval(AfterArgumentsRange);
+    ReplaceCallWithArg(CallMove, Diag, SM, getLangOpts());
+  } else if (ReceivingExpr) {
+    // Ignore macro body and argument expansions.
+    //
+    // Macro argument expansions need to be ignored to handle this case
+    // correctly:
+    //
+    //   CALL([non_moveable] {
+    //     MACRO(NonMoveable, non_moveable);
+    //   });
+    //
+    //   where
+    //
+    //   #define MACRO(T, obj) do { \
+    //     T other = std::move(obj); \
+    //   } while(true)
+    //
+    //   #define CALL(func) (func)()
+    //
+    // Here, the std::move() is part of a macro expansion, but
+    // isMacroBodyExpansion() nevertheless returns false because that macro
+    // expansion is inside a lambda which, in turn, is an argument to a macro --
+    // and that appears to take precendence.
+    if (SM.isMacroBodyExpansion(CallMove->getLocStart()) ||
+        SM.isMacroArgExpansion(CallMove->getLocStart())) {
+      return;
     }
+
+    auto Diag = diag(FileMoveRange.getBegin(),
+                     "passing result of std::move() as a const reference "
+                     "argument; no move will actually happen");
+
+    ReplaceCallWithArg(CallMove, Diag, SM, getLangOpts());
   }
 }
 

docs/clang-tidy/checks/misc-move-const-arg.rst

 misc-move-const-arg
 ===================
 
-The check warns if ``std::move()`` is called with a constant argument or an
-argument of a trivially-copyable type, e.g.:
+The check warns
+
+  - if ``std::move()`` is called with a constant argument,
+  - if ``std::move()`` is called with an argument of a trivially-copyable type,
+    or
+  - if the result of ``std::move()`` is passed as a const reference argument.
+
+In all three cases, the check will suggest a fix that removes the
+``std::move()``.
+
+Here are examples of each of the three cases:
 
 .. code:: c++
 

 
   int x;
   return std::move(x);  // Warning: std::move of the variable of a trivially-copyable type has no effect
+
+  void f(const string &s);
+  string s;
+  f(std::move(s));  // Warning: passing result of std::move as a const reference argument; no move will actually happen

test/clang-tidy/misc-move-const-arg.cpp

   f10<int>(1);
   f10<double>(1);
 }
+
+struct NonMoveable {
+ public:
+  NonMoveable();
+  NonMoveable(const NonMoveable &);
+
+  NonMoveable &operator=(const NonMoveable &);
+};
+
+void callByConstRef(const NonMoveable &);
+void callByConstRef(int i, const NonMoveable &);
+
+void moveToConstReferencePositives() {
+  NonMoveable obj;
+
+  // Basic case.
+  callByConstRef(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as
+  // CHECK-FIXES: callByConstRef(obj);
+
+  // Also works for second argument.
+  callByConstRef(1, std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: passing result of std::move() as
+  // CHECK-FIXES: callByConstRef(1, obj);
+
+  // Works if std::move() applied to a temporary.
+  callByConstRef(std::move(NonMoveable()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as
+  // CHECK-FIXES: callByConstRef(NonMoveable());
+
+  // Works if calling a copy constructor.
+  NonMoveable other(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: passing result of std::move() as
+  // CHECK-FIXES: NonMoveable other(obj);
+
+  // Works if calling assignment operator.
+  other = std::move(obj);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: passing result of std::move() as
+  // CHECK-FIXES: other = obj;
+}
+
+struct Moveable {
+ public:
+  Moveable();
+  Moveable(Moveable &&);
+
+  Moveable &operator=(Moveable &&);
+};
+
+void callByValue(Moveable);
+
+void callByRValueRef(Moveable &&);
+
+template <class T>
+void templateFunction(T obj) {
+  T other = std::move(obj);
+}
+
+#define M3(T, obj)            \
+  do {                        \
+    T other = std::move(obj); \
+  } while (true)
+
+#define CALL(func) (func)()
+
+void moveToConstReferenceNegatives() {
+  // No warning when actual move takes place.
+  Moveable moveable;
+  callByValue(std::move(moveable));
+  callByRValueRef(std::move(moveable));
+  Moveable other(std::move(moveable));
+  other = std::move(moveable);
+
+  // No warning if std::move() not used.
+  NonMoveable non_moveable;
+  callByConstRef(non_moveable);
+
+  // No warning if instantiating a template.
+  templateFunction(non_moveable);
+
+  // No warning inside of macro expansions.
+  M3(NonMoveable, non_moveable);
+
+  // No warning inside of macro expansion, even if the macro expansion is inside
+  // a lambda that is, in turn, an argument to a macro.
+  CALL([non_moveable] { M3(NonMoveable, non_moveable); });
+}