[Sema] Fix rejects-valid where parameter pack was not expanded in type alias

[erik.pilkington]

Summary

Clang rejects the following valid code:

template <class T> struct Foo {};
template <class... Ts> using FooAlias = Foo<void(Ts...)>;
template <class... Ts> int bar(FooAlias<Ts...>);
int main() { bar(FooAlias<>()); }

The problem is that in the substitution of FooAlias<Ts...> during the parsing of bar, the pack expansion in Foo<void(Ts...)> is transformed into a parameter with the unexpanded flag set, as opposed to another pack expansion. This leads to deduction failing in main, which is incorrect. The fix is to expand the parameter pack when needed.

Fixes PR25250 & PR26017.

Incidentally, there was an incorrect comment in a very similar function that said that a path wasn't reachable. This isn't the case, although no existing test case exercised it, so I added one that did. Please let me know if it would be cleaner to commit that separately!

Thanks!

[erik.pilkington]

Ping!

[faisalv]

Thanks for working on this bug too!

lib/Sema/TreeTransform.h

4784 ↗

(On Diff #59682)

Any thoughts on the value of threading EllipsisLoc and PatternRange into this function (they are used primarily for a diagnostic that is triggered in Sema if NewType does not contain an unexpandedparameter pack - which would never get triggered here, so it would be ok to pass in invalid sourelocations and sourceranges with an appropriate comment here) - but the fact that RebuildPackExpandionType is a customization point, it might not be a bad idea to preserve them)?
Alternatively, we could forego calling RebuildPackExpansionType - since OldType is decomposed directly into a PackExpansionType -- NewType could be recomposed directly via Context.getPackExpansionType(NewType, NumExpansions=0)?

Which brings me to - when we create the new pack expansion type, shouldn't the NumExpansions be reset to 0? The fact that we will have a SubstTemplateParameterType replacing the TemplateParameterType within the pattern might influence the answer to that...

Also - we should be able to assert here that *NumExpansions should be one? Can you think of a case that it wouldn't be?

[erik.pilkington]

This new patch replaces the call to RebuildPackExpansion with a direct call to getPackExpansionType. Also use None instead of NumExpansions as argument.

As far as the assert(*NumExpansions == 1), I don't believe that is always the case. Consider:

template<class T> struct Foo {};
template<class... Ts> using FooAlias = Foo<void(Ts...)>;
template<class... Us> using FooAliasAlias = FooAlias<Us..., Us...>;

Here, the Ts in FooAlias are expanded into 2 Us in FooAliasAlias, meaning that there are 2 expansions that need to be separately rebuilt. I added this case to the test file.

Thanks for the review!

[erik.pilkington]

Ping!

[rsmith]

LGTM

cfe/trunk/lib/Sema/TreeTransform.h

if (PackExpansionExpr *Expansion = dyn_cast<PackExpansionExpr>(Inputs[I])) {
       // The transform has determined that we should perform an elementwise
       // expansion of the pattern. Do so.
       for (unsigned I = 0; I != *NumExpansions; ++I) {
         Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(getSema(), I);
         ExprResult Out = getDerived().TransformExpr(Pattern);
         if (Out.isInvalid())
           return true;
 
-        // FIXME: Can this happen? We should not try to expand the pack
-        // in this case.
         if (Out.get()->containsUnexpandedParameterPack()) {
           Out = getDerived().RebuildPackExpansion(
               Out.get(), Expansion->getEllipsisLoc(), OrigNumExpansions);
           if (Out.isInvalid())
             return true;
         }
 
         Outputs.push_back(Out.get());
if (const PackExpansionType *Expansion
         // Expand the function parameter pack into multiple, separate
         // parameters.
         for (unsigned I = 0; I != *NumExpansions; ++I) {
           Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(getSema(), I);
           QualType NewType = getDerived().TransformType(Pattern);
           if (NewType.isNull())
             return true;
 
+          if (NewType->containsUnexpandedParameterPack()) {
+            NewType =
+                getSema().getASTContext().getPackExpansionType(NewType, None);
+
+            if (NewType.isNull())
+              return true;
+          }
+
           if (ParamInfos)
             PInfos.set(OutParamTypes.size(), ParamInfos[i]);
           OutParamTypes.push_back(NewType);
           if (PVars)
             PVars->push_back(nullptr);
         }
 
         // We're done with the pack expansion.

cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/p5.cpp

namespace PR21289 {
   template<int ...Ns> void g() {
     f(T<Ns>()...);
     f(S<T<Ns>>::value...);
     f(vt<T<Ns>>...);
   }
   template void g<>();
   template void g<1, 2, 3>();
 }
+
+template <class... Ts>
+int var_expr(Ts... ts);
+
+template <class... Ts>
+auto a_function(Ts... ts) -> decltype(var_expr(ts...));
+
+template <class T>
+using partial = decltype(a_function<int, T>);
+
+int use_partial() { partial<char> n; }
+
+namespace PR26017 {
+template <class T>
+struct Foo {};
+template <class... Ts>
+using FooAlias = Foo<void(Ts...)>;
+
+template <class... Ts>
+using FooAliasAlias = FooAlias<Ts..., Ts...>;
+
+template <class... Ts>
+void bar(const FooAlias<Ts...> &) {}
+
+int fn() {
+  FooAlias<> a;
+  bar(a);
+
+  FooAlias<int> b;
+  bar(b);
+}
+}