[clang-tidy] Finish cppcoreguidelines-avoid-capturing-lambda-coroutines check

[PiotrZSL]

Summary

This commit implements check for CP.51 C++ Core Guidelines

Depends on D137514

[PiotrZSL]

Change base-line

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

126–128

Just Flags.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.rst

6–24

Ditto.

[PiotrZSL]

Improve documentation (review comments)

[PiotrZSL]

Change baseline

[ChuanqiXu]

LGTM. Thanks!

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.h

 //===--- AvoidCapturingLambdaCoroutinesCheck.h - clang-tidy -----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTURINGLAMBDACOROUTINESCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTURINGLAMBDACOROUTINESCHECK_H
 
 #include "../ClangTidyCheck.h"
 
-namespace clang {
-namespace tidy {
-namespace cppcoreguidelines {
-
-/// The normal usage of captures in lambdas are problematic when the lambda is a
-/// coroutine because the captures are destroyed after the first suspension
-/// point. Using the captures after this point is a use-after-free issue.
+namespace clang::tidy::cppcoreguidelines {
+
+/// Flags C++20 coroutine lambdas with non-empty capture lists that may cause
+/// use-after-free errors and suggests avoiding captures or ensuring the lambda
+/// closure object has a guaranteed lifetime.
 ///
 /// For the user-facing documentation see:
-/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-capturing-lambda-coroutines.html
+/// https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.html
 class AvoidCapturingLambdaCoroutinesCheck : public ClangTidyCheck {
 public:
   AvoidCapturingLambdaCoroutinesCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
 };
 
-} // namespace cppcoreguidelines
-} // namespace tidy
-} // namespace clang
+} // namespace clang::tidy::cppcoreguidelines
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTURINGLAMBDACOROUTINESCHECK_H

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.cpp

 //===--- AvoidCapturingLambdaCoroutinesCheck.cpp - clang-tidy -------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AvoidCapturingLambdaCoroutinesCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
