PR c++/50852 - Revisit dependant template parameter

Jason Merrill <jason@redhat.com> writes:

> > +                                    /*we will fixup the siblings for
> Space and capital W.

Fixed in my local tree.

> 
> > +get_root_index_same_level (tree index)
> > +{
> > +  tree i = index;
> > +
> > +  if (TREE_CODE (TEMPLATE_PARM_DECL (i)) == CONST_DECL)
> > +    {
> > +      if (TEMPLATE_PARM_ORIG_LEVEL (i) == TEMPLATE_PARM_LEVEL (index))
> > +       i = TEMPLATE_PARM_ORIG_INDEX (i);
> 
> If the levels are the same, shouldn't the ORIG_INDEX already be the
> same as index?

Not necessarily.  I believe they can have the same index and at the
same time, one can be the fixed-up version of the other, without any
level-reducing happening in the process.

> 
> > +  else
> > +    {
> > +      if (template_type_parameter_type_p (TREE_TYPE (i))
> > +         && TREE_CODE (TEMPLATE_PARM_DECL (i)) != CONST_DECL
> > +         && TYPE_CANONICAL (TREE_TYPE (i)) != NULL_TREE)
> 
> You already checked the TREE_CODE of TEMPLATE_PARM_DECL in the outer
> if. And if it's not a CONST_DECL, it must be either a type parameter
> or template parameter.  So I think you only need the last test here.

Right.  This is an artifact of an earlier version of the patch and I
forgot to remove the earlier test.  Fixed.

> 
> > +get_template_parameter_level_and_index (tree parameter, int *level, int *index)
> > +{
> > +  int l = 0, i = -1;
> > +  tree parm;
> > +
> > +  if (TREE_CODE (parameter) == TYPE_DECL
> > +      || TREE_CODE (parameter) == TEMPLATE_DECL)
> > +    parm = TREE_TYPE (parameter);
> > +  else if (TREE_CODE (parameter) == PARM_DECL)
> > +    parm = DECL_INITIAL (parameter);
> > +  else
> > +    parm = parameter;
> > +
> > +  template_parm_level_and_index (parm, &l, &i);
> 
> Why not extend template_parm_level_and_index to handle these cases
> as well?

Done in my local tree.

> 
> > +    properly appends the descender of INDEX to that
> 
> descendant
> 
> > +      if (*where == NULL_TREE)
> > +       TREE_VEC_ELT (TEMPLATE_PARM_DESCENDANTS (index),
> > +                     TEMPLATE_PARM_LEVEL (descendant) - 1) = descendant;
> 
> Why not "*where = descendant"?

Another factorization miss from me.  Thanks for catching it.  Fixed in
my local tree.

> 
> > +             && (DECL_SOURCE_LOCATION (TEMPLATE_PARM_DECL (result))
> > +                 != DECL_SOURCE_LOCATION (TEMPLATE_PARM_DECL (orig_index))))
> > +           {
> > +             /*  We have gotten an equivalent index, that was reduced
> > +                 from index from ORIG_INDEX, but which (location of)
> > +                 DECL is different.  This can lead to having error
> > +                 messages pointing to the wrong location, so let's
> > +                 build an equivalent TEMPLATE_PARM_INDEX with a DECL
> > +                 pointing to the same location as ORIG_INDEX for
> > +                 RESULT.  */
> 
> How can this happen?  Hmm, I guess it's because of
> 
> > +       i = TEMPLATE_TYPE_PARM_INDEX (TYPE_CANONICAL (TREE_TYPE (i)));
> 

Yes.

> Why do we want to go to the canonical type parameter (which has no
> name) rather than just TYPE_MAIN_VARIANT?

It took me a lot of time to understand why this wouldn't work on e.g,
the 25_algorithms/copy/streambuf_iterators/char/1.cc test case of
libstdc++ and distill a smaller example that illustrates the issue.

Consider this:

    template<class T> class I {};

    template<class T, class U = I<T> > class S1;
    template<class T> class S2;

    template<class T>
    class S0
    {
	template<class W> friend S1<W> foo (); //#1
    };

    template<class T, class U>
    class S1
    {
    };

    template<class T>
    class S2
    {
	template<class V> //#2
	friend void access(S1<V>, S2<V>); //#3
	bool p; // ---> error: ‘bool S2<int>::p’ is private
    };

    template<class U>
    void
    access(S1<U> s1, S2<U> s2) //#4
    {
	s2.p = true; // ---> error in this context.
	return s1;
    }

    int
    main()
    {
	S1<int> s1;
	S2<int> s2; //#5
	access(s1, s2);
    }

