Implement P0857R0 -Part B: requires clause for template-template params

[erichkeane]

Summary

P0857R0 part-b allows a 'requires' clause on template-template
parameters. This patch adds this in. However, no part of the standard
seems to require checking of these constraints, nor do any of the 3
major implementations (EDG, GCC, MSVC) that implement this feature.

Additionally, Part A of the document allows requires clauses on lambdas
which appears to already be implemented, so update cxx_status.html to
mark this paper as implemented.

[aaron.ballman]

clang/lib/Parse/ParseTemplate.cpp

876

Rather than duplicate the grammar for each standard revision, I think we usually try to have one grammar section that incorporates all of the changes over the years, like how we added [C++1z] on line 873 above. Any appetite for trying to rearrange like that?

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

60–62

I agree that none of the other implementations seems to be checking constraints here, but I would have guessed that https://eel.is/c++draft/temp.arg.template#3 was what would trigger checking the constraints here (the and for template template-parameters, each of their corresponding template-parameters matches, recursively. bit, specifically).

Usually the answer to "Am I misreading the standard or are these three implementations all wrong in the same way?" is "I misread the standard.", but I'd appreciate some confirmation here. :-)

[aaron.ballman]

Btw, the CI failure for x64 debian > Clang.SemaTemplate::concepts.cpp looks relevant. Not certain why I don't see a similar failure for Windows though.

[erichkeane]

clang/lib/Parse/ParseTemplate.cpp

876

C++20 reworded this to the point that there is very little in common, so i feared that the grammar would be particularly rough here. I tried a merge at one point, and it was just pretty messy (since the productions don't particularly well match between them).

type-parameter in particular ends up being messy, since the leading 'template < param list >' was removed from there.

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

60–62

Usually the answer to "Am I misreading the standard or are these three implementations all wrong in the same way?" is "I misread the standard."

This is typically _MY_ response to this as well, so I was hoping @rsmith or @hubert.reinterpretcast could tell me the answer here (and perhaps help write a Core issue if the implementations are all 'right').

[erichkeane]

In D110641#3028173, @aaron.ballman wrote:

Btw, the CI failure for x64 debian > Clang.SemaTemplate::concepts.cpp looks relevant. Not certain why I don't see a similar failure for Windows though.

Looks like I miseed up the 'scope' of template-template parameters being usable in that requires clause there. I don't have a good idea of how to fix it, but looking into it.

clang/lib/Parse/ParseTemplate.cpp

923

I'm not positive about this skip-until and 'TryConsumeToken' either. The other place we do optional requires clause parsing does this, but the above 'ParseTemplateParameters' does not.

[erichkeane]

Ok, Aaron and I are pretty confused here I think (based on offline discussions), so I'm hoping for some guidance.

ParseTemplateTemplateParameter does all the work to parse a template-template parameter, then calls ActOnTemplateTemplateParameter, which handles adding the top-level name to the scope (as well as warning if the params list is 0).

Additionally, we noticed that:

template<template<typename T> class C = std::enable_if_t<T::value>> struct s{};

Is illegal in all compilers, T is not available in the enable_if_t call. This makes a lot of sense to me, since the T is essentially worthless: It can never be matched, it never gets a concrete type, etc.

However, the test failure above does the equivilant of:

template<template<typename T> requires T::value class C> struct s{};

I note that MSVC and GCC accept this (no idea about EDG). However, this seems worthless/useless for the same reason. I guess it somewhat makes sense that the requires clause associated with the template-template-parameter-parameter-list should have access to names from the list, but they are still seemingly worthless to me.

My expectation is that I could implement this by introducing a new Scope that contains all the names of the template-template-parameter-list and goes out of scope as soon as we're done with the requires clause. It seems strange to do so (and ends up having an ActOnTemplateTemplateParameterParameterList for this purpose, plus some level of 'pop-scope'), but seems consistent with the other compilers?

I guess what I'm saying is in addition to the other questions raised above regarding whether these requires clauses are useful AT ALL, is that we likely require guidance to finish implementing this paper

[erichkeane]

Moments after my last comment, i figured out the scope issue, which makes the names in the template-template-parameter parameter-list in scope for this requires clause.

I don't think it makes SENSE that this is the case, but I suspect thats what the standard says based on the other compiler's behavior.

This fixes the exmaple in the concepts.cpp test.

[aaron.ballman]

clang/lib/Parse/ParseTemplate.cpp

876

Okay, SGTM then, thanks!

923

I think we should be skipping until we hit a > or >> rather than } right? We can recover after we've finished the template template parameter parsing, we don't need to parse to the end of the class.

[erichkeane]

clang/lib/Parse/ParseTemplate.cpp

923

Perhaps we should copy line 1000 and skip until >, >> or , with no consume?

[aaron.ballman]

