Re: [PATCH] c++: partial spec constraint checking context [PR105220]

[Patrick Palka <ppalka@redhat.com>]

On Fri, 3 Nov 2023, Patrick Palka wrote:

> On Tue, 3 May 2022, Jason Merrill wrote:
> 
> > On 5/2/22 14:50, Patrick Palka wrote:
> > > Currently when checking the constraints of a class template, we do so in
> > > the context of the template, not the specialized type.  This is the best
> > > we can do for a primary template since the specialized type is valid
> > > only if the primary template's constraints are satisfied.
> > 
> > Hmm, that's unfortunate.  It ought to be possible, if awkward, to form the
> > type long enough to check its constraints.
> 
> (Sorry, lost track of this patch...)
> 
> Seems doable, but I'm not sure if would make any difference in practice?
> 
> If the access context during satisfaction of a primary class template's
> constraints is the specialization rather than the primary template,
> then that should only make a difference if there's some friend declaration
> naming the specialization.  But that'd mean the specialization's
> constraints had to have been satisfied at that point, before the friend
> declaration went into effect.  So either the constraints don't depend on
> the access granted by the friend declaration anyway, or they do and the
> program is ill-formed (due to either satifaction failure or instability) IIUC.
> 
> For example, I don't think an adapted version of the testcase without a
> partial specialization is valid, regardless of whether the access context
> during satisfaction of A<B> is A<B> or just A:
> 
>     template<class T>
>     concept fooable = requires(T t) { t.foo(); };
> 
>     template<fooable T>
>     struct A { };
> 
>     struct B {
>     private:
>       friend struct A<B>; // satisfaction failure at this point
>       void foo();
>     };
> 
>     template struct A<B>;

... so in light of the above, I wonder if the original patch can go in
as-is?

> 
> 
> > 
> > > But for a
> > > partial specialization, we can assume the specialized type is valid (as
> > > a consequence of constraints being checked only when necessary), so we
> > > arguably should check the constraints on a partial specialization more
> > > specifically in the context of the specialized type, not the template.
> > > 
> > > This patch implements this by substituting and setting the access
> > > context appropriately in satisfy_declaration_constraints.  Note that
> > > setting the access context in this case is somewhat redundant since the
> > > relevant caller most_specialized_partial_spec will already have set the
> > > access context to the specialiation, but this redundancy should be harmless.
> > > 
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > > trunk and perhaps 12.2 (after the branch is thawed)?
> > > 
> > > 	PR c++/105220
> > > 
> > > gcc/cp/ChangeLog:
> > > 
> > > 	* constraint.cc (satisfy_declaration_constraints): When checking
> > > 	the constraints of a partial template specialization, do so in
> > > 	the context of the specialized type not the template.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 
> > > 	* g++.dg/cpp2a/concepts-partial-spec12.C: New test.
> > > ---
> > >   gcc/cp/constraint.cc                          | 17 ++++++++++++++---
> > >   .../g++.dg/cpp2a/concepts-partial-spec12.C    | 19 +++++++++++++++++++
> > >   2 files changed, 33 insertions(+), 3 deletions(-)
> > >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-partial-spec12.C
> > > 
> > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> > > index 94f6222b436..772f8532b47 100644
> > > --- a/gcc/cp/constraint.cc
> > > +++ b/gcc/cp/constraint.cc
> > > @@ -3253,11 +3253,22 @@ satisfy_declaration_constraints (tree t, tree args,
> > > sat_info info)
> > >       {
> > >         if (!push_tinst_level (t, args))
> > >   	return result;
> > > -      tree pattern = DECL_TEMPLATE_RESULT (t);
> > > +      tree ascope = DECL_TEMPLATE_RESULT (t);
> > > +      if (CLASS_TYPE_P (TREE_TYPE (t))
> > > +	  && CLASSTYPE_TEMPLATE_SPECIALIZATION (TREE_TYPE (t)))
> > > +	{
> > > +	  gcc_checking_assert (t == most_general_template (t));
> > > +	  /* When checking the constraints on a partial specialization,
> > > +	     do so in the context of the specialized type, not the template.
> > > +	     This substitution should always succeed since we shouldn't
> > > +	     be checking constraints thereof unless the specialized type
> > > +	     is valid.  */
> > > +	  ascope = tsubst (ascope, args, tf_none, info.in_decl);
> > > +	}
> > >         push_to_top_level ();
> > > -      push_access_scope (pattern);
> > > +      push_access_scope (ascope);
> > >         result = satisfy_normalized_constraints (norm, args, info);
> > > -      pop_access_scope (pattern);
> > > +      pop_access_scope (ascope);
> > >         pop_from_top_level ();
> > >         pop_tinst_level ();
> > >       }
> > > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-partial-spec12.C
> > > b/gcc/testsuite/g++.dg/cpp2a/concepts-partial-spec12.C
> > > new file mode 100644
> > > index 00000000000..641d456722d
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-partial-spec12.C
> > > @@ -0,0 +1,19 @@
> > > +// PR c++/105220
> > > +// { dg-do compile { target c++20 } }
> > > +
> > > +template<class T>
> > > +concept fooable = requires(T t) { t.foo(); };
> > > +
> > > +template<class>
> > > +struct A;        // #1, incomplete
> > > +
> > > +template<fooable T>
> > > +struct A<T> { }; // #2
> > > +
> > > +struct B {
> > > +private:
> > > +  friend struct A<B>;
> > > +  void foo();
> > > +};
> > > +
> > > +template struct A<B>; // OK, B::foo() is accessible from #2
> > 
> > 
>