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

Wed Oct 27 07:53:13 PDT 2021

Quuxplusone added a comment.

@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.

================
Comment at: clang/docs/LanguageExtensions.rst:2872
 https://wg21.link/P0057. The following four are intended to be used by the
-standard library to implement `std::experimental::coroutine_handle` type.
+standard library to implement `std::coroutine_handle` type.

----------------
```
to implement the ``std::coroutine_handle`` type.
```

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11002
 def err_coroutine_handle_missing_member : Error<
-  "std::experimental::coroutine_handle missing a member named '%0'">;
+  "std::coroutine_handle missing a member named '%0'">;
 def err_malformed_std_coroutine_traits : Error<
----------------
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.)

================
Comment at: clang/include/clang/Sema/Sema.h:10274
+  /// Lookup 'coroutine_traits' in std namespace and std::experimental
+  /// namespace. The namespace found would be recorded in Namespace.
   ClassTemplateDecl *lookupCoroutineTraits(SourceLocation KwLoc,
----------------
`s/would be/is/`

================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1668
+    if (!CoroNamespace || !LookupQualifiedName(Result, CoroNamespace)) {
+      /// TODO: Lookup in std::expeirmental namespace for compability.
+      /// Remove this once users get familiar with coroutine under std
----------------
ldionne wrote:
> 
```
/// 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.)

================
Comment at: clang/test/AST/coroutine-source-location-crash.cpp:12-14
 #include "Inputs/std-coroutine.h"

+using namespace std;
----------------
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 at 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!

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

https://reviews.llvm.org/D108696