False positive compile error when expanding packs in function parameters

[yabinc]

Repro: https://godbolt.org/z/G6e3cK4W9

template <typename A, typename B>
using ConditionalRewrite = B;

template <typename T>
using SignatureType = int;

template <typename... Args>
struct Type1 {
  template <typename... Params>
  using Return = SignatureType<int(ConditionalRewrite<Args, Params>...)>;

};

template <typename... Args>
struct Type2 {
  using T1 = Type1<Args...>;

  template <typename... Params>
  using Return = typename T1::template Return<Params...>;

};

template <typename T>
typename T::template Return<int, int> InvokeMethod() {
  return 3;
}

int Function1() {
  return InvokeMethod<Type2<int, int>>();
}

When compiling the above code, Clang reports below error:

<source>:10:3: error: pack expansion contains parameter packs 'Args' and 'Params' that have different lengths (2 vs. 1)
  using Return = SignatureType<int(ConditionalRewrite<Args, Params>...)>;
  ^

However, the length of Args and Params are the same when instantiating Type2<int, int>::Return<int, int>.
The problem happens after https://reviews.llvm.org/D131802.

My understanding of the problem is below:

The error was reported in https://github.com/root-project/root/blob/master/interpreter/llvm/src/tools/clang/lib/Sema/SemaTemplateVariadic.cpp#L727 in function Sema::CheckParameterPacksForExpansion(). CheckParameterPacksForExpansion() checks if the lengths of all unexpanded
parameter packs are the same.

