[Clang][SemaCXX][Coroutines] Fix misleading diagnostics with -Wunsequenced

[bruno]

Summary

D115187 exposed CoroutineSuspendExpr's operand, which makes some nodes to show up twice during the traversal, confusing the check for unsequenced operations. Skip the operand since it's already handled as part of the common expression and get rid of the misleading warnings.

https://github.com/llvm/llvm-project/issues/56768

[bruno]

Test failure is unrelated.

[nridge]

clang/lib/Sema/SemaChecking.cpp

15186

Out of curiosity, since getCommonExpr() is itself a subexpression of getReadyExpr(), getSuspendExpr(), and getResumeExpr() (which are also children of the CoroutineSuspendExpr), shouldn't getCommonExpr() be skipped for the same reason?

[nridge]

clang/test/SemaCXX/warn-unsequenced-coro.cpp

7

Consider including "Inputs/std-coroutine.h" instead, as in e.g. this test

[ChuanqiXu]

clang/lib/Sema/SemaChecking.cpp

15186

since getCommonExpr() is itself a subexpression of getReadyExpr(), getSuspendExpr(), and getResumeExpr()

This looks not true. For this example:

co_await foo();

foo() is the Operand. And the Common is auto __tmp__ = operator co_await(foo()). ReadyExpr should be __tmp__.await_ready();. SuspendExpr should be __tmp__.await_suspend(); and ResumeExpr should be __tmp__.await_resume();.

So the method here looks good to me.

[bruno]

Update patch to reuse std-coroutine.h and add a few more other bits there.

[bruno]

Thanks for the review, will push once all tests pass!

clang/lib/Sema/SemaChecking.cpp

15186

foo() is the Operand. And the Common is auto __tmp__ = operator co_await(foo()). ReadyExpr should be __tmp__.await_ready();. SuspendExpr should be __tmp__.await_suspend(); and ResumeExpr should be __tmp__.await_resume();.

Exactly!

[bruno]

Update checks that rely on coroutine_traits in std-coroutine.h

[bruno]

Only test failing is ClangScanDeps/modules-full.cpp on Windoes, which is unrelated to this change.

[nridge]

clang/lib/Sema/SemaChecking.cpp

15186

Thank you for the explanation! I overlooked the fact that ReadyExpr/SuspendExpr/ResumeExpr use an OpaqueValueExpr wrapping CommonExpr rather than using CommonExpr directly.

clang/lib/Sema/SemaChecking.cpp

void VisitStmt(const Stmt *S) {
     // Skip all statements which aren't expressions for now.
   }
 
   void VisitExpr(const Expr *E) {
     // By default, just recurse to evaluated subexpressions.
     Base::VisitStmt(E);
   }
 
+  void VisitCoroutineSuspendExpr(const CoroutineSuspendExpr *CSE) {
+    for (auto *Sub : CSE->children()) {
+      const Expr *ChildExpr = dyn_cast_or_null<Expr>(Sub);
+      if (!ChildExpr)
+        continue;
+
+      if (ChildExpr == CSE->getOperand())
+        // Do not recurse over a CoroutineSuspendExpr's operand.
+        // The operand is also a subexpression of getCommonExpr(), and
+        // recursing into it directly could confuse object management
+        // for the sake of sequence tracking.
+        continue;
+
+      Visit(Sub);
+    }
+  }
+
   void VisitCastExpr(const CastExpr *E) {
     Object O = Object();
     if (E->getCastKind() == CK_LValueToRValue)
       O = getObject(E->getSubExpr(), false);
 
     if (O)
       notePreUse(O, E);
     VisitExpr(E);

clang/test/SemaCXX/warn-unsequenced-coro.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify -std=c++20 -Wno-unused -Wno-uninitialized -Wunsequenced %s
+
+// expected-no-diagnostics
+
+typedef __PTRDIFF_TYPE__ ptrdiff_t;
+
+namespace std {
+
+template<typename T> struct remove_reference       { typedef T type; };
+template<typename T> struct remove_reference<T &>  { typedef T type; };
+template<typename T> struct remove_reference<T &&> { typedef T type; };
+
+template<typename T>
+typename remove_reference<T>::type &&move(T &&t) noexcept;
+
+template <class Ret, typename... T>
+struct coroutine_traits { using promise_type = typename Ret::promise_type; };
+
+template <class Promise = void>
+struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  static coroutine_handle from_promise(Promise &promise);
+};
+template <>
+struct coroutine_handle<void> {
+  template <class PromiseType>
+  coroutine_handle(coroutine_handle<PromiseType>) noexcept;
+  static coroutine_handle from_address(void *);
+};
+
+struct suspend_always {
+  bool await_ready() noexcept { return false; }
+  void await_suspend(coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
+};
+
+struct suspend_never {
+  bool await_ready() noexcept { return true; }
+  void await_suspend(coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
+};
+
+struct input_iterator_tag {};
+struct forward_iterator_tag : public input_iterator_tag {};
+
+} // namespace std
+
+using namespace std;
+
+template<class T>
+struct Task {
+    struct promise_type {
+        Task<T> get_return_object() noexcept;
+        suspend_always initial_suspend() noexcept;
+        suspend_always final_suspend() noexcept;
+        void return_value(T);
+        void unhandled_exception();
+        auto yield_value(Task<T>) noexcept { return final_suspend(); }
+    };
+    bool await_ready() noexcept { return false; }
+    void await_suspend(coroutine_handle<>) noexcept {}
+    T await_resume();
+};
+
+template<>
+struct Task<void> {
+    struct promise_type {
+        Task<void> get_return_object() noexcept;
+        suspend_always initial_suspend() noexcept;
+        suspend_always final_suspend() noexcept;
+        void return_void() noexcept;
+        void unhandled_exception() noexcept;
+        auto yield_value(Task<void>) noexcept { return final_suspend(); }
+    };
+    bool await_ready() noexcept { return false; }
+    void await_suspend(coroutine_handle<>) noexcept {}
+    void await_resume() noexcept {}
+};
+
+template <typename T>
+class generator
+{
+  struct Promise
+  {
+    auto get_return_object() { return generator{*this}; }
+    auto initial_suspend() { return suspend_never{}; }
+    auto final_suspend() noexcept { return suspend_always{}; }
+    void unhandled_exception() {}
+    void return_void() {}
+
+    auto yield_value(T value)
+    {
+      value_ = std::move(value);
+      return suspend_always{};
+    }
+
+    T value_;
+  };
+
+  using Handle = coroutine_handle<Promise>;
+
+  struct sentinel{};
+  struct iterator
+  {
+    using iterator_category = input_iterator_tag;
+    using value_type = T;
+    using difference_type = ptrdiff_t;
+    using reference = T &;
+    using const_reference = const T &;
+    using pointer = T *;
+
+    iterator &operator++()
+    {
+      h_.resume();
+      return *this;
+    }
+    const_reference &operator*() const { return h_.promise().value_; }
+    bool operator!=(sentinel) { return !h_.done(); }
+
+    Handle h_;
+  };
+
+  explicit generator(Promise &p) : h_(Handle::from_promise(p)) {}
+  Handle h_;
+public:
+  using promise_type = Promise;
+  auto begin() { return iterator{h_}; }
+  auto end() { return sentinel{}; }
+};
+
+Task<void> c(int i) {
+  co_await (i = 0, std::suspend_always{});
+}
+
+generator<int> range(int start, int end)
+{
+  while (start < end)
+    co_yield start++;
+}
+
+Task<int> go(int const& val);
+Task<int> go1(int x) {
+  co_return co_await go(++x);
+}
 No newline at end of file