[clang] Mixed coroutine namespaces

[urnathan]

Summary

Chuanqi's patch to detect mixed std and std::experimental namespace use for coroutine types (D108696) was a little aggressive.

a) Users converting code might use a using-decl from std::experimental::coroutine_traits to std::coroutine_traits, or vice-versa. We should be silent about that, it's not problematic.

b) Users who have conflicting declarations in the two namespaces should be told about it, but we should not prevent compilation -- there's nothing ill-defined about having stuff in std::experimental. But you are probably confused. So this becomes a warning.

While there I reorganized the code -- we look in both places, if we found nothing bail. Otherwise check the thing we found is a template (prefering std's result here). Then finally if we found it only in std::experimental, of if we found different things in both places also warn.

The diagnostics provide a note indicating the location of what we found, so the user is not left guessing where the problematic declaration resides.

[• Quuxplusone]

Just nits and I'm-confused comments from me. :) I defer to someone who knows the codepaths involved. (@ChuanqiXu?)

clang/include/clang/Basic/DiagnosticSemaKinds.td

11049–11052

Perhaps s/for coroutine/for coroutine components/. And then I wonder why there's a second sentence in this message — could you just emit warn_deprecated_coroutine_namespace as well?
If we're tightening up these messages, then maybe

def warn_deprecated_coroutine_namespace : Warning<
  "support for std::experimental::%0 will be removed in LLVM 15. "
  "Use std::%0 instead.">,
  InGroup<DeprecatedExperimentalCoroutine>;

For simple wording changes like the removal of "Please", or the refactor I just suggested, which then have trivial but distracting effects in the test files, personally I would suggest you split those wording changes into a teeny-tiny separate PR and get them landed ASAP. IIUC, the main point of this PR is to downgrade this err_ into a warn_.

clang/lib/Sema/SemaCoroutine.cpp

1727–1728

Is this a rewrite of the comment on old lines 1669-1671? I find the old comment good and this new comment confusing. Is the new comment saying that something was fixed on 2021-12-16? that it should be fixed on/by that date? That we should look in ::std::experimental but then also we should and/or did remove std::experimental handling? Is std::experimental different from ::std::experimental? Basically this comment is just super confusing, and it's only through the context from the old code that I'm guessing it's probably intended to mean the-same-thing-as-the-old-comment.

1767–1768

"Not ill-formed" seems wrong to me; I believe it is ill-formed pre-C++20 to use std::coroutine_traits at all, and post-C++20 it's ill-formed to specialize std::coroutine_traits in a way that ultimately depends on stdex::coroutine_handle<> instead of the mandated std::coroutine_handle<>. So if Clang's going to be nice and not-error-out here, that's not because of anything about ill-formedness; it's more about Clang going out of its way to be nice to a certain use-case (which it would be helpful to explain right here in the comment, if possible).

clang/test/SemaCXX/Inputs/std-coroutine-exp-namespace.h

18–22 ↗

(On Diff #395138)

I'm surprised that CI is happy with this trick. I would not have expected the parser-of-expected-comments to respect arbitrary #ifs. I would have expected to see these comments done like what's in clang/test/CXX/class/class.init/class.copy.elision/p3.cpp.

That said, I'm also confused about why this .h file has a RUN: line at the top at all.
(If what you've got seems to work, and nobody can identify a reason it's secretly not working, please don't let my confusion be a blocker.)

clang/test/SemaCXX/co_await-range-for-exp-namespace.cpp

54–56 ↗

(On Diff #395138)

Please revert the formatting changes here.
(Trunk clang-format should be able to handle for co_await as of a week or two ago, IIRC.)

clang/test/SemaCXX/coroutine-mixed3-exp-namespace.cpp

3

Here and above: compliler

[urnathan]

thanks for the comments

clang/include/clang/Basic/DiagnosticSemaKinds.td

11049–11052

ok, old habit of just folding in drive by cleanups.

clang/lib/Sema/SemaCoroutine.cpp

1727–1728

I put the date there to remind us all of when this code was commited and how stale the fixme has become -- I did that with a bunch of zygoloid's hacks working around various brokenness in gcc's libstdc++ for instance. I see that the lack of a white space between the two comments is confusing things. Perhaps move the FIXME to the outermost block or something like that?

1767–1768

I'm not sure what you're saying and/or asking for here.

Are you saying that it's ill-formed to place things inside ::std that the std does not mention? (and therefore finding std::experimental::coroutine_traits there would be ill-formed, and we should reject). ISTM that that would be picky to reject translation because 'hey, we found this decl the std does not specify'

Are you asking for -std=c++17 -fcoroutines to continue to look in std::experimental indefinitely -- that seems undesired. Pedantically, that combination of options has already stepped outside the std.

clang/test/SemaCXX/Inputs/std-coroutine-exp-namespace.h

18–22 ↗

(On Diff #395138)

oh, I was unaware of the copy.elision way of doing this and had previously learned the #if way of doing it! No I don't know why there's a RUN line there.

clang/test/SemaCXX/co_await-range-for-exp-namespace.cpp

54–56 ↗

(On Diff #395138)

ok. /me goes refresh his clang-format

[ChuanqiXu]

Hi, thanks for looking into this! At least, this is a good refactor. The code itself is more clear now. I thought the mixed-use of std and stdex namespace as error since that <coroutine> and <experimental/coroutine> are two different files. And I think the case only exists when people are careless to clean coroutine uses in experimental namespace. A basic assumption I made is that it shouldn't be hard (I want to say easy in fact) to move coroutine components from stdex namespace to std namespace. I spent minutes to make the conversion in our code bases.

I don't have a strong objection in downgrade the error to the warning. But the test coroutine-mixed-exp-namespace.cpp shows that there is still another error after we downgrade the error to the warning. This looks no good. At least the original diagnostic is more clear for users. So I think we should keep the original diagnostic as error.

And I disagree with the argument:

Users converting code might use a using-decl from std::experimental::coroutine_traits to std::coroutine_traits, or vice-versa. We should be silent about that, it's not problematic.

I think we should warn/error for this definitely. It is just a workaround. And <experimental/coroutine> would be removed in recent future. So user should know about it. Otherwise they might met unimagined error after they update clang/libcxx someday.

clang/lib/Sema/SemaCoroutine.cpp

1722

I think we could remove the comment since the code itself is clear enough.

1727–1728

I prefer to remain the original comment. We could add a blank line if we prefer.

1743–1749

How about "Prefer coroutine_traits in std namespace"?

1747

Maybe:

According to [dcl.fct.def.coroutine]/p2, std::coroutine_traits is required to be template declaration.

1757

How about:

Warn if coroutine_traits only exists in deprecated std::experimental namespace.

1767–1768

I guess what Arthur saying is the wording about "ill-formed". The word "ill-formed" is about the standard. And from the perspective of standard, the mixed use should be ill-formed. So the comment is wrong. So we couldn't use the term "ill-formed" here.

clang/test/SemaCXX/Inputs/std-coroutine-exp-namespace.h

18–22 ↗

(On Diff #395138)

+1 to that it is surprising that the macro could be used with expected directives.

clang/test/SemaCXX/coroutine-mixed-exp-namespace.cpp

28 ↗

(On Diff #395138)

If there is still another error, I prefer to remain the original diagnostic as error. At least it is more clear for users.

[• Quuxplusone]

clang/lib/Sema/SemaCoroutine.cpp

1767–1768

Right. From context, "this is not ill-formed" should mean "the programmer's code, that mixes std::experimental and std, is not ill-formed according to the standard" — and I claim that's wrong. Mixing std::experimental and std is ill-formed. Especially if you end up with a specialization of std::coroutine_traits that mentions std::experimental::coroutine_handle, or vice versa.

Are you asking for -std=c++17 -fcoroutines to continue to look in std::experimental indefinitely

I wasn't consciously thinking at that level of detail at all... but honestly, I think that makes sense to me! libc++ is not planning to support C++20 <coroutine> outside of C++20 mode, and we are planning to deprecate and remove <experimental/coroutine>, so (AFAIK) libc++'s position is that if the programmer wants to use coroutiney stuff in C++, they need to migrate to C++20 ASAP. AIUI, there is no reason for the C++17 compiler ever to look in std::coroutine_traits, because the C++17 libc++ will never provide it.

I don't think it matters what the C++17-mode behavior becomes, as long as we don't break existing C++17 code (that uses <experimental/coroutine>) for a couple more releases.

[ChuanqiXu]

clang/lib/Sema/SemaCoroutine.cpp

1767–1768

Agreed. We should keep the behavior under C++17.

[urnathan]

This is that diff I was aiming for. When transitioning code from coroutines-ts to c++20, it can be useful to add a using declaration to std::experimental pointing to std::coroutine_traits. This permits that use by checking whether lookup in std::experimentl finds a different decl to lookup in std. You still get a warning about std::experimental::coroutine_traits being a thing, just not an error.

(depends on D116164)

[ChuanqiXu]

In D115943#3206518, @urnathan wrote:

This is that diff I was aiming for. When transitioning code from coroutines-ts to c++20, it can be useful to add a using declaration to std::experimental pointing to std::coroutine_traits. This permits that use by checking whether lookup in std::experimentl finds a different decl to lookup in std. You still get a warning about std::experimental::coroutine_traits being a thing, just not an error.

(depends on D116164)

May I ask a question about transiting code from coroutines-ts to c++20? I am wondering why we need even a middle state when transiting codes from coroutines-ts to c+++20. It looks pretty easy for me. I write coroutine codes too. And it is the reason why I made previous decision.
(No mean to offense. As you may know, I am not a native speaker so that I couldn't take care of the emotion feeling of the words)

[urnathan]

sure, this is actually an issue where I work, and the folly library does this, so is now breaking with (what will be) clang-14. My understanding is it's too hard to transition everything all at once, and the using-decl trick allows this conversion to be more gradual.

[ChuanqiXu]

In D115943#3207811, @urnathan wrote:

sure, this is actually an issue where I work, and the folly library does this, so is now breaking with (what will be) clang-14. My understanding is it's too hard to transition everything all at once, and the using-decl trick allows this conversion to be more gradual.

So I guess the problem happens when the projects depends on 3rd-party coroutine libraries. Generally, we don't want to edit codes in 3rd party libraries. So it becomes an issue, is it? This patch looks good to me. Let's wait for D116164 landed.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11049–11051

Maybe we could add a note here to notice user that they could use using to avoid the error.

[urnathan]

Updated expected-diag marking

[urnathan]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11049–11051

There are many ways to resolve the problem, and we want to drive users to removing std::experimental::coroutine_traits I guess. So let's not drive them towards the using-decl workaround?

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11049–11051

Yeah, I agree that we should tell users to remove std::experimental traits.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_implied_coroutine_type_not_found : Error<
   "%0 type was not found; include <coroutine> before defining "
   "a coroutine; include <experimental/coroutine> if your version "
   "of libcxx is less than 14.0">;
 def warn_deprecated_coroutine_namespace : Warning<
   "support for std::experimental::%0 will be removed in LLVM 15; "
   "use std::%0 instead">,
   InGroup<DeprecatedExperimentalCoroutine>;
 def err_mixed_use_std_and_experimental_namespace_for_coroutine : Error<
-  "mixed use of std and std::experimental namespaces for "
+  "conflicting mixed use of std and std::experimental namespaces for "
   "coroutine components">;
 def err_implicit_coroutine_std_nothrow_type_not_found : Error<
   "std::nothrow was not found; include <new> before defining a coroutine which "
   "uses get_return_object_on_allocation_failure()">;
 def err_malformed_std_nothrow : Error<
   "std::nothrow must be a valid variable declaration">;
 def err_malformed_std_coroutine_handle : Error<
   "std::coroutine_handle isn't a class template">;
 def err_coroutine_handle_missing_member : Error<
   "std::coroutine_handle must have a member named '%0'">;

clang/lib/Sema/SemaCoroutine.cpp

 }
 
 ClassTemplateDecl *Sema::lookupCoroutineTraits(SourceLocation KwLoc,
                                                SourceLocation FuncLoc,
                                                NamespaceDecl *&Namespace) {
   if (!StdCoroutineTraitsCache) {
     // Because coroutines moved from std::experimental in the TS to std in
     // C++20, we look in both places to give users time to transition their
     // TS-specific code to C++20.  Diagnostics are given when the TS usage is
     // discovered.
     // TODO: Become stricter when <experimental/coroutine> is removed.
 
     auto const &TraitIdent = PP.getIdentifierTable().get("coroutine_traits");
 
     NamespaceDecl *StdSpace = getStdNamespace();
     LookupResult ResStd(*this, &TraitIdent, FuncLoc, LookupOrdinaryName);
     bool InStd = StdSpace && LookupQualifiedName(ResStd, StdSpace);
 
     NamespaceDecl *ExpSpace = lookupStdExperimentalNamespace();
     LookupResult ResExp(*this, &TraitIdent, FuncLoc, LookupOrdinaryName);
     bool InExp = ExpSpace && LookupQualifiedName(ResExp, ExpSpace);
 
     if (!InStd && !InExp) {
       // The goggles, they found nothing!
       Diag(KwLoc, diag::err_implied_coroutine_type_not_found)
           << "std::coroutine_traits";
       return nullptr;
     }
 
     // Prefer ::std to std::experimental.
     auto &Result = InStd ? ResStd : ResExp;
     CoroTraitsNamespaceCache = InStd ? StdSpace : ExpSpace;
 
     // coroutine_traits is required to be a class template.
     StdCoroutineTraitsCache = Result.getAsSingle<ClassTemplateDecl>();
     if (!StdCoroutineTraitsCache) {
       Result.suppressDiagnostics();
       NamedDecl *Found = *Result.begin();
       Diag(Found->getLocation(), diag::err_malformed_std_coroutine_traits);
       return nullptr;
     }
 
     if (InExp) {
       // Found in std::experimental
       Diag(KwLoc, diag::warn_deprecated_coroutine_namespace)
           << "coroutine_traits";
       ResExp.suppressDiagnostics();
       auto *Found = *ResExp.begin();
       Diag(Found->getLocation(), diag::note_entity_declared_at) << Found;
 
-      if (InStd) {
-        // Also found in std
+      if (InStd &&
+          StdCoroutineTraitsCache != ResExp.getAsSingle<ClassTemplateDecl>()) {
+        // Also found something different in std
         Diag(KwLoc,
              diag::err_mixed_use_std_and_experimental_namespace_for_coroutine);
         Diag(StdCoroutineTraitsCache->getLocation(),
              diag::note_entity_declared_at)
             << StdCoroutineTraitsCache;
 
         return nullptr;
       }
     }
   }
   Namespace = CoroTraitsNamespaceCache;
   return StdCoroutineTraitsCache;
 }

clang/test/SemaCXX/coroutine-mixed3-exp-namespace.cpp

+// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
+
+#include "Inputs/std-coroutine.h"
+
+namespace std::experimental {
+using std::coroutine_handle;
+using std::coroutine_traits; // expected-note{{declared here}}
+} // namespace std::experimental
+
+struct my_awaitable {
+  bool await_ready() noexcept;
+  void await_suspend(std::coroutine_handle<> coro) noexcept;
+  void await_resume() noexcept;
+};
+
+struct promise_void {
+  void get_return_object();
+  my_awaitable initial_suspend();
+  my_awaitable final_suspend() noexcept;
+  void return_void();
+  void unhandled_exception();
+};
+
+template <>
+struct std::coroutine_traits<void> { using promise_type = promise_void; };
+
+void test() {
+  co_return;
+  // expected-warning@-1{{support for std::experimental::coroutine_traits will be removed}}
+}

clang/test/SemaCXX/coroutine-mixed4-exp-namespace.cpp

+// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
+
+#include "Inputs/std-coroutine.h"
+
+namespace std::experimental {
+// expected-note@+1{{declared here}}
+template <typename T> using coroutine_traits = std::coroutine_traits<T>;
+using std::coroutine_handle;
+} // namespace std::experimental
+
+struct my_awaitable {
+  bool await_ready() noexcept;
+  void await_suspend(std::experimental::coroutine_handle<> coro) noexcept;
+  void await_resume() noexcept;
+};
+
+struct promise_void {
+  void get_return_object();
+  my_awaitable initial_suspend();
+  my_awaitable final_suspend() noexcept;
+  void return_void();
+  void unhandled_exception();
+};
+
+template <>
+struct std::coroutine_traits<void> { using promise_type = promise_void; };
+
+void test() {
+  co_return; // expected-error {{mixed use of std and std::experimental namespaces for coroutine components}}
+  // expected-warning@-1{{support for std::experimental::coroutine_traits will be removed}}
+  // expected-note@Inputs/std-coroutine.h:8 {{'coroutine_traits' declared here}}
+}