[clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[zinovy.nis]

Summary

std::move for const values is NO-OP, so it makes sense to generate an additional note about this (similar to bugprone-use-after-move):

void simpleConst(const A a) {
  A other_a = std::move(a); // note: variable 'a' declared const here
  a.foo();                  // note: 'a' used after it was moved; move of a 'const' argument has no effect [bugprone-use-after-move]
}

Anyway, redundant std::move for consts can also be found with performance-move-const-arg check.

[mboehme]

I've added some individual comments above, but I'd question whether it's really necessary to make this change? Even if no move actually happens, the code is still wrong (it shouldn't be using std::move in the first place), and it's likely the author actually intended for a move to happen, so we should warn that that would cause a use-after-move.

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

425

Is this in the right place? It looks as if it's applying to the std::move, not its argument?

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

137

Generally, I would avoid CHECK-NOTES-NOT if possible. Just leave off the comments entirely; the test will fail if there were any unexpected diagnostics, and the test is more resilient this way.

[zinovy.nis]

In D74692#1879039, @mboehme wrote:

Even if no move actually happens, the code is still wrong

IMHO, it's not wrong, but it's inefficient. And will be reported by performance-move-const-arg.

[• Quuxplusone]

(I sent this to the mailing list, but I guess it doesn't show up here unless I do it through Phab. Quoting myself—)

I see your point about how users who care should always be passing this check alongside "performance-move-const-arg"; but IMHO it still makes sense for clang-tidy to warn unconditionally about code of the form

x = std::move(y);
use(y);

regardless of the incidental type of y. Sure, it's technically not wrong if y is const... or if y is a primitive type, such as Widget*... or if y is a well-defined library type, such as std::shared_ptr<Widget>... or if y is a library type that works in practice, such as std::list<int>... but regardless of its technical merits, the code is still logically semantically wrong, and I think that's what the clang-tidy check should be trying to diagnose.

[zinovy.nis]

In D74692#1884054, @Quuxplusone wrote:

(I sent this to the mailing list, but I guess it doesn't show up here unless I do it through Phab. Quoting myself—)

I see your point about how users who care should always be passing this check alongside "performance-move-const-arg"; but IMHO it still makes sense for clang-tidy to warn unconditionally about code of the form

x = std::move(y);
use(y);

regardless of the incidental type of y. Sure, it's technically not wrong if y is const... or if y is a primitive type, such as Widget*... or if y is a well-defined library type, such as std::shared_ptr<Widget>... or if y is a library type that works in practice, such as std::list<int>... but regardless of its technical merits, the code is still logically semantically wrong, and I think that's what the clang-tidy check should be trying to diagnose.

Ok, maybe it worth just adding an additional text into the warning, like "Use of potentially moved value detected. But value is const so no actual move occurrs"?

[zinovy.nis]

Warning on use after move is still generated. But there's also an additional note

std::move of the const expression has no effect; remove std::move() or make the variable non-const

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

329

It continues to seem silly to me that you give an extra note here saying that line 325 doesn't do anything, when of course line 336 doesn't do anything either (and you don't give any extra note there).

This clang-tidy warning isn't supposed to be about what physically happens in the machine code during any particular compilation run; it's supposed to be about helping the user avoid semantic bugs by cleaning up their codebase's logical behavior. The rule is "don't use things after moving from them," period.

Analogously, if there were a clang-tidy warning to say "always indent four spaces after an if," and you proposed to add a note to some cases that said "...but here a three-space indent is OK, because C++ is whitespace-insensitive" — I'd also find that note to be mildly objectionable. We want to train people to do the right thing, not to do the right thing "except in this case because hey, it doesn't matter to the machine."

[zinovy.nis]

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

329

Thanks for the feedback. I got your point. But my note (may be expressed with wrong words) is about that there're 2 ways to fix the issue: either get rid of 'std::move' or somehow remove 'const'. That was the main purpose of my commit.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

419–420

I don't think this diagnostic makes sense on the location of the declaration -- the diagnostic is talking about expressions but the code the user is looking at is a declaration. I think it may make more sense to change the diagnostic at the use location to read '%0' used after it was moved; move of a 'const' argument has no effect and then this note can read variable '%0' declared const here (so long as you get the correct declaration location -- the lambda example in the test below would be incorrect, for instance).

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

329

In this particular instance, I find the new note to be perplexing -- the constant expression being referenced is the original declaration of a which is not const (the captured a within the lambda is what's const).

[zinovy.nis]

1. Special handling for captured variables in lambdas,
2. messages texts were changed.

[zinovy.nis]

Typo fixed.

[zinovy.nis]

Any further comments?

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

381

Drop the top-level const and make it an integer type so that you don't need to use the ?: below. The implicit conversion will do the right thing in terms of the integer value.

389–390

Range-based for loop over captures()?

392

Same suggestions here as above.

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

342

One more test case to try out (it might be a FIXME because I imagine this requires flow control to get right):

A a;
std::move(a);

auto lambda = [=] {
  a.foo(); // Use of 'a' after it was moved
}

[zinovy.nis]

Cosmetic and style fixes.

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

342

a in lambda is const, but it's not moved inside lambda, so my warning is not expected to be shown.

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

342

The "use" of a that Aaron was talking about is actually

auto lambda = [=] { a.foo(); };
               ^ here

where the moved-from object is captured-by-copy into the lambda. Making a copy of a moved-from object should warn. (Your patch shouldn't have affected this AFAICT; I think he's just asking for a test to verify+document the currently expected behavior.)

[• Quuxplusone]

Anyway, I still don't see the point of this patch. It seems like you're just duplicating the work of performance-move-const-arg. People who want to be shown notes about moves-of-const-args should just enable that check instead.

[zinovy.nis]

In D74692#1911150, @Quuxplusone wrote:

Anyway, I still don't see the point of this patch. It seems like you're just duplicating the work of performance-move-const-arg. People who want to be shown notes about moves-of-const-args should just enable that check instead.

Well, I understand your point. But moving a const is NOP so bugprone-* should not be warned at all?

[zinovy.nis]

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

342

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp#L342

Seems these tests have already been added.

[zinovy.nis]

I'm done, Aaron, Quuxplusone, do you have any comments?

[aaron.ballman]

LGTM aside from the const nits. You should wait for @Quuxplusone before committing in case there are any additional comments.

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

381

Top-level const is still here on the declaration.

391

Top-level const is still here as well.

[zinovy.nis]

Removed top-level consts.

[• Quuxplusone]

I still think this entire patch is misguided; there's no reason to make the note for const std::string s; std::move(s) any longer than the note for int i; std::move(i) or volatile std::string v; std::move(v). People should not be using moved-from objects, period; and those who want to use moved-from objects, should not enable this clang-tidy check.

However, I have no further comments besides philosophical opposition to the whole idea.

[zinovy.nis]

In D74692#1923191, @Quuxplusone wrote:

I still think this entire patch is misguided; there's no reason to make the note for const std::string s; std::move(s) any longer than the note for int i; std::move(i) or volatile std::string v; std::move(v). People should not be using moved-from objects, period; and those who want to use moved-from objects, should not enable this clang-tidy check.

However, I have no further comments besides philosophical opposition to the whole idea.

By this patch I'd like to provide more helpful info to the user on why the code is wrong.
Anyway I don't like to submit this patch if you still have such strong objections.

[aaron.ballman]

In D74692#1923315, @zinovy.nis wrote:

In D74692#1923191, @Quuxplusone wrote:

I still think this entire patch is misguided; there's no reason to make the note for const std::string s; std::move(s) any longer than the note for int i; std::move(i) or volatile std::string v; std::move(v). People should not be using moved-from objects, period; and those who want to use moved-from objects, should not enable this clang-tidy check.

However, I have no further comments besides philosophical opposition to the whole idea.

By this patch I'd like to provide more helpful info to the user on why the code is wrong.
Anyway I don't like to submit this patch if you still have such strong objections.

I don't feel strongly in favor of this patch, but I also am not strongly opposed to it. I've added some more reviewers to see if there are other opinions on the matter.

[zinovy.nis]

Gentle ping)

[zinovy.nis]

Rebase over the current master.

[zinovy.nis]

One more gentle ping)

[ldionne]

I personally don't think the note is useful. What we're trying to tell the user is "don't use X after you've moved out of X" -- whether the move actually has an effect or not is not useful information. To me, adding ; move of a 'const' argument has no effect just makes things less clear -- are you trying to tell me I shouldn't use-after-move, or something else more subtle?

Just my .02

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

for (const auto &Match : Matches) {
         if (TheDeclRef)
           DeclRefs->insert(TheDeclRef);
       }
     }
   }
 }
 
 static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
