[Sema] Implement DR2233

[tambre]

Summary

When a parameter pack is expanded the expanded parameters don't have default values.
Thus if a parameter pack is expanded after any arguments with a default value, the function is incorrectly rejected in CheckCXXDefaultArguments().

Fix this by removing default arguments for parameters before the last non-empty parameter pack.
By doing this we implement DR2233, which supersedes DR777.

Fixes PR23029.

[tambre]

Simplify code, improve comments.

[tambre]

ping

[• Quuxplusone]

clang/test/CXX/drs/dr7xx.cpp

221–222

Is this even supposed to compile? The only valid specializations of f require T... to be an empty pack, which violates temp.res/8.3.

The comment mentions DR777, but DR777 doesn't explain the circumstances under which its wording change matters. It seems only to apply to templates that are already ill-formed by temp.res/8.3.

[rjmccall]

I wrote this comment but apparently never submitted it on Phabricator, sorry.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

1987

I don't think this simple comparison works; the relationship can be complicated because there might be multiple packs in play. By the same token, I think it's possible that there can be multiple interleavings of defaultable parameters with packs. I think you need to scan backwards looking for a parameter without a default argument that was instantiated from a pack expansion and then remove any earlier parameters with defaults.

That is, assuming this is a reasonable implementation approach at all compared to just teaching other parts of the compiler about this case, which I think Richard needs to weigh in on.

[rjmccall]

clang/test/CXX/drs/dr7xx.cpp

221–222

Yeah, Richard made this point in DR2233, and the wording was weakened to allow it, in a way that essentially makes the earlier default arguments dead.

[• Quuxplusone]

clang/test/CXX/drs/dr7xx.cpp

221–222

Huh. Ick. Indeed the other vendors seem to be implementing DR777/DR2233, so I guess Clang ought to catch up even if it's a silly direction to go in. :( I do see a small bit of implementation divergence in https://godbolt.org/z/ZMCvAX —

template<class... Ts>
int f(int i=1, Ts... ts) { return (i + ... + ts); }

template<>
int f<int>(int i, int j) { return 42; }

GCC rejects as ill-formed. MSVC makes the specialization callable with 2 arguments only. EDG (ICC) makes the specialization callable with 0 or 2 arguments (and does crazy things when you call it with 0 arguments).

[tambre]

Handle multiple parameter packs interleaved with default values.
Mark DR777 as superseded by DR2233. Mark DR2233 as resolved.
Moved tests from dr7xx.cpp to dr22xx.cpp. Added note in dr7xx.cpp about DR777 being superseded.
Add more tests that cover bugs observed in other implementations and the deficiency in my first implementation.

[tambre]

Improve comment

[tambre]

Thanks for the reviews!
I believe this now handles all cases and with this we'll be standards-conforming in regard to DR777 and DR2233.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

1987

Good point, my current change doesn't work in the following case:

template<class... Ts, class... As>
void g(int i = 1, Ts... ts, int j = 3, As... as)
{
}

void testg()
{
	g<int>(2, 3, 4, 5);
}

Fixed and added that as a test case.

I think this approach is fine, as keeping info for each parameter for how they were expanded doesn't seem reasonable. Cases such as this ought to be checked and handled early during the actual template instantiation not during later checking of the instantiated function declaration.

clang/test/CXX/drs/dr7xx.cpp

221–222

MSVC seems to be the most standard-conforming in this regard. And now with the revised patch Clang is on par. :)

GCC bug
IntelliSense bug

I also submitted bug reports to ICC, but those seem to need moderator approval before they're public.

[rjmccall]

A couple of minor requests. Otherwise this looks good to me, although I really would like @rsmith to sign off on this approach, especially given his involvement with the DRs.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

1985

There should be a space in "in between", or just use "between".

I don't think the line about "if a parameter pack is expanded..." is true as written. What you're trying to say is something like "It's okay to remove earlier default arguments because the expanded parameters are required to have matching arguments, which they only can if all the earlier parameters have arguments as well."

[rsmith]

Unfortunately, I think this approach basically can't work, because we need to consider inheriting default arguments from a previous (or subsequent!) declaration before dropping default arguments preceding a pack. I think we will instead need to detect this situation when issuing the diagnostic for a parameter with a default argument followed by a parameter without one, and will need to make sure that all the parts of Clang that look at default arguments can cope with them being discontiguous.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

1978

