[clang] Require parameter pack to be last argument in concepts.

[luken-google]

Summary

Fixes GH48182.
Applies the patch proposed by @rsmith in the bug and adds a test.

[luken-google]

Adding release note.

[ilya-biryukov]

The error itself makes sense, but I don't quite understand why usages of invalid<int> did not produce errors before. Any idea why that happened? Are there some other bugs hiding?
It seems that at some point when parsing this code:

static_assert(invalid<int> also here ;

we chose to silently recover by skipping until the semicolon, but never actually produced any errors. Is there a code path that should also be updated to handle that?

[ilya-biryukov]

E.g. gcc actually accepts the concept definition, but the later usage fails with "wrong number of template arguments". I wonder why Clang has not been doing the latter before this patch.

[luken-google]

There is logic in the template expansion that returns an error state if the template parameter pack was not at the end of the template parameter list. It returns true but does not generate any diagnostics, but the outer code error machinery assumes that diagnostics have already been generated and so considers the expression invalid and moves on.

As this fix no longer accepts these ill-formed templates as valid, that code no longer fires. I have strengthened the check there to an assert. PTAL.

[luken-google]

Strengthen assert.

[ilya-biryukov]

Thanks for the explanation. LGTM! And thanks for adding an assert.

It's interesting that recovery for classes seems to be a bit better here:

template <class ...T, class ...U> struct invalid {};
int a = invalid<int>(10); // there is no error: undefined identified 'invalid'

but I suspect chasing the improvements there is out of scope for this particular GH issue. Maybe worth adding a FIXME/filing a GH issue for improving this in the future.

[luken-google]

committed at afd800fc5679ccfe6f32b097586c658628500867

[dblaikie]

In D138210#3989385, @luken-google wrote:

committed at afd800fc5679ccfe6f32b097586c658628500867

For future reference it's handy to include the "Differential Revision: https://reviews.llvm.org/DXXXX" (arc will create this for you, if you use that) in the commit message both so folks looking at the commit can find the review/know that it was reviewed on Phab, and that way Phab will auto-close the review and include a link to the commit.

Also I think we're still using the "PR" prefix predominantly for bug references, rather than the "GH" suffix you've used here (handy for consistency - easier to search up the bugs if the prefix is more consistent).

clang/docs/ReleaseNotes.rst

- Clang implements DR2621, correcting a defect in ``using enum`` handling.  The
   name is found via ordinary lookup so typedefs are found.
 - Implemented `P0634r3 <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0634r3.html>`_,
   which removes the requirement for the ``typename`` keyword in certain contexts.
 - Implemented The Equality Operator You Are Looking For (`P2468 <http://wg21.link/p2468r2>`_).
 - Implemented `P2113R0: Proposed resolution for 2019 comment CA 112 <https://wg21.link/P2113R0>`_
   ([temp.func.order]p6.2.1 is not implemented, matching GCC).
 - Implemented `P0857R0 <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0857r0.html>`_,
   which specifies constrained lambdas and constrained template *template-parameter*\s.
+- Required parameter pack to be provided at the end of the concept parameter list. This
+  fixes `Issue 48182 <https://github.com/llvm/llvm-project/issues/48182>`_.
 
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 <https://github.com/llvm/llvm-project/issues/50886>`_.
 
 C++2b Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 - Support label at end of compound statement (`P2324 <https://wg21.link/p2324r2>`_).

clang/lib/Sema/SemaTemplate.cpp

for (TemplateParameterList::iterator Param = Params->begin(),
     // arguments, just break out now and we'll fill in the argument pack below.
     if ((*Param)->isTemplateParameterPack()) {
       assert(!getExpandedPackSize(*Param) &&
              "Should have dealt with this already");
 
       // A non-expanded parameter pack before the end of the parameter list
       // only occurs for an ill-formed template parameter list, unless we've
       // got a partial argument list for a function template, so just bail out.
-      if (Param + 1 != ParamEnd)
+      if (Param + 1 != ParamEnd) {
+        assert(
+            (Template->getMostRecentDecl()->getKind() != Decl::Kind::Concept) &&
+            "Concept templates must have parameter packs at the end.");
         return true;
+      }
 
       SugaredConverted.push_back(
           TemplateArgument::CreatePackCopy(Context, SugaredArgumentPack));
       SugaredArgumentPack.clear();
 
       CanonicalConverted.push_back(
           TemplateArgument::CreatePackCopy(Context, CanonicalArgumentPack));
       CanonicalArgumentPack.clear();
if (!DC->getRedeclContext()->isFileContext()) {
     return nullptr;
   }
 
   if (TemplateParameterLists.size() > 1) {
     Diag(NameLoc, diag::err_concept_extra_headers);
     return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
     Diag(NameLoc, diag::err_concept_no_parameters);
     return nullptr;
   }
 
+  // Ensure that the parameter pack, if present, is the last parameter in the
+  // template.
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+                                             ParamEnd = Params->end();
+       ParamIt != ParamEnd; ++ParamIt) {
+    Decl const *Param = *ParamIt;
+    if (Param->isParameterPack()) {
+      if (++ParamIt == ParamEnd)
+        break;
+      Diag(Param->getLocation(),
+           diag::err_template_param_pack_must_be_last_template_parameter);
+      return nullptr;
+    }
+  }
+
   if (DiagnoseUnexpandedParameterPack(ConstraintExpr))
     return nullptr;
 
-  ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, NameLoc, Name,
-                                             TemplateParameterLists.front(),
-                                             ConstraintExpr);
+  ConceptDecl *NewDecl =
+      ConceptDecl::Create(Context, DC, NameLoc, Name, Params, ConstraintExpr);
 
   if (NewDecl->hasAssociatedConstraints()) {
     // C++2a [temp.concept]p4:
     // A concept shall not have associated constraints.
     Diag(NameLoc, diag::err_concept_no_associated_constraints);
     NewDecl->setInvalidDecl();
   }
 

clang/test/SemaTemplate/concepts.cpp

struct __iterator_traits_member_pointer_or_arrow_or_void<_Ip> {
   using type = decltype(declval<_Ip&>().operator->());
 };
 
 
 void use2() {
   __iterator_traits_member_pointer_or_arrow_or_void<counted_iterator<int>> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template<typename, typename..., typename = int> // expected-error{{template parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template<typename> requires invalid<int> // expected-error{{use of undeclared identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid<int> also here ; // expected-error{{use of undeclared identifier 'invalid'}}
+
+int foo() {
+    bool b;
+    b = invalid<int> not just in declarations; // expected-error{{expected ';' after expression}}
+                                               // expected-error@-1{{use of undeclared identifier 'invalid'}}
+                                               // expected-error@-2{{expected ';' after expression}}
+                                               // expected-error@-3{{use of undeclared identifier 'just'}}
+                                               // expected-error@-4{{unknown type name 'in'}}
+    return b;
+}
+} // namespace GH48182