[clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[PiotrZSL]

Summary

The diff adds processing for std::move inside lambda captures at bugprone-use-after-move check. Especially it detects invalid std::move inside following code

int foo() {
  int a = 0;
  auto fun = [aa = std::move(a)]{
    return aa;
  };
  return a;
}

Test Plan

./bin/llvm-lit -v  ../clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

[sammccall]

This is a subtle and complicated check I'm not so familiar with. I have some ideas but I'm not confident in the suggestions or if we're missing something.

@mboehme is the expert on this check, he's OOO this week/next week. I'm happy to muddle along or we can wait for him to take a look :-)

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

404

Hmm, I think the original name (CallMoveMatcher) was good as it matches the actual call.

I'd probably name the lambda one LambdaWithMoveInitMatcher and the other... actually it's very close to CallMoveMatcher itself, maybe just inline it?

416

do we want to bind the lambda itself as moving-call?

416

we should probably have a comment explaining *why* lambdas get handled specially.

If I understand right:

• the normal matcher would already match
• but either MovingCall or ContainingLambda (which?) point at unhelpful nodes and
• we end up doing the analysis inside the lambda rather than in the enclosing function
• so never find the following use

(I wonder if it's possible to fix this slightly more directly by tweaking the MovingCall or ContainingLambda logic)

418

This only matches when the initializer is exactly std::move(x).
Not e.g. if it's SomeType(std::move(x)).

Former is probably the common case, but is this deliberate? If we're not sure exactly which cases are safe, maybe add a FIXME?

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

1356

It's pretty interesting that use-after-move fires for ints!

Someone might decide to "fix" that though, so probably best to use A like the other tests.

[ivanmurashko]

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

416

If I understand right:

There are some troubles with the original matcher. The most obvious one is correctly described at your comment :
The original matcher

callExpr(callee(functionDecl(hasName("::std::move"))),
               ... hasAncestor(lambdaExpr().bind("containing-lambda")),
               ...

applied to the code

auto []() {                     // lambda_1
   int a = 0;
   ...
   auto [](aa = std::move(a)) { // lambda_2
   }
}

will return *lambda_2* binded to the "containing-lambda" but we expect it to be *lambda_1*

(I wonder if it's possible to fix this slightly more directly by tweaking the MovingCall or ContainingLambda logic)

It would be good to find it if it's possible

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

1356

There is also another case that I want to address as a separate patch.

void autoCapture() {
  auto var = [](auto &&res) {
    auto f = [blk = std::move(res)]() {};
    return std::move(res);
  };
}

This one is matched as UnresolvedLookupExpr and requires another modified matcher

[mboehme]

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

403

I believe this can be done more simply.

IIUC, the root of the problem is that a std::move() in a lambda capture is being associated with the lambda, when in fact it should be associated with the function that contains the lambda.

This is because the hasAncestor(lambdaExpr()) matches not just a std::move inside the body of the lambda, but also in captures.

I believe this can be solved simply by changing this line so that it only matches a std::move inside the body of the lambda, i.e. something like this:

hasAncestor(compoundStmt(hasParent(lambdaExpr().bind("containing-lambda"))))

This would then no longer match a std::move in a capture; the existing logic would instead associate it with the function that contains the lambda.

418

If this isn't a deliberate restriction, can you add a test for this?

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

1355

Can you put this test with the other tests for lambdas? My suggestion would be to insert it below line 418.

1356

I assume the reason you don't want to address this case within this particular patch is that it requires a lot of additional code?

1363

Can you put these CHECK-NOTES after the line return a + b + c?

The other tests put the CHECK-NOTES after the use, not the move, and it would be nice to be consistent with that.

[mboehme]

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

403

It appears that this works:

https://reviews.llvm.org/D126780

[mboehme]

Note: https://reviews.llvm.org/D126780, which fixes the same bug as this patch, has now been landed.

[LegalizeAdulthood]

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

• fold your changes into the appropriate subdirs, stripping the module prefix from new files.
• make the target check-clang-extra to validate your tests
• make the target docs-clang-tools-html to validate any docs changes

[PiotrZSL]

Already fixed by D126780

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

static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
   } 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 =
+  auto AncestorMatcher =
+      anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
+            hasAncestor(functionDecl().bind("containing-func")));
+
+  auto BaseMoveMatcher =
       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()),
                // try_emplace is a common maybe-moving function that returns a
                // bool to tell callers whether it moved. Ignore std::move inside
                // try_emplace to avoid false positives as we don't track uses of
                // the bool.
                unless(hasParent(cxxMemberCallExpr(
                    callee(cxxMethodDecl(hasName("try_emplace")))))))
           .bind("call-move");
 
+  auto CallMoveMatcherLambda = lambdaExpr(
+      forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
+          hasInitializer(expr(ignoringParenImpCasts(BaseMoveMatcher))))))),
+      AncestorMatcher);
+
+  Finder->addMatcher(CallMoveMatcherLambda, this);
+
+  auto CallMoveMatcherNoLambda =
+      expr(ignoringParenImpCasts(BaseMoveMatcher), AncestorMatcher);
+
   Finder->addMatcher(
       traverse(
           TK_AsIs,
           // 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))),
+              forEach(CallMoveMatcherNoLambda),
               // 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")))))

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

PR38187(std::string val) : val_(std::move(val)) {
     val.empty();
     // CHECK-NOTES: [[@LINE-1]]:5: warning: 'val' used after it was moved
     // CHECK-NOTES: [[@LINE-3]]:30: note: move occurred here
   }
 
 private:
   std::string val_;
 };
+
+// Check std::move usage inside lambda captures
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;
+  int c = 0;
+  auto foo = [aa = std::move(a), bb = std::move(b), cc = c] {
+    // CHECK-NOTES: [[@LINE+6]]:10: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-2]]:20: note: move occurred here
+    // CHECK-NOTES: [[@LINE+4]]:14: warning: 'b' used after it was moved
+    // CHECK-NOTES: [[@LINE-4]]:39: note: move occurred here
+    return aa + bb + cc;
+  };
+  return a + b + c;
+};