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

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

[Patrick Palka]

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.  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
-- 
2.36.0.44.g0f828332d5

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

[Jason Merrill]

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.

> 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

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

[Patrick Palka]

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>;


> 
> > 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
> 
> 

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

[Patrick Palka]

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
> > 
> > 
>