The expansion we care in the example is in L10 ConditionalRewrite<Args, Params>.... Since we instantiate Type1<int, int>,
Args is a pack TemplateArgument: <int, int>, which has pack length 2. But for Params, its content changes in multiple runs of
CheckParameterPacksForExpansion. It can be either <type-parameter-0-0...> or <int, int>. And when Params is <type-parameter-0-0...>, CheckParameterPacksForExpansion() thinks its length is 1 (which doesn't match the length of Args), and reports the error.

I experimented adding below lines in CheckParameterPacksForExpansion() to skip the length check when a template arg is expandable:

      const TemplateArgument& Arg = TemplateArgs(Depth, Index);
      if (Arg.pack_size() == 1 && Arg.pack_begin()->isPackExpansion()) {
        ShouldExpand = false;
        continue;
      }

It can avoid the compile error. But I don't know if there are other cases to consider. So I'd like to leave it to maintainers for a proper fix.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[mizvekov]

Reduction: https://godbolt.org/z/dET4odqqq

template <typename... As> struct A {
  template <typename... Bs> using B = void(As...(Bs));
};

template <typename... Cs> struct C {
  template <typename... Ds> using D = typename A<Cs...>::template B<Ds...>;
};

using t1 = C<int, int>::template D<int, int>;

[pirama-arumuga-nainar]

@mizvekov Do you know how to fix this error? We reverted https://reviews.llvm.org/D131802 downstream in Android as a workaround but the revert is becoming harder to maintain.

(The fix @yabinc mentioned in the original report breaks other tests so is not a viable path forward. It'd be great for someone familiar with this area to take a look. @AaronBallman any suggestions?)

[AaronBallman]

Tagging @erichkeane for awareness (just an FYI, Erich is at C++ standards meetings this week so there may be a delay before hearing from him).

[erichkeane]

Yeah, I don't have much time to look at it this week, but will when I get a chance. I wouldn't be surprised if we're setting up template args in getInstantedTemplateArgs incorrectly AGAIN.

[mizvekov]

I started working on a fix, but I am off for vacations and the dev meeting, so I probably won't be able to resume working on this for the next two weeks at least.

I believe the issue here is a pre-existing one, and the repro was working by coincidence due to being a degenerate case.

I think the code in https://discourse.llvm.org/t/pack-expansion-bug/64910 repros the same bug, and was already broken / can't be 'fixed' by a simple revert.

The issue here is that alias templates present a bit of a special case that the pack expansion analyzer can't deal with, in that they are the only kind of template that can be substituted with its arguments not fully expanded.

[languagelawyer]

Dup of #32252 #17042?

[Romain-Geissler-1A]

Hi,

I also think I hit this bug. Using the latest libstdc++ 13 headers + the latest clang 16, this code (compiled with -std=gnu++2b) raises a similar error to what has already been reported in https://discourse.llvm.org/t/pack-expansion-bug/64910:

#include <tuple>

void g(std::tuple<std::tuple<int, int>>);

void f() 
{
    g(std::forward_as_tuple<std::tuple<int, int>>({0, 0}));
}

In file included from <source>:1:
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/tuple:690:2: error: pack expansion contains parameter packs '_UTypes' and '_Types' that have different lengths (2 vs. 1)
        using __convertible = __and_<is_convertible<_UTypes, _Types>...>;
        ^~~~~
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/tuple:852:27: note: in instantiation of template type alias '__convertible' requested here
          = _TCC<true>::template __convertible<_Args...>::value;
                                 ^
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/tuple:947:12: note: in instantiation of static data member 'std::tuple<std::tuple<int, int> &&>::__convertible<int &, int &>' requested here
        explicit(!__convertible<_UElements&...>)
                  ^
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/tuple:1999:36: note: while substituting deduced template arguments into function template 'tuple' [with _UElements = <int, int>]
    { return tuple<_Elements&&...>(std::forward<_Elements>(__args)...); }
                                   ^
<source>:7:12: note: in instantiation of function template specialization 'std::forward_as_tuple<std::tuple<int, int>>' requested here
    g(std::forward_as_tuple<std::tuple<int, int>>({0, 0}));
           ^
In file included from <source>:1:
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/tuple:690:2: error: pack expansion contains parameter packs '_UTypes' and '_Types' that have different lengths (2 vs. 1)
        using __convertible = __and_<is_convertible<_UTypes, _Types>...>;
        ^~~~~
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/tuple:852:27: note: in instantiation of template type alias '__convertible' requested here
          = _TCC<true>::template __convertible<_Args...>::value;
                                 ^
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/tuple:958:12: note: in instantiation of static data member 'std::tuple<std::tuple<int, int> &&>::__convertible<const int, const int>' requested here
        explicit(!__convertible<const _UElements...>)
                  ^
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/tuple:1999:36: note: while substituting deduced template arguments into function template 'tuple' [with _UElements = <int, int>]
    { return tuple<_Elements&&...>(std::forward<_Elements>(__args)...); }
                                   ^
<source>:7:12: note: in instantiation of function template specialization 'std::forward_as_tuple<std::tuple<int, int>>' requested here
    g(std::forward_as_tuple<std::tuple<int, int>>({0, 0}));
           ^
2 errors generated.
Compiler returned: 1

[kongy]

@mizvekov Any update on a bug fix? This remains broken on top of trunk LLVM.

[Romain-Geissler-1A]

With LLVM 16 branch being created, is there a way to make this bug a release blocker, as it rejects valid code (which isn't too hard to hit when using pair/tuple for libstdc++ >= 13) ?

[erichkeane]

With LLVM 16 branch being created, is there a way to make this bug a release blocker, as it rejects valid code (which isn't too hard to hit when using pair/tuple for libstdc++ >= 13) ?

No, I don't believe that is something we can do. We have a large number of equally as serious template bugs that have been around for quite a while, and we don't really have anyone actively working on this sufficiently to not overly delay 16.0.

[stbergmann]

Dup of #32252 #17042?

Note that when rewinding back to before https://reviews.llvm.org/D131802, b8a1b69 "[clang] fix missing initialization of original number of expansions" (i.e., to b8ecf32 "DynamicMemRefType: iteration and access by indices"): The reproducers for this issue and for #32252 succeed again, while the reproducer for #17042 still fails.

[erichkeane]

I've reconsidered this... i think this DOES need to be blocking. I've added it to the LLVM16 release milestone. I'll continue to look at this, but I'm a bit lost here, I'm hopeful @mizvekov can share what he learned/thinks needs to happen here.

[erichkeane]

This commit did a lot of work here: https://reviews.llvm.org/D128095 including rewriting a large amount of the code that does this diagnostic. I'm still trying to figure out what is going on here with the original patch, and it looks like the author (@mizvekov ) isn't coming back. I'm not sure what the consequence of reverting that is however.

[erichkeane]

I made an attempt at reverting the original patch, but it is really messy and probably not all that possible. I am away from the office the next 3 days, but spent the last few days looking into this. I believe the problem is that Sema::CheckParameterPacksForExpansion doesn't properly care about cases where a pack is unexpanded, so it is just using an existing pack substitution (with a size of '1', though it is an unexpanded pack!) and causing a problem.

I had SOME luck (though it broke a ton of other tests) by checking for dependence inside of the if (Pos) { branch in that function, and escaping, similar to how the PartialPack code below it works. This caused a bunch of trouble with partial specialization tests and type deduction tests, but DID at least make the minimum reproducer + some slight variations to make sure it diagnosed those work, so I feel like i'm on the right track?

I'm hopeful that if I can't get this solved today that someone else can come along and continue the work.

[tstellar]

We are getting close to the final 16.0.0 release, this fix may need to go into 16.0.1 instead.

[erichkeane]

This is going to be a pretty damaging one to miss 16.0 I think, not being able to compile Tuple is pretty bad. How long do we have to make the decision?

[tstellar]

I was planning to do the final release candidate (-rc4) today and then the final release in a week. Would you be able to fix it in the next week? If so, given the concerns raised by you and also @AaronBallman on IRC, I think it would be fine to delay the final release for this bug, but I don't want to delay the release indefinitely. There needs to be some kind of time box on this.

[erichkeane]

I can definitely try. I got pulled away due to 'pay the bills' lately (plus needing to take some personal time), but am hopefully going to have the next few days exclusively for this. I DO want to give a revert another try, but it has been my 'last ditch effort' so far.

It itself might end up being a minor regression against trunk (and results in a crash in invalid code I think, as well as being a messy revert, as I've failed once at it). BUT, due to the time constraint, it is perhaps worth evaluating it/digging into that now.

[erichkeane]

So attempts to revert D128095 and D131802 have BOTH failed to fix the reduced example, as well as the original report. So I'm not confident in a revert being possible, until I can figure out what actually causes this now...

[AaronBallman]

CC @zygoloid in case he's got spare cycles to help investigate.

[erichkeane]

I did a bisect myself, and confirmed that D131802 is the patch that 'caused' this, but reverting it doesn't fix the problem. I guess I can try an additional bisect with reverting this to see what second patch makes this come back, but I'm not very optimistic that it will make a difference.

I'm out of my depth debugging packs + deduction here it seems, so I'm having a tough time doing valuable debugging here. At the moment, I think if @zygoloid or @mizvekov don't come by and help, I don't think this is something I can fix in time for the 16.0 release. And I unfortunately think that means that we're going to have a 16.0 release that is pretty broken, at least for Tuple.

[tstellar]

Seems like reverting is going to be the best option. I have some time to try bisecting and doing reverts if that helps.

[erichkeane]

You're welcome to try too, but my bisect ended up at the same commit as OPs, but the revert of that patch still doesn't fix the problem.

[erichkeane]

OK, done bisecting a 2nd time (with a manual revert on each step of D13802 as identified by OP), and came up with D128095 SEPARATELY ALSO causing the problem.

So I'm hopeful we can revert BOTH together and have it work? I'm still looking into it.

[erichkeane]

I can confirm that reverting BOTH seems to fix the two minimized versions we see above. I've submitted a patch here: https://reviews.llvm.org/D145605
and am hoping that the pre-commit CI can bless this before committing.

[erichkeane]

Ok, committed in https://reviews.llvm.org/rGacecf68c8b7c3c625cfa00f00f8ddc8f15baae44

@tstellar : I'm thinking this is pretty low risk for the 16.0 branch, but will require a bit more work (fixing up the release notes) to get the cherry-pick ready. I'm not really sure how to do this, can you help/advise?

[tstellar]

/cherry-pick acecf68

[tstellar]

@erichkeane Can we update the release notes in a separate commit?

[llvmbot]

/branch llvm/llvm-project-release-prs/issue58452

[llvmbot]

/pull-request llvm/llvm-project-release-prs#358

[erichkeane]

@erichkeane Can we update the release notes in a separate commit?

Yep! I can definitely do that. Do I just commit it directly to the release branch? Or is there a different way?

[tstellar]

You can just push to the release branch, and it doesn't need to be done before the final RC, it just needs to be done before the final release. We don't spin new RCs just for release notes changes.

[erichkeane]

You can just push to the release branch, and it doesn't need to be done before the final RC, it just needs to be done before the final release. We don't spin new RCs just for release notes changes.

Ok, great! As soon as this request goes in, I can push the update for the release notes, its easy enough to do and I have a patch all ready.

[Romain-Geissler-1A]

Hi,

I still have issues with the snippet I posted in my earlier comment #58452 (comment) and libstc++'s 13 tuple implementation, using C++23.

I have reported it in #61415.

Cheers,
Romain