[clangtidy] Allow safe suspensions in coroutine-hostile-raii check

[usx95]

Certain awaitable types could be safe to co_await on even when we have suspension-hostile RAII objects in scope.
This PR adds a way for users to mark such safe awaitable and silence false positive warnings in co_await expressions involving such an awaitable.

co_await-ing an expression of awaitable type is considered safe if the type is part of SafeAwaiatablesList check option. RAII objects persisting across such a co_await expression are
considered safe and hence are not flagged.

[github-actions]

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

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Utkarsh Saxena (usx95)

Changes

Certain awaitable types could be safe to co_await on even when we have suspension-hostile RAII objects in scope.
This PR adds a way for users to mark such safe awaitable and silence false positive warnings in co_await expressions involving such an awaitable.

co_await-ing an expression of awaitable type is considered safe if the awaitable type is annotated with
[[clang::annotate("coro_raii_safe_suspend")]]. RAII objects persisting across such a co_await expression are
considered safe and hence are not flagged.

---

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

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp (+21-5)
• (modified) clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst (+40-4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp (+10)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index e820cd39d83d21b..ee7565d5c714d4d 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -52,6 +52,19 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
   }
   return IsHostile;
 }
+
+AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>,
+              InnerMatcher) {
+  return Node.getCommonExpr() &&
+         InnerMatcher.matches(*Node.getCommonExpr(), Finder, Builder);
+}
+
+AST_MATCHER(Decl, isRAIISafe) {
+  for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>())
+    if (Attr->getAnnotation() == "coro_raii_safe_suspend")
+      return true;
+  return false;
+}
 } // namespace
 
 CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
@@ -68,11 +81,14 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
   auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration(
                                namedDecl(hasAnyName(RAIITypesList))))))
                        .bind("raii");
-  Finder->addMatcher(expr(anyOf(coawaitExpr(), coyieldExpr()),
-                          forEachPrevStmt(declStmt(forEach(
-                              varDecl(anyOf(ScopedLockable, OtherRAII))))))
-                         .bind("suspension"),
-                     this);
+  auto SafeSuspend =
+      awaiatable(hasType(hasCanonicalType(hasDeclaration(isRAIISafe()))));
+  Finder->addMatcher(
+      expr(anyOf(coawaitExpr(unless(SafeSuspend)), coyieldExpr()),
+           forEachPrevStmt(
+               declStmt(forEach(varDecl(anyOf(ScopedLockable, OtherRAII))))))
+          .bind("suspension"),
+      this);
 }
 
 void CoroutineHostileRAIICheck::check(const MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
index b8698ba3de85300..93e9158d9f3fd16 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst
@@ -29,15 +29,51 @@ Following types are considered as hostile:
 .. code-block:: c++
 
   // Call some async API while holding a lock.
-  {
-    const my::MutexLock l(&mu_);
+  task coro() {
+    const std::lock_guard l(&mu_);
 
     // Oops! The async Bar function may finish on a different
-    // thread from the one that created the MutexLock object and therefore called
-    // Mutex::Lock -- now Mutex::Unlock will be called on the wrong thread.
+    // thread from the one that created the lock_guard (and called
+    // Mutex::Lock). After suspension, Mutex::Unlock will be called on the wrong thread.
     co_await Bar();
   }
 
+Exclusions
+-------
+It is possible to make the check treat certain suspensions as safe.
+``co_await``-ing an expression of ``awaitable`` type is considered
+safe if the ``awaitable`` type is annotated with 
+``[[clang::annotate("coro_raii_safe_suspend")]]``.
+RAII objects persisting across such a ``co_await`` expression are
+considered safe and hence are not flagged.
+
+This annotation can be used to mark ``awaitable`` types which can be safely
+awaited while having hostile RAII objects in scope. For example, such safe
+``awaitable`` could ensure resumption on the same thread or even unlock the mutex
+on suspension and reacquire on resumption.
+
+Example usage:
+
+.. code-block:: c++
+
+  struct [[clang::annotate("coro_raii_safe_suspend")]] safe_awaitable {
+    bool await_ready() noexcept { return false; }
+    void await_suspend(std::coroutine_handle<>) noexcept {}
+    void await_resume() noexcept {}
+  };
+
+  task coro() {
+    const std::lock_guard l(&mu_);
+    co_await safe_awaitable{};
+  }
+
+  auto wait() { return safe_awaitable{}; }
+
+  task coro() {
+    const std::lock_guard l(&mu_); // No warning.
+    co_await safe_awaitable{};
+    co_await wait();
+  }
 
 Options
 -------
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
index 2d022e21c85d566..84649ae8afa52b8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
@@ -135,6 +135,16 @@ ReturnObject scopedLockableTest() {
     absl::Mutex no_warning_5;
 }
 
+struct [[clang::annotate("coro_raii_safe_suspend")]] raii_safe_suspend {
+  bool await_ready() noexcept { return false; }
+  void await_suspend(std::coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
+};
+ReturnObject RAIISafeSuspendTest() {
+  absl::Mutex a;
+  co_await raii_safe_suspend{};
+} 
+
 void lambda() {
   absl::Mutex no_warning;
   auto lambda = []() -> ReturnObject {

[PiotrZSL]

Maybe instead of "coro_raii_safe_suspend" we should just use an list of "allowed types" ?

[usx95]

That is a good solution and does not involve changing the code. Done.

[PiotrZSL]

Overall looks +- fine for me.
Wait like 1-2 weeks before merging if someone got some more comments.

[bjosv]

Nice. Added a naming comment/nit.