This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by erik.pilkington on Jun 6 2016, 9:52 AM.

Details

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!

Diff Detail

Event Timeline

erik.pilkington retitled this revision from to [Sema] Fix rejects-valid where parameter pack was not expanded in type alias.
erik.pilkington updated this object.
erik.pilkington added a reviewer: rsmith.
erik.pilkington added a subscriber: cfe-commits.
faisalv edited edge metadata.Jun 14 2016, 8:48 PM

Thanks for working on this bug too!

lib/Sema/TreeTransform.h
4784

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 edited edge metadata.

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!

rsmith accepted this revision.Jul 1 2016, 6:34 PM
rsmith edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 1 2016, 6:34 PM
This revision was automatically updated to reflect the committed changes.