[PATCH] c++: Parameter pack in requires parameter list [PR94808]

Tue Apr 28 18:45:12 GMT 2020

On 4/28/20 1:41 PM, Patrick Palka wrote:
> On Tue, 28 Apr 2020, Patrick Palka wrote:
> 
>> On Tue, 28 Apr 2020, Jason Merrill wrote:
>>> On 4/28/20 9:48 AM, Patrick Palka wrote:
>>>> When printing the substituted parameter list of a requires-expression as
>>>> part of the "in requirements with ..." context line during concepts
>>>> diagnostics, we weren't considering that substitution into a parameter
>>>> pack can yield zero or multiple parameters.
>>>>
>>>> Since this patch affects only concepts diagnostics, so far I tested with
>>>> RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer ICE
>>>> with the unreduced testcase in the PR.  Is this OK to commit after a
>>>> full bootstrap and regtest?
>>>
>>> OK.
>>
>> Thanks for the review.
>>
>>>
>>> Though I wonder about printing the dependent form and template arguments
>>> instead.  Do you have an opinion?
>>
>> I was going to say that on the one hand, it's convenient to have the
>> substituted form printed since it would let us to see through
>> complicated dependent type aliases, but it seems we don't strip type
>> aliases when pretty printing a parameter and its type with '%q#D'
>> anyway.  And I can't think of any other possible advantage of printing
>> the substituted form.
>>
>> So IMHO printing the dependent form and template arguments instead would
>> be better here.  I'll try to write a patch for this today.
> 
> Like so, tested so far with RUNTESTFLAGS="dg.exp=*concepts*" and also
> verified we no longer ICE with the unreduced testcase in the PR.  Does
> the following look OK to commit after a full bootstrap and regtest?
> 
> -- >8 --
> 
> Subject: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
> 
> When printing the substituted parameter list of a requires-expression as
> part of the "in requirements with ..." context line during concepts
> diagnostics, we weren't considering that substitution into a parameter
> pack can yield zero or multiple parameters.
> 
> This patch changes the way we print the parameter list of a
> requires-expression in print_requires_expression_info.  We now print the
> dependent form of the parameter list (along with its template parameter
> mapping) instead of printing its substituted form.  Besides being an
> improvement in its own, this also sidesteps the above parameter pack
> expansion issue altogether.
> 
> Tested with RUNTESTFLAGS="dg.exp=*concepts*" and also verified we longer
> ICE with the unreduced testcase in the PR.  Does this look OK to commit
> after a bootstrap and regtest?
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/94808
> 	* error.c (print_requires_expression_info): Print the dependent
> 	form of the parameter list with its template parameter mapping,
> 	rather than printing the substituted form.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/94808
> 	* g++.dg/concepts/diagnostic12.C: New test.
> 	* g++.dg/concepts/diagnostic5.C: Adjust expected diagnostic.
> ---
>   gcc/cp/error.c                               | 16 ++++------------
>   gcc/testsuite/g++.dg/concepts/diagnostic12.C | 16 ++++++++++++++++
>   gcc/testsuite/g++.dg/concepts/diagnostic5.C  |  4 ++--
>   3 files changed, 22 insertions(+), 14 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
> 
> diff --git a/gcc/cp/error.c b/gcc/cp/error.c
> index 98c163db572..46970f9b699 100644
> --- a/gcc/cp/error.c
> +++ b/gcc/cp/error.c
> @@ -3746,7 +3746,6 @@ print_requires_expression_info (diagnostic_context *context, tree constr, tree a
>     map = tsubst_parameter_mapping (map, args, tf_none, NULL_TREE);
>     if (map == error_mark_node)
>       return;
> -  args = get_mapped_args (map);
>   
>     print_location (context, cp_expr_loc_or_input_loc (expr));
>     pp_verbatim (context->printer, "in requirements ");
> @@ -3756,19 +3755,12 @@ print_requires_expression_info (diagnostic_context *context, tree constr, tree a
>       pp_verbatim (context->printer, "with ");
>     while (parms)
>       {
> -      tree next = TREE_CHAIN (parms);
> -
> -      TREE_CHAIN (parms) = NULL_TREE;
> -      cp_unevaluated u;
> -      tree p = tsubst (parms, args, tf_none, NULL_TREE);
> -      pp_verbatim (context->printer, "%q#D", p);
> -      TREE_CHAIN (parms) = next;
> -
> -      if (next)
> +      pp_verbatim (context->printer, "%q#D", parms);
> +      if (TREE_CHAIN (parms))
>           pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
> -
> -      parms = next;
> +      parms = TREE_CHAIN (parms);
>       }
> +  pp_cxx_parameter_mapping ((cxx_pretty_printer *)context->printer, map);
>   
>     pp_verbatim (context->printer, "\n");
>   }
> diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic12.C b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
> new file mode 100644
> index 00000000000..a757342f754
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
> @@ -0,0 +1,16 @@
> +// PR c++/94808
> +// { dg-do compile { target concepts } }
> +
> +template<typename T, typename... Args>
> +  concept c1 = requires (T t, Args... args) { *t; };
> +// { dg-message "in requirements with .T t., .Args ... args. .with.* Args = \{\}" "" { target *-*-* } .-1 }

Doesn't this print a binding for T?

> +
> +static_assert(c1<int>); // { dg-error "failed" }
> +
> +void f(...);
> +
> +template<typename... Args>
> +  concept c2 = requires (Args... args) { f(*args...); };
> +// { dg-message "in requirements with .Args ... args. .with Args = \{int, char\}" "" { target *-*-* } .-1 }
> +
> +static_assert(c2<int, char>); // { dg-error "failed" }
> diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic5.C b/gcc/testsuite/g++.dg/concepts/diagnostic5.C
> index 0d890d6f548..81705f6a0c6 100644
> --- a/gcc/testsuite/g++.dg/concepts/diagnostic5.C
> +++ b/gcc/testsuite/g++.dg/concepts/diagnostic5.C
> @@ -9,7 +9,7 @@ template<typename T>
>   template<typename T>
>     concept c2 = requires (T x) { *x; };
>   // { dg-message "satisfaction of .c2<T>. .with T = char." "" { target *-*-* } .-1 }
> -// { dg-message "in requirements with .char x." "" { target *-*-* } .-2 }
> +// { dg-message "in requirements with .T x. .with T = char." "" { target *-*-* } .-2 }
>   // { dg-message "required expression .* is invalid" "" { target *-*-* } .-3 }
>   
>   template<typename T>
> @@ -25,7 +25,7 @@ template<typename T>
>   template<typename T>
>     concept c5 = requires (T x) { { &x } -> c1; };
>   // { dg-message "satisfaction of .c5<T>. .with T = char." "" { target *-*-* } .-1 }
> -// { dg-message "in requirements with .char x." "" { target *-*-* } .-2 }
> +// { dg-message "in requirements with .T x. .with T = char." "" { target *-*-* } .-2 }
>   
>   template<typename T>
>     requires (c1<T> || c2<T>) || (c3<T> || c4<T>) || c5<T> // { dg-message "49: no operand" }
> 