[clang-tools-extra] [llvm] [lld] [mlir] [clang] [flang] [clangtidy] Allow safe suspensions in coroutine-hostile-raii check (PR #72954)

Utkarsh Saxena via cfe-commits cfe-commits at lists.llvm.org
Mon Nov 27 07:20:30 PST 2023


https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/72954

>From c863646669d0b2b54e1c1c353b063a8209730528 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 06:51:48 +0100
Subject: [PATCH 01/14] [clangtidy]Allow safe suspensions in
 coroutine-hostile-raii check

---
 .../misc/CoroutineHostileRAIICheck.cpp          | 17 ++++++++++++++++-
 .../checkers/misc/coroutine-hostile-raii.cpp    | 11 +++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index e820cd39d83d21b..1c39a6624a1da69 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, isRAIISafeAwaitable) {
+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,7 +81,9 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
   auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration(
                                namedDecl(hasAnyName(RAIITypesList))))))
                        .bind("raii");
-  Finder->addMatcher(expr(anyOf(coawaitExpr(), coyieldExpr()),
+  auto Allowed = awaiatable(
+      hasType(hasCanonicalType(hasDeclaration(isRAIISafeAwaitable()))));
+  Finder->addMatcher(expr(anyOf(coawaitExpr(unless(Allowed)), coyieldExpr()),
                           forEachPrevStmt(declStmt(forEach(
                               varDecl(anyOf(ScopedLockable, OtherRAII))))))
                          .bind("suspension"),
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..4a1dc61b34ded18 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
@@ -62,6 +62,12 @@ struct suspend_always {
 };
 } // namespace std
 
+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 {}
+};
+
 struct ReturnObject {
     struct promise_type {
         ReturnObject get_return_object() { return {}; }
@@ -135,6 +141,11 @@ ReturnObject scopedLockableTest() {
     absl::Mutex no_warning_5;
 }
 
+ReturnObject RAIISafeSuspendTest() {
+  absl::Mutex a;
+  co_await raii_safe_suspend{};
+} 
+
 void lambda() {
   absl::Mutex no_warning;
   auto lambda = []() -> ReturnObject {

>From b35e1fece9b35f5faf1a28a5fa744ed29ddbf250 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 07:03:16 +0100
Subject: [PATCH 02/14] fix format

---
 clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index 1c39a6624a1da69..235bb584be14754 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -60,7 +60,7 @@ AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>,
 }
 
 AST_MATCHER(Decl, isRAIISafeAwaitable) {
-for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>())
+  for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>())
     if (Attr->getAnnotation() == "coro_raii_safe_suspend")
       return true;
   return false;

>From 9d1c6d90e3fd7b4eb673bd67ccbf48c1d24ce329 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 07:06:51 +0100
Subject: [PATCH 03/14] rename var

---
 .../clang-tidy/misc/CoroutineHostileRAIICheck.cpp   | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index 235bb584be14754..6b438b6c44d855c 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -81,13 +81,14 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
   auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration(
                                namedDecl(hasAnyName(RAIITypesList))))))
                        .bind("raii");
-  auto Allowed = awaiatable(
+  auto SafeSuspend = awaiatable(
       hasType(hasCanonicalType(hasDeclaration(isRAIISafeAwaitable()))));
-  Finder->addMatcher(expr(anyOf(coawaitExpr(unless(Allowed)), coyieldExpr()),
-                          forEachPrevStmt(declStmt(forEach(
-                              varDecl(anyOf(ScopedLockable, OtherRAII))))))
-                         .bind("suspension"),
-                     this);
+  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) {

>From 37f412080ee64d98527aeb1a06429846d935de81 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 07:40:13 +0100
Subject: [PATCH 04/14] more work

---
 .../clang-tidy/misc/CoroutineHostileRAIICheck.cpp     |  6 +++---
 .../checkers/misc/coroutine-hostile-raii.cpp          | 11 +++++------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index 6b438b6c44d855c..ee7565d5c714d4d 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -59,7 +59,7 @@ AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>,
          InnerMatcher.matches(*Node.getCommonExpr(), Finder, Builder);
 }
 