When we consider S2<int> in #5, it triggers the partial instantiation
of the friend 'access' member template in #2 and #3.  In #2 (as we
have a handle on the list of siblings of V there), V is level-reduced
to level 1, and is fixed-up (by tsubst_template_parms).

Lets call Vi the initial V of level 2, Vr the level-reduced V of level
1 and Vrf the level-reduced and fixed-up V; and similarly Wi, Wr and
Wrf.

After fix-up, the question (which you are asking in essence) is, which
Vi do we hang Vrf on?

Suppose then that we hang it on TYPE_MAIN_VARIANT of Vi in #2.

In #3, during the type substitution of Vrf into the declaration of
"access" (in tsubst_decl), we have a problem with the default argument
of the second parameter of S1 in S1<V>.

To understand the problem, one must note that S1<V> in #3 (before the
level-reduction-and-fix-up) really is S1<V, I<W> >.  In theory, it is
S1<V, I<V> >, but the I<V> there is equivalent to I<W> that
lookup_template_class built earlier at #1, so S1<V, I<W> > really is
what we have in #3.  This subtlety becomes meaningful as it follows
that the TYPE_MAIN_VARIANT for W is different from the
TYPE_MAIN_VARIANT for V, in #3.

So during the type substitution of Vrf into the declaration of
"access" in #3, the Wi parm in the I<W> default template argument is
level-reduced.  Note that in this case, the level-reduction cannot be
followed by type fix-up because we don't have a handle on all template
parameters siblings in a row, like we did in #2.  Thus
reduce_template_parm_level on Vi *must* yield Vrf here (for the whole
scheme to work), not Vr.

So reduce_template_parm_level looks in TYPE_MAIN_VARIANT of Wi to see
if there is a Wrf hung on there, so that it could return it as a
result of the level-reduction.  But it won't find any because we hung
the Wrf (really Vrf) on the TYPE_MAIN_VARIANT for Vi, and
TYPE_MAIN_VARIANT of Vi is different from TYPE_MAIN_VARIANT for Wi.

As a matter of fact, the level-reduction of Wi here yields a reduced
parameter that is not fixed up: Wr or rather Vr.  Precisely what we
want to avoid.

That in turns makes the "access" declaration in #4 be different from
the one declared in #3.  Oops.

In contrast, if we hang Vrf on TYPE_CANONICAL of Vi, it will be found
when reduce_template_parm_level looks it up on the TYPE_CANONICAL of
Wi, because Vi and Wi have the same canonical type.

Also, I don't understand when you said above that the TYPE_CANONICAL
for V doesn't have a name.  In this case, it does have seem to have a
name (a TYPE_NAME), which is W.  What am I missing?

> 
> > +  /* Template parms ought to be fixed-up from left to right.  */
> > +  parms = nreverse (parms);
> 
> I'm a bit nervous about something called by fixup getting confused by
> this change to the template parms before we change it back.

Do you have a particular spot in mind?

Would you prefer to copy the linked list parms, and do the reversing
and fixup on it instead?

> 
> > +/*  Transform a template parameter into an argument, suitable to be
> > +    passed to tsubst as an element of its ARGS parameter.  */
> > +
> > +static tree
> > +template_parms_to_args (tree parms)
> 
> The comment seems to be talking about a single parm.

Fixed in my local tree.

> 
> > +      /* This can happen for template parms of a template template
> > +        parameter, e.g:
> > +
> > +        template<template<class T, class U> class TT> struct S;
> > +
> > +        Consider the level of the parms of TT; T and U both have
> > +        level 2; TT has no template parm of level 1. So in this case
> > +        the first element of full_template_args is NULL_TREE. If we
> > +        leave it like this TMPL_ARG_DEPTH on args returns 1 instead
> > +        of 2. This will make tsubst wrongly consider that T and U
> > +        have level 1. Instead, let's create a dummy vector as the
> > +        first element of full_template_args so that TMPL_ARG_DEPTH
> > +        returns the correct depth for args.  */
> 
> Hmm, it seems odd that the parms wouldn't have level 1.  I wonder if
> changing that would also avoid needing to use structural equality for
> template template parameters.

Yes, it looks like so.  I'd like to address that in a subsequent patch
once the other core parts of this one are OK, because I suspect there
are a few other places I'd need to adjust if I touch this.  That and
I'll probably need to chase a fallout or two.  :-)

I'll then probably merge that subsequent patch with this one, as you
see fit.  Is that OK?

