Page MenuHomePhabricator

[Coroutines] [Frontend] Lookup in std namespace first
ClosedPublic

Authored by ChuanqiXu on Aug 25 2021, 5:33 AM.

Details

Summary

Now in libcxx and clang, all the coroutine components are defined in std::experimental namespace.
And now the coroutine TS is merged into C++20. So in the working draft like N4892, we could find the coroutine components is defined in std namespace instead of std::experimental namespace.
And the coroutine support in clang seems to be relatively stable. So I think it may be suitable to move the coroutine component into the experiment namespace now.

But move the coroutine component into the std namespace may be an break change. So I planned to split this change into two patch. One in clang and other in libcxx.

This patch would make clang lookup coroutine_traits in std namespace first. For the compatibility consideration, clang would lookup in std::experimental namespace if it can't find definitions in std namespace. So the existing codes wouldn't be break after update compiler.

Test Plan: check-clang, check-libcxx

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
lxfind accepted this revision.Sep 2 2021, 2:34 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Sep 2 2021, 2:34 PM
This revision was landed with ongoing or failed builds.Sep 2 2021, 7:24 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2021, 7:24 PM
uabelho added a subscriber: uabelho.Sep 3 2021, 2:29 AM

This patch makes several libcxx tests fail due to the new warning. See e.g.
http://lab.llvm.org:8011/#/builders/60/builds/4632

gulfem added a subscriber: gulfem.Sep 3 2021, 10:32 AM

We started seeing the following test failures in our Fuchsia builds after this patch:

Failed Tests (10):
  libc++ :: libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.prom/promise.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/await_result.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/bool_await_suspend.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/expected.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/generator.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/go.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/multishot_func.pass.cpp
  libc++ :: std/experimental/language.support/support.coroutines/end.to.end/oneshot_func.pass.cpp

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8837144494402864417/+/u/clang/test/stdout

Do you have any idea what might be going wrong?

ldionne reopened this revision.Sep 3 2021, 1:00 PM
ldionne added a subscriber: ldionne.

Based on the commit description, I don't understand this change at all. Why do we want to tweak the name lookup just for std::coroutine? Yes, we do have an action item to finish coroutines in libc++ already, and I'd love to see a patch that does that, but I don't think that mandates changing Clang.

The rollout plan for coroutines should be:

  1. Make sure we implement coroutines fully
  2. Duplicate it all into namespace std
  3. In two LLVM releases, remove all the coroutines stuff in std::experimental.

I'm going to revert this for now on the basis that it breaks libc++ CI. Let's have a discussion about the above if you think I'm mistaken or if I'm misunderstanding what this patch does.

This revision is now accepted and ready to land.Sep 3 2021, 1:00 PM
ldionne reopened this revision.Sep 3 2021, 1:02 PM

(woops, this closed the rev when I committed the revert)

This revision is now accepted and ready to land.Sep 3 2021, 1:02 PM
ldionne requested changes to this revision.Sep 3 2021, 1:03 PM
This revision now requires changes to proceed.Sep 3 2021, 1:03 PM
ChuanqiXu added a comment.EditedSep 5 2021, 7:07 PM

Based on the commit description, I don't understand this change at all. Why do we want to tweak the name lookup just for std::coroutine? Yes, we do have an action item to finish coroutines in libc++ already, and I'd love to see a patch that does that, but I don't think that mandates changing Clang.

The rollout plan for coroutines should be:

  1. Make sure we implement coroutines fully
  2. Duplicate it all into namespace std
  3. In two LLVM releases, remove all the coroutines stuff in std::experimental.

I'm going to revert this for now on the basis that it breaks libc++ CI. Let's have a discussion about the above if you think I'm mistaken or if I'm misunderstanding what this patch does.

Many thanks for reverting this!

Why do we want to tweak the name lookup just for std::coroutine? Yes, we do have an action item to finish coroutines in libc++ already

This is due to a practice that:

  • People want to use clang but they prefer to use libstdc++ instead of libc++.
  • So that they would implement a coroutine.h by coping from libc++. So that they could depend on libstdc++ still.

