[clang-tidy] Improve bugprone-use-after-move to handle lambdas

[zeyi2]

Closes #172018

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: mitchell (zeyi2)

Changes

This PR is not ready for review yet.

TODOs:

• Add documentation
• Enhance testcases
• Code cleanup

---

Full diff: https://github.com/llvm/llvm-project/pull/173192.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp (+69-1)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp (+109)

diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index 98b0202a87f2d..cd4bf400d3985 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -8,8 +8,10 @@
 
 #include "UseAfterMoveCheck.h"
 
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/LambdaCapture.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h"
 #include "clang/Analysis/CFG.h"
@@ -79,7 +81,19 @@ class UseAfterMoveFinder {
   llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
 };
 
-} // namespace
+AST_MATCHER_P(CXXRecordDecl, hasCaptureByReference, const ValueDecl *,
+              TargetDecl) {
+  if (!Node.isLambda())
+    return false;
+
+  for (const auto &Capture : Node.captures()) {
+    if (Capture.capturesVariable() && Capture.getCaptureKind() == LCK_ByRef &&
+        Capture.getCapturedVar() == TargetDecl) {
+      return true;
+    }
+  }
+  return false;
+}
 
 static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
   return anyOf(hasAnyName("::std::move", "::std::forward"),
@@ -150,6 +164,29 @@ makeReinitMatcher(const ValueDecl *MovedVariable,
       .bind("reinit");
 }
 
+bool isVariableResetInLambda(const Stmt *Body, const ValueDecl *MovedVariable,
+                             ASTContext *Context,
+                             llvm::ArrayRef<StringRef> InvalidationFunctions) {
+  if (!Body)
+    return false;
+
+  // If the variable is not mentioned at all in the lambda body,
+  // it cannot be reinitialized.
+  const auto VariableMentionMatcher = stmt(anyOf(
+      hasDescendant(declRefExpr(hasDeclaration(equalsNode(MovedVariable)))),
+      hasDescendant(memberExpr(hasDeclaration(equalsNode(MovedVariable))))));
+
+  if (match(VariableMentionMatcher, *Body, *Context).empty())
+    return false;
+
+  const auto ReinitMatcher =
+      makeReinitMatcher(MovedVariable, InvalidationFunctions);
+
+  return !match(findAll(ReinitMatcher), *Body, *Context).empty();
+}
+
+} // namespace
+
 // Matches nodes that are
 // - Part of a decltype argument or class template argument (we check this by
 //   seeing if they are children of a TypeLoc), or
@@ -374,6 +411,13 @@ void UseAfterMoveFinder::getReinits(
   const auto ReinitMatcher =
       makeReinitMatcher(MovedVariable, InvalidationFunctions);
 
+  // Match calls to lambdas that capture the moved variable by reference.
+  const auto LambdaCallMatcher =
+      cxxOperatorCallExpr(
+          hasOverloadedOperatorName("()"),
+          callee(cxxMethodDecl(ofClass(hasCaptureByReference(MovedVariable)))))
+          .bind("lambda-call");
+
   Stmts->clear();
   DeclRefs->clear();
   for (const auto &Elem : *Block) {
@@ -397,6 +441,30 @@ void UseAfterMoveFinder::getReinits(
           DeclRefs->insert(TheDeclRef);
       }
     }
+
+    // Check for calls to lambdas that capture the moved variable
+    // by reference and reinitialize it within their body.
+    const SmallVector<BoundNodes, 1> LambdaMatches =
+        match(findAll(LambdaCallMatcher), *S->getStmt(), *Context);
+
+    for (const auto &Match : LambdaMatches) {
+      const auto *Operator =
+          Match.getNodeAs<CXXOperatorCallExpr>("lambda-call");
+
+      if (Operator && BlockMap->blockContainingStmt(Operator) == Block) {
+        const auto *MD =
+            dyn_cast_or_null<CXXMethodDecl>(Operator->getDirectCallee());
+        if (!MD)
+          continue;
+
+        const auto *RD = MD->getParent();
+        const auto *LambdaBody = MD->getBody();
+        if (RD && RD->isLambda() && LambdaBody &&
+            isVariableResetInLambda(LambdaBody, MovedVariable, Context,
+                                    InvalidationFunctions))
+          Stmts->insert(Operator);
+      }
+    }
   }
 }
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index b2df2638106e0..b1ed032cbb27b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -1703,3 +1703,112 @@ void Run() {
 }
 
 } // namespace custom_invalidation
