Re: [PATCH] c++: constraint matching, TEMPLATE_ID_EXPR, current inst

[Patrick Palka <ppalka@redhat.com>]

On Fri, 16 Sep 2022, Jason Merrill wrote:

> On 9/15/22 11:58, Patrick Palka wrote:
> > Here we're crashing during constraint matching for the instantiated
> > hidden friends due to two issues with dependent substitution into a
> > TEMPLATE_ID_EXPR naming a template from the current instantiation
> > (as performed from maybe_substitute_reqs_for for C<3> with T=T):
> > 
> >    * tsubst_copy substitutes into such a TEMPLATE_DECL by looking it
> >      up from the substituted class scope.  But for this to not fail when
> >      the args are dependent, we need to pass entering_scope=true for the
> >      class scope substitution so that we obtain the primary template type
> >      A<T> (which has TYPE_BINFO) instead of the implicit instantiation
> >      A<T> (which doesn't).
> >    * lookup_and_finish_template_variable shouldn't instantiate a
> >      TEMPLATE_ID_EXPR that names a TEMPLATE_DECL which has more than
> >      one level of (unsubstituted) parameters (such as A<T>::C).
> > 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* pt.cc (lookup_and_finish_template_variable): Don't
> > 	instantiate if the template's scope is dependent.
> > 	(tsubst_copy) <case TEMPLATE_DECL>: Pass entering_scope=true
> > 	when substituting the class scope.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp2a/concepts-friend10.C: New test.
> > ---
> >   gcc/cp/pt.cc                                  | 14 +++++++------
> >   .../g++.dg/cpp2a/concepts-friend10.C          | 21 +++++++++++++++++++
> >   2 files changed, 29 insertions(+), 6 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-friend10.C
> > 
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index db4e808adec..bfcbe0b8670 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -10475,14 +10475,15 @@ tree
> >   lookup_and_finish_template_variable (tree templ, tree targs,
> >   				     tsubst_flags_t complain)
> >   {
> > -  templ = lookup_template_variable (templ, targs);
> > -  if (!any_dependent_template_arguments_p (targs))
> > +  tree var = lookup_template_variable (templ, targs);
> > +  if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (templ)) == 1
> > +      && !any_dependent_template_arguments_p (targs))
> 
> I notice that finish_id_expression_1 uses the equivalent of
> type_dependent_expression_p (var).  Does that work here?

Hmm, it does, but kind of by accident: type_dependent_expression_p
returns true for all variable TEMPLATE_ID_EXPRs because of their empty
TREE_TYPE (as set by finish_template_variable).  So testing t_d_e_p here
is equivalent to testing processing_template_decl, it seems -- maximally
conservative.

We can improve type_dependent_expression_p for variable TEMPLATE_ID_EXPR
by ignoring its (always empty) TREE_TYPE and just considering dependence
of its template and args directly.

Doing so exposes that value_dependent_expression_p is wrong for
(non-type-dependent) variable template specializations -- since we don't
set/track DECL_DEPENDENT_INIT_P for them, the VAR_DECL branch ends up
returning false even if the initializer depends on outer args.  Instead,
I suppose we can give a reasonably conservative answer by considering
dependence of its enclosing scope as we do for FUNCTION_DECL.

Does the following seem reasonable?  Bootstrapped and regtested on
x86_64-pc-linux-gnu.

-- >8 --

gcc/cp/ChangeLog:

	* pt.cc (finish_template_variable): Consider only the innermost
	template parms since we have only the innermost args.
	(lookup_and_finish_template_variable): Check
	type_dependent_expression_p instead.
	(tsubst_copy) <case TEMPLATE_DECL>: Pass entering_scope=true
	when substituting the class scope.
	(value_dependent_expression_p) <case FUNCTION_DECL>: Move below ...
	<case VAR_DECL>: ... here.  Fall through for variable template
	specializations.
	(type_dependent_expression_p): Handle variable TEMPLATE_ID_EXPR
	precisely.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1y/noexcept1.C: Expect another ahead of time error.
	* g++.dg/cpp1y/var-templ70.C: New test.
	* g++.dg/cpp2a/concepts-friend10.C: New test.
---
 gcc/cp/pt.cc                                  | 53 ++++++++++++-------
 gcc/testsuite/g++.dg/cpp1y/noexcept1.C        |  2 +-
 gcc/testsuite/g++.dg/cpp1y/var-templ70.C      | 22 ++++++++
 .../g++.dg/cpp2a/concepts-friend10.C          | 24 +++++++++
 4 files changed, 82 insertions(+), 19 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/var-templ70.C
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-friend10.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index db4e808adec..88a09891a00 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -10447,10 +10447,10 @@ finish_template_variable (tree var, tsubst_flags_t complain)
   tree templ = TREE_OPERAND (var, 0);
   tree arglist = TREE_OPERAND (var, 1);
 
-  tree parms = DECL_TEMPLATE_PARMS (templ);
-  arglist = coerce_innermost_template_parms (parms, arglist, templ, complain,
-					     /*req_all*/true,
-					     /*use_default*/true);
+  tree parms = DECL_INNERMOST_TEMPLATE_PARMS (templ);
+  arglist = coerce_template_parms (parms, arglist, templ, complain,
+				   /*req_all*/true,
+				   /*use_default*/true);
   if (arglist == error_mark_node)
     return error_mark_node;
 