This is a reason that I introduce the warning in the compiler side instead of libc++. But if this warning matters, I am Ok to emit the warning in the compiler side.

Then let's talk about the plans to move coroutine components into std namespace:

In D108697, the successor of this patch, we had duplicated all the components into namespace std. (And I add an warning in std::experimental by #warning directive, I am not sure if this warning may break the CI)
Then here is the problem, we must commit this before D108697, otherwise the compiler wouldn't search coroutine components in std namespace.
The solution I got now is that I should remove the warning message in this patch. Then the compiler would still try to lookup in std::experimental namespace without warning if it can't find things in std namespace.
Do you @ldionne @Quuxplusone think this solution workable?

ChuanqiXu updated this revision to Diff 370838.Sep 5 2021, 8:29 PM
ChuanqiXu edited the summary of this revision. (Show Details)

Remove warning if the compiler found std::experimental::coroutine_traits to avoid libcxx CI problems.


I still think this warning is beneficial. We need to note the user who update clang only that they need to update libcxx too.
So I think we should add this warning after D108697 merged in.

ChuanqiXu planned changes to this revision.Sep 7 2021, 6:52 PM

We should proceed after D108697 accepted.

@ldionne @Quuxplusone Now the compiler wouldn't emit warning for <experimental/coroutine> any more. So I think this is good. Do you feel good with this? This is necessary to conform the implementation of coroutine since the compiler would search coroutine components in corresponding namespace (std::experimental before and both std and std::experimental after this patch).

ChuanqiXu requested review of this revision.Sep 21 2021, 7:01 PM

This sounds more reasonable to me, however we need to ship <coroutine> in libc++ before we enable this, or else we're going to start suggesting that users include <coroutine> when we don't have it.

clang/lib/Sema/SemaCoroutine.cpp
1668
1677

Add a warning about what?

clang/test/SemaCXX/coroutines-exp-namespace.cpp
3
ChuanqiXu updated this revision to Diff 374709.Sep 23 2021, 6:59 PM

Address comments.

This sounds more reasonable to me, however we need to ship <coroutine> in libc++ before we enable this, or else we're going to start suggesting that users include <coroutine> when we don't have it.

We couldn't ship '<coroutine>' before we enable this. Otherwise the compiler couldn't find coroutine components defined in std namespace.
The patch before tries to emit warning for user who including <experimental/coroutine> but it broke the CI system of libcxx. That's the reason that the first commit get reverted.
I think the right order may be:

  • Check in this.
  • Then check in D109433.
  • Add a warning to tell people that they should include <coroutine> instead of <experimental/coroutine>. This patch should be simple enough to check in quickly.

How do you think about this order?

ChuanqiXu updated this revision to Diff 380306.Oct 18 2021, 1:36 AM

Rebase with trunk.
Hi @Quuxplusone. This patch is necessary for D109433. This patch would search the std namespace first and search std::experimental namespace if the search in std namespace failed. The reason why it got reverted before was that this original patch would emit warning when find components in std::experimental namespace. And now it wouldn't emit any warning. Would you be happy with this? I want to check-in this to avoid extra rebase work.

Hi @ChuanqiXu

sorry for what might be naive questions, but just to make sure I understand the context of this patch correctly:

This patch fixes the look up of the coroutine_traits, so that clang now search both the std and the std::experimental namespace. As such, it must land before D109433 since otherwise the new std::coroutine_traits wouldn't be found by the compiler, correct?

Also, does this patch enable me to use clang-cl to build programs which use the coroutine header from Microsoft's STL? Or are there still other incompatibilities between clang and the Microsoft STL with regards to coroutines?

Hi @ChuanqiXu

sorry for what might be naive questions, but just to make sure I understand the context of this patch correctly:

Hi, you are welcome : )

This patch fixes the look up of the coroutine_traits, so that clang now search both the std and the std::experimental namespace. As such, it must land before D109433 since otherwise the new std::coroutine_traits wouldn't be found by the compiler, correct?