-AST_MATCHER(Decl, isRAIISafeAwaitable) {
+AST_MATCHER(Decl, isRAIISafe) {
   for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>())
     if (Attr->getAnnotation() == "coro_raii_safe_suspend")
       return true;
@@ -81,8 +81,8 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
   auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration(
                                namedDecl(hasAnyName(RAIITypesList))))))
                        .bind("raii");
-  auto SafeSuspend = awaiatable(
-      hasType(hasCanonicalType(hasDeclaration(isRAIISafeAwaitable()))));
+  auto SafeSuspend =
+      awaiatable(hasType(hasCanonicalType(hasDeclaration(isRAIISafe()))));
   Finder->addMatcher(
       expr(anyOf(coawaitExpr(unless(SafeSuspend)), coyieldExpr()),
            forEachPrevStmt(
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 608f9aec6ba0a71..6fd649bb021b414 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
@@ -62,12 +62,6 @@ struct suspend_always {
 };
 } // namespace std
 
-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 {}
-};
-
 struct ReturnObject {
     struct promise_type {
         ReturnObject get_return_object() { return {}; }
@@ -141,6 +135,11 @@ 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{};

>From e438500e8775887f72f234395a781da66277c803 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 08:02:46 +0100
Subject: [PATCH 05/14] add docs

---
 .../checks/misc/coroutine-hostile-raii.rst    | 21 +++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

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..b36bded6866641b 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,28 @@ 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();
   }
 
+.. 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{};
+  }
+
 
 Options
 -------

>From 5a3af35ee3f17b384dd6314e5ea1918160f19856 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 09:00:39 +0100
Subject: [PATCH 06/14] update docs

(cherry picked from commit f2092a62148dca3ee6cb3ea7a80fc7c9ad640f50)
---
 .../checks/misc/coroutine-hostile-raii.rst    | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

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 b36bded6866641b..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
@@ -38,6 +38,22 @@ Following types are considered as hostile:
     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 {
@@ -51,6 +67,13 @@ Following types are considered as hostile:
     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
 -------

>From 5f249bab1ada4f362bff0a269bd2bd866ef849ba Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 09:26:15 +0100
Subject: [PATCH 07/14] add comment for matchers

---
 .../clang-tidy/misc/CoroutineHostileRAIICheck.cpp              | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index ee7565d5c714d4d..838d2f5667cc210 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -53,12 +53,15 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
   return IsHostile;
 }
 
+// Matches the expression awaited by the `co_await`.
 AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>,
               InnerMatcher) {
   return Node.getCommonExpr() &&
          InnerMatcher.matches(*Node.getCommonExpr(), Finder, Builder);
 }
 
+// Matches a declaration annotated with
+// [[clang::annotate("coro_raii_safe_suspend")]].
 AST_MATCHER(Decl, isRAIISafe) {
   for (const auto &Attr : Node.specific_attrs<clang::AnnotateAttr>())
     if (Attr->getAnnotation() == "coro_raii_safe_suspend")

>From 3c62c83de88d80797fe216287ba5633213658353 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 13:09:31 +0100
Subject: [PATCH 08/14] use clangtidy options instead of annotation

---
 .../misc/CoroutineHostileRAIICheck.cpp        | 19 ++----
 .../misc/CoroutineHostileRAIICheck.h          |  3 +
 .../checks/misc/coroutine-hostile-raii.rst    | 67 +++++++++----------
 .../checkers/misc/coroutine-hostile-raii.cpp  | 15 +++--
 4 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index 838d2f5667cc210..3a98db34b3a887d 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -59,22 +59,15 @@ AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>,
   return Node.getCommonExpr() &&
          InnerMatcher.matches(*Node.getCommonExpr(), Finder, Builder);
 }
