Skip to content

[clang][regression][rejects-valid] clang trunk rejecting valid pack expansion #91787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ericniebler opened this issue May 10, 2024 · 11 comments · Fixed by #91833
Closed

[clang][regression][rejects-valid] clang trunk rejecting valid pack expansion #91787

ericniebler opened this issue May 10, 2024 · 11 comments · Fixed by #91833
Assignees
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" duplicate Resolved as duplicate rejects-valid

Comments

@ericniebler
Copy link

I believe the following C++20 code should compile:

template <class...>
concept True = true;

template <bool>
struct I {
  template <template <class...> class F, class... As>
  using f = F<As...>;
};

template <template <class...> class F, class... As>
using meval = I<True<As...>>::template f<F, As...>;

template <class F, class... As>
concept P = true;

template <class F, class... As>
  requires P<F, As...>
using V = void;

template <class...Ts>
auto f() -> meval<V, int, Ts...> {
}

int main() {
  f();
}

Clang 18 and earlier accepts it, as does gcc. With clang-trunk, I get:

<source>:10:29: error: pack expansion used as argument for non-pack parameter of alias template
   10 | template <template <class...> class F, class... As>
      |                             ^
1 error generated.
Compiler returned: 1

See repro here: https://godbolt.org/z/c1dG74EPd

@github-actions github-actions bot added the clang Clang issues not falling into any other category label May 10, 2024
@cor3ntin cor3ntin added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label May 10, 2024
@llvmbot
Copy link
Member

llvmbot commented May 10, 2024

@llvm/issue-subscribers-clang-frontend

Author: Eric Niebler (ericniebler)

I believe the following C++20 code should compile:
template &lt;class...&gt;
concept True = true;

template &lt;bool&gt;
struct I {
  template &lt;template &lt;class...&gt; class F, class... As&gt;
  using f = F&lt;As...&gt;;
};

template &lt;template &lt;class...&gt; class F, class... As&gt;
using meval = I&lt;True&lt;As...&gt;&gt;::template f&lt;F, As...&gt;;

template &lt;class F, class... As&gt;
concept P = true;

template &lt;class F, class... As&gt;
  requires P&lt;F, As...&gt;
using V = void;

template &lt;class...Ts&gt;
auto f() -&gt; meval&lt;V, int, Ts...&gt; {
}

int main() {
  f();
}

Clang 18 and earlier accepts it, as does gcc. With clang-trunk, I get:

&lt;source&gt;:10:29: error: pack expansion used as argument for non-pack parameter of alias template
   10 | template &lt;template &lt;class...&gt; class F, class... As&gt;
      |                             ^
1 error generated.
Compiler returned: 1

See repro here: https://godbolt.org/z/c1dG74EPd

@EugeneZelenko EugeneZelenko added rejects-valid and removed clang Clang issues not falling into any other category labels May 10, 2024
@mizvekov
Copy link
Contributor

That's CWG1430

We can ask guidance from core if it should apply in the template argument deduction that happens within TTP matching (as specified to happen in P0522R0, in terms of function template rewrite).

I see no such exclusion in the proposed solution.

@ericniebler
Copy link
Author

I'll note that this is breaking at least two popular open source libraries: stdexec and libunifex. Possibly others.

@cor3ntin
Copy link
Contributor

template <template <class...> class F, class... As>
using Alias = F<As...>;

template <template <class...> class F, class... As>
using meval = Alias<F, As...>;

template <class F, class... As>
concept P = true;

template <class F, class... As>
requires P<F, As...>
using V = void;

template <class...Ts>
auto f() -> meval<V, int, Ts...> {
}

int main() {
  f();
}

After #89807,
Clang implements https://eel.is/c++draft/temp.arg.template#4, and when deducing V (afaict)
https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1430 triggers. (note that there is no wording for this issue but all compilers do seem to implement it)

It's unclear what the conforming behavior here should be. Maybe we should ignore CWG1430 during partial ordering?
@zygoloid

I'll try to raise the issue in CWG but I think this issue give us ground to temporarily revert the default of
-fno-relaxed-template-template-args until we can either resolve CWG1430 in clang in a way that does not break code,
or we can gain enough clarity to determine stdexec is not conforming and we know how to fix it.

This is disrupting everyone using stdexec, which is at least 3 vendors.

@mizvekov are you able to work on a revert (switch back the flag to be off by default + remove the depreciation warning) ?

@mizvekov
Copy link
Contributor

When this sort of significant breakage happens, it's also usual that we would 'take the matter in to our own hands' and amend the rules ourselves, as we did to improve the situation with P0522.

@shafik
Copy link
Collaborator

shafik commented May 10, 2024

Note everyone is consistent on the examples from CWG1430: https://godbolt.org/z/xavdor73e

and currently clang is the only one that rejects this example: https://godbolt.org/z/qe1ercoqj

@cor3ntin raising some questions that looks like they need resolving, so it seems like we should revert so we can get some clarity here.

@ericniebler
Copy link
Author

ericniebler commented May 10, 2024

Commenting out the requires clause in the definition of the alias template V causes clang to accept the code. Which doesn't make any sense, and suggests to me that this is a bug in clang rather than in the c++ standard or stdexec.

template <class F, class... As>
//requires P<F, As...>
using V = void;

@mizvekov
Copy link
Contributor

It makes sense: There is a fast path that doesn't involve any template argument deduction, and it applies when the template parameter lists are close enough, but doesn't apply when constraints are involved.

If CWG1430 required us to diagnose this, then this fast path would be wrong to accept that code, but that doesn't help your problem.

@mizvekov
Copy link
Contributor

After deliberation about this issue, I think we should accept the code.

The template template parameter matching rules are a duck-typed, anything goes matter, where we will figure out if the use was valid later when we instantiate the template.
If we passed a parameter with no valid uses, then it would be good we diagnose it.
But what is happening here in this example is valid for some uses, and applying a rule that requires all uses be valid would be incongruous with the whole duck-typing thing.

@cor3ntin
Copy link
Contributor

We operated a partial revert of the problematic commit here #91811
That should fix the issues in trunk while we investigate the regression more thoroughly.

@mizvekov
Copy link
Contributor

Duplicate of #62529

@mizvekov mizvekov marked this as a duplicate of #62529 May 13, 2024
@mizvekov mizvekov closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2024
@EugeneZelenko EugeneZelenko added the duplicate Resolved as duplicate label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" duplicate Resolved as duplicate rejects-valid
Projects
Status: Done
6 participants