Yes, correct.

Also, does this patch enable me to use clang-cl to build programs which use the coroutine header from Microsoft's STL? Or are there still other incompatibilities between clang and the Microsoft STL with regards to coroutines?

After this patch, it should be able to compile <coroutine> MS STL. Otherwise, there must be a bug in either the clang part or in the MS STL part.

@Quuxplusone gentle ping~

I think this PR is mostly above my pay grade. :)
IIUC, there is a chicken-and-egg problem between D108696 and D109433? If I understand the situation correctly (which maybe I don't), I think you should add D108696 as a "Parent Revision" of D109433, and tag both of them with both "libc++" and "clang" project tags, and then poke buildkite to re-run the CI on both of them. I would hope that D109433's test results would still be green. And then you'd land both D108696 and D109433 in very quick succession. But, I'm not going to be an approver on D108696 because it's over my head. Originally you pinged @rjmccall @lxfind @junparser ; are they still happy with D108696 and the general direction we're taking on coroutines stuff in 14.x and 15.x?

(AIUI, the intent is for libc++ 14.x to support both C++11 <experimental/coroutine> and C++20 <coroutine>, and then in libc++ 15.x to support only C++20 <coroutine>.)

clang/test/SemaCXX/coroutine_handle-addres-return-type.cpp
1

Pre-existing: in the name of this file, addres should be address

clang/test/SemaCXX/coroutines-exp-namespace.cpp
3

...and not just "could work," but "works." :)

// This file is the same as coroutines.cpp, except the components are defined in namespace std::experimental.
// The intent of this test is to make sure the std::experimental implementation still works.
// TODO: Remove this test once we drop support for <experimental/coroutine>.

Also, it occurs to me that you should probably be testing both <coroutine> and <experimental/coroutine> in all the other tests, as well; e.g. coroutine_handle-address-return-type.cpp should have a matching coroutine_handle-address-return-type-exp-namespace.cpp and so on. Otherwise, aren't you removing a lot of regression tests for <experimental/coroutine>, despite that we still claim to support <experimental/coroutine> in Clang 14.x?

In many cases, it should be possible to do something like

// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -fsyntax-only -verify %s -DINCLUDE_EXPERIMENTAL=1
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s -DINCLUDE_EXPERIMENTAL=0

#if INCLUDE_EXPERIMENTAL
#include <experimental/coroutine>
namespace coro = std::experimental;
#else
#include <coroutine>
namespace coro = std;
#endif

[...]

@Quuxplusone gentle ping~

I think this PR is mostly above my pay grade. :)

Sorry for disturbing. I would remind it.

IIUC, there is a chicken-and-egg problem between D108696 and D109433? If I understand the situation correctly (which maybe I don't), I think you should add D108696 as a "Parent Revision" of D109433, and tag both of them with both "libc++" and "clang" project tags, and then poke buildkite to re-run the CI on both of them. I would hope that D109433's test results would still be green. And then you'd land both D108696 and D109433 in very quick succession. But, I'm not going to be an approver on D108696 because it's over my head. Originally you pinged @rjmccall @lxfind @junparser ; are they still happy with D108696 and the general direction we're taking on coroutines stuff in 14.x and 15.x?

(AIUI, the intent is for libc++ 14.x to support both C++11 <experimental/coroutine> and C++20 <coroutine>, and then in libc++ 15.x to support only C++20 <coroutine>.)

Yeah, this is a Parent Revision of D109433. But I didn't tag this with 'libc++' since it didn't touch libcxx part. Do you mean it is necessary to tag it as 'libc++' even if it didn't touch libcxx?
(I would try to make D109433 green, of course)

This revision is approved originally but reverted due to the unwanted warning. I would ask them if they are happy with the current direction.

After all, thanks for looking into this : )

clang/test/SemaCXX/coroutines-exp-namespace.cpp
3

Thanks for reporting this. I would try to test both by duplicating the tests. The method to use macro may not be good due to it needs to mangled.

