diff mbox series

[v2] Fix auto deduction for template specialization scopes [PR114915]

Message ID 20240504151503.661288-1-sska1377@gmail.com
State New
Headers show
Series [v2] Fix auto deduction for template specialization scopes [PR114915] | expand

Commit Message

Seyed Sajad Kahani May 4, 2024, 3:15 p.m. UTC 
The limitations of the initial patch (checking specializiation template usage), have been discussed.

> I realized that for the case where we have a member function template
> of a class template, and a specialization of the enclosing class only
> (like below),
>
> template <>
> template <typename U>
> void S<int>::f() {
>   // some constrained auto
> }
>
> When using S<int>::f<double>, DECL_TEMPLATE_INFO(fn) is non-zero, and
> DECL_TEMPLATE_SPECIALIZATION(fn) is zero, while
> DECL_TEMPLATE_SPECIALIZATION(DECL_TI_TEMPLATE(fn)) is non-zero.
> So it means that the patch will extract DECL_TI_ARGS(fn) as
> outer_targs, and it would be <int> <double> while the type of the
> constrained auto will be as template <typename U> ... and will not be
> dependent on the parameters of the enclosing class.
> This means that again (outer_targs + targs) will have more depth than
> auto_node levels.
> This means that for the case where the function is not an explicit
> specialization, but it is defined in an explicitly specialized scope,
> the patch will not work.

As described in more detail below, this patch attempts to resolve this issue by trimming full_targs.