+
+void lambdaReinit() {
+  {
+    std::string s;
+    auto g = [&]() {
+      s = std::string();
+      return true;
+    };
+    for (int i = 0; i < 10; ++i) {
+      if (g()) {
+        if (!s.empty()) {
+          std::move(s);
+        }
+      }
+    }
+  }
+
+  {
+    std::string s;
+    auto g = [&]() {
+      s.clear();
+      return true;
+    };
+    for (int i = 0; i < 10; ++i) {
+      if (g()) {
+        if (!s.empty()) {
+          std::move(s);
+        }
+      }
+    }
+  }
+
+  {
+    std::string s;
+    auto g = [&]() {
+      s.assign(10, 'a');
+      return true;
+    };
+    for (int i = 0; i < 10; ++i) {
+      if (g()) {
+        if (!s.empty()) {
+          std::move(s);
+        }
+      }
+    }
+  }
+
+  {
+    std::string s;
+    auto g = [&]() {
+      return !s.empty();
+    };
+    for (int i = 0; i < 10; ++i) {
+      if (g()) {
+        std::move(s);
+        // CHECK-NOTES: [[@LINE-1]]:19: warning: 's' used after it was moved
+        // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here
+        // CHECK-NOTES: [[@LINE-3]]:19: note: the use happens in a later loop
+      }
+    }
+  }
+
+  {
+    std::string s;
+    auto g = [s]() mutable {
+      s = std::string();
+      return true;
+    };
+    for (int i = 0; i < 10; ++i) {
+      if (g()) {
+        std::move(s);
+        // CHECK-NOTES: [[@LINE-1]]:19: warning: 's' used after it was moved
+        // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here
+        // CHECK-NOTES: [[@LINE-3]]:19: note: the use happens in a later loop
+      }
+    }
+  }
+
+  {
+    std::string s;
+    while ([&]() { s = std::string(); return true; }()) {
+      // CHECK-NOTES: [[@LINE-1]]:13: warning: 's' used after it was moved
+      // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here
+      // CHECK-NOTES: [[@LINE-3]]:13: note: the use happens in a later loop
+      if (!s.empty()) {
+        std::move(s);
+      }
+    }
+  }
+}
+
+void lambdaReinitInDeadCode() {
+  // FIXME: This is a known False Negative. The check currently
+  // lacks the ability to do control flow analysis.
+  {
+    std::string s;
+    auto g = [&]() {
+      if (false) {
+        s = std::string();
+      }
+    };
+    std::move(s);
+
+    g();
+
+    s.clear();
+    // CHECK-NOTES-NOT: [[@LINE-2]]:5: warning: 's' used after it was moved
+  }
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code linter.

[zeyi2]

I'm not sure whether there is a better way to do this. It seems that the CFG built by UseAfterMoveFinder::find doesn't contain information inside the Lambda body and I find it hard to inline the lambda's CFG into the parent's CFG, so I choose to build a separate one to detect unreachable reinitializations.

[github-actions]

🐧 Linux x64 Test Results

• 3062 tests passed
• 7 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🪟 Windows x64 Test Results

• 3000 tests passed
• 30 tests skipped

✅ The build succeeded and all tests passed.

[vbvictor]

Unfortunately, I don't feel I have enough knowledge to judge the "lambda cfg approach".

One improvement could be to refactor common cfg part into separate function (not to duplicate options e.g.)

[vbvictor]

Please use real type

[zwuis]

Not sure if there are better approaches, but there are similar cases which should be handled in the future if we land this patch.

[&] { // nested lambdas, seems low priority?
  [&] { use(std::move(x)); }(); // lambdas invalidating `x`
  [&] { reinit(x); }(); // correctly handled by this patch
  [&] { use(x); }(); // lambdas using `x`
}();