Re: [PATCH] c++: NTTP object wrapper substitution fixes [PR103346, ...]

[Patrick Palka <ppalka@redhat.com>]

On Mon, 19 Dec 2022, Jason Merrill wrote:

> On 12/6/22 13:35, Patrick Palka wrote:
> > This patch fixes some issues with substitution into a C++20 template
> > parameter object wrapper:
> > 
> > * The first testcase demonstrates a situation where the same_type_p
> >    assert in relevant case of tsubst_copy doesn't hold, because (partial)
> >    substitution of {int,} into the VIEW_CONVERT_EXPR wrapper yields
> >    A<int> but substitution into the underlying TEMPLATE_PARM_INDEX is a
> >    nop and yields A<T> due to tsubst's level == 1 early exit test.
> 
> We exit in that case because continuing would reduce the level to an
> impossible 0.  Why doesn't the preceding code find a binding for T?

Whoops, I misspoke.  The problem is that there's no binding for V since
only T=int was explicitly specified, so when substituting into the
TEMPLATE_PARM_INDEX for V in 'void f(B<V>)', we hit that early exit and
never get a chance to substitute T=int into the tpi's TREE_TYPE (which
would yield A<int> as desired).  So the TREE_TYPE of the wrapped tpi
remains A<T> whereas the substituted TREE_TYPE of the wrapper is A<int>,
a mismatch.

> 
> >    So
> >    this patch just gets rid of the assert; the type mismatch doesn't
> >    seem to be a problem in practice, I suppose because the coercion is
> >    from one dependent type to another.
> > 
> > * In the second testcase, dependent substitution into the underlying
> >    TEMPLATE_PARM_INDEX yields a CALL_EXPR with empty TREE_TYPE, which
> >    tsubst_copy doesn't expect.  This patch fixes this by handling empty
> >    TREE_TYPE the same way as a non-const TREE_TYPE.  Moreover, after
> >    this substitution we're left with a VIEW_CONVERT_EXPR wrapping a
> >    CALL_EXPR instead of a TEMPLATE_PARM_INDEX, which during the subsequent
> >    non-dependent substitution tsubst_copy doesn't expect either.  So
> >    this patch also relaxes tsubst_copy to accept such VIEW_CONVERT_EXPR
> >    too.
> > 
> > * In the third testcase, we end up never resolving the call to
> >    f.modify() since tsubst_copy doesn't do overload resolution.
> >    This patch fixes this by moving the handling of these
> >    VIEW_CONVERT_EXPR wrappers from tsubst_copy to tsubst_copy_and_build.
> >    For good measure tsubst_copy_and_build should also handle
> >    REF_PARENTHESIZED_P wrappers instead of delegating to tsubst_copy.
> > 
> > After this patch, VIEW_CONVERT_EXPR substitution is ultimately just
> > moved from tsubst_copy to tsubst_copy_and_build and made more
> > permissive.
> > 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?
> > 
> > 	PR c++/103346
> > 	PR c++/104278
> > 	PR c++/102553
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* pt.cc (tsubst_copy) <case VIEW_CONVERT_EXPR>: In the handling
> > 	of C++20 template parameter object wrappers: Remove same_type_p
> > 	assert.  Accept non-TEMPLATE_PARM_INDEX inner operand.  Handle
> > 	empty TREE_TYPE on substituted inner operand.  Move it to ...
> > 	(tsubst_copy_and_build): ... here.  Also handle REF_PARENTHESIZED_P
> > 	VIEW_CONVERT_EXPRs.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp2a/nontype-class52a.C: New test.
> > 	* g++.dg/cpp2a/nontype-class53.C: New test.
> > 	* g++.dg/cpp2a/nontype-class54.C: New test.
> > 	* g++.dg/cpp2a/nontype-class55.C: New test.
> > ---
> >   gcc/cp/pt.cc                                  | 73 ++++++++++---------
> >   gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C | 15 ++++
> >   gcc/testsuite/g++.dg/cpp2a/nontype-class53.C  | 25 +++++++
> >   gcc/testsuite/g++.dg/cpp2a/nontype-class54.C  | 23 ++++++
> >   gcc/testsuite/g++.dg/cpp2a/nontype-class55.C  | 15 ++++
> >   5 files changed, 116 insertions(+), 35 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class53.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class54.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class55.C
> > 
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index 2d8e4fdd4b5..0a196f069ad 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -17271,42 +17271,16 @@ tsubst_copy (tree t, tree args, tsubst_flags_t
> > complain, tree in_decl)
> >   	      return maybe_wrap_with_location (op0, EXPR_LOCATION (t));
> >   	    }
> >   	  tree op = TREE_OPERAND (t, 0);
> > -	  if (code == VIEW_CONVERT_EXPR
> > -	      && TREE_CODE (op) == TEMPLATE_PARM_INDEX)
> > -	    {
> > -	      /* Wrapper to make a C++20 template parameter object const.  */
> > -	      op = tsubst_copy (op, args, complain, in_decl);
> > -	      if (!CP_TYPE_CONST_P (TREE_TYPE (op)))
> > -		{
> > -		  /* The template argument is not const, presumably because
> > -		     it is still dependent, and so not the const template parm
> > -		     object.  */
> > -		  tree type = tsubst (TREE_TYPE (t), args, complain, in_decl);
> > -		  gcc_checking_assert
> > (same_type_ignoring_top_level_qualifiers_p
> > -				       (type, TREE_TYPE (op)));
> > -		  if (TREE_CODE (op) == CONSTRUCTOR
> > -		      || TREE_CODE (op) == IMPLICIT_CONV_EXPR)
> > -		    {
> > -		      /* Don't add a wrapper to these.  */
> > -		      op = copy_node (op);
> > -		      TREE_TYPE (op) = type;
> > -		    }
> > -		  else
> > -		    /* Do add a wrapper otherwise (in particular, if op is
> > -		       another TEMPLATE_PARM_INDEX).  */
> > -		    op = build1 (code, type, op);
> > -		}
> > -	      return op;
> > -	    }
> >   	  /* force_paren_expr can also create a VIEW_CONVERT_EXPR.  */
> > -	  else if (code == VIEW_CONVERT_EXPR && REF_PARENTHESIZED_P (t))
> > +	  if (code == VIEW_CONVERT_EXPR && REF_PARENTHESIZED_P (t))
> >   	    {
> >   	      op = tsubst_copy (op, args, complain, in_decl);
> >   	      op = build1 (code, TREE_TYPE (op), op);
> >   	      REF_PARENTHESIZED_P (op) = true;
> >   	      return op;
> >   	    }
> > -	  /* We shouldn't see any other uses of these in templates.  */
> > +	  /* We shouldn't see any other uses of these in templates
> > +	     (tsubst_copy_and_build handles C++20 tparm object wrappers).  */
> >   	  gcc_unreachable ();
> >   	}
> >   @@ -21569,12 +21543,41 @@ tsubst_copy_and_build (tree t,
> >         case NON_LVALUE_EXPR:
> >       case VIEW_CONVERT_EXPR:
> > -      if (location_wrapper_p (t))
> > -	/* We need to do this here as well as in tsubst_copy so we get the
> > -	   other tsubst_copy_and_build semantics for a PARM_DECL operand.  */
> > -	RETURN (maybe_wrap_with_location (RECUR (TREE_OPERAND (t, 0)),
> > -					  EXPR_LOCATION (t)));
> > -      /* fallthrough.  */
> > +      {
> > +	tree op = RECUR (TREE_OPERAND (t, 0));
> > +	if (location_wrapper_p (t))
> > +	  /* We need to do this here as well as in tsubst_copy so we get the
> > +	     other tsubst_copy_and_build semantics for a PARM_DECL operand.
> > */
> > +	  RETURN (maybe_wrap_with_location (op, EXPR_LOCATION (t)));
> > +
> > +	gcc_checking_assert (TREE_CODE (t) == VIEW_CONVERT_EXPR);
> > +	if (REF_PARENTHESIZED_P (t))
> > +	  /* force_paren_expr can also create a VIEW_CONVERT_EXPR.  */
> > +	  RETURN (finish_parenthesized_expr (op));
> > +
> > +	/* Otherwise, we're dealing with a wrapper to make a C++20 template
> > +	   parameter object const.  */
> > +	if (TREE_TYPE (op) == NULL_TREE
> > +	    || !CP_TYPE_CONST_P (TREE_TYPE (op)))
> > +	  {
> > +	    /* The template argument is not const, presumably because
> > +	       it is still dependent, and so not the const template parm
> > +	       object.  */
> > +	    tree type = tsubst (TREE_TYPE (t), args, complain, in_decl);
> > +	    if (TREE_CODE (op) == CONSTRUCTOR
> > +		|| TREE_CODE (op) == IMPLICIT_CONV_EXPR)
> > +	      {
> > +		/* Don't add a wrapper to these.  */
> > +		op = copy_node (op);
> > +		TREE_TYPE (op) = type;
> > +	      }
> > +	    else
> > +	      /* Do add a wrapper otherwise (in particular, if op is
> > +		 another TEMPLATE_PARM_INDEX).  */
> > +	      op = build1 (VIEW_CONVERT_EXPR, type, op);
> > +	  }
> > +	RETURN (op);
> > +      }
> >         default:
> >         /* Handle Objective-C++ constructs, if appropriate.  */
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C
> > b/gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C
> > new file mode 100644
> > index 00000000000..ae5d5df70ac
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class52a.C
> > @@ -0,0 +1,15 @@
> > +// A version of nontype-class52.C where explicit template arguments are
> > +// given in the call to f (which during deduction need to be partially
> > +// substituted into the NTTP object V in f's signature).
> > +// { dg-do compile { target c++20 } }
> > +
> > +template<class> struct A { };
> > +
> > +template<auto> struct B { };
> > +
> > +template<class T, A<T> V> void f(B<V>);
> > +
> > +int main() {
> > +  constexpr A<int> a;
> > +  f<int>(B<a>{});
> > +}
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class53.C
> > b/gcc/testsuite/g++.dg/cpp2a/nontype-class53.C
> > new file mode 100644
> > index 00000000000..9a6398c5f57
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class53.C
> > @@ -0,0 +1,25 @@
> > +// PR c++/103346
> > +// { dg-do compile { target c++20 } }
> > +
> > +struct Item {};
> > +
> > +template<class T, T... ts>
> > +struct Sequence { };
> > +
> > +template<Item... items>
> > +using ItemSequence = Sequence<Item, items...>;
> > +
> > +template<Item... items>
> > +constexpr auto f() {
> > +  constexpr auto l = [](Item item) { return item; };
> > +  return ItemSequence<l(items)...>{};
> > +}
> > +
> > +using ty0 = decltype(f<>());
> > +using ty0 = ItemSequence<>;
> > +
> > +using ty1 = decltype(f<{}>());
> > +using ty1 = ItemSequence<{}>;
> > +
> > +using ty3 = decltype(f<{}, {}, {}>());
> > +using ty3 = ItemSequence<{}, {}, {}>;
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class54.C
> > b/gcc/testsuite/g++.dg/cpp2a/nontype-class54.C
> > new file mode 100644
> > index 00000000000..8127b1f5426
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class54.C
> > @@ -0,0 +1,23 @@
> > +// PR c++/104278
> > +// { dg-do compile { target c++20 } }
> > +
> > +struct foo {
> > +  int value;
> > +  constexpr foo modify() const { return { value + 1 }; }
> > +};
> > +
> > +template<foo f, bool Enable = f.value & 1>
> > +struct bar {
> > +  static void run() { }
> > +};
> > +
> > +template<foo f>
> > +struct qux {
> > +  static void run() {
> > +    bar<f.modify()>::run();
> > +  }
> > +};
> > +
> > +int main() {
> > +  qux<foo{}>::run();
> > +}
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class55.C
> > b/gcc/testsuite/g++.dg/cpp2a/nontype-class55.C
> > new file mode 100644
> > index 00000000000..afcb3d64495
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class55.C
> > @@ -0,0 +1,15 @@
> > +// PR c++/102553
> > +// { dg-do compile { target c++20 } }
> > +
> > +struct s1{};
> > +template<int> inline constexpr s1 ch{};
> > +
> > +template<s1 f> struct s2{};
> > +template<s1 f> using alias1 = s2<f>;
> > +
> > +template<class T>
> > +void general(int n) {
> > +  alias1<ch<1>>{};
> > +}
> > +
> > +template void general<int>(int);
> 
>