[Frontend] [Coroutines] Emit error when we found incompatible allocation function in promise_type

[ChuanqiXu]

Summary

According to https://cplusplus.github.io/CWG/issues/2585.html, this fixes https://github.com/llvm/llvm-project/issues/54881

Simply, the clang tried to found (do lookup and overload resolution. Is there any better word to use than found?) allocation function in promise_type and global scope. However, this is not consistent with the standard. The standard behavior would be that the compiler shouldn't lookup in global scope in case we lookup the allocation function name in promise_type. In other words, the program is ill-formed if there is incompatible allocation function in promise type.

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11244

The error message here following the wording of GCC.

[ChuanqiXu]

Remove invalid character

[erichkeane]

clang/lib/Sema/SemaCoroutine.cpp

1312

Slight preference to just have this as a part of LookupAllocationFunction.

1329

It looks like the NewScope is the only difference between these? I wonder if it might be better off doing the PromiseContainsNew work to just set a variable to THAT and use it later?

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/lib/Sema/SemaCoroutine.cpp

1312

Yeah, the code would look better in that way. But it would do redundant lookups in that way. So I prefer the current style.

[rsmith]

clang/lib/Sema/SemaCoroutine.cpp

1312

Nit, should be PromiseContainsNew.

1320

I don't see any tests covering the case where the lookup is ambiguous. Eg, you find operator new in two different base classes of the promise type. Please can you add one?

1337

This is no longer correct given the resolution of DR2585: the function arguments should only be passed to a class-scope allocation function, not to a global one.

The logic should be:

1. See if there's any class-scope allocation functions. If so, form an argument list based on the function's parameters and try looking for an allocation function. If that succeeds, we're done.
2. Try looking for an allocation function in class scope (if there were any class-scope allocation functions) or in global scope (if not), passing just the size argument.

clang/test/SemaCXX/coroutine-allocs.cpp

23

Please can you include a complete error including the "with the function signature of 'f1'" part?

I also wonder if there's some way we can get a candidate list included here.

41

Typo 'actual'

[ChuanqiXu]

clang/lib/Sema/SemaCoroutine.cpp

1320

Sure. I've addressed this in https://github.com/llvm/llvm-project/commit/448995c521b5ccef71d063bb80f084138ac9d352

1337

Yeah. This is addressed in https://github.com/llvm/llvm-project/commit/a1ffba8d528681d55c901a997beedbc69946eb90

clang/test/SemaCXX/coroutine-allocs.cpp

23

Done. It should be possible and helpful to add a candidate list here. But it might be fine in practice since promise_type wouldn't contain too many allocation functions.

clang/docs/ReleaseNotes.rst

 - Fixed a false positive diagnostic about an unevaluated expression having no
   side effects when the expression is of VLA type and is an operand of the
   ``sizeof`` operator. Fixes `Issue 48010 <https://github.com/llvm/llvm-project/issues/48010>`_.
 - Fixed a false positive diagnostic about scoped enumerations being a C++11
   extension in C mode. A scoped enumeration's enumerators cannot be named in C
   because there is no way to fully qualify the enumerator name, so this
   "extension" was unintentional and useless. This fixes
   `Issue 42372 <https://github.com/llvm/llvm-project/issues/42372>`_.