-                           const UseAfterMove &Use, ClangTidyCheck *Check,
+                           const UseAfterMove &Use,
+                           const LambdaExpr *Lambda, ClangTidyCheck *Check,
                            ASTContext *Context) {
   SourceLocation UseLoc = Use.DeclRef->getExprLoc();
   SourceLocation MoveLoc = MovingCall->getExprLoc();
+  int MoveArgIsConst = MoveArg->getType().isConstQualified();
 
-  Check->diag(UseLoc, "'%0' used after it was moved")
-      << MoveArg->getDecl()->getName();
+  Check->diag(UseLoc, "'%0' used after it was moved%select{|; move of a "
+                      "'const' argument has no effect}1")
+      << MoveArg->getDecl()->getName() << MoveArgIsConst;
   Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note);
+  if (MoveArgIsConst) {
+    if (Lambda) {
+      for (const auto &Capture : Lambda->captures()) {
+        if (MoveArg->getDecl() == Capture.getCapturedVar()) {
+          int IsExplicitCapture = Capture.isExplicit();
+          Check->diag(IsExplicitCapture ? Capture.getLocation()
+                                        : Lambda->getCaptureDefaultLoc(),
+                      "variable %0 %select{implicitly|}1 captured const here",
+                      DiagnosticIDs::Note)
+              << Capture.getCapturedVar() << IsExplicitCapture;
+          break;
+        }
+      }
+    } else {
+      Check->diag(MoveArg->getDecl()->getLocation(),
+                  "variable '%0' declared const here", DiagnosticIDs::Note);
+    }
+  }
+
   if (Use.EvaluationOrderUndefined) {
     Check->diag(UseLoc,
                 "the use and move are unsequenced, i.e. there is no guarantee "
                 "about the order in which they are evaluated",
                 DiagnosticIDs::Note);
   } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
     Check->diag(UseLoc,
                 "the use happens in a later loop iteration than the move",
                 DiagnosticIDs::Note);
   }
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   auto CallMoveMatcher =
       callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
                hasArgument(0, declRefExpr().bind("arg")),
                anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
                      hasAncestor(functionDecl().bind("containing-func"))),
                unless(inDecltypeOrTemplateArg()))
           .bind("call-move");
 
   Finder->addMatcher(
       // To find the Stmt that we assume performs the actual move, we look for
       // the direct ancestor of the std::move() that isn't one of the node
       // types ignored by ignoringParenImpCasts().
       stmt(forEach(expr(ignoringParenImpCasts(CallMoveMatcher))),
            // Don't allow an InitListExpr to be the moving call. An InitListExpr
            // has both a syntactic and a semantic form, and the parent-child
void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   // Ignore the std::move if the variable that was passed to it isn't a local
   // variable.
   if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod())
     return;
 
   UseAfterMoveFinder finder(Result.Context);
   UseAfterMove Use;
   if (finder.find(FunctionBody, MovingCall, Arg->getDecl(), &Use))
-    emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
+    emitDiagnostic(MovingCall, Arg, Use, ContainingLambda, this, Result.Context);
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

void simple() {
   A a;
   a.foo();
   A other_a = std::move(a);
   a.foo();
   // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved
   // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
 }
 
+void simpleConst() {
+  const A a;
+  a.foo();
+  A other_a = std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:11: note: variable 'a' declared const here
+}
+
 // A warning should only be emitted for one use-after-move.
 void onlyFlagOneUseAfterMove() {
   A a;
   a.foo();
   A other_a = std::move(a);
   a.foo();
   // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved
   // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here

 
 void lambdas() {
   // Use-after-moves inside a lambda should be detected.
   {
     A a;
     auto lambda = [a] {
       std::move(a);
       a.foo();
-      // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
+      // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
       // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+      // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' captured const here
+    };
+  }
+  // Use-after-moves inside a lambda should be detected.
+  {
+    A a;
+    // Implicit capture
+    auto lambda = [=] {
+      std::move(a);
+      a.foo();
+      // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+      // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+      // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured const here
     };
   }
   // This is just as true if the variable was declared inside the lambda.
   {
     auto lambda = [] {
       A a;
       std::move(a);
       a.foo();
       // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved

 void passByConstPointerIsUse() {
   {
     // Declaring 'a' as const so that no ImplicitCastExpr is inserted into the
     // AST -- we wouldn't want the check to rely solely on that to detect a
     // const pointer argument.
     const A a;
     std::move(a);
     passByConstPointer(&a);
-    // CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved; move of a 'const' argument has no effect
     // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+    // CHECK-NOTES: [[@LINE-5]]:13: note: variable 'a' declared const here
   }
   const A a;
   std::move(a);
   passByConstReference(a);
-  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:11: note: variable 'a' declared const here
 }
 
 // Clearing a standard container using clear() is treated as a
 // re-initialization.
 void standardContainerClearIsReinit() {
   {
     std::string container;
     std::move(container);