ChuanqiXu added inline comments.Oct 25 2021, 12:31 AM
clang/test/SemaCXX/coroutines-exp-namespace.cpp
3

(Ignore this) I just noticed that we could use the macro based method for Sema tests.

ChuanqiXu updated this revision to Diff 381878.Oct 25 2021, 1:38 AM
ChuanqiXu added a project: Restricted Project.

Rebase and address comments. Test both std::experimental::coroutine and std::coroutine in SemaCXX. All the name of tests testing std::experimental::coroutine ends with *-exp-namespace.cpp so that we could remove them easily once we want.
@lxfind @rjmccall @junparser Do you feel good with the plan that support <experiemental/coroutine> after LLVM15? Since libcxx plans to stop supporting <experimental/coroutine> in LLVM15, I think we could make a plan to remove the support for std::experimental::coroutine in clang until LLVM16.

So am I correct in understanding that the main issue with the chicken/egg problem for updating both the compiler to use the new stdlib facilities and updating the stdlib facilities is that we don't want to issue warnings about using <experimental/coroutine> and telling users to use <coroutine> instead if there is no <coroutine> header.

I wonder if a suitable transition approach here might be to have the compiler do:

  • try lookup of std::coroutine_handle/coroutine_traits; if found try to instantiate the expression (e.g. await-suspend) using the std:: type
  • if not found then try lookup of std::experimental::coroutine_handle/coroutine_traits; and if found try to instantiate the expression using the std::experimental:: type; and if successful, then;
    • if std:: type was found but instantiating the expression failed, then issue a warning about using deprecated std::experimental:: types
    • otherwise if std:: type was not found, try to find <coroutine> header and if present then issue a warning suggesting using <coroutine> instead of <experimental/coroutine>

Then you should be able to land the compiler change and it should continue to have the same behaviour for existing code using std::experimental:: types with the existing stdlib.
It is only once the stdlib changes that land which add the <coroutine> header that users would start seeing the warnings about using <coroutine> instead of <experimental/coroutine>

Note that there are still some potential breakages for code that could go on here, however.

Imagine that some header I included has been updated to use <coroutine> while my current header is still using <experimental/coroutine> so that both are in scope.

I might have defined an awaitable type that looks like the following:

c++
#include <other/header.hpp> // which transitively includes <coroutine>
#include <experimental/coroutine>

struct my_awaitable {
  bool await_ready();
  void await_suspend(std::experimental::coroutine_handle<> coro);
  void await_resume();
};

In this case, the compiler would find the std::coroutine_handle<> and try to instantiate the await-suspend expression with that type, which would fail because await_suspend() is not callable with a std::coroutine_handle.
The compiler would need to fall back to instantiating the expression with std::experimental::coroutine_handle<P>.

There is also the variation where await_suspend() is a template function that deduces the promise-type argument.
e.g.

c++
template<typename Promise>
void await_suspend(std::experimental::coroutine_handle<Promise> coro);

There are also cases where the template parameter to await_suspend() is unconstrained.
e.g.

c++
struct promise_type {
  ...
  std::experimental::coroutine_handle<> continuation;
};

struct my_awaitable {
  bool await_ready();
  template<typename CoroHandle>
  auto await_suspend(CoroHandle handle) {
    coro.promise().continuation = handle;
    return coro;
  }
  void await_resume();

  std::experimental::coroutine_handle<promise_type> coro;
};

Such a type would successfully deduce the template parameter using std::coroutine_handle and so the await-suspend expression would be valid, but the instantiation of the await_suspend() body would fail as a std::coroutine_handle<P> cannot be assigned to an lvalue of type std::experimentl::coroutine_handle<>. This would not produce a SFINAE-friendly error that the compiler could retry with std::experimental types.

I'm not sure if there's a great way of keeping such code working in a mixed <coroutine> and <experimental/coroutine> world.
Maybe the cost of doing so is not worth the benefit?

The only way I can think of making this work is to just make std::experimental::* an alias for std::*.
But that only works for std::experimental::coroutine_handle. It doesn't work for std::experimental::coroutine_traits as you can't add specialisations through an alias.

So am I correct in understanding that the main issue with the chicken/egg problem for updating both the compiler to use the new stdlib facilities and updating the stdlib facilities is that we don't want to issue warnings about using <experimental/coroutine> and telling users to use <coroutine> instead if there is no <coroutine> header.

I wonder if a suitable transition approach here might be to have the compiler do:

  • try lookup of std::coroutine_handle/coroutine_traits; if found try to instantiate the expression (e.g. await-suspend) using the std:: type
  • if not found then try lookup of std::experimental::coroutine_handle/coroutine_traits; and if found try to instantiate the expression using the std::experimental:: type; and if successful, then;
    • if std:: type was found but instantiating the expression failed, then issue a warning about using deprecated std::experimental:: types
    • otherwise if std:: type was not found, try to find <coroutine> header and if present then issue a warning suggesting using <coroutine> instead of <experimental/coroutine>

Then you should be able to land the compiler change and it should continue to have the same behaviour for existing code using std::experimental:: types with the existing stdlib.
It is only once the stdlib changes that land which add the <coroutine> header that users would start seeing the warnings about using <coroutine> instead of <experimental/coroutine>

It looks odd for me with the sentence:

otherwise if std:: type was not found, try to find <coroutine> header and if present then issue a warning suggesting using <coroutine> instead of <experimental/coroutine>

If we could find the <coroutine> header, we should be able to find std::coro* types. Otherwise, the <coroutine> header is fake.

My transition plan would be:

  • Land this.
  • Land D109433, it is the change who added <coroutine>.
  • Land another patch in clang to issue a warning for the deprecated use of <experimental/coroutine>. The reason why we need to split this with the first step is that we don't want to trigger the CI system for libc++. (It's the reason this patch is rejected before).
  • Remove <experimental/coroutine> in libcxx in LLVM15.
  • Remove the support for std::experimental::coro* in clang in LLVM16.

Note that there are still some potential breakages for code that could go on here, however.

Imagine that some header I included has been updated to use <coroutine> while my current header is still using <experimental/coroutine> so that both are in scope.

I might have defined an awaitable type that looks like the following:

c++
#include <other/header.hpp> // which transitively includes <coroutine>
#include <experimental/coroutine>

struct my_awaitable {
  bool await_ready();
  void await_suspend(std::experimental::coroutine_handle<> coro);
  void await_resume();
};

In this case, the compiler would find the std::coroutine_handle<> and try to instantiate the await-suspend expression with that type, which would fail because await_suspend() is not callable with a std::coroutine_handle.
The compiler would need to fall back to instantiating the expression with std::experimental::coroutine_handle<P>.

My personal opinion is that this case should fail instead of requiring the compiler to fall back to instantiating with std::experimental::coro*. There are two reasons:

  • It is not easy to implement such fall back to instantiating logics. It would make the current search process much more complex.
  • It is not worth to do so in my opinion. Since <experimental/coroutine> is the deprecated use. It is odd for me that we need to do extra work to conform it.

There is also the variation where await_suspend() is a template function that deduces the promise-type argument.
e.g.

c++
template<typename Promise>
void await_suspend(std::experimental::coroutine_handle<Promise> coro);

There are also cases where the template parameter to await_suspend() is unconstrained.
e.g.

c++
struct promise_type {
  ...
  std::experimental::coroutine_handle<> continuation;
};

struct my_awaitable {
  bool await_ready();
  template<typename CoroHandle>
  auto await_suspend(CoroHandle handle) {
    coro.promise().continuation = handle;
    return coro;
  }
  void await_resume();

  std::experimental::coroutine_handle<promise_type> coro;
};

Such a type would successfully deduce the template parameter using std::coroutine_handle and so the await-suspend expression would be valid, but the instantiation of the await_suspend() body would fail as a std::coroutine_handle<P> cannot be assigned to an lvalue of type std::experimentl::coroutine_handle<>. This would not produce a SFINAE-friendly error that the compiler could retry with std::experimental types.

I'm not sure if there's a great way of keeping such code working in a mixed <coroutine> and <experimental/coroutine> world.
Maybe the cost of doing so is not worth the benefit?

Thanks for offering these examples. It is valuable. But I think the root cause may be the mixed use for <coroutine> and <experimental/coroutine>. It is too bad.
IMHO, it is not worth to do so.

The only way I can think of making this work is to just make std::experimental::* an alias for std::*.
But that only works for std::experimental::coroutine_handle. It doesn't work for std::experimental::coroutine_traits as you can't add specialisations through an alias.

Yes, the compiler would look up to std::experimental::coroutine_traits to get the namespace used. We have talked about the method to make std::experimental::* to be an alias for std::*. It should be done in libcxx part. And it is not valid since it doesn't obey the rule.
Originally, I am changing <experimental/coroutine> directly. But then I am required to keep <experimental/coroutine> untouched. It makes sense to me so I didn't complain about that.

IMO, the key point here is that if the mixed use of <coroutine> and <experimental/coroutine> need to be considered. I think we could get rid of it personally.

Address the comments in coroutines-exp-namespace.cpp.

@lewissbaker wrote:

#include <other/header.hpp> // which transitively includes <coroutine>
#include <experimental/coroutine>

Good example! I had not previously been thinking about transitive includes. I believe we "obviously" don't need to cater to code that manually includes both <coroutine> and <experimental/coroutine> in the same source file; but transitive includes are vastly more likely to happen in practice, and so if we're not going to care about them, that's a policy decision. Might still be a good tradeoff, to break some code in the short term in exchange for a simpler compiler (also in the short term), but its goodness is not obvious.

The only way I can think of making this work is to just make std::experimental::* an alias for std::*.
But that only works for std::experimental::coroutine_handle. It doesn't work for std::experimental::coroutine_traits as you can't add specialisations through an alias.

We could use a using-declaration to bring std::coroutine_traits into namespace std::experimental. That works, and you can still add specializations for std::experimental::coroutine_traits<U> because that's just a name that looks-up-to the same template. https://godbolt.org/z/fWGrT5js5 However, as shown in that godbolt, this would have the (salutary) effect of breaking users who are out there (in the year of our lord 2021!) still reopening namespace std just to add a template specialization.

But! My understanding is that the only reason we're keeping <experimental/coroutine> alive at all, in 14.x, is to provide continuity for its users and not break their existing code right away. If we go changing the API of <experimental/coroutine> (by aliasing it to <coroutine>), then we do break those users right away (because their code depends on the old experimental API, not the new conforming one). So "alias it to <coroutine>" doesn't seem like a viable path forward, anyway. (Also, <coroutine> wants to use C++20-only features, but <experimental/coroutine> must continue to work in C++14.) I think we need to start from the premise that <experimental/coroutine> and <coroutine> will have different APIs; and if that makes it difficult to support Lewis's very reasonable transitive-include scenario, then we have to either implement something difficult, or else make a policy decision that 14.x simply won't support translation units that transitively include both APIs. (15.x certainly will not support such TUs, because it won't support any TUs that include <experimental/coroutine>, transitively or otherwise.)

IOW, it sounds like we're all (@ChuanqiXu @lewissbaker @Quuxplusone) reluctantly OK with the resolution "Do not support translation units that transitively include both APIs"; but it would be helpful to have someone more authoritative weigh in (with either "yes that's OK policy" or "no we need to find some other solution"), if such a person is watching.

clang/docs/LanguageExtensions.rst
2875
to implement the ``std::coroutine_handle`` type.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11002

Pre-existing: It's weird that the surrounding messages are of the form "foo must be bar," and then this one is "foo isn't baz". This message could be re-worded as std::coroutine_handle must have a member named '%0' for consistency. (But that might be out of scope for this PR.)

clang/include/clang/Sema/Sema.h
10243

s/would be/is/

clang/lib/Sema/SemaCoroutine.cpp
1668
/// Look up in namespace std::experimental, for compatibility.
/// TODO: Remove this extra lookup when <experimental/coroutine> is removed.

(The extra lookup is done, not TODO. The removal is the TODO part. Also, grammar nits.)

clang/test/AST/coroutine-source-location-crash.cpp
12–14

Thanks for adding -exp-namespace versions of the Sema tests. I think you should do the same for all of these tests as well, for the same reason: we don't want to remove test coverage related to std::experimental until those codepaths are actually gone.

Also, just as a practical matter: This test says it runs in c++14 mode, but our actual std::coroutine_handle relies on C++20isms. So, my conclusion is that right now you've got a file Inputs/std-coroutine.h that is claiming to be a mockup of libc++'s std::coroutine_handle, but actually is a mockup of std::experimental::coroutine_handle. That's going to be really confusing. I think we need both Inputs/std-experimental-coroutine.h and Inputs/std-coroutine.h, and we need -exp-namespace copies of all these tests too.

As a bonus, this will also allow you to test the compiler's behavior on a TU like

#include "Inputs/std-coroutine.h"
#include "Inputs/std-experimental-coroutine.h"

struct my_awaitable {
  bool await_ready();
  // expected-error@1 {{'await_suspend' must be callable with 'std::coroutine_handle<>' or whatever}}
  void await_suspend(std::experimental::coroutine_handle<> coro);
  void await_resume();
};

in order to prove (and regression-test) that the compiler does not "support" this scenario but also does not crash!

ChuanqiXu updated this revision to Diff 383257.Oct 29 2021, 1:52 AM

Changes includes:

  • Add -exp-namespace copies for all of these tests.
  • Add an error if detected mixed use of std:: and std::experimental.
  • Add test for mixed use of std:: and std::experimental:: namespaces.
  • Run all non exp-namespaces tests in C++20 mode.
  • Other wordy changes.
ChuanqiXu marked 5 inline comments as done.Oct 29 2021, 2:01 AM

@lewissbaker wrote:

#include <other/header.hpp> // which transitively includes <coroutine>
#include <experimental/coroutine>

Good example! I had not previously been thinking about transitive includes. I believe we "obviously" don't need to cater to code that manually includes both <coroutine> and <experimental/coroutine> in the same source file; but transitive includes are vastly more likely to happen in practice, and so if we're not going to care about them, that's a policy decision. Might still be a good tradeoff, to break some code in the short term in exchange for a simpler compiler (also in the short term), but its goodness is not obvious.

The only way I can think of making this work is to just make std::experimental::* an alias for std::*.
But that only works for std::experimental::coroutine_handle. It doesn't work for std::experimental::coroutine_traits as you can't add specialisations through an alias.

We could use a using-declaration to bring std::coroutine_traits into namespace std::experimental. That works, and you can still add specializations for std::experimental::coroutine_traits<U> because that's just a name that looks-up-to the same template. https://godbolt.org/z/fWGrT5js5 However, as shown in that godbolt, this would have the (salutary) effect of breaking users who are out there (in the year of our lord 2021!) still reopening namespace std just to add a template specialization.

But! My understanding is that the only reason we're keeping <experimental/coroutine> alive at all, in 14.x, is to provide continuity for its users and not break their existing code right away. If we go changing the API of <experimental/coroutine> (by aliasing it to <coroutine>), then we do break those users right away (because their code depends on the old experimental API, not the new conforming one). So "alias it to <coroutine>" doesn't seem like a viable path forward, anyway. (Also, <coroutine> wants to use C++20-only features, but <experimental/coroutine> must continue to work in C++14.) I think we need to start from the premise that <experimental/coroutine> and <coroutine> will have different APIs; and if that makes it difficult to support Lewis's very reasonable transitive-include scenario, then we have to either implement something difficult, or else make a policy decision that 14.x simply won't support translation units that transitively include both APIs. (15.x certainly will not support such TUs, because it won't support any TUs that include <experimental/coroutine>, transitively or otherwise.)

IOW, it sounds like we're all (@ChuanqiXu @lewissbaker @Quuxplusone) reluctantly OK with the resolution "Do not support translation units that transitively include both APIs"; but it would be helpful to have someone more authoritative weigh in (with either "yes that's OK policy" or "no we need to find some other solution"), if such a person is watching.

Yeah, the last key point here may be the problem that how do we treat for programs which contains both APIs. Since there are other experimental/* headers moved in to normal include paths, I guess there may be similar problems before. I think this problem is not limited in coroutine. So how does libc++ do before for this situation @Quuxplusone ?

Since it looks like that people here may agree that we don't support both APIs. So I add an error if the compiler founds the mixed use of std::coro* and std::experimental::coro*. I think this is the best we could do if we decide to not support it.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11002

Oh, many thanks for the detailed reviews. I edit this to the way you introduced. Otherwise we couldn't remember this defect after this patch.

ChuanqiXu updated this revision to Diff 383280.Oct 29 2021, 2:51 AM
ChuanqiXu marked an inline comment as done.

Implement __cpp_impl_coroutine feature macro.

Since there are other experimental/* headers moved in to normal include paths, I guess there may be similar problems before. I think this problem is not limited in coroutine. So how does libc++ do before for this situation @Quuxplusone ?

I'm not aware of any similar situations involving <experimental/*>. Most stuff in <experimental/*> is just library stuff, like std::experimental::pmr, where the two versions can happily coexist because we have namespaces. Coroutines are special (and really annoying) because they're a core-language feature with a "magic" relationship to a specific library header. (Core-language syntax doesn't respect namespaces.) The only other "magic" features like that are

  • typeid looks for std::type_info
  • {} looks for std::initializer_list
  • auto [a,b] looks for std::tuple_size etc.
  • <=> looks for std::strong_ordering etc.

None of these were ever prototyped in <experimental/*> before being put into the Standard, so they never ran into this problem.

Since there are other experimental/* headers moved in to normal include paths, I guess there may be similar problems before. I think this problem is not limited in coroutine. So how does libc++ do before for this situation @Quuxplusone ?

I'm not aware of any similar situations involving <experimental/*>. Most stuff in <experimental/*> is just library stuff, like std::experimental::pmr, where the two versions can happily coexist because we have namespaces. Coroutines are special (and really annoying) because they're a core-language feature with a "magic" relationship to a specific library header. (Core-language syntax doesn't respect namespaces.) The only other "magic" features like that are

  • typeid looks for std::type_info
  • {} looks for std::initializer_list
  • auto [a,b] looks for std::tuple_size etc.
  • <=> looks for std::strong_ordering etc.

None of these were ever prototyped in <experimental/*> before being put into the Standard, so they never ran into this problem.

Oh, got it. So now is the time to do policy decision and the community didn't meet similar problem before. Here is my reason for not supporting both:

  • It is complex to implement.
  • It would be removed in recent future. Complexity is not a very strong reason to refuse a need. But it is frustrating to implement a feature which would be removed in a predictable recent future.
  • From the perspective of users, it shouldn't be hard to switch from std::experimental::coro* to std::coro*. I write coroutine codes in production too. AFAIK, it should be easy. (I understand that I couldn't stand for all the users)

Here are my reasons. But if we couldn't get in consensus. I guess we could only post this to cfe-dev to ask more people for involving in.

lxfind accepted this revision.Nov 3 2021, 7:49 PM

I also agree that we should try to keep the compiler simple and not support the complicated case.
It should be fairly straightforward for a codebase to update fully to use std instead of std::experimental (we have a large coroutine codebase as well).
Given that everyone is mostly supportive, I will accept the change.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 3 2021, 8:54 PM
This revision was automatically updated to reflect the committed changes.

@lxfind @Quuxplusone @lewissbaker Thanks for looking into this!
I have committed this. Since the remained problem is that whether or not should we support the case that user uses "std::coroutine*" and "std::experimental::coroutine*" at the same time.
As the reason we stated above, it is hard to implement and the help is limited. Even in the case that someone points out necessary and strong reasons that we need to support both "std::coroutine*" and "std:::experimental::coroutine". We could support it in later patches.