> 
> >    FIXME: This function does an approximation, as it only checks that
> >    the levels of PARM_PACK/EXPANSION and ARG_PACK do match.  For it to
> >    be precise, I think the TEMPLATE_PARM_INDEX of PARM_PACK should
> >    track its pre-fixup type, so that ARG_PACK could be compared with
> >    that type instead.  But that would increase the size of the
> >    template_parm_index_s struct, as I don't see where else I could
> >    store the pre-fixup type.  */
> 
> Doesn't ...ORIG_INDEX work for this?  Indeed, I'd think you could
> change arg_from_parm_pack_p to compare ORIG_INDEX and then it would
> handle both cases.

It's close but it doesn't fit perfectly.  For a parameter T, if let's
call Ti its initial (pre-fixup) version, Tf its fixed up version, Tr
the reduced version, and Trf the reduced and fixed up version.

At the moment, for a fixed-up type, TEMPLATE_PARM_ORIG_INDEX(Tf) and
TEMPLATE_PARM_ORIG_INDEX(Trf) all point to Tf.  That is, they point to
a fixed-up version of T.

To make this work, I think I'd need to make TEMPLATE_PARM_ORIG_INDEX
point to Ti instead.  I think it's possible to get the fixed-up
version of a given pre-fix-up one (by poking the TREE_TYPE (TYPE_NAME
(Ti))) so I believe this should work.  I am looking into this at the
moment.

> > +      if (tinfo && TREE_CODE (TI_TEMPLATE (tinfo)) == TEMPLATE_DECL)
> > +       {
> > +         ++processing_template_decl;
> > +         /* prepare possible partial instantiation of member
> > +            template by fixing-up template parms which level are
> > +            going to be reduced by the partial instantiation.  */
> > +         tsubst_template_parms (DECL_TEMPLATE_PARMS (TI_TEMPLATE (tinfo)),
> > +                                args, tf_none);
> > +         --processing_template_decl;
> > +       }
> 
> This doesn't seem necessary for member functon templates, since we fix
> up the parms first thing in tsubst_decl.  Is it necessary for member
> class templates?  Can we handle it at a lower level there, too?

Indeed.  I actually started the patch by putting the
reducing-and-fixup call here first, to be sure I wasn't forgetting any
spot.  I then moved handle it a lower level but it seems like I forgot
to remove this.  Now I realize I forgot to handle the case of member
typename declarations that have their own template header.  Fixed in
my local tree.