-namespace clang {
-namespace tidy {
-namespace cppcoreguidelines {
+namespace clang::tidy::cppcoreguidelines {
 
-void AvoidCapturingLambdaCoroutinesCheck::registerMatchers(
-    MatchFinder *Finder) {
-  Finder->addMatcher(lambdaExpr().bind("lambda"), this);
+namespace {
+AST_MATCHER(LambdaExpr, hasCoroutineBody) {
+  const Stmt *Body = Node.getBody();
+  return Body != nullptr && CoroutineBodyStmt::classof(Body);
 }
 
-void AvoidCapturingLambdaCoroutinesCheck::check(
-    const MatchFinder::MatchResult &Result) {
-  const auto *Lambda = Result.Nodes.getNodeAs<LambdaExpr>("lambda");
-  if (!Lambda) {
-    return;
-  }
+AST_MATCHER(LambdaExpr, hasCaptures) { return Node.capture_size() != 0U; }
+} // namespace
 
-  const auto *Body = dyn_cast<CoroutineBodyStmt>(Lambda->getBody());
-  if (!Body) {
-    return;
-  }
+void AvoidCapturingLambdaCoroutinesCheck::registerMatchers(
+    MatchFinder *Finder) {
+  Finder->addMatcher(
+      lambdaExpr(hasCaptures(), hasCoroutineBody()).bind("lambda"), this);
 
-  if (Lambda->captures().empty()) {
-    return;
-  }
+bool AvoidCapturingLambdaCoroutinesCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus20;
 
-  diag(Lambda->getBeginLoc(), "found capturing coroutine lambda");
+void AvoidCapturingLambdaCoroutinesCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedLambda = Result.Nodes.getNodeAs<LambdaExpr>("lambda");
+  diag(MatchedLambda->getExprLoc(),
+       "coroutine lambda may cause use-after-free, avoid captures or ensure "
+       "lambda closure object has guaranteed lifetime");
 }
 
-} // namespace cppcoreguidelines
-} // namespace tidy
-} // namespace clang
+} // namespace clang::tidy::cppcoreguidelines

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`llvmlibc-inline-function-decl
   <clang-tidy/checks/llvmlibc/inline-function-decl>` check.
 
   Checks that all implicit and explicit inline functions in header files are
   tagged with the ``LIBC_INLINE`` macro.
 
 - New :doc:`cppcoreguidelines-avoid-capturing-lambda-coroutines
   <clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines>` check.
 
-  Adds check for cpp core guideline: "CP.51: Do not use capturing lambdas that
-  are coroutines."
-
+  Flags C++20 coroutine lambdas with non-empty capture lists that may cause
+  use-after-free errors and suggests avoiding captures or ensuring the lambda
+  closure object has a guaranteed lifetime.
 
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc24-c
   <clang-tidy/checks/cert/msc24-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.rst

 .. title:: clang-tidy - cppcoreguidelines-avoid-capturing-lambda-coroutines
 
 cppcoreguidelines-avoid-capturing-lambda-coroutines
 ===================================================
 
-Warns if a capturing lambda is a coroutine. For example:
+Flags C++20 coroutine lambdas with non-empty capture lists that may cause
+use-after-free errors and suggests avoiding captures or ensuring the lambda
+closure object has a guaranteed lifetime.
+
+This check implements `CP.51
+<https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rcoro-capture>`_
+from the C++ Core Guidelines.
+
+Using coroutine lambdas with non-empty capture lists can be risky, as capturing
+variables can lead to accessing freed memory after the first suspension point.
+This issue can occur even with refcounted smart pointers and copyable types.
+When a lambda expression creates a coroutine, it results in a closure object
+with storage, which is often on the stack and will eventually go out of scope.
+When the closure object goes out of scope, its captures also go out of scope.
+While normal lambdas finish executing before this happens, coroutine lambdas may
+resume from suspension after the closure object has been destructed, resulting
+in use-after-free memory access for all captures.
 
-.. code-block:: c++
+Consider the following example:
 
-   int c;
+.. code-block:: c++
 
-   [c] () -> task { co_return; };
-   [&] () -> task { int y = c; co_return; };
-   [=] () -> task { int y = c; co_return; };
-
-   struct s {
-       void m() {
-           [this] () -> task { co_return; };
-       }
+    int value = get_value();
+    std::shared_ptr<Foo> sharedFoo = get_foo();
+    {
+        const auto lambda = [value, sharedFoo]() -> std::future<void>
+        {
+            co_await something();
+            // "sharedFoo" and "value" have already been destroyed
+            // the "shared" pointer didn't accomplish anything
         };
+        lambda();
+    } // the lambda closure object has now gone out of scope
 
-All of the cases above will trigger the warning. However, implicit captures
-do not trigger the warning unless the body of the lambda uses the capture.
-For example, the following do not trigger the warning.
-
-.. code-block:: c++
-
-   int c;
+In this example, the lambda object is defined with two captures: value and
+``sharedFoo``. When ``lambda()`` is called, the lambda object is created on the
+stack, and the captures are copied into the closure object. When the coroutine
+is suspended, the lambda object goes out of scope, and the closure object is
+destroyed. When the coroutine is resumed, the captured variables may have been
+destroyed, resulting in use-after-free bugs.
+
+In conclusion, the use of coroutine lambdas with non-empty capture lists can
+lead to use-after-free errors when resuming the coroutine after the closure
+object has been destroyed. This check helps prevent such errors by flagging
+C++20 coroutine lambdas with non-empty capture lists and suggesting avoiding
+captures or ensuring the lambda closure object has a guaranteed lifetime.
 
-   [&] () -> task { co_return; };
-   [=] () -> task { co_return; };
+Following these guidelines can help ensure the safe and reliable use of
+coroutine lambdas in C++ code.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `clang-analyzer-osx.cocoa.NonNilReturnValue <clang-analyzer/osx.cocoa.NonNilReturnValue.html>`_,
    `clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak <clang-analyzer/osx.cocoa.RunLoopAutoreleaseLeak.html>`_,
    `clang-analyzer-valist.CopyToSelf <clang-analyzer/valist.CopyToSelf.html>`_,
    `clang-analyzer-valist.Uninitialized <clang-analyzer/valist.Uninitialized.html>`_,
    `clang-analyzer-valist.Unterminated <clang-analyzer/valist.Unterminated.html>`_,
    `concurrency-mt-unsafe <concurrency/mt-unsafe.html>`_,
    `concurrency-thread-canceltype-asynchronous <concurrency/thread-canceltype-asynchronous.html>`_,
    `cppcoreguidelines-avoid-capture-default-when-capturing-this <cppcoreguidelines/avoid-capture-default-when-capturing-this.html>`_,
-   `cppcoreguidelines-avoid-capturing-lambda-coroutines <cppcoreguidelines/avoid-capturing-lambda-coroutines.html>`_, "Yes"
+   `cppcoreguidelines-avoid-capturing-lambda-coroutines <cppcoreguidelines/avoid-capturing-lambda-coroutines.html>`_,
    `cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines/avoid-const-or-ref-data-members.html>`_,
    `cppcoreguidelines-avoid-do-while <cppcoreguidelines/avoid-do-while.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines/avoid-goto.html>`_,
    `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines/avoid-non-const-global-variables.html>`_,
    `cppcoreguidelines-avoid-reference-coroutine-parameters <cppcoreguidelines/avoid-reference-coroutine-parameters.html>`_,
    `cppcoreguidelines-init-variables <cppcoreguidelines/init-variables.html>`_, "Yes"
    `cppcoreguidelines-interfaces-global-init <cppcoreguidelines/interfaces-global-init.html>`_,
    `cppcoreguidelines-macro-usage <cppcoreguidelines/macro-usage.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/Inputs/system/coroutines.h

+#pragma once
+
+namespace std {
+
+template <typename ret_t, typename... args_t>
+struct coroutine_traits {
+  using promise_type = typename ret_t::promise_type;
+};
+
+template <class promise_t>
+struct coroutine_handle {
+  static constexpr coroutine_handle from_address(void *addr) noexcept { return {}; };
+};
+
+} // namespace std
+
+struct never_suspend {
+  bool await_ready() noexcept { return false; }
+  template <typename coro_t>
+  void await_suspend(coro_t handle) noexcept {}
+  void await_resume() noexcept {}
+};
+
+struct task {
+  struct promise_type {
+    task get_return_object() noexcept { return {}; }
+    never_suspend initial_suspend() noexcept { return {}; }
+    never_suspend final_suspend() noexcept { return {}; }
+    void return_void() {}
+    void unhandled_exception() {}
+  };
+};

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capturing-lambda-coroutines.cpp

-// RUN: %check_clang_tidy -std=c++20-or-later %s cppcoreguidelines-avoid-capturing-lambda-coroutines %t -- -- \
-// RUN:   -isystem %S/readability/Inputs/identifier-naming/system
+// RUN: %check_clang_tidy -std=c++20-or-later %s cppcoreguidelines-avoid-capturing-lambda-coroutines %t -- -- -isystem %S/Inputs/system
 
 #include <coroutines.h>
 
 void Caught() {
     int v;
 
     [&] () -> task { int y = v; co_return; };
-    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines]
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines]
     [=] () -> task { int y = v; co_return; };
-    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines]
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines]
     [v] () -> task { co_return; };
-    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines]
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines]
     [&v] () -> task { co_return; };
-    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines]
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines]
     [y=v] () -> task { co_return; };
-    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines]
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines]
     [y{v}] () -> task { co_return; };
-    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines]
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines]
 }
 
 struct S {
     void m() {
         [this] () -> task { co_return; };
-        // CHECK-MESSAGES: [[@LINE-1]]:9: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines]
+        // CHECK-MESSAGES: [[@LINE-1]]:9: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines]
     }
 };
 
 void Safe() {
     int v;
     [] () -> task { co_return; };
     [&] () -> task { co_return; };
     [=] () -> task { co_return; };
+
+    [&v]{++v;}();
 }