clang/lib/Parse/ParseTemplate.cpp

923

That's what I'm thinking, yes.

[erichkeane]

Change the skip-until code after a requires-clause failed parse.

[erichkeane]

Ping!

[cor3ntin]

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

60–62

I've spent the last hour trying to come up with scenarios where GCC does something interesting here, or where there would be a use for this here, but I have nothing.

[cor3ntin]

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

60–62

I am a bit confused by GCC https://godbolt.org/z/TTrP81jjx

[erichkeane]

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

60–62

Huh.... is this some level of the subsumes check that needs to happen? Though you show that the 'same constraint name + more info' thing doesn't work? I'm very confused...

[cor3ntin]

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

60–62

My current reading of the tea leaves (with no confidence whatsoever) is that the require clauses should match - aka be exactly the same.
And in the last case, maybe GCC has a bug where it fails to do the check if the template argument has no require clause whatsoever.
Maybe?

[erichkeane]

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

60–62

Maybe? But MSVC's implementation doesn't seem to be checking at all either: https://godbolt.org/z/nbY8EEG1M

[erichkeane]

Ping! Does anyone have any feedback on this? I'd really like to start getting us caught up on standards implementation, and this seems like it might be an easy win (unless someone on the list knows why it is just plain wrong/more complicated than I think!).

[cor3ntin]

In D110641#3064200, @erichkeane wrote:

Ping! Does anyone have any feedback on this? I'd really like to start getting us caught up on standards implementation, and this seems like it might be an easy win (unless someone on the list knows why it is just plain wrong/more complicated than I think!).

Have we figured out what the intended semantic purposes of that requires clause is supposed to be yet? Because I'm still very confused

[erichkeane]

In D110641#3319787, @cor3ntin wrote:

In D110641#3064200, @erichkeane wrote:

Ping! Does anyone have any feedback on this? I'd really like to start getting us caught up on standards implementation, and this seems like it might be an easy win (unless someone on the list knows why it is just plain wrong/more complicated than I think!).

Have we figured out what the intended semantic purposes of that requires clause is supposed to be yet? Because I'm still very confused

Nope, I have yet to be able to come up with a reproducer that would actually care about this in any of the compilers :/

[erichkeane]

Comment based on @rsmith and EWG reflector feedback as to where this should be checked.

clang/test/SemaTemplate/concepts.cpp

63 ↗