> 
> >      case UNBOUND_CLASS_TEMPLATE:
> >        {
> > +       /* If T is a member template being partially instantiated,
> > +          fixup its template parmeters that are to be level-reduced,
> > +          upfront.  */
> > +       tree parm_list =
> > +         tsubst_template_parms (DECL_TEMPLATE_PARMS (TYPE_NAME (t)),
> > +                                args, complain);
> 
> This comment could be clearer; an UNBOUND_CLASS_TEMPLATE isn't a
> member template, though it will eventually be replaced with one.

Fixed in my local tree.

Here is what currently passes bootstrap and regression tests in my local
tree.

    gcc/cp/

    	PR c++/50852
    	* cp-tree.h (struct template_parm_index_s)<num_siblings,orig_level>: Remove.
    	(struct template_parm_index_s)<siblings, orig_index>: New members.
    	(COMPARE_NO_SIBLINGS): New type comparison control flag.
    	(TEMPLATE_PARM_SIBLINGS): New accessor macro.
    	(TEMPLATE_PARM_DESCENDANTS): Add comments.
    	(TEMPLATE_PARM_ORIG_INDEX): New accessor.
    	(TEMPLATE_PARM_ORIG_LEVEL): Rewrite in terms of
    	TEMPLATE_PARM_ORIG_INDEX.
    	(process_template_parm): Change the integer num_siblings parm into
    	a tree siblings parm.
    	(comp_template_parms_siblings, comp_template_parm_levels)
    	(comp_template_parms_guided, comp_template_args_guided)
    	(same_type_ignoring_top_level_qualifiers_guided_p, compare_trees)
    	(template_type_parameter_type_p): Declare new functions.
    	(fixup_current_template_parms): New, better name for
    	fixup_template_parms.
    	(fixup_template_parms): Take the set of parameters to fixup in
    	parameters.
    	* pt.c (arg_from_reduced_and_fixedup_parm_pack_of_expansion_p)
    	(get_root_index_same_level, add_descendant_to_parm_index)
    	(get_descendant_of_parm_index, get_template_parm_index)
    	(comp_template_parm_levels, template_parms_to_args)
    	(template_type_parameter_type_p): Define new functions.
    	(current_template_args): Use the new template_parms_to_args
    	function.
    	(fixup_template_type_parm_type)
    	(fixup_template_parm_index, build_template_parm_index)
    	(fixup_template_parm_index, process_template_parm)
    	(fixup_template_type_parm_type, fixup_template_parm)
    	(process_template_parm): Change the integer num_siblings parm into
    	a tree siblings parm.  Update comments.
    	(build_template_parm_index): Likewise.  Replace ORIG_LEVEL integer
    	parm with an ORIG_INDEX tree.  Add a few asserts.
    	(get_template_info): Make this more robust and support getting
    	template info from TYPENAME_TYPE nodes.
    	(comp_template_parms_guided): New function.  Takes an additional
    	parm to control comparison.  Factorized out of ...
    	(comp_template_parms): ... this.
    	(reduce_template_parm_level): Use TEMPLATE_PARM_SIBLINGS instead
    	of TEMPLATE_PARM_NUM_SIBLINGS.  Update usage of
    	TEMPLATE_PARM_DESCENDANTS.  Use the new
    	get_descendant_of_parm_index.
    	(template_args_equal_guided): Take an additional parm to control
    	comparison.  Use template_args_equal_guided instead of
    	template_args_equal.  Factorize this out of ...
    	(template_args_equal): ... this.
    	(comp_template_args_guided): Take an additional parm to control
    	comparison.  Renamed from comp_template_args_with_info.  Make this
    	public.
    	(comp_template_args, unify_pack_expansion): Adjust.
    	(fixup_innermost_template_parms): Transformed former
    	fixup_template_parms in to this.  Make it take a set of parms to
    	fixup and an arglist.  Update comments.
    	(fixup_template_parms): Take the parms to fixup in argument.
    	(fixup_current_template_parms): This is the new name for the
    	former fixup_template_parms.  It has be re-written in terms of
    	fixup_innermost_template_parms.
    	* tree.c (compare_trees): Factorized this out of cp_tree_equal.
    	Take a comparison control parameter. Use tree comparison functions
    	that take a comparison control parm.
    	(cp_tree_equal): Use the factorized compare_trees.
    	(tsubst_template_parms):  Fixup template parameters that are
    	level-reduced.
    	(push_template_decl_real): Support declarations of friend template
    	represented by TYPENAME_TYPE nodes.
    	(instantiate_class_template_1, tsubst_aggr_type, tsubst_decl)
    	(tsubst<UNBOUND_CLASS_TEMPLATE>): Call tsubst_template_parms
    	upfront to fixup dependant types that are to be level-reduced by a
    	possible partial instantiation of a member template.
    	(tsubst_pack_expansion): During type fixup, take in account parm
    	packs that have been level-reduced.
    	(get_mostly_instantiated_function_type): Fix this by setting the
    	proper size for the args of tsubst, so that partial instantiation
    	happens there.  Otherwise, tsubst indirectly and wrongly calls
    	reduce_template_parm_level with a reduced level of 0.
    	(make_auto): Adjust for sibling_parms instead of
    	num_sibling_parms.
    	* typeck.c (compparms_guided): Take a comparison control parameter.
    	Split out of ...
    	(compparms): ... this.
    	(comp_template_parms_siblings): Define new function.
    	(comp_template_parms_position): Take an additional parameter to
    	control the comparison.  Use the new comp_template_parms_siblings.
    	(structural_comptypes): Use type comparison routines variants that
    	take the comparison control parm.
    	(same_type_ignoring_top_level_qualifiers_guided_p): Take an
    	additional parm to control comparison.  Use comptypes instead of
    	comptypes.  Factorize out of ...
    	(same_type_ignoring_top_level_qualifiers_p): ... this.
    	* parser.c (cp_parser_template_parameter_list): Adjust for the
    	change to using sibling_parms instead of num_sibling_parms.
    	(cp_parser_template_declaration_after_export): Adjust
    	to call fixup_current_template_parms instead of
    	fixup_template_parms.
    	(cp_parser_single_declaration):  Make friend typename declarations
    	carry a template info.

    gcc/testsuite/

    	PR c++/50852
    	* g++.dg/template/typedef39.C: New test.
    	* g++.dg/template/typedef40.C: Likewise.
    	* g++.dg/template/typedef41.C: Likewise.
    	* g++.dg/template/crash84.C: This test should actually pass
    	without error.

PR c++/50852 - Revisit dependant template parameter

Commit Message

Patch