[clang-tidy] misc-use-after-move: Fix failing assertion

[mboehme]

Summary

I've added a test case that (without the fix) triggers the assertion,
which happens when a move happens in an implicitly called conversion
operator.

This patch also fixes nondeterministic behavior in the source code
location reported for the move when the move is constained in an init list;
this was causing buildbot failures in the previous attempt to submit
this patch (see D30569 and rL297004).

[mboehme]

test/clang-tidy/misc-use-after-move.cpp

285 ↗

(On Diff #90692)

The column in this message was nondeterministically being reported as either 6 or 7. Disallowing InitListExpr as the moving call should guarantee that this will always be 7.

[alexfh]

LG. So you won the flappy column game? ;)

[mboehme]

In D30650#693136, @alexfh wrote:

LG. So you won the flappy column game? ;)

I hope so... ;)

[Eugene.Zelenko]

I think we should refactor this check as part of Static Analyzer, since it's path-sensitive.

[alexfh]

In D30650#693165, @Eugene.Zelenko wrote:

I think we should refactor this check as part of Static Analyzer, since it's path-sensitive.

We can think about trying this as a SA checker, but it's irrelevant to this patch.

[klimek]

In D30650#693165, @Eugene.Zelenko wrote:

I think we should refactor this check as part of Static Analyzer, since it's path-sensitive.

Are you saying it should be path sensitive? Because currently it's not, right?

[mboehme]

In D30650#695350, @klimek wrote:

In D30650#693165, @Eugene.Zelenko wrote:

I think we should refactor this check as part of Static Analyzer, since it's path-sensitive.

Are you saying it should be path sensitive? Because currently it's not, right?

(Will submit in the meantime because this is a wider discussion that doesn't impact this patch specifically. But let's continue the discussion.)

[Eugene.Zelenko]

In D30650#695350, @klimek wrote:

In D30650#693165, @Eugene.Zelenko wrote:

I think we should refactor this check as part of Static Analyzer, since it's path-sensitive.

Are you saying it should be path sensitive? Because currently it's not, right?

It handle paths, as far as I could tell from regression test. But will it handle all possible paths? What about path reporting? And this is definitely duplication of Static Analyzer functionality.

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

auto CallMoveMatcher =
                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
+           // relationships are different between the two. This could cause an
+           // InitListExpr to be analyzed as the moving call in addition to the
+           // Expr that we actually want, resulting in two diagnostics with
+           // different code locations for the same move.
+           unless(initListExpr()),
            unless(expr(ignoringParenImpCasts(equalsBoundNode("call-move")))))
           .bind("moving-call"),
       this);
 }
 
 void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *ContainingLambda =
       Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda");
   const auto *ContainingFunc =
       Result.Nodes.getNodeAs<FunctionDecl>("containing-func");
   const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
   const auto *MovingCall = Result.Nodes.getNodeAs<Expr>("moving-call");
   const auto *Arg = Result.Nodes.getNodeAs<DeclRefExpr>("arg");
 
-  if (!MovingCall)
+  if (!MovingCall || !MovingCall->getExprLoc().isValid())
     MovingCall = CallMove;
 
   Stmt *FunctionBody = nullptr;
   if (ContainingLambda)
     FunctionBody = ContainingLambda->getBody();
   else if (ContainingFunc)
     FunctionBody = ContainingFunc->getBody();
   else

clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp

 void moveInInitList() {
   struct S {
     A a;
   };
   A a;
   S s{std::move(a)};
   a.foo();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
-  // CHECK-MESSAGES: [[@LINE-3]]:6: note: move occurred here
+  // CHECK-MESSAGES: [[@LINE-3]]:7: note: move occurred here
 }
 
 void lambdas() {
   // Use-after-moves inside a lambda should be detected.
   {
     A a;
     auto lambda = [a] {
       std::move(a);
void movedTypeIsDependentType() {
   T t;
   std::move(t);
   t.foo();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 't' used after it was moved
   // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here
 }
 template void movedTypeIsDependentType<A>();
 
+// We handle the case correctly where the move consists of an implicit call
+// to a conversion operator.
+void implicitConversionOperator() {
+  struct Convertible {
+    operator A() && { return A(); }
+  };
+  void takeA(A a);
+
+  Convertible convertible;
+  takeA(std::move(convertible));
+  convertible;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'convertible' used after it was moved
+  // CHECK-MESSAGES: [[@LINE-3]]:9: note: move occurred here
+}
+
 // Using decltype on an expression is not a use.
 void decltypeIsNotUse() {
   A a;
   std::move(a);
   decltype(a) other_a;
 }
 
 // Ignore moves or uses that occur as part of template arguments.