> > Another more context-unaware approach to fix this might be to only
> > use the innermost level from 'full_targs' for satisfaction if
> > TEMPLATE_TYPE_ORIG_LEVEL is 1 (which means this constrained auto
> > appeared in a context that doesn't have template parameters such as an
> > explicit specialization or ordinary non-template function, and so
> > only the level corresponding to the deduced type is needed for
> > satisfaction.)
> >
> > Generalizing on that, another approach might be to handle missing_levels < 0
> > by removing -missing_levels from full_targs via get_innermost_template_args.
> > But AFAICT it should suffice to handle the TEMPLATE_TYPE_ORIG_LEVEL == 1
> > case specifically.
>
> I was unable to understand why you think that it might not handle
> TEMPLATE_TYPE_ORIG_LEVEL > 1 cases, so I tried to formulate my
> reasoning as follows.
>
> Assuming contexts adc_variable_type, adc_return_type, adc_decomp_type:
> For any case where missing_level < 0, it means that the type depends
> on fewer levels than the template arguments used to materialize it.
> This can only happen when the type is defined in an explicit
> specialization scope. This explicit specialization might not occur in
> its immediate scope.
> Note that partial specialization (while changing the set of
> parameters) cannot reduce the number of levels for the type.
> Because of the fact that the enclosing scope of any explicit
> specialization is explicitly specialized
> (https://eel.is/c++draft/temp.expl.spec#16), the type will not depend
> on template parameters outside of its innermost explicit specialized
> scope.
> Assuming that there are no real missing levels, by removing those
> levels, missing_level should be = 0. As a result, by roughly doing
>
> if (missing_levels < 0) {
>   tree trimmed_full_args = get_innermost_template_args(full_targs,
> TEMPLATE_TYPE_ORIG_LEVEL(auto_node));
>   full_targs = trimmed_full_args;
> }
> in pt.cc:31262, where we calculate and check missing_levels, we would
> be able to fix the errors.
> Note that, for the case where there are real missing levels, we are
> putting irrelevant template arguments for the missing levels instead
> of make_tree_vec(0). By this change:
> - If the type is independent of those missing levels: it works fine either way.
> - If the type is dependent on those missing levels: Instead of raising
> an ICE, the compiler exhibits undefined behavior.
---
 gcc/cp/pt.cc                                  | 14 ++++++--
 .../g++.dg/cpp2a/concepts-placeholder14.C     | 19 +++++++++++
 .../g++.dg/cpp2a/concepts-placeholder15.C     | 15 +++++++++
 .../g++.dg/cpp2a/concepts-placeholder16.C     | 33 +++++++++++++++++++
 4 files changed, 78 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder14.C
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder15.C
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder16.C

Comments

Patrick Palka May 6, 2024, 6:46 p.m. UTC | #1 
On Sat, 4 May 2024, Seyed Sajad Kahani wrote:

> The limitations of the initial patch (checking specializiation template usage), have been discussed.
> 
> > I realized that for the case where we have a member function template
> > of a class template, and a specialization of the enclosing class only
> > (like below),
> >
> > template <>
> > template <typename U>
> > void S<int>::f() {
> >   // some constrained auto
> > }
> >
> > When using S<int>::f<double>, DECL_TEMPLATE_INFO(fn) is non-zero, and
> > DECL_TEMPLATE_SPECIALIZATION(fn) is zero, while
> > DECL_TEMPLATE_SPECIALIZATION(DECL_TI_TEMPLATE(fn)) is non-zero.
> > So it means that the patch will extract DECL_TI_ARGS(fn) as
> > outer_targs, and it would be <int> <double> while the type of the
> > constrained auto will be as template <typename U> ... and will not be
> > dependent on the parameters of the enclosing class.
> > This means that again (outer_targs + targs) will have more depth than
> > auto_node levels.
> > This means that for the case where the function is not an explicit
> > specialization, but it is defined in an explicitly specialized scope,
> > the patch will not work.

Ah yes, good point!  This demonstrates that it doesn't suffice to
handle the TEMPLATE_TYPE_ORIG_LEVEL == 1 case contrary to what I
suggested earlier.

> 
> As described in more detail below, this patch attempts to resolve this issue by trimming full_targs.
> 
> > > Another more context-unaware approach to fix this might be to only
> > > use the innermost level from 'full_targs' for satisfaction if
> > > TEMPLATE_TYPE_ORIG_LEVEL is 1 (which means this constrained auto
> > > appeared in a context that doesn't have template parameters such as an
> > > explicit specialization or ordinary non-template function, and so
> > > only the level corresponding to the deduced type is needed for
> > > satisfaction.)
> > >
> > > Generalizing on that, another approach might be to handle missing_levels < 0
> > > by removing -missing_levels from full_targs via get_innermost_template_args.
> > > But AFAICT it should suffice to handle the TEMPLATE_TYPE_ORIG_LEVEL == 1
> > > case specifically.
> >
> > I was unable to understand why you think that it might not handle
> > TEMPLATE_TYPE_ORIG_LEVEL > 1 cases, so I tried to formulate my
> > reasoning as follows.

Yes, sorry about that misleading suggestion.

> >
> > Assuming contexts adc_variable_type, adc_return_type, adc_decomp_type:
> > For any case where missing_level < 0, it means that the type depends
> > on fewer levels than the template arguments used to materialize it.
> > This can only happen when the type is defined in an explicit
> > specialization scope. This explicit specialization might not occur in
> > its immediate scope.
> > Note that partial specialization (while changing the set of
> > parameters) cannot reduce the number of levels for the type.
> > Because of the fact that the enclosing scope of any explicit
> > specialization is explicitly specialized
> > (https://eel.is/c++draft/temp.expl.spec#16), the type will not depend
> > on template parameters outside of its innermost explicit specialized
> > scope.
> > Assuming that there are no real missing levels, by removing those
> > levels, missing_level should be = 0. As a result, by roughly doing
> >
> > if (missing_levels < 0) {
> >   tree trimmed_full_args = get_innermost_template_args(full_targs,
> > TEMPLATE_TYPE_ORIG_LEVEL(auto_node));
> >   full_targs = trimmed_full_args;
> > }
> > in pt.cc:31262, where we calculate and check missing_levels, we would
> > be able to fix the errors.
> > Note that, for the case where there are real missing levels, we are
> > putting irrelevant template arguments for the missing levels instead
> > of make_tree_vec(0). By this change:
> > - If the type is independent of those missing levels: it works fine either way.
> > - If the type is dependent on those missing levels: Instead of raising
> > an ICE, the compiler exhibits undefined behavior.

Makes sense.

> ---
>  gcc/cp/pt.cc                                  | 14 ++++++--
>  .../g++.dg/cpp2a/concepts-placeholder14.C     | 19 +++++++++++
>  .../g++.dg/cpp2a/concepts-placeholder15.C     | 15 +++++++++
>  .../g++.dg/cpp2a/concepts-placeholder16.C     | 33 +++++++++++++++++++
>  4 files changed, 78 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder14.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder15.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder16.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 3b2106dd3..bdf03a1a7 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -31044,7 +31044,8 @@ unparenthesized_id_or_class_member_access_p (tree init)
>     OUTER_TARGS is used during template argument deduction (context == adc_unify)
>     to properly substitute the result.  It's also used in the adc_unify and
>     adc_requirement contexts to communicate the necessary template arguments
> -   to satisfaction.  OUTER_TARGS is ignored in other contexts.
> +   to satisfaction.  OUTER_TARGS will be used for other contexts if it is a
> +   function scope deduction. Otherwise it is ignored.

The use of OUTER_TARGS for satisfaction is already generically covered
in the last paragraph of the function comment:

   For partial-concept-ids, extra args from OUTER_TARGS, TMPL and the current
   scope may be appended to the list of deduced template arguments prior to
   determining constraint satisfaction as appropriate.

So this change is probably unnecessary.

>  
>     Additionally for adc_unify contexts TMPL is the template for which TYPE
>     is a template parameter type.
> @@ -31260,8 +31261,15 @@ do_auto_deduction (tree type, tree init, tree auto_node,
>  	 these missing levels, but this hack otherwise allows us to handle a
>  	 large subset of possible constraints (including all non-dependent
>  	 constraints).  */
> -      if (int missing_levels = (TEMPLATE_TYPE_ORIG_LEVEL (auto_node)
> -				- TMPL_ARGS_DEPTH (full_targs)))
> +      int missing_levels = (TEMPLATE_TYPE_ORIG_LEVEL (auto_node)
> +				- TMPL_ARGS_DEPTH (full_targs));
> +      if (missing_levels < 0)
> +	{
> +	  tree trimmed_full_args = get_innermost_template_args(full_targs,
> +				TEMPLATE_TYPE_ORIG_LEVEL (auto_node));
> +	  full_targs = trimmed_full_args;
> +	}
> +      if (missing_levels > 0)

Looks correct to me!  (Though in order to be consistent the GNU coding
style while respecting the 80-character line limit, this should be
formatted as

      if (missing_levels < 0)
	{
	  tree trimmed_full_args = get_innermost_template_args
	    (full_targs, TEMPLATE_TYPE_ORIG_LEVEL (auto_node));
	  full_targs = trimmed_full_args;
	}

We could also/instead consider defining

  int want = TEMPLATE_TYPE_ORIG_LEVEL (auto_node);
  int have = TMPL_ARGS_DEPTH (full_targs);

and express the logic in terms of these instead of the possibly
negative 'missing_levels'.  The 'want < have' case could have a comment
mentioning that this occurs for a constrained auto within an explicit
specialization.)

Note that GCC doesn't currently support explicit specializations within
template scope, e.g.

    template<class T>
    struct A {
      template<class U> struct B { };
      template<> struct B<int> {
        static inline C auto x = 1;
      };
    };

which might otherwise complicate things here.  But since we don't yet
support it we don't have to worry about this case :)  I think stripping
the outermost extraneous levels as your patch does will currently always
do the right thing for us.  But in the above example which we don't
support, we probably would need to strip _innermost_ extraneous levels,
i.e. the <int>.

>  	{
>  	  tree dummy_levels = make_tree_vec (missing_levels);
>  	  for (int i = 0; i < missing_levels; ++i)
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder14.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder14.C
> new file mode 100644
> index 000000000..fcdbd7608
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder14.C
> @@ -0,0 +1,19 @@
> +// PR c++/114915
> +// { dg-do compile { target c++20 } }
> +
> +template<typename T>
> +concept C = __is_same(T, int);
> +
> +template<typename T>
> +void f() {
> +}
> +
> +template<>
> +void f<int>() {
> +  C auto x = 1;
> +}
> +
> +int main() {
> +  f<int>();
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder15.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder15.C
> new file mode 100644
> index 000000000..b4f73f407
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder15.C
> @@ -0,0 +1,15 @@
> +// PR c++/114915
> +// { dg-do compile { target c++20 } }
> +
> +template<typename T, typename U>
> +concept C = __is_same(T, U);
> +
> +template<typename T>
> +int x = 0;
> +
> +template<>
> +C<double> auto x<double> = 1.0;
> +
> +int main() {
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder16.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder16.C
> new file mode 100644
> index 000000000..f808ef1b6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder16.C
> @@ -0,0 +1,33 @@
> +// PR c++/114915
> +// { dg-do compile { target c++20 } }
> +
> +template<typename T, typename U>
> +concept C = __is_same(T, U);
> +
> +template<typename T>
> +struct A
> +{ 
> +    template<typename U>
> +    void f() {
> +    }
> +};
> + 
> +template<>
> +template<>
> +void A<int>::f<int>() {
> +  C<int> auto x = 1;
> +}
> +
> +template<>
> +template<typename U>
> +void A<bool>::f() {
> +  C<int> auto x = 1;
> +}
> +
> +int main() {
> +  A<bool> a;
> +  a.f<char>();
> +  A<int> b;
> +  b.f<int>();
> +  return 0;
> +}

Thanks for these extra testcases!

> -- 
> 2.45.0
> 
>
Seyed Sajad Kahani May 10, 2024, 12:43 p.m. UTC | #2 
Thanks for your suggestions. I will apply them in the upcoming patch (v3).

On Mon, May 6, 2024 at 7:46 PM Patrick Palka <ppalka@redhat.com> wrote:
> We could also/instead consider defining
>
> int want = TEMPLATE_TYPE_ORIG_LEVEL (auto_node);
> int have = TMPL_ARGS_DEPTH (full_targs);
>
> and express the logic in terms of these instead of the possibly
> negative 'missing_levels'. The 'want < have' case could have a comment
> mentioning that this occurs for a constrained auto within an explicit
> specialization.)

I converted missing_levels into want and have variables. Additionally, I
added an assertion to explicitly state that our trimming logic assumes the
context to be one of adc_variable_type, adc_return_type, or adc_decomp_type.

> > > Note that, for the case where there are real missing levels, we are
> > > putting irrelevant template arguments for the missing levels instead
> > > of make_tree_vec(0). By this change:
> > > - If the type is independent of those missing levels: it works fine either way.
> > > - If the type is dependent on those missing levels: Instead of raising
> > > an ICE, the compiler exhibits undefined behavior.
>
> Makes sense.

For the record, I have looked into the history of this hack to find out in
which scenarios we may have to deal with missing levels.
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b49d23f3e238c08bdbc5b892b2ed0a57b5f5caf9
As far as I can understand, it was limited to the adc_unify context at that
time, but it might have been changed since then. Since I was not sure about
it, I have not added an assertion for this case.

Many thanks!
diff mbox series

Patch

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 3b2106dd3..bdf03a1a7 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -31044,7 +31044,8 @@  unparenthesized_id_or_class_member_access_p (tree init)
    OUTER_TARGS is used during template argument deduction (context == adc_unify)
    to properly substitute the result.  It's also used in the adc_unify and
    adc_requirement contexts to communicate the necessary template arguments
-   to satisfaction.  OUTER_TARGS is ignored in other contexts.
+   to satisfaction.  OUTER_TARGS will be used for other contexts if it is a
+   function scope deduction. Otherwise it is ignored.
 
    Additionally for adc_unify contexts TMPL is the template for which TYPE
    is a template parameter type.
@@ -31260,8 +31261,15 @@  do_auto_deduction (tree type, tree init, tree auto_node,
 	 these missing levels, but this hack otherwise allows us to handle a
 	 large subset of possible constraints (including all non-dependent
 	 constraints).  */
-      if (int missing_levels = (TEMPLATE_TYPE_ORIG_LEVEL (auto_node)
-				- TMPL_ARGS_DEPTH (full_targs)))
+      int missing_levels = (TEMPLATE_TYPE_ORIG_LEVEL (auto_node)
+				- TMPL_ARGS_DEPTH (full_targs));
+      if (missing_levels < 0)
+	{
+	  tree trimmed_full_args = get_innermost_template_args(full_targs,
+				TEMPLATE_TYPE_ORIG_LEVEL (auto_node));
+	  full_targs = trimmed_full_args;
+	}
+      if (missing_levels > 0)
 	{
 	  tree dummy_levels = make_tree_vec (missing_levels);
 	  for (int i = 0; i < missing_levels; ++i)
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder14.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder14.C
new file mode 100644
index 000000000..fcdbd7608
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder14.C
@@ -0,0 +1,19 @@ 
+// PR c++/114915
+// { dg-do compile { target c++20 } }
+
+template<typename T>
+concept C = __is_same(T, int);
+
+template<typename T>
+void f() {
+}
+
+template<>
+void f<int>() {
+  C auto x = 1;
+}
+
+int main() {
+  f<int>();
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder15.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder15.C
new file mode 100644
index 000000000..b4f73f407
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder15.C
@@ -0,0 +1,15 @@ 
+// PR c++/114915
+// { dg-do compile { target c++20 } }
+
+template<typename T, typename U>
+concept C = __is_same(T, U);
+
+template<typename T>
+int x = 0;
+
+template<>
+C<double> auto x<double> = 1.0;
+
+int main() {
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder16.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder16.C
new file mode 100644
index 000000000..f808ef1b6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder16.C
@@ -0,0 +1,33 @@ 
+// PR c++/114915
+// { dg-do compile { target c++20 } }
+
+template<typename T, typename U>
+concept C = __is_same(T, U);
+
+template<typename T>
+struct A
+{ 
+    template<typename U>
+    void f() {
+    }
+};
+ 
+template<>
+template<>
+void A<int>::f<int>() {
+  C<int> auto x = 1;
+}
+
+template<>
+template<typename U>
+void A<bool>::f() {
+  C<int> auto x = 1;
+}
+
+int main() {
+  A<bool> a;
+  a.f<char>();
+  A<int> b;
+  b.f<int>();
+  return 0;
+}