@@ -10475,14 +10475,14 @@ tree
 lookup_and_finish_template_variable (tree templ, tree targs,
 				     tsubst_flags_t complain)
 {
-  templ = lookup_template_variable (templ, targs);
-  if (!any_dependent_template_arguments_p (targs))
+  tree var = lookup_template_variable (templ, targs);
+  if (!type_dependent_expression_p (var))
     {
-      templ = finish_template_variable (templ, complain);
-      mark_used (templ);
+      var = finish_template_variable (var, complain);
+      mark_used (var);
     }
 
-  return convert_from_reference (templ);
+  return convert_from_reference (var);
 }
 
 /* If the set of template parameters PARMS contains a template parameter
@@ -17282,7 +17282,8 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 	     TEMPLATE_DECL with `D<T>' as its DECL_CONTEXT.  Now we
 	     have to substitute this with one having context `D<int>'.  */
 
-	  tree context = tsubst (DECL_CONTEXT (t), args, complain, in_decl);
+	  tree context = tsubst_aggr_type (DECL_CONTEXT (t), args, complain,
+					   in_decl, /*entering_scope=*/true);
 	  return lookup_field (context, DECL_NAME(t), 0, false);
 	}
       else
@@ -27684,13 +27685,6 @@ value_dependent_expression_p (tree expression)
       /* A dependent member function of the current instantiation.  */
       return dependent_type_p (BINFO_TYPE (BASELINK_BINFO (expression)));
 
-    case FUNCTION_DECL:
-      /* A dependent member function of the current instantiation.  */
-      if (DECL_CLASS_SCOPE_P (expression)
-	  && dependent_type_p (DECL_CONTEXT (expression)))
-	return true;
-      break;
-
     case IDENTIFIER_NODE:
       /* A name that has not been looked up -- must be dependent.  */
       return true;
@@ -27725,7 +27719,19 @@ value_dependent_expression_p (tree expression)
 		  && value_expr == error_mark_node))
 	    return true;
 	}
-      return false;
+      if (!variable_template_specialization_p (expression))
+	/* For variable template specializations, also consider dependence
+	   of the enclosing scope.  For other specializations it seems we
+	   can trust DECL_DEPENDENT_INIT_P.  */
+	return false;
+      /* Fall through.  */
+
+    case FUNCTION_DECL:
+      /* A dependent member of the current instantiation.  */
+      if (DECL_CLASS_SCOPE_P (expression)
+	  && dependent_type_p (DECL_CONTEXT (expression)))
+	return true;
+      break;
 
     case DYNAMIC_CAST_EXPR:
     case STATIC_CAST_EXPR:
@@ -28095,6 +28101,17 @@ type_dependent_expression_p (tree expression)
       expression = BASELINK_FUNCTIONS (expression);
     }
 
+  /* A variable TEMPLATE_ID_EXPR is type-dependent iff the template or
+     arguments are.  */
+  if (TREE_CODE (expression) == TEMPLATE_ID_EXPR)
+    {
+      tree tmpl = TREE_OPERAND (expression, 0);
+      tree args = TREE_OPERAND (expression, 1);
+      if (variable_template_p (tmpl) && !variable_concept_p (tmpl))
+	return type_dependent_expression_p (tmpl)
+	  || any_dependent_template_arguments_p (args);
+    }
+
   /* A function or variable template-id is type-dependent if it has any
      dependent template arguments.  */
   if (VAR_OR_FUNCTION_DECL_P (expression)
diff --git a/gcc/testsuite/g++.dg/cpp1y/noexcept1.C b/gcc/testsuite/g++.dg/cpp1y/noexcept1.C
index 86e46c96148..a660b9d6c9e 100644
--- a/gcc/testsuite/g++.dg/cpp1y/noexcept1.C
+++ b/gcc/testsuite/g++.dg/cpp1y/noexcept1.C
@@ -8,6 +8,6 @@ struct C {
   template <typename> friend int foo() noexcept(b<1>); // { dg-error "not usable in a constant expression|different exception specifier" }
 };
 
-template <typename> int foo() noexcept(b<1>);
+template <typename> int foo() noexcept(b<1>); // { dg-error "not usable in a constant expression" }
 
 auto a = C<int>();
diff --git a/gcc/testsuite/g++.dg/cpp1y/var-templ70.C b/gcc/testsuite/g++.dg/cpp1y/var-templ70.C
new file mode 100644
index 00000000000..e1040165c34
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/var-templ70.C
@@ -0,0 +1,22 @@
+// Verify we correctly compute value dependence for variable template
+// specializations.
+// { dg-do compile { target c++17 } }
+
+template<class T> static constexpr int value = false;
+
+template<class T>
+void f() {
+  // value<int> is not dependent, so we can check this ahead of time.
+  static_assert(value<int>, ""); // { dg-error "assertion failed" }
+}
+
+template<class T>
+struct A {
+  template<class U> static constexpr bool member = T();
+  auto f() {
+    // member<int> is not type dependent, but we consider it value dependent
+    // since it's a member of the current instantiation, so we don't check
+    // this ahead of time.
+    static_assert(member<int>, "");
+  }
+};
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-friend10.C b/gcc/testsuite/g++.dg/cpp2a/concepts-friend10.C
new file mode 100644
index 00000000000..bb5e1e6038b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-friend10.C
@@ -0,0 +1,24 @@
+// Verify we don't crash during constraint matching containing a
+// TEMPLATE_ID_EXPR that names a template from the current instantiation.
+// { dg-do compile { target c++20 } }
+
+template<class T> static constexpr bool False = false;
+
+template<class T>
+struct A {
+  template<int N> static constexpr bool C = sizeof(T) > N;
+  friend constexpr void f(A) requires C<1> { }
+  friend constexpr void f(A) requires C<1> && False<T> { }
+};
+
+template<class T>
+struct A<T*> {
+  template<int N> static constexpr bool C = sizeof(T) > N;
+  friend constexpr void g(A) requires C<1> { }
+  friend constexpr void g(A) requires C<1> && False<T> { }
+};
+
+int main() {
+  f(A<int>{});
+  g(A<int*>{});
+}
-- 
2.38.0.rc0