[clang] Delay dependent co_return promise calls

Project: llvm/llvm-project | Language: C++ | Concepts: type-systems, error-handling, refactoring | Difficulty: senior
Link: https://github.com/llvm/llvm-project/pull/218779

The problem

Clang previously selected either promise.return_value() or promise.return_void() at parse time for a co_return statement, even when the operand's type was dependent (i.e., not yet known due to templates). This could cause incorrect code generation in cases where, after template instantiation, the operand actually resolved to void—but Clang had already committed to calling return_value. This broke valid coroutine code using type-dependent co_return expressions.

Why it's non-trivial

A naive fix would be to always defer the decision, but this risks missing required semantic checks or mishandling temporaries and cleanups associated with the operand expression. The change must ensure that all necessary processing (like registering cleanups for temporaries with non-trivial destructors) still happens, and that the correct promise method is called after type resolution, without leaking resources or breaking the parse/instantiate pipeline.

The approach

1. Release notes update
Documents the bug and its fix for users:

+- Fixed a bug where Clang incorrectly required `promise.return_value()` for a
+  dependent `co_return` operand that inits to `void`, instead of using
+  `promise.return_void()`. (#GH218368)

This informs users that dependent co_return expressions now correctly defer the promise call selection.

2. CFG construction fix
Handles the case where the promise call might not be set yet:

-  if (CFGBlock *R = Visit(CRS->getPromiseCall()))
-    B = R;
+  if (Expr *PromiseCall = CRS->getPromiseCall())
+    if (CFGBlock *R = Visit(PromiseCall))
+      B = R;

Before, it always visited the promise call, but now it checks if the promise call exists (it may be null for type-dependent co_return), preventing a null dereference.

3. SemaCoroutine: Delay promise call selection
Defers the decision for type-dependent operands:

+  if (E && !isa<InitListExpr>(E) && E->isTypeDependent())
+    return new (Context) CoreturnStmt(Loc, E, /*PromiseCall=*/nullptr,
+                                      IsImplicit);

If the operand is type-dependent (and not an init-list), immediately build a CoreturnStmt without setting the promise call. This leaves the choice for later, after template instantiation, when the operand type is known.

4. Test coverage
Adds a test that covers the dependent operand case:

+namespace dependent_void_coreturn {
+struct coro {
+  struct promise_type {
+    coro get_return_object();
+    suspend_never initial_suspend();
+    suspend_never final_suspend() noexcept;
+    void unhandled_exception();
+    void return_void();
+  };
+};
+
+struct Ctx {
+  template <typename T>
+  T &get();
+  void f(int);
+};
+
+template <typename T>
+coro f(Ctx &ctx) {
+  auto &v = ctx.get<T>();
+  co_return ctx.f(v);
+}
+
+void use(Ctx &ctx) { f<int>(ctx); }
+}

This test ensures that a type-dependent co_return operand, which resolves to void, will use return_void as intended.

From the review thread

xakep8:
I don't think this will change anything during CIR lowering but its worth considering a defensive null check in CIRGen, it might not be necessary but just a suggestion. Maybe with an assertion like

assert(S.getPromiseCall() && "co_return should have a promise call");

so that if any other change down the line tries to lower an unresolved dependent co_return then it would be caught in debug build and it would also make this more solid.

Please let me know what you think about this.

Andres-Salamanca:
Hi, no problem from my side. In CIR, co_return is actually a block terminator that jumps to the final suspend region and does not carry a value, so I don't see any issue with this change affecting the lowering to CIR.

For example, the void case is already handled as:

cir.call @_ZN5folly4coro4TaskIvE12promise_type11return_voidEv(...) nothrow : (...) -> ()
cir.co_return

So the distinction between return_void and return_value is handled before reaching the cir.co_return terminator.

MatzeB:
We have identified this change for causing compilation problems in our codebase. Minimized repro:

#include <coroutine>

struct NT { ~NT(); };                       // non-trivial dtor -> registers a cleanup

struct Task {
  struct promise_type {
    Task get_return_object();
    std::suspend_always initial_suspend() noexcept;
    std::suspend_always final_suspend() noexcept;
    void return_value(NT);
    void unhandled_exception();
  };
};

template <class A, class B> NT mk(A, B);

template <class T>
struct C {
  // co_return operand is type-dependent (T), but contains a concrete
  // temporary NT() whose destructor registers a cleanup.
  Task f() { co_return mk(NT(), T()); }
};

$ clang++ -std=c++20 -fsyntax-only min.cpp
...
clang++: llvm-project/clang/lib/Sema/SemaDecl.cpp:17308: Decl *clang::Sema::ActOnFinishFunctionBody(Decl *, Stmt *, bool, bool): Assertion `!Cleanup.exprNeedsCleanups() && "Unaccounted cleanups in function"' failed.
...

MatzeB:
My agent explains (haven't checked this explanation myself, only tested the repro is fine after reverting this change):

In Sema::BuildCoreturnStmt, the new early return builds the CoreturnStmt and returns immediately, bypassing the full-expression handling below it. A temporary in the operand registers a cleanup that is then never consumed, so Cleanup.exprNeedsCleanups() is still set when the body finishes.

Three ingredients, all required — dropping any one compiles clean:
1. type-dependent co_return operand → takes the new early-return path
2. a concrete temporary with a non-trivial destructor inside it → actually registers a cleanup
3. the coroutine is a member of a class template — a late-parsed inline method body. The same code as a free function template does not assert, which matches ParseLexedMethodDef in the stack.

Takeaways

• When deferring semantic decisions for type-dependent constructs, ensure that all necessary cleanup and resource management code still executes, even if the main semantic action is delayed.
• Defensive programming (e.g., null checks or assertions) is important when introducing new states (like unset promise calls) to avoid downstream errors or crashes.
• Comprehensive regression tests for template-dependent code paths are essential, as subtle changes in the parse/instantiate pipeline can have wide-ranging effects, especially with temporaries and destructors.