This code is only reached when substituting into non-member functions, so this change would not address the corresponding problem for member function templates or for member functions of class templates. It would be better to handle this in InitFunctionInstantiation, which is called for all function instantiations. (But as noted below, doing this during instantiation doesn't seem to be correct.)

In any case, we should have some testcases for member functions.

2090

We need to do this lookup and the merging below in CheckFunctionDeclaration before we drop any default arguments. Consider this pathological-but-valid testcase (that Clang currently accepts and the other compilers incorrectly reject):

template<typename ...T> void f() {
    void g(int, int = 0);
    void g(int = 0, T...);
    g();
}
void h() { f<int>(); }

Here, we only find out that the second g redeclares the first one during instantiation, and only after that do we inherit the =0 for the second parameter of the first g onto the second parameter of the second g.

And that's not even the worst testcase of this kind. Consider:

template<typename ...T> void f() {
    void g(int = 0, T...);
    void g(int, int = 0);
    g();
}
void h() { f<int>(); }

This is valid under DR2233. The first declaration of g provides a default argument for the first parameter, the second provides a default argument for the second parameter, and the call to g() is valid because both parameters have default arguments. So it is not correct to discard the default arguments when instantiating the first declaration of g.

[tambre]

In D79800#2058985, @rsmith wrote:

Unfortunately, I think this approach basically can't work, because we need to consider inheriting default arguments from a previous (or subsequent!) declaration before dropping default arguments preceding a pack. I think we will instead need to detect this situation when issuing the diagnostic for a parameter with a default argument followed by a parameter without one, and will need to make sure that all the parts of Clang that look at default arguments can cope with them being discontiguous.

Thanks for reviewing!
Abandoning, as I lack the motivation to implement the proper solution you laid out.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (TemplateParams) {
     }
   } else if (FunctionTemplate) {
     // Record this function template specialization.
     ArrayRef<TemplateArgument> Innermost = TemplateArgs.getInnermost();
     Function->setFunctionTemplateSpecialization(FunctionTemplate,
                             TemplateArgumentList::CreateCopy(SemaRef.Context,
                                                              Innermost),
                                                 /*InsertPos=*/nullptr);
+
+    // DR777, DR2233.
+    // Parameter packs are allowed after and inbetween parameters with default
+    // values. We need to remove default arguments for parameters before the
+    // first expanded parameter pack to prevent the declaration being diagnosed
+    // as invalid due to the expanded parameters after parameters with default
+    // values lacking default values. This is safe to do because if a parameter
+    // pack is expanded the user must've provided arguments for all parameters
+    // before it.
+    FunctionDecl *TemplatedDecl = FunctionTemplate->getTemplatedDecl();
+    unsigned FirstPack = Function->getNumParams();
+    bool RemoveDefaults = false;
+
+    // Go backwards through the template declaration parameters and find the
+    // first parameter pack, which has non-zero number of arguments.
+    for (unsigned p = TemplatedDecl->getNumParams(); p-- > 0;) {
+      ParmVarDecl *Param = TemplatedDecl->getParamDecl(p);
+
+      if (Param->isParameterPack()) {
+        llvm::Optional<unsigned> Args =
+            SemaRef.getNumArgumentsInExpansion(Param->getType(), TemplateArgs);
+        assert(Args != None && "Unknown number of pack expansion arguments.");
+
+        if (Args.getValue() == 0)
+          continue;
+
+        FirstPack -= Args.getValue();
+        RemoveDefaults = true;
+        break;
+      } else {
+        FirstPack--;
+      }
+    }
+
+    // If we found such a parameter pack, then remove default arguments for all
+    // parameters before it.
+    if (RemoveDefaults) {
+      for (unsigned p = 0; p < FirstPack; p++) {
+        ParmVarDecl *Param = Function->getParamDecl(p);
+        Param->setDefaultArg(nullptr);
+      }
+    }
   } else if (isFriend && D->isThisDeclarationADefinition()) {
     // Do not connect the friend to the template unless it's actually a
     // definition. We don't want non-template functions to be marked as being
     // template instantiations.
     Function->setInstantiationOfMemberFunction(D, TSK_ImplicitInstantiation);
   }
 
   if (isFriend)