-
-// Matches a declaration annotated with
-// [[clang::annotate("coro_raii_safe_suspend")]].
-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,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       RAIITypesList(utils::options::parseStringList(
-          Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))) {}
+          Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))),
+      SafeAwaitablesList(utils::options::parseStringList(
+          Options.get("SafeAwaitablesList", ""))) {}
 
 void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
   // A suspension happens with co_await or co_yield.
@@ -84,8 +77,8 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
   auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration(
                                namedDecl(hasAnyName(RAIITypesList))))))
                        .bind("raii");
-  auto SafeSuspend =
-      awaiatable(hasType(hasCanonicalType(hasDeclaration(isRAIISafe()))));
+  auto SafeSuspend = awaiatable(hasType(hasCanonicalType(
+      hasDeclaration(namedDecl(hasAnyName(SafeAwaitablesList))))));
   Finder->addMatcher(
       expr(anyOf(coawaitExpr(unless(SafeSuspend)), coyieldExpr()),
            forEachPrevStmt(
@@ -113,5 +106,7 @@ void CoroutineHostileRAIICheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "RAIITypesList",
                 utils::options::serializeStringList(RAIITypesList));
+  Options.store(Opts, "SafeAwaitableList",
+                utils::options::serializeStringList(SafeAwaitablesList));
 }
 } // namespace clang::tidy::misc
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index a5e9cb89ef67695..3821e255a81c2c0 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -43,6 +43,9 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
   // List of fully qualified types which should not persist across a suspension
   // point in a coroutine.
   std::vector<StringRef> RAIITypesList;
+  // List of fully qualified awaitable types for which are considered safe to
+  // co_await.
+  std::vector<StringRef> SafeAwaitablesList;
 };
 
 } // namespace clang::tidy::misc
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 93e9158d9f3fd16..4e99ebc7b5c4cb7 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
@@ -38,49 +38,46 @@ Following types are considered as hostile:
     co_await Bar();
   }
 
-Exclusions
+Options
 -------
-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.
+.. option:: RAIITypesList
 
-Example usage:
+    A semicolon-separated list of qualified types which should not be allowed to
+    persist across suspension points.
+    Eg: ``my::lockable; a::b;::my::other::lockable;``
+    The default value of this option is `"std::lock_guard;std::scoped_lock"`.
 
-.. code-block:: c++
+.. option:: SafeAwaitablesList
 
-  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 {}
-  };
+    A semicolon-separated list of qualified types of awaitables types which can
+    be safely awaited while having hostile RAII objects in scope.
 
-  task coro() {
-    const std::lock_guard l(&mu_);
-    co_await safe_awaitable{};
-  }
+    ``co_await``-ing an expression of ``awaitable`` type is considered
+    safe if the ``awaitable`` type is part of this list.
+    RAII objects persisting across such a ``co_await`` expression are
+    considered safe and hence are not flagged.
 
-  auto wait() { return safe_awaitable{}; }
+    Example usage:
 
-  task coro() {
-    const std::lock_guard l(&mu_); // No warning.
-    co_await safe_awaitable{};
-    co_await wait();
-  }
+    .. code-block:: c++
 
-Options
--------
+      // Cosnider option SafeAwaitablesList = "safe_awaitable"
+      struct safe_awaitable {
+        bool await_ready() noexcept { return false; }
+        void await_suspend(std::coroutine_handle<>) noexcept {}
+        void await_resume() noexcept {}
+      };
+      auto wait() { return safe_awaitable{}; }
 
-.. option:: RAIITypesList
+      task coro() {
+        // This persists across both the co_await's but is not flagged
+        // because the awaitable is considered safe to await on.
+        const std::lock_guard l(&mu_);
+        co_await safe_awaitable{};
+        co_await wait();
+      }
+
+    Eg: ``my::safe::awaitable;other::awaitable``
+    The default value of this option is empty string `""`.
 
