Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[misc-coroutine-hostile-raii] Use getOperand instead of getCommonExpr. #79206

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

usx95
Copy link
Contributor

@usx95 usx95 commented Jan 23, 2024

We were previously allowlisting awaitable types returned by await_transform instead of the type of the operand of the co_await expression.

This previously used to give false positives and not respect the AllowedAwaitablesList flag when await_transform is used. See added test cases for such examples.

@usx95 usx95 requested a review from PiotrZSL January 23, 2024 20:24
@usx95 usx95 marked this pull request as ready for review January 23, 2024 20:24
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 23, 2024

@llvm/pr-subscribers-clang-tidy

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

Author: Utkarsh Saxena (usx95)

Changes

We were previously allowlisting awaitable types returned by await_transform instead of the type of the operand of the co_await expression.

This previously used to give false positives and not respect the AllowedAwaitablesList flag when await_transform is used. See added test cases for such examples.


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

2 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp (+1-1)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp (+33-1)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index a0e8700b0522bc5..360335b86c6418e 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -56,7 +56,7 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
 // Matches the expression awaited by the `co_await`.
 AST_MATCHER_P(CoawaitExpr, awaitable, ast_matchers::internal::Matcher<Expr>,
               InnerMatcher) {
-  if (Expr *E = Node.getCommonExpr())
+  if (Expr *E = Node.getOperand())
     return InnerMatcher.matches(*E, Finder, Builder);
   return false;
 }
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 55a7e4b8f2954aa..c23c355dac1b2ae 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.AllowedAwaitablesList: 'safe::awaitable; ::my::other::awaitable' \
+// RUN:             misc-coroutine-hostile-raii.AllowedAwaitablesList: 'safe::awaitable; ::transformable::awaitable' \
 // RUN:             }}"
 
 namespace std {
@@ -136,6 +136,9 @@ ReturnObject scopedLockableTest() {
     absl::Mutex no_warning_5;
 }
 
+// ================================================================================
+// Safe awaitable
+// ================================================================================
 namespace safe {
   struct awaitable {
   bool await_ready() noexcept { return false; }
@@ -150,6 +153,32 @@ ReturnObject RAIISafeSuspendTest() {
   co_await other{};
 } 
 
+// ================================================================================
+// Safe transformable awaitable
+// ================================================================================
+struct transformable { struct awaitable{}; };
+using alias_transformable_awaitable = transformable::awaitable;
+struct UseTransformAwaitable {
+  struct promise_type {
+    UseTransformAwaitable get_return_object() { return {}; }
+    std::suspend_always initial_suspend() { return {}; }
+    std::suspend_always final_suspend() noexcept { return {}; }
+    void unhandled_exception() {}
+    std::suspend_always await_transform(transformable::awaitable) { return {}; }
+  };
+};
+
+auto retAwaitable() { return transformable::awaitable{}; }
+UseTransformAwaitable RAIISafeSuspendTest2() {
+  absl::Mutex a;
+  co_await retAwaitable();
+  co_await transformable::awaitable{};
+  co_await alias_transformable_awaitable{};
+}
+
+// ================================================================================
+// Lambdas
+// ================================================================================
 void lambda() {
   absl::Mutex no_warning;
   auto lambda = []() -> ReturnObject {
@@ -164,6 +193,9 @@ void lambda() {
   absl::Mutex no_warning_2;
 }
 
+// ================================================================================
+// Denylisted RAII
+// ================================================================================
 template<class T>
 ReturnObject raii_in_template(){
   T a;

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
But please note that I'm not too familiar with coroutines.

@usx95
Copy link
Contributor Author

usx95 commented Jan 23, 2024

Thanks. This is trivial and might not require another pair of eyes.

@usx95 usx95 merged commit 729657d into llvm:main Jan 23, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants