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

[Patrick Palka <ppalka@redhat.com>]

On Fri, 16 Sep 2022, Patrick Palka wrote:

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

... as set by lookup_template_variable, rather

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