-    A semicolon-separated list of qualified types which should not be allowed to
-    persist across suspension points.
-    Eg: ``my::lockable; a::b;::my::other::lockable;``
-    The default value of this option is `"std::lock_guard;std::scoped_lock"`.
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 84649ae8afa52b8..246a0d2a01d017f 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
@@ -1,7 +1,8 @@
 // RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \
-// RUN:   -config="{CheckOptions: \
-// RUN:             {misc-coroutine-hostile-raii.RAIITypesList: \
-// RUN:               'my::Mutex; ::my::other::Mutex'}}"
+// RUN:   -config="{CheckOptions: {\
+// RUN:             misc-coroutine-hostile-raii.RAIITypesList: 'my::Mutex; ::my::other::Mutex', \
+// RUN:             misc-coroutine-hostile-raii.SafeAwaitablesList: 'safe::awaitable; ::my::other::awaitable' \
+// RUN:             }}"
 
 namespace std {
 
@@ -135,14 +136,18 @@ ReturnObject scopedLockableTest() {
     absl::Mutex no_warning_5;
 }
 
-struct [[clang::annotate("coro_raii_safe_suspend")]] raii_safe_suspend {
+namespace safe {
+  struct awaitable {
   bool await_ready() noexcept { return false; }
   void await_suspend(std::coroutine_handle<>) noexcept {}
   void await_resume() noexcept {}
 };
+} // namespace safe
 ReturnObject RAIISafeSuspendTest() {
   absl::Mutex a;
-  co_await raii_safe_suspend{};
+  co_await safe::awaitable{};
+  using other = safe::awaitable;
+  co_await other{};
 } 
 
 void lambda() {

>From fd455bf3bb32191e728f1961917d7eda5cdc4227 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 13:13:31 +0100
Subject: [PATCH 09/14] typo fix

---
 clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index 3821e255a81c2c0..9399374518c24a3 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -43,8 +43,7 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
   // List of fully qualified types which should not persist across a suspension
   // point in a coroutine.
   std::vector<StringRef> RAIITypesList;
-  // List of fully qualified awaitable types for which are considered safe to
-  // co_await.
+  // List of fully qualified awaitable types which are considered safe to co_await.
   std::vector<StringRef> SafeAwaitablesList;
 };
 

>From 73c512f312f7333722551f76c371601223eb507b Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 13:16:34 +0100
Subject: [PATCH 10/14] format

---
 clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index 9399374518c24a3..b07351debc405ce 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -43,7 +43,8 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
   // List of fully qualified types which should not persist across a suspension
   // point in a coroutine.
   std::vector<StringRef> RAIITypesList;
-  // List of fully qualified awaitable types which are considered safe to co_await.
+  // List of fully qualified awaitable types which are considered safe to
+  // co_await.
   std::vector<StringRef> SafeAwaitablesList;
 };
 

>From 6a4d8d96ef3718b6c2d2187643007ac273a8c042 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 13:27:43 +0100
Subject: [PATCH 11/14] refactor typeWithNameIn

---
 .../clang-tidy/misc/CoroutineHostileRAIICheck.cpp    | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index 3a98db34b3a887d..c5623f40ad09058 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -59,6 +59,11 @@ AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>,
   return Node.getCommonExpr() &&
          InnerMatcher.matches(*Node.getCommonExpr(), Finder, Builder);
 }
+
+auto typeWithNameIn(const std::vector<StringRef> &Names) {
+  return hasType(
+      hasCanonicalType(hasDeclaration(namedDecl(hasAnyName(Names)))));
+}
 } // namespace
 
 CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
@@ -74,11 +79,8 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
   auto ScopedLockable = varDecl(hasType(hasCanonicalType(hasDeclaration(
                                     hasAttr(attr::Kind::ScopedLockable)))))
                             .bind("scoped-lockable");