+- Clang shouldn't lookup allocation function in global scope for coroutines
+  in case it found the allocation function name in the promise_type body.
+  This fixes Issue `Issue 54881 <https://github.com/llvm/llvm-project/issues/54881>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

clang/include/clang/Basic/DiagnosticSemaKinds.td

 >;
 def note_coroutine_function_declare_noexcept : Note<
   "must be declared with 'noexcept'"
 >;
 def warn_always_inline_coroutine : Warning<
   "this coroutine may be split into pieces; not every piece is guaranteed to be inlined"
   >,
   InGroup<AlwaysInlineCoroutine>;
+def err_coroutine_unusable_new : Error<
+  "'operator new' provided by %0 is not usable with the function signature of %1"
+>;
 } // end of coroutines issue category
 
 let CategoryName = "Documentation Issue" in {
 def warn_not_a_doxygen_trailing_member_comment : Warning<
   "not a Doxygen trailing comment">, InGroup<Documentation>, DefaultIgnore;
 } // end of documentation issue category
 
 let CategoryName = "Nullability Issue" in {

clang/lib/Sema/SemaCoroutine.cpp

for (auto *PD : FD.parameters()) {
     ExprResult PDRefExpr =
         S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
                            ExprValueKind::VK_LValue, PDLoc);
     if (PDRefExpr.isInvalid())
       return false;
 
     PlacementArgs.push_back(PDRefExpr.get());
   }
-  S.FindAllocationFunctions(Loc, SourceRange(), /*NewScope*/ Sema::AFS_Class,
+
+  bool PromiseContainNew = [this, &PromiseType]() -> bool {
+    DeclarationName NewName =
+        S.getASTContext().DeclarationNames.getCXXOperatorName(OO_New);
+    LookupResult R(S, NewName, Loc, Sema::LookupOrdinaryName);
+
+    if (PromiseType->isRecordType())
+      S.LookupQualifiedName(R, PromiseType->getAsCXXRecordDecl());
+
+    return !R.empty() && !R.isAmbiguous();
+  }();
+
+  auto LookupAllocationFunction = [&]() {
+    // [dcl.fct.def.coroutine]p9
+    //   The allocation function's name is looked up by searching for it in the
+    // scope of the promise type.
+    // - If any declarations are found, ...
+    if (PromiseContainNew)
+      S.FindAllocationFunctions(Loc, SourceRange(),
+                                /*NewScope*/ Sema::AFS_Class,
                                 /*DeleteScope*/ Sema::AFS_Both, PromiseType,
                                 /*isArray*/ false, PassAlignment, PlacementArgs,
                                 OperatorNew, UnusedResult, /*Diagnose*/ false);
+    // - Otherwise, a search is performed in the global scope.
+    else
+      S.FindAllocationFunctions(Loc, SourceRange(),
+                                /*NewScope*/ Sema::AFS_Global,
+                                /*DeleteScope*/ Sema::AFS_Both, PromiseType,
+                                /*isArray*/ false, PassAlignment, PlacementArgs,
+                                OperatorNew, UnusedResult, /*Diagnose*/ false);
+  };
+
+  LookupAllocationFunction();
 
   // [dcl.fct.def.coroutine]p9
   //   If no viable function is found ([over.match.viable]), overload resolution
   // is performed again on a function call created by passing just the amount of
   // space required as an argument of type std::size_t.
   if (!OperatorNew && !PlacementArgs.empty()) {
     PlacementArgs.clear();
-    S.FindAllocationFunctions(Loc, SourceRange(), /*NewScope*/ Sema::AFS_Class,
-                              /*DeleteScope*/ Sema::AFS_Both, PromiseType,
-                              /*isArray*/ false, PassAlignment, PlacementArgs,
-                              OperatorNew, UnusedResult, /*Diagnose*/ false);
-  }
-
-  // [dcl.fct.def.coroutine]p9
-  //   The allocation function's name is looked up by searching for it in the
-  // scope of the promise type.
-  // - If any declarations are found, ...
-  // - Otherwise, a search is performed in the global scope.
-  if (!OperatorNew) {
-    S.FindAllocationFunctions(Loc, SourceRange(), /*NewScope*/ Sema::AFS_Global,
-                              /*DeleteScope*/ Sema::AFS_Both, PromiseType,
-                              /*isArray*/ false, PassAlignment, PlacementArgs,
-                              OperatorNew, UnusedResult);
+    LookupAllocationFunction();
   }
 
   bool IsGlobalOverload =
       OperatorNew && !isa<CXXRecordDecl>(OperatorNew->getDeclContext());
   // If we didn't find a class-local new declaration and non-throwing new
   // was is required then we need to lookup the non-throwing global operator
   // instead.
   if (RequiresNoThrowAlloc && (!OperatorNew || IsGlobalOverload)) {
     auto *StdNoThrow = buildStdNoThrowDeclRef(S, Loc);
     if (!StdNoThrow)
       return false;
     PlacementArgs = {StdNoThrow};
     OperatorNew = nullptr;
     S.FindAllocationFunctions(Loc, SourceRange(), /*NewScope*/ Sema::AFS_Both,
                               /*DeleteScope*/ Sema::AFS_Both, PromiseType,
                               /*isArray*/ false, PassAlignment, PlacementArgs,
                               OperatorNew, UnusedResult);
   }
 
-  if (!OperatorNew)
+  if (!OperatorNew) {
+    if (PromiseContainNew)
+      S.Diag(Loc, diag::err_coroutine_unusable_new) << PromiseType << &FD;
+
     return false;
+  }
 
   if (RequiresNoThrowAlloc) {
     const auto *FT = OperatorNew->getType()->castAs<FunctionProtoType>();
     if (!FT->isNothrow(/*ResultIfDependent*/ false)) {
       S.Diag(OperatorNew->getLocation(),
              diag::err_coroutine_promise_new_requires_nothrow)
           << OperatorNew;
       S.Diag(Loc, diag::note_coroutine_promise_call_implicitly_required)

clang/test/SemaCXX/coroutine-allocs.cpp

+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -verify
+#include "Inputs/std-coroutine.h"
+
+namespace std {
+typedef decltype(sizeof(int)) size_t;
+}
+
+struct Allocator {};
+
+struct resumable {
+  struct promise_type {
+    void *operator new(std::size_t sz, Allocator &);
+
+    resumable get_return_object() { return {}; }
+    auto initial_suspend() { return std::suspend_always(); }
+    auto final_suspend() noexcept { return std::suspend_always(); }
+    void unhandled_exception() {}
+    void return_void(){};
+  };
+};
+
+resumable f1() { // expected-error {{'operator new' provided by 'std::coroutine_traits<resumable>::promise_type' (aka 'resumable::promise_type') is not usable}}
+  co_return;
+}
+
+// NOTE: Although the argument here is a rvalue reference and the corresponding
+// allocation function in resumable::promise_type have lvalue references, it looks
+// the signature of f2 is invalid. But according to [dcl.fct.def.coroutine]p4:
+//
+//   In the following, pi is an lvalue of type Pi, where p1 denotes the object 
+//   parameter and pi+1 denotes the ith non-object function parameter for a
+//   non-static member function.
+//
+// And [dcl.fct.def.coroutine]p9.1
+//
+//   overload resolution is performed on a function call created by assembling an argument list.
+//   The first argument is the amount of space requested, and has type std::size_­t.
+//   The lvalues p1…pn are the succeeding arguments.
+//
+// So the acctual type passed to resumable::promise_type::operator new is lvalue
+// Allocator. It is allowed  to convert a lvalue to a lvalue reference. So the 
+// following one is valid.
+resumable f2(Allocator &&) {
+  co_return;
+}
+
+resumable f3(Allocator &) {
+  co_return;
+}
+
+resumable f4(Allocator) {
+  co_return;
+}
+
+resumable f5(const Allocator) { // expected-error {{operator new' provided by 'std::coroutine_traits<resumable, const Allocator>::promise_type' (aka 'resumable::promise_type') is not usable}}
+  co_return;
+}
+
+resumable f6(const Allocator &) { // expected-error {{operator new' provided by 'std::coroutine_traits<resumable, const Allocator &>::promise_type' (aka 'resumable::promise_type') is not usable}}
+  co_return;
+}