[PATCH] c++: satisfaction and ARGUMENT_PACK_SELECT [PR105644]

[PATCH] c++: satisfaction and ARGUMENT_PACK_SELECT [PR105644]

[Patrick Palka]

This testcase demonstrates we can legitimately enter satisfaction with
an ARGUMENT_PACK_SELECT argument, which is problematic because we can't
store such arguments in the satisfaction cache (or any other hash table).

Since this appears to be possible only during constrained auto deduction
for a return-type-requirement, the most appropriate spot to fix this seems
to be from do_auto_deduction, by calling preserve_args to strip A_P_S args
before entering satisfaction.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk/12?

	PR c++/105644

gcc/cp/ChangeLog:

	* pt.cc (do_auto_deduction): Call preserve_args before entering
	satisfaction for adc_requirement contexts.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-requires36.C: New test.
---
 gcc/cp/pt.cc                                     |  6 ++++++
 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C | 12 ++++++++++++
 2 files changed, 18 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 4429ae66b68..821e0035c08 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -30965,6 +30965,12 @@ do_auto_deduction (tree type, tree init, tree auto_node,
 	    return type;
 	}
 
+      /* We can see an ARGUMENT_PACK_SELECT argument when evaluating
+	 a return-type-requirement.  Get rid of them before entering
+	 satisfaction, since the satisfaction cache can't handle them.  */
+      if (context == adc_requirement)
+	outer_targs = preserve_args (outer_targs);
+
       if (context == adc_return_type
 	  || context == adc_variable_type
 	  || context == adc_decomp_type)
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
new file mode 100644
index 00000000000..7d13b9b3e54
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
@@ -0,0 +1,12 @@
+// PR c++/105644
+// { dg-do compile { target c++20 } }
+
+template<class T, class U>
+concept same_as = __is_same(T, U);
+
+template<class... Ts>
+concept C = (requires { { Ts() } -> same_as<Ts>; } && ...);
+
+static_assert(C<int, char>);
+static_assert(!C<int, const char>);
+static_assert(!C<const int, char>);
-- 
2.40.0.153.g6369acd968

Re: [PATCH] c++: satisfaction and ARGUMENT_PACK_SELECT [PR105644]

[Jason Merrill]

On 4/3/23 10:49, Patrick Palka wrote:
> This testcase demonstrates we can legitimately enter satisfaction with
> an ARGUMENT_PACK_SELECT argument, which is problematic because we can't
> store such arguments in the satisfaction cache (or any other hash table).
> 
> Since this appears to be possible only during constrained auto deduction
> for a return-type-requirement, the most appropriate spot to fix this seems
> to be from do_auto_deduction, by calling preserve_args to strip A_P_S args
> before entering satisfaction.
> 
> +++ b/gcc/cp/pt.cc
> @@ -30965,6 +30965,12 @@ do_auto_deduction (tree type, tree init, tree auto_node,
>   	    return type;
>   	}
>   
> +      /* We can see an ARGUMENT_PACK_SELECT argument when evaluating
> +	 a return-type-requirement.  Get rid of them before entering
> +	 satisfaction, since the satisfaction cache can't handle them.  */
> +      if (context == adc_requirement)
> +	outer_targs = preserve_args (outer_targs);

I'd like to get do_auto_deduction out of the business of handling 
return-type-requirements, since there is no longer any actual deduction 
involved (as there was in the TS).  So I'd prefer not to add any more 
tweaks there.

Maybe this should happen higher up, in tsubst_requires_expr?  Maybe just 
before the call to add_extra_args?

Jason

Re: [PATCH] c++: satisfaction and ARGUMENT_PACK_SELECT [PR105644]

[Patrick Palka]

On Mon, 3 Apr 2023, Jason Merrill wrote:

> On 4/3/23 10:49, Patrick Palka wrote:
> > This testcase demonstrates we can legitimately enter satisfaction with
> > an ARGUMENT_PACK_SELECT argument, which is problematic because we can't
> > store such arguments in the satisfaction cache (or any other hash table).
> > 
> > Since this appears to be possible only during constrained auto deduction
> > for a return-type-requirement, the most appropriate spot to fix this seems
> > to be from do_auto_deduction, by calling preserve_args to strip A_P_S args
> > before entering satisfaction.
> > 
> > +++ b/gcc/cp/pt.cc
> > @@ -30965,6 +30965,12 @@ do_auto_deduction (tree type, tree init, tree
> > auto_node,
> >   	    return type;
> >   	}
> >   +      /* We can see an ARGUMENT_PACK_SELECT argument when evaluating
> > +	 a return-type-requirement.  Get rid of them before entering
> > +	 satisfaction, since the satisfaction cache can't handle them.  */
> > +      if (context == adc_requirement)
> > +	outer_targs = preserve_args (outer_targs);
> 
> I'd like to get do_auto_deduction out of the business of handling
> return-type-requirements, since there is no longer any actual deduction
> involved (as there was in the TS).  So I'd prefer not to add any more tweaks
> there.
> 
> Maybe this should happen higher up, in tsubst_requires_expr?  Maybe just
> before the call to add_extra_args?

Interestingly, calling preserve_args from tsubst_requires_expr breaks
cases where a requires-expression contains an inner pack expansion over
the same pack as the outer expansion, such as 'Ts... ts' in

  template<class... Ts>
  concept C = (requires (Ts... ts) { Ts(); } && ...);

  static_assert(C<int, char>);

and 'sizeof...(Ts)' in

  template<int> struct A;

  template<class... Ts>
  concept C = (requires { typename A<sizeof...(Ts)>; } && ...);

  static_assert(C<int, char>);

because we need to hold on to the ARGUMENT_PACK_SELECT version of the
argument in order to properly substitute 'Ts... ts' and 'sizeof...(Ts)'
during each outer expansion, but calling preserve_args gets rid of
the A_P_S.

And on second thought, narrowly calling preserve_args from
do_auto_deduction (or type_deducible_p) isn't quite correct either,
consider:

  template<class T, class U, int N>
  concept C = __is_same(T, U) && N > 1;

  template<class... Ts>
  concept D = (requires { { Ts() } -> C<Ts, sizeof...(Ts)>; } && ...);

  static_assert(D<int, char>);

We need to hold on to the A_P_S in order to check the
return-type-requirement C<Ts, sizeof...(Ts)>, but the satisfaction
cache can't handle A_P_S!

The following non-requires-expr version of that example works:

  template<class T, class U, int N>
  concept C = __is_same(T, U) && N > 1;

  template<class... Ts>
  concept D = (C<Ts, Ts, sizeof...(Ts)> && ...);

  static_assert(D<int, char>);

because here we directly substitute into the concept-id
C<Ts, Ts, sizeof...(Ts)>, so we effectively end up checking
satisfaction of C<int, int, 2> and C<char, char, 2> directly
and never enter satisfaction with an A_P_S argument.

So ISTM the satisfaction cache might need to be able to cache A_P_S
arguments perhaps by changing preserve_args to instead make a copy of
each A_P_S and set a flag on this copy to indicate it's "stable"
and therefore suitable for hashing etc.