-  auto OtherRAII = varDecl(hasType(hasCanonicalType(hasDeclaration(
-                               namedDecl(hasAnyName(RAIITypesList))))))
-                       .bind("raii");
-  auto SafeSuspend = awaiatable(hasType(hasCanonicalType(
-      hasDeclaration(namedDecl(hasAnyName(SafeAwaitablesList))))));
+  auto OtherRAII = varDecl(typeWithNameIn(RAIITypesList)).bind("raii");
+  auto SafeSuspend = awaiatable(typeWithNameIn(SafeAwaitablesList));
   Finder->addMatcher(
       expr(anyOf(coawaitExpr(unless(SafeSuspend)), coyieldExpr()),
            forEachPrevStmt(

>From fd7aaf63d039bc82abeb9d317dfaed62001cd222 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 13:29:47 +0100
Subject: [PATCH 12/14] typo fix

---
 .../docs/clang-tidy/checks/misc/coroutine-hostile-raii.rst      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 4e99ebc7b5c4cb7..5dd951615100797 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
@@ -62,7 +62,7 @@ Options
 
     .. code-block:: c++
 
-      // Cosnider option SafeAwaitablesList = "safe_awaitable"
+      // Consider option SafeAwaitablesList = "safe_awaitable"
       struct safe_awaitable {
         bool await_ready() noexcept { return false; }
         void await_suspend(std::coroutine_handle<>) noexcept {}

>From 411e0ab1b624a1a34e9fa56d5c65730d59cfc76c Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 21 Nov 2023 16:05:07 +0100
Subject: [PATCH 13/14] address comments

---
 .../clang-tidy/misc/CoroutineHostileRAIICheck.cpp | 15 ++++++++-------
 .../clang-tidy/misc/CoroutineHostileRAIICheck.h   |  2 +-
 .../checks/misc/coroutine-hostile-raii.rst        |  4 ++--
 .../checkers/misc/coroutine-hostile-raii.cpp      |  2 +-
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index c5623f40ad09058..6c8dd671539a813 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -56,8 +56,9 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
 // Matches the expression awaited by the `co_await`.
 AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>,
               InnerMatcher) {
-  return Node.getCommonExpr() &&
-         InnerMatcher.matches(*Node.getCommonExpr(), Finder, Builder);
+  if (Expr *E = Node.getCommonExpr())
+    return InnerMatcher.matches(*E, Finder, Builder);
+  return false;
 }
 
 auto typeWithNameIn(const std::vector<StringRef> &Names) {
@@ -71,8 +72,8 @@ CoroutineHostileRAIICheck::CoroutineHostileRAIICheck(StringRef Name,
     : ClangTidyCheck(Name, Context),
       RAIITypesList(utils::options::parseStringList(
           Options.get("RAIITypesList", "std::lock_guard;std::scoped_lock"))),
-      SafeAwaitablesList(utils::options::parseStringList(
-          Options.get("SafeAwaitablesList", ""))) {}
+      AllowedAwaitablesList(utils::options::parseStringList(
+          Options.get("AllowedAwaitablesList", ""))) {}
 
 void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
   // A suspension happens with co_await or co_yield.
@@ -80,9 +81,9 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
                                     hasAttr(attr::Kind::ScopedLockable)))))
                             .bind("scoped-lockable");
   auto OtherRAII = varDecl(typeWithNameIn(RAIITypesList)).bind("raii");
-  auto SafeSuspend = awaiatable(typeWithNameIn(SafeAwaitablesList));
+  auto AllowedSuspend = awaiatable(typeWithNameIn(AllowedAwaitablesList));
   Finder->addMatcher(
-      expr(anyOf(coawaitExpr(unless(SafeSuspend)), coyieldExpr()),
+      expr(anyOf(coawaitExpr(unless(AllowedSuspend)), coyieldExpr()),
            forEachPrevStmt(
                declStmt(forEach(varDecl(anyOf(ScopedLockable, OtherRAII))))))
           .bind("suspension"),
@@ -109,6 +110,6 @@ void CoroutineHostileRAIICheck::storeOptions(
   Options.store(Opts, "RAIITypesList",
                 utils::options::serializeStringList(RAIITypesList));
   Options.store(Opts, "SafeAwaitableList",
-                utils::options::serializeStringList(SafeAwaitablesList));
+                utils::options::serializeStringList(AllowedAwaitablesList));
 }
 } // namespace clang::tidy::misc
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
index b07351debc405ce..be925097692a485 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.h
@@ -45,7 +45,7 @@ class CoroutineHostileRAIICheck : public ClangTidyCheck {
   std::vector<StringRef> RAIITypesList;
   // List of fully qualified awaitable types which are considered safe to
   // co_await.
-  std::vector<StringRef> SafeAwaitablesList;
+  std::vector<StringRef> AllowedAwaitablesList;
 };
 
 } // namespace clang::tidy::misc
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 5dd951615100797..a39c1853b313c49 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
@@ -48,7 +48,7 @@ Options
     Eg: ``my::lockable; a::b;::my::other::lockable;``
     The default value of this option is `"std::lock_guard;std::scoped_lock"`.
 
-.. option:: SafeAwaitablesList
+.. option:: AllowedAwaitablesList
 
     A semicolon-separated list of qualified types of awaitables types which can
     be safely awaited while having hostile RAII objects in scope.
@@ -62,7 +62,7 @@ Options
 
     .. code-block:: c++
 
-      // Consider option SafeAwaitablesList = "safe_awaitable"
+      // Consider option AllowedAwaitablesList = "safe_awaitable"
       struct safe_awaitable {
         bool await_ready() noexcept { return false; }
         void await_suspend(std::coroutine_handle<>) noexcept {}
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 246a0d2a01d017f..55a7e4b8f2954aa 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
@@ -1,7 +1,7 @@
 // RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \
 // RUN:   -config="{CheckOptions: {\
 // RUN:             misc-coroutine-hostile-raii.RAIITypesList: 'my::Mutex; ::my::other::Mutex', \
-// RUN:             misc-coroutine-hostile-raii.SafeAwaitablesList: 'safe::awaitable; ::my::other::awaitable' \
+// RUN:             misc-coroutine-hostile-raii.AllowedAwaitablesList: 'safe::awaitable; ::my::other::awaitable' \
 // RUN:             }}"
 
 namespace std {

>From b33e46ae67bbd73aeba72514ab27794c0b472466 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Mon, 27 Nov 2023 16:20:08 +0100
Subject: [PATCH 14/14] fix typo

---
 .../clang-tidy/misc/CoroutineHostileRAIICheck.cpp             | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index 6c8dd671539a813..a0e8700b0522bc5 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -54,7 +54,7 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
 }
 
 // Matches the expression awaited by the `co_await`.
-AST_MATCHER_P(CoawaitExpr, awaiatable, ast_matchers::internal::Matcher<Expr>,
+AST_MATCHER_P(CoawaitExpr, awaitable, ast_matchers::internal::Matcher<Expr>,
               InnerMatcher) {
   if (Expr *E = Node.getCommonExpr())
     return InnerMatcher.matches(*E, Finder, Builder);
@@ -81,7 +81,7 @@ void CoroutineHostileRAIICheck::registerMatchers(MatchFinder *Finder) {
                                     hasAttr(attr::Kind::ScopedLockable)))))
                             .bind("scoped-lockable");
   auto OtherRAII = varDecl(typeWithNameIn(RAIITypesList)).bind("raii");
-  auto AllowedSuspend = awaiatable(typeWithNameIn(AllowedAwaitablesList));
+  auto AllowedSuspend = awaitable(typeWithNameIn(AllowedAwaitablesList));
   Finder->addMatcher(
       expr(anyOf(coawaitExpr(unless(AllowedSuspend)), coyieldExpr()),
            forEachPrevStmt(



More information about the cfe-commits mailing list