if (SemaRef.CheckFunctionTemplateSpecialization(Function,
                                                     Previous))
       Function->setInvalidDecl();
 
     IsExplicitSpecialization = true;
   } else if (TemplateParams || !FunctionTemplate) {
     // Look only into the namespace where the friend would be declared to
     // find a previous declaration. This is the innermost enclosing namespace,
     // as described in ActOnFriendFunctionDecl.
     SemaRef.LookupQualifiedName(Previous, DC->getRedeclContext());
 
     // In C++, the previous declaration we find might be a tag type
     // (class or enum). In this case, the new declaration will hide the
     // tag type. Note that this does does not apply if we're declaring a
     // typedef (C++ [dcl.typedef]p4).
     if (Previous.isSingleTagDecl())
       Previous.clear();
   }

clang/test/CXX/drs/dr22xx.cpp

 namespace dr2292 { // dr2292: 9
 #if __cplusplus >= 201103L
   template<typename T> using id = T;
   void test(int *p) {
     p->template id<int>::~id<int>();
   }
 #endif
 }
+
+namespace dr2233 { // dr2233: 11
+#if __cplusplus >= 201103L
+template <typename... T>
+void f(int i = 0, T... args) {}
+
+template <typename... T>
+void g(int i = 0, T... args, T... args2) {}
+
+template <typename... T>
+void h(int i = 0, T... args, int j = 1) {}
+
+template <typename... T, typename... U>
+void i(int i = 0, T... args, int j = 1, U... args2) {}
+
+template <class... Ts>
+void j(int i = 0, Ts... ts) {}
+
+template <>
+void j<int>(int i, int j) {}
+
+// PR23029
+// Ensure instantiating the templates works.
+void use() {
+  f();
+  f(0, 1);
+  f<int>(1, 2);
+  g<int>(1, 2, 3);
+  h(0, 1);
+  i();
+  i(3);
+  i<int>(3, 2);
+  i<int>(3, 2, 1);
+  i<int, int>(1, 2, 3, 4, 5);
+  j();
+  j(1);
+  j(1, 2);
+  j<int>(1, 2);
+}
+#endif
+} // namespace dr2233

clang/test/CXX/drs/dr7xx.cpp

template<typename T, typename U> struct Collision {
     template<> struct S1<U>;
 
     template<typename> struct S2;
     template<> struct S2<T> {}; // expected-note {{previous}}
     template<> struct S2<U> {}; // expected-error {{redefinition}}
   };
   Collision<int, int> c; // expected-note {{in instantiation of}}
 }
 
-namespace dr777 { // dr777: 3.7
-#if __cplusplus >= 201103L
-template <typename... T>
-void f(int i = 0, T ...args) {}
-void ff() { f(); }
-
-template <typename... T>
-void g(int i = 0, T ...args, T ...args2) {}
-
-template <typename... T>
-void h(int i = 0, T ...args, int j = 1) {}
-#endif
-}
+// dr777 superseded by dr2233

clang/www/cxx_dr_status.html

<tr id="776">
     <td>CD2</td>
     <td>Delegating constructors, destructors, and <TT>std::exit</TT></td>
     <td class="none" align="center">Unknown</td>
   </tr>
   <tr id="777">
     <td><a href="https://wg21.link/cwg777">777</a></td>
     <td>CD2</td>
     <td>Default arguments and parameter packs</td>
-    <td class="full" align="center">Clang 3.7</td>
+    <td class="full" align="center">Superseded by <a href="#2233">2233</a></td>
   </tr>
   <tr id="778">
     <td><a href="https://wg21.link/cwg778">778</a></td>
     <td>C++11</td>
     <td>Template parameter packs in non-type template parameters</td>
     <td class="none" align="center">Unknown</td>
   </tr>
   <tr id="779">
<tr class="open" id="2232">
     <td>open</td>
     <td><TT>thread_local</TT> anonymous unions</td>
     <td align="center">Not resolved</td>
   </tr>
   <tr id="2233">
     <td><a href="https://wg21.link/cwg2233">2233</a></td>
     <td>DRWP</td>
     <td>Function parameter packs following default arguments</td>
-    <td class="none" align="center">Unknown</td>
+    <td class="full" align="center">Clang 11</td>
   </tr>
   <tr id="2234">
     <td><a href="https://wg21.link/cwg2234">2234</a></td>
     <td>DRWP</td>
     <td>Missing rules for <I>simple-template-id</I> as <I>class-name</I></td>
     <td class="none" align="center">Unknown</td>
   </tr>
   <tr id="2235">