(On Diff #376257)

Seemingly if "X" contains U<int>, the constraint should be checked (according to EWG).

Based on reflector conversations, it seems this should ALSO be checked in:

http://eel.is/c++draft/temp.arg.template#3
and
http://eel.is/c++draft/temp.arg.template#4
, particularly the 'rewrite to function templates in /4 should produce a constrained function template, and the 'at least as specialized' check takes these into consideration.

They are ALSO considered in partial ordering.

[erichkeane]

clang/test/SemaTemplate/concepts.cpp

63 ↗

(On Diff #376257)

That example(X has a U<int> in it) asserts against main since it causes us to not properly instantiate the constraints. On top of my 'deferred constraints instantiation' patch (https://reviews.llvm.org/D119544 ) it fails to diagnose (consistent with GCC, but not EDG).

[erichkeane]

Taken over by someone else here: https://reviews.llvm.org/D134128

clang/lib/Parse/ParseTemplate.cpp

 ///       type-parameter:    [C++ temp.param]
 ///         'template' '<' template-parameter-list '>' type-parameter-key
 ///                  ...[opt] identifier[opt]
 ///         'template' '<' template-parameter-list '>' type-parameter-key
 ///                  identifier[opt] = id-expression
 ///       type-parameter-key:
 ///         'class'
 ///         'typename'       [C++1z]
+///
+/// In C++20:
+///       template-head: [C++ temp.pre]
+///         template '<' template-parameter-list '>' requires-clause[opt]
+///
+///       template-parameter: [C++ temp.param]
+///         type-parameter
+///         parameter-declaration
+///
+///       type-parameter:
+///         type-parameter-key ...[opt] identifier[opt]
+///         type-parameter-key identifier[opt] = type-id
+///         type-constraint ...[opt] identifier[opt]
+///         type-constraint identifier[opt] = type-id
+///         template-head type-parameter-key ...[opt] identifier[opt]
+///         template-head type-parameter-key identifier[opt] = id-expression
+///
+///       type-parameter-key:
+///         'class'
+///         'typename'
+///
+///       type-constraint:
+///         nested-name-specifier[opt] concept-name
+///         nested-name-specifier[opt] concept-name '<'
+///               template-argument-list[opt] '>'
 NamedDecl *
 Parser::ParseTemplateTemplateParameter(unsigned Depth, unsigned Position) {
   assert(Tok.is(tok::kw_template) && "Expected 'template' keyword");
 
   // Handle the template <...> part.
   SourceLocation TemplateLoc = ConsumeToken();
   SmallVector<NamedDecl*,8> TemplateParams;
   SourceLocation LAngleLoc, RAngleLoc;
   {
     MultiParseScope TemplateParmScope(*this);
     if (ParseTemplateParameters(TemplateParmScope, Depth + 1, TemplateParams,
                                 LAngleLoc, RAngleLoc)) {
       return nullptr;
     }
   }
 
+  // Parse optional requires-clause.
+  ExprResult OptionalRequiresClauseConstraintER;
+  if (getLangOpts().CPlusPlus20 && TryConsumeToken(tok::kw_requires)) {
+    OptionalRequiresClauseConstraintER =
+        Actions.ActOnRequiresClause(ParseConstraintLogicalOrExpression(
+            /*IsTrailingRequiresClause=*/false));
+    if (!OptionalRequiresClauseConstraintER.isUsable()) {
+      // Skip until the semi-colon or a '}'.
+      SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
+      TryConsumeToken(tok::semi);
+      return nullptr;
+    }
+  }
+
   // Provide an ExtWarn if the C++1z feature of using 'typename' here is used.
   // Generate a meaningful error if the user forgot to put class before the
   // identifier, comma, or greater. Provide a fixit if the identifier, comma,
   // or greater appear immediately or after 'struct'. In the latter case,
   // replace the keyword with 'class'.
   if (!TryConsumeToken(tok::kw_class)) {
     bool Replace = Tok.isOneOf(tok::kw_typename, tok::kw_struct);
     const Token &Next = Tok.is(tok::kw_struct) ? NextToken() : Tok;
if (Tok.is(tok::identifier)) {
     return nullptr;
   }
 
   // Recover from misplaced ellipsis.
   bool AlreadyHasEllipsis = EllipsisLoc.isValid();
   if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
     DiagnoseMisplacedEllipsis(EllipsisLoc, NameLoc, AlreadyHasEllipsis, true);
 
-  TemplateParameterList *ParamList =
-    Actions.ActOnTemplateParameterList(Depth, SourceLocation(),
-                                       TemplateLoc, LAngleLoc,
-                                       TemplateParams,
-                                       RAngleLoc, nullptr);
+  TemplateParameterList *ParamList = Actions.ActOnTemplateParameterList(
+      Depth, SourceLocation(), TemplateLoc, LAngleLoc, TemplateParams,
+      RAngleLoc, OptionalRequiresClauseConstraintER.get());
 
   // Grab a default argument (if available).
   // Per C++0x [basic.scope.pdecl]p9, we parse the default argument before
   // we introduce the template parameter into the local scope.
   SourceLocation EqualLoc;
   ParsedTemplateArgument DefaultArg;
   if (TryConsumeToken(tok::equal, EqualLoc)) {
     DefaultArg = ParseTemplateTemplateArgument();

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

 template <template <typename...> class C>
 struct S3;
 
 template <C T>
 using N = typename T::type;
 
 using s31 = S3<N>;
 using s32 = S3<Z>;
+
+template<typename T>
+struct Evals {
+  bool f() { return true; }
+};
+
+template<>
+struct Evals<float> {
+  bool f() { return false; }
+};
+
+template <typename T, template <typename> requires C<T> typename P>
+struct S4 {};
+
+template <typename T, template <typename>
+requires requires { requires T::f(); }
+typename P >
+    void func(const P<T> &p){};
+
+template <typename T, template <typename> requires false typename P>
+void func_always_requires_false(const P<T> &p) {};
+
+void use() {
+  S4<int, Evals> s4;
+  func(Evals<int>{});
+  // A naive individual might expect the following to all fail concept checking,
+  // but there does not seem to be any requirement to check these in the
+  // standard, and none of the other implementations do so either.
+  S4<float, Evals> s4b;
+  func(Evals<float>{});
+  func_always_requires_false(Evals<int>{});
+}

clang/www/cxx_status.html

</tr>
     </tr>
     <tr id="p0734">
       <td rowspan="12">Concepts</td>
       <td><a href="https://wg21.link/p0734r0">P0734R0</a></td>
       <td class="full" align="center">Clang 10</td>
     </tr>
       <tr> <!-- from Albuquerque -->
         <td><a href="https://wg21.link/p0857r0">P0857R0</a></td>
-        <td class="partial" align="center">Partial</td>
+        <td class="unreleased" align="center">Clang 14</td>
       </tr>
       <tr> <!-- from San Diego -->
         <td><a href="https://wg21.link/p1084r2">P1084R2</a></td>
         <td rowspan="2" class="full" align="center">Clang 10</td>
       </tr>
       <tr>
         <td><a href="https://wg21.link/p1141r2">P1141R2</a></td>
       </tr>