[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D108696#3086011 <https://reviews.llvm.org/D108696#3086011>, @lewissbaker wrote:

> 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 <https://reviews.llvm.org/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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108696/new/

https://reviews.llvm.org/D108696