[2/2] c++/modules: Retrofit imported partial specs over existing implicit instantiations [PR113814]

  I wasn't sure whether I should include the ambiguity checking logic from
process_partial_specialization; we don't do this anywhere else in the
modules handling code that I could see so I left it out for now.

I could also rework process_partial_specialization to call the new
create_mergeable_partial_spec function if you prefer, to reduce code
duplication, though the early exit for VAR_DECL and the call to
associate_classtype_constraints that relies on global state complicated
it enough I didn't bother for this patch.

Finally, I'm not entirely sure whether the choice of DECL_TEMPLATE_PARMS
that I'm providing is correct, especially considering this hunk:

  /* Give template template parms a DECL_CONTEXT of the template
     for which they are a parameter.  */
  for (i = 0; i < ntparms; ++i)
    {
      tree parm = TREE_VALUE (TREE_VEC_ELT (inner_parms, i));
      if (TREE_CODE (parm) == TEMPLATE_DECL)
	DECL_CONTEXT (parm) = tmpl;
    }

I haven't been able to produce any testcase that relies on this context
setting to cause a failure, though, and there doesn't seem to be one in
the testsuite for the matching hunk in process_partial_specialization.
Any hints on where this might be needed?

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

In some cases, when we go to import a partial specialisation there might
already be an incomplete implicit instantiation in the specialisation
table.  This causes ICEs described in the linked PR as we now have two
separate matching specialisations for this same arguments.

This issue doesn't appear to happen for variable templates as it doesn't
appear that we can create incomplete specialisations in the same way.

This patch attempts to solve the issue by retrofitting the existing
implicit instantiation as a partial specialisation (similarly to how
maybe_process_partial_specialization operates) and then relying on the
existing merging logic to fill in the definition of the type.  This
works because for types almost all relevant details are streamed and
merged in 'read_class_def'.

As an optimisation, we also mark the newly retrofitted specialisation as
imported, since there isn't any useful information on the declaration
that will be provided from this TU anyway, and so we don't need to write
the decl into our own BMI but can instead just seed it as an import.

	PR c++/113814

gcc/cp/ChangeLog:

	* cp-tree.h (create_mergeable_partial_spec):
	* module.cc (trees_in::key_mergeable):
	* pt.cc (create_mergeable_partial_spec):

gcc/testsuite/ChangeLog:

	* g++.dg/modules/partial-6.h: New test.
	* g++.dg/modules/partial-6_a.H: New test.
	* g++.dg/modules/partial-6_b.H: New test.
	* g++.dg/modules/partial-6_c.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h                           |  1 +
 gcc/cp/module.cc                           | 26 +++++++++++
 gcc/cp/pt.cc                               | 50 ++++++++++++++++++++++
 gcc/testsuite/g++.dg/modules/partial-6.h   |  7 +++
 gcc/testsuite/g++.dg/modules/partial-6_a.H | 11 +++++
 gcc/testsuite/g++.dg/modules/partial-6_b.H | 23 ++++++++++
 gcc/testsuite/g++.dg/modules/partial-6_c.C | 12 ++++++
 7 files changed, 130 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-6.h
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_b.H
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_c.C
  

On 10/24/24 3:25 AM, Nathaniel Shead wrote:
> I wasn't sure whether I should include the ambiguity checking logic from
> process_partial_specialization; we don't do this anywhere else in the
> modules handling code that I could see so I left it out for now.

The relevant bit in the standard seems to be
https://eel.is/c++draft/temp#spec.partial.general-1
"A partial specialization shall be reachable from any use of a template 
specialization that would make use of the partial specialization as the 
result of an implicit or explicit instantiation; no diagnostic is required."

So we aren't required to diagnose an imported partial spec after an 
instantiation.  It's not entirely clear whether the ambiguity checking 
in https://eel.is/c++draft/temp#spec.partial.match-1.2 qualifies as 
"make use", but that seems a natural interpretation.

So checking these on import seems optional, but could also be helpful in 
finding subtle bugs.

> I could also rework process_partial_specialization to call the new
> create_mergeable_partial_spec function if you prefer, to reduce code
> duplication, though the early exit for VAR_DECL and the call to
> associate_classtype_constraints that relies on global state complicated
> it enough I didn't bother for this patch.

That sounds desirable, if we need to create the mergeable partial spec 
at all.

> Finally, I'm not entirely sure whether the choice of DECL_TEMPLATE_PARMS
> that I'm providing is correct, especially considering this hunk:
> 
>    /* Give template template parms a DECL_CONTEXT of the template
>       for which they are a parameter.  */
>    for (i = 0; i < ntparms; ++i)
>      {
>        tree parm = TREE_VALUE (TREE_VEC_ELT (inner_parms, i));
>        if (TREE_CODE (parm) == TEMPLATE_DECL)
> 	DECL_CONTEXT (parm) = tmpl;
>      }
> 
> I haven't been able to produce any testcase that relies on this context
> setting to cause a failure, though, and there doesn't seem to be one in
> the testsuite for the matching hunk in process_partial_specialization.
> Any hints on where this might be needed?

I believe that's for the benefit of coerce_template_args_for_ttp.

> In some cases, when we go to import a partial specialisation there might
> already be an incomplete implicit instantiation in the specialisation
> table.  This causes ICEs described in the linked PR as we now have two
> separate matching specialisations for this same arguments.
> 
> This issue doesn't appear to happen for variable templates as it doesn't
> appear that we can create incomplete specialisations in the same way.
> 
> This patch attempts to solve the issue by retrofitting the existing
> implicit instantiation as a partial specialisation (similarly to how
> maybe_process_partial_specialization operates) and then relying on the
> existing merging logic to fill in the definition of the type.  This
> works because for types almost all relevant details are streamed and
> merged in 'read_class_def'.

I find it surprising that we would need to build a partial 
specialization to merge with, when in a single TU we have no trouble 
merging a partial specialization with an existing implicit 
instantiation.  Why can't we do that in this case as well?

> As an optimisation, we also mark the newly retrofitted specialisation as
> imported, since there isn't any useful information on the declaration
> that will be provided from this TU anyway, and so we don't need to write
> the decl into our own BMI but can instead just seed it as an import.

Agreed.

Jason
  

On Thu, Oct 24, 2024 at 12:05:18PM -0400, Jason Merrill wrote:
> On 10/24/24 3:25 AM, Nathaniel Shead wrote:
> > I wasn't sure whether I should include the ambiguity checking logic from
> > process_partial_specialization; we don't do this anywhere else in the
> > modules handling code that I could see so I left it out for now.
> 
> The relevant bit in the standard seems to be
> https://eel.is/c++draft/temp#spec.partial.general-1
> "A partial specialization shall be reachable from any use of a template
> specialization that would make use of the partial specialization as the
> result of an implicit or explicit instantiation; no diagnostic is required."
> 
> So we aren't required to diagnose an imported partial spec after an
> instantiation.  It's not entirely clear whether the ambiguity checking in
> https://eel.is/c++draft/temp#spec.partial.match-1.2 qualifies as "make use",
> but that seems a natural interpretation.
> 
> So checking these on import seems optional, but could also be helpful in
> finding subtle bugs.
> 

Makes sense.  I might add this as a follow-up patch then, since if we're
doing this here it would make sense to do in add_mergeable_specialization
as well now that I think about it.

> > I could also rework process_partial_specialization to call the new
> > create_mergeable_partial_spec function if you prefer, to reduce code
> > duplication, though the early exit for VAR_DECL and the call to
> > associate_classtype_constraints that relies on global state complicated
> > it enough I didn't bother for this patch.
> 
> That sounds desirable, if we need to create the mergeable partial spec at
> all.
> 
> > Finally, I'm not entirely sure whether the choice of DECL_TEMPLATE_PARMS
> > that I'm providing is correct, especially considering this hunk:
> > 
> >    /* Give template template parms a DECL_CONTEXT of the template
> >       for which they are a parameter.  */
> >    for (i = 0; i < ntparms; ++i)
> >      {
> >        tree parm = TREE_VALUE (TREE_VEC_ELT (inner_parms, i));
> >        if (TREE_CODE (parm) == TEMPLATE_DECL)
> > 	DECL_CONTEXT (parm) = tmpl;
> >      }
> > 
> > I haven't been able to produce any testcase that relies on this context
> > setting to cause a failure, though, and there doesn't seem to be one in
> > the testsuite for the matching hunk in process_partial_specialization.
> > Any hints on where this might be needed?
> 
> I believe that's for the benefit of coerce_template_args_for_ttp.
> 

OK thanks, I'll do some exploring to see if I can cause a failure here
then.

> > In some cases, when we go to import a partial specialisation there might
> > already be an incomplete implicit instantiation in the specialisation
> > table.  This causes ICEs described in the linked PR as we now have two
> > separate matching specialisations for this same arguments.
> > 
> > This issue doesn't appear to happen for variable templates as it doesn't
> > appear that we can create incomplete specialisations in the same way.
> > 
> > This patch attempts to solve the issue by retrofitting the existing
> > implicit instantiation as a partial specialisation (similarly to how
> > maybe_process_partial_specialization operates) and then relying on the
> > existing merging logic to fill in the definition of the type.  This
> > works because for types almost all relevant details are streamed and
> > merged in 'read_class_def'.
> 
> I find it surprising that we would need to build a partial specialization to
> merge with, when in a single TU we have no trouble merging a partial
> specialization with an existing implicit instantiation.  Why can't we do
> that in this case as well?
> 

As far as I can tell, this is the normal behaviour for declarations;
when we do

  template <typename T> struct S;

  template <typename T> struct S<T*>
                               ^~~~~

This registers an implicit instantiation in the specialisation table
already; the parser then calls 'maybe_process_partial_specialization'
which overwrites the implicit instantiation as a partial specialization
instead, see:

      /* This is for ordinary explicit specialization and partial
	 specialization of a template class such as:

	   template <> class C<int>;

	 or:

	   template <class T> class C<T*>;

	 Make sure that `C<int>' and `C<T*>' are implicit instantiations.  */

      if (maybe_new_partial_specialization (type))
	{
	  if (!check_specialization_namespace (CLASSTYPE_TI_TEMPLATE (type))
	      && !at_namespace_scope_p ())
	    return error_mark_node;
	  SET_CLASSTYPE_TEMPLATE_SPECIALIZATION (type);
	  DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)) = input_location;
	  if (processing_template_decl)
	    {
	      tree decl = push_template_decl (TYPE_MAIN_DECL (type));
	      if (decl == error_mark_node)
		return error_mark_node;
	      return TREE_TYPE (decl);
	    }
	}

The difference with modules is that rather than being able to overwrite
the existing instantiation with a definition in-place, we instead
currently get given a new tree with the entire template/decl/type of a
partial specialisation, but there is an existing template/decl/type of
an implicit instantiation in the specialisation table (and pointed to by
other by decls, in the case of the PR via the return type of a function).

So we need to somehow update in-place the existing type to be the type
of the partial specialisation so that we don't break any decls that
refer to it already.  My approach here is to attempt to copy the
relevant parts of what maybe_process_partial_specialization is doing so
we can pretend that we're merging an existing partial specialisation
(and use the rest of the existing logic) rather than doing part of the
in-place update of the implicit instantiation from within
add_mergeable_specialization, and then again once we get to
read_class_def.

Happy to try another route if you think there's something I've missed
here though.

> > As an optimisation, we also mark the newly retrofitted specialisation as
> > imported, since there isn't any useful information on the declaration
> > that will be provided from this TU anyway, and so we don't need to write
> > the decl into our own BMI but can instead just seed it as an import.
> 
> Agreed.
> 
> Jason
>
  

On 10/24/24 5:10 PM, Nathaniel Shead wrote:
> On Thu, Oct 24, 2024 at 12:05:18PM -0400, Jason Merrill wrote:
>> On 10/24/24 3:25 AM, Nathaniel Shead wrote:
>>> I wasn't sure whether I should include the ambiguity checking logic from
>>> process_partial_specialization; we don't do this anywhere else in the
>>> modules handling code that I could see so I left it out for now.
>>
>> The relevant bit in the standard seems to be
>> https://eel.is/c++draft/temp#spec.partial.general-1
>> "A partial specialization shall be reachable from any use of a template
>> specialization that would make use of the partial specialization as the
>> result of an implicit or explicit instantiation; no diagnostic is required."
>>
>> So we aren't required to diagnose an imported partial spec after an
>> instantiation.  It's not entirely clear whether the ambiguity checking in
>> https://eel.is/c++draft/temp#spec.partial.match-1.2 qualifies as "make use",
>> but that seems a natural interpretation.
>>
>> So checking these on import seems optional, but could also be helpful in
>> finding subtle bugs.
>>
> 
> Makes sense.  I might add this as a follow-up patch then, since if we're
> doing this here it would make sense to do in add_mergeable_specialization
> as well now that I think about it.
> 
>>> I could also rework process_partial_specialization to call the new
>>> create_mergeable_partial_spec function if you prefer, to reduce code
>>> duplication, though the early exit for VAR_DECL and the call to
>>> associate_classtype_constraints that relies on global state complicated
>>> it enough I didn't bother for this patch.
>>
>> That sounds desirable, if we need to create the mergeable partial spec at
>> all.
>>
>>> Finally, I'm not entirely sure whether the choice of DECL_TEMPLATE_PARMS
>>> that I'm providing is correct, especially considering this hunk:
>>>
>>>     /* Give template template parms a DECL_CONTEXT of the template
>>>        for which they are a parameter.  */
>>>     for (i = 0; i < ntparms; ++i)
>>>       {
>>>         tree parm = TREE_VALUE (TREE_VEC_ELT (inner_parms, i));
>>>         if (TREE_CODE (parm) == TEMPLATE_DECL)
>>> 	DECL_CONTEXT (parm) = tmpl;
>>>       }
>>>
>>> I haven't been able to produce any testcase that relies on this context
>>> setting to cause a failure, though, and there doesn't seem to be one in
>>> the testsuite for the matching hunk in process_partial_specialization.
>>> Any hints on where this might be needed?
>>
>> I believe that's for the benefit of coerce_template_args_for_ttp.
>>
> 
> OK thanks, I'll do some exploring to see if I can cause a failure here
> then.
> 
>>> In some cases, when we go to import a partial specialisation there might
>>> already be an incomplete implicit instantiation in the specialisation
>>> table.  This causes ICEs described in the linked PR as we now have two
>>> separate matching specialisations for this same arguments.
>>>
>>> This issue doesn't appear to happen for variable templates as it doesn't
>>> appear that we can create incomplete specialisations in the same way.
>>>
>>> This patch attempts to solve the issue by retrofitting the existing
>>> implicit instantiation as a partial specialisation (similarly to how
>>> maybe_process_partial_specialization operates) and then relying on the
>>> existing merging logic to fill in the definition of the type.  This
>>> works because for types almost all relevant details are streamed and
>>> merged in 'read_class_def'.
>>
>> I find it surprising that we would need to build a partial specialization to
>> merge with, when in a single TU we have no trouble merging a partial
>> specialization with an existing implicit instantiation.  Why can't we do
>> that in this case as well?
>>
> 
> As far as I can tell, this is the normal behaviour for declarations;
> when we do
> 
>    template <typename T> struct S;
> 
>    template <typename T> struct S<T*>
>                                 ^~~~~
> 
> This registers an implicit instantiation in the specialisation table
> already; the parser then calls 'maybe_process_partial_specialization'
> which overwrites the implicit instantiation as a partial specialization
> instead, see:
> 
>        /* This is for ordinary explicit specialization and partial
> 	 specialization of a template class such as:
> 
> 	   template <> class C<int>;
> 
> 	 or:
> 
> 	   template <class T> class C<T*>;
> 
> 	 Make sure that `C<int>' and `C<T*>' are implicit instantiations.  */
> 
>        if (maybe_new_partial_specialization (type))
> 	{
> 	  if (!check_specialization_namespace (CLASSTYPE_TI_TEMPLATE (type))
> 	      && !at_namespace_scope_p ())
> 	    return error_mark_node;
> 	  SET_CLASSTYPE_TEMPLATE_SPECIALIZATION (type);
> 	  DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)) = input_location;
> 	  if (processing_template_decl)
> 	    {
> 	      tree decl = push_template_decl (TYPE_MAIN_DECL (type));
> 	      if (decl == error_mark_node)
> 		return error_mark_node;
> 	      return TREE_TYPE (decl);
> 	    }
> 	}
> 
> The difference with modules is that rather than being able to overwrite
> the existing instantiation with a definition in-place, we instead
> currently get given a new tree with the entire template/decl/type of a
> partial specialisation, but there is an existing template/decl/type of
> an implicit instantiation in the specialisation table (and pointed to by
> other by decls, in the case of the PR via the return type of a function).

Yes.

> So we need to somehow update in-place the existing type to be the type
> of the partial specialisation so that we don't break any decls that
> refer to it already.  My approach here is to attempt to copy the
> relevant parts of what maybe_process_partial_specialization is doing so
> we can pretend that we're merging an existing partial specialisation
> (and use the rest of the existing logic) rather than doing part of the
> in-place update of the implicit instantiation from within
> add_mergeable_specialization, and then again once we get to
> read_class_def.
> 
> Happy to try another route if you think there's something I've missed
> here though.

Basically I was unclear why we don't automatically merge the types already.

But actually, it seems just as well that the types aren't merged; the 
primary template pattern and its equivalent specialization are separate 
types even though they compare identical, and with concepts we can have 
multiple partial specializations of the same template/args with 
different constraints; it seems like it might be better to consistently 
distinguish the partial specialization pattern from the matching 
instance of the template, even when there's only a single partial 
specialization of that type.

That way, I think we only need to make sure that the A<T*> in the 
partial specialization and the A<T*> in the declaration of f have the 
same TYPE_CANONICAL.  The attached seems to be enough to make the 
testcase compile, anyway.

Jason
  

On Fri, Oct 25, 2024 at 11:25:15AM -0400, Jason Merrill wrote:
> On 10/24/24 5:10 PM, Nathaniel Shead wrote:
> > On Thu, Oct 24, 2024 at 12:05:18PM -0400, Jason Merrill wrote:
> > > On 10/24/24 3:25 AM, Nathaniel Shead wrote:
> > > > I wasn't sure whether I should include the ambiguity checking logic from
> > > > process_partial_specialization; we don't do this anywhere else in the
> > > > modules handling code that I could see so I left it out for now.
> > > 
> > > The relevant bit in the standard seems to be
> > > https://eel.is/c++draft/temp#spec.partial.general-1
> > > "A partial specialization shall be reachable from any use of a template
> > > specialization that would make use of the partial specialization as the
> > > result of an implicit or explicit instantiation; no diagnostic is required."
> > > 
> > > So we aren't required to diagnose an imported partial spec after an
> > > instantiation.  It's not entirely clear whether the ambiguity checking in
> > > https://eel.is/c++draft/temp#spec.partial.match-1.2 qualifies as "make use",
> > > but that seems a natural interpretation.
> > > 
> > > So checking these on import seems optional, but could also be helpful in
> > > finding subtle bugs.
> > > 
> > 
> > Makes sense.  I might add this as a follow-up patch then, since if we're
> > doing this here it would make sense to do in add_mergeable_specialization
> > as well now that I think about it.
> > 
> > > > I could also rework process_partial_specialization to call the new
> > > > create_mergeable_partial_spec function if you prefer, to reduce code
> > > > duplication, though the early exit for VAR_DECL and the call to
> > > > associate_classtype_constraints that relies on global state complicated
> > > > it enough I didn't bother for this patch.
> > > 
> > > That sounds desirable, if we need to create the mergeable partial spec at
> > > all.
> > > 
> > > > Finally, I'm not entirely sure whether the choice of DECL_TEMPLATE_PARMS
> > > > that I'm providing is correct, especially considering this hunk:
> > > > 
> > > >     /* Give template template parms a DECL_CONTEXT of the template
> > > >        for which they are a parameter.  */
> > > >     for (i = 0; i < ntparms; ++i)
> > > >       {
> > > >         tree parm = TREE_VALUE (TREE_VEC_ELT (inner_parms, i));
> > > >         if (TREE_CODE (parm) == TEMPLATE_DECL)
> > > > 	DECL_CONTEXT (parm) = tmpl;
> > > >       }
> > > > 
> > > > I haven't been able to produce any testcase that relies on this context
> > > > setting to cause a failure, though, and there doesn't seem to be one in
> > > > the testsuite for the matching hunk in process_partial_specialization.
> > > > Any hints on where this might be needed?
> > > 
> > > I believe that's for the benefit of coerce_template_args_for_ttp.
> > > 
> > 
> > OK thanks, I'll do some exploring to see if I can cause a failure here
> > then.
> > 
> > > > In some cases, when we go to import a partial specialisation there might
> > > > already be an incomplete implicit instantiation in the specialisation
> > > > table.  This causes ICEs described in the linked PR as we now have two
> > > > separate matching specialisations for this same arguments.
> > > > 
> > > > This issue doesn't appear to happen for variable templates as it doesn't
> > > > appear that we can create incomplete specialisations in the same way.
> > > > 
> > > > This patch attempts to solve the issue by retrofitting the existing
> > > > implicit instantiation as a partial specialisation (similarly to how
> > > > maybe_process_partial_specialization operates) and then relying on the
> > > > existing merging logic to fill in the definition of the type.  This
> > > > works because for types almost all relevant details are streamed and
> > > > merged in 'read_class_def'.
> > > 
> > > I find it surprising that we would need to build a partial specialization to
> > > merge with, when in a single TU we have no trouble merging a partial
> > > specialization with an existing implicit instantiation.  Why can't we do
> > > that in this case as well?
> > > 
> > 
> > As far as I can tell, this is the normal behaviour for declarations;
> > when we do
> > 
> >    template <typename T> struct S;
> > 
> >    template <typename T> struct S<T*>
> >                                 ^~~~~
> > 
> > This registers an implicit instantiation in the specialisation table
> > already; the parser then calls 'maybe_process_partial_specialization'
> > which overwrites the implicit instantiation as a partial specialization
> > instead, see:
> > 
> >        /* This is for ordinary explicit specialization and partial
> > 	 specialization of a template class such as:
> > 
> > 	   template <> class C<int>;
> > 
> > 	 or:
> > 
> > 	   template <class T> class C<T*>;
> > 
> > 	 Make sure that `C<int>' and `C<T*>' are implicit instantiations.  */
> > 
> >        if (maybe_new_partial_specialization (type))
> > 	{
> > 	  if (!check_specialization_namespace (CLASSTYPE_TI_TEMPLATE (type))
> > 	      && !at_namespace_scope_p ())
> > 	    return error_mark_node;
> > 	  SET_CLASSTYPE_TEMPLATE_SPECIALIZATION (type);
> > 	  DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)) = input_location;
> > 	  if (processing_template_decl)
> > 	    {
> > 	      tree decl = push_template_decl (TYPE_MAIN_DECL (type));
> > 	      if (decl == error_mark_node)
> > 		return error_mark_node;
> > 	      return TREE_TYPE (decl);
> > 	    }
> > 	}
> > 
> > The difference with modules is that rather than being able to overwrite
> > the existing instantiation with a definition in-place, we instead
> > currently get given a new tree with the entire template/decl/type of a
> > partial specialisation, but there is an existing template/decl/type of
> > an implicit instantiation in the specialisation table (and pointed to by
> > other by decls, in the case of the PR via the return type of a function).
> 
> Yes.
> 
> > So we need to somehow update in-place the existing type to be the type
> > of the partial specialisation so that we don't break any decls that
> > refer to it already.  My approach here is to attempt to copy the
> > relevant parts of what maybe_process_partial_specialization is doing so
> > we can pretend that we're merging an existing partial specialisation
> > (and use the rest of the existing logic) rather than doing part of the
> > in-place update of the implicit instantiation from within
> > add_mergeable_specialization, and then again once we get to
> > read_class_def.
> > 
> > Happy to try another route if you think there's something I've missed
> > here though.
> 
> Basically I was unclear why we don't automatically merge the types already.
> 
> But actually, it seems just as well that the types aren't merged; the
> primary template pattern and its equivalent specialization are separate
> types even though they compare identical, and with concepts we can have
> multiple partial specializations of the same template/args with different
> constraints; it seems like it might be better to consistently distinguish
> the partial specialization pattern from the matching instance of the
> template, even when there's only a single partial specialization of that
> type.
> 
> That way, I think we only need to make sure that the A<T*> in the partial
> specialization and the A<T*> in the declaration of f have the same
> TYPE_CANONICAL.  The attached seems to be enough to make the testcase
> compile, anyway.
> 
> Jason

Interesting; I'm not sure I fully understand how TYPE_CANONICAL
interacts with templates, but I think that makes sense; it also seems to
line up with this comment in 'maybe_new_partial_specialization':

      /* We only need a separate type node for storing the definition of this
	 partial specialization; uses of S<T*> are unconstrained, so all are
	 equivalent.  So keep TYPE_CANONICAL the same.  */
      TYPE_CANONICAL (t) = TYPE_CANONICAL (type);

I bootstrapped and regtested on x86_64-pc-linux-gnu a variant of your
patch which does indeed seem to work properly.  OK for trunk?

-- >8 --

Subject: [PATCH] c++/modules: Propagate TYPE_CANONICAL for partial
 specialisations [PR113814]

In some cases, when we go to import a partial specialisation there might
already be an incomplete implicit instantiation in the specialisation
table.  This causes ICEs described in the linked PR as we now have two
separate matching specialisations for this same arguments with different
TYPE_CANONICAL.

We already support multiple specialisations with the same args however,
as they may be differently constrained.  So we can solve this by simply
ensuring that the TYPE_CANONICAL of the new partial specialisation
matches the existing specialisation.

	PR c++/113814

gcc/cp/ChangeLog:

	* pt.cc (add_mergeable_specialization): Propagate
	TYPE_CANONICAL.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/partial-6.h: New test.
	* g++.dg/modules/partial-6_a.H: New test.
	* g++.dg/modules/partial-6_b.H: New test.
	* g++.dg/modules/partial-6_c.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Co-authored-by: Jason Merrill <jason@redhat.com>
---
 gcc/cp/pt.cc                               | 14 +++++++++-----
 gcc/testsuite/g++.dg/modules/partial-6.h   |  7 +++++++
 gcc/testsuite/g++.dg/modules/partial-6_a.H | 11 +++++++++++
 gcc/testsuite/g++.dg/modules/partial-6_b.H | 21 +++++++++++++++++++++
 gcc/testsuite/g++.dg/modules/partial-6_c.C | 12 ++++++++++++
 5 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-6.h
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_b.H
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_c.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index e20038a81ee..fcff282db37 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -31728,12 +31728,16 @@ add_mergeable_specialization (bool decl_p, spec_entry *elt, tree decl,
       auto *slot = type_specializations->find_slot (elt, INSERT);
 
       /* We don't distinguish different constrained partial type
-	 specializations, so there could be duplicates.  Everything else
-	 must be new.   */
-      if (!(flags & 2 && *slot))
+	 specializations, so there could be duplicates.  In that case we
+	 must propagate TYPE_CANONICAL so that they are treated as the
+	 same type.  Everything else must be new.   */
+      if (*slot)
+	{
+	  gcc_checking_assert (flags & 2);
+	  TYPE_CANONICAL (elt->spec) = TYPE_CANONICAL ((*slot)->spec);
+	}
+      else
 	{
-	  gcc_checking_assert (!*slot);
-
 	  auto entry = ggc_alloc<spec_entry> ();
 	  *entry = *elt;
 	  *slot = entry;
diff --git a/gcc/testsuite/g++.dg/modules/partial-6.h b/gcc/testsuite/g++.dg/modules/partial-6.h
new file mode 100644
index 00000000000..702c9a1b7ec
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-6.h
@@ -0,0 +1,7 @@
+// PR c++/113814
+
+template <typename> struct A {};
+template <typename T> A<T*> f();
+
+template <template <typename> typename, typename> struct B;
+template <template <typename> typename TT> B<TT, int> g();
diff --git a/gcc/testsuite/g++.dg/modules/partial-6_a.H b/gcc/testsuite/g++.dg/modules/partial-6_a.H
new file mode 100644
index 00000000000..6e0d5ddcaf0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-6_a.H
@@ -0,0 +1,11 @@
+// PR c++/113814
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+
+#include "partial-6.h"
+
+template <typename T>
+struct A<T*> { int a; };
+
+template <template <typename> typename TT>
+struct B<TT, int> { int b; };
diff --git a/gcc/testsuite/g++.dg/modules/partial-6_b.H b/gcc/testsuite/g++.dg/modules/partial-6_b.H
new file mode 100644
index 00000000000..569959b4f03
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-6_b.H
@@ -0,0 +1,21 @@
+// PR c++/113814
+// { dg-additional-options "-fmodules-ts -fdump-lang-module-alias" }
+// { dg-module-cmi {} }
+
+#include "partial-6.h"
+import "partial-6_a.H";
+
+template <typename, typename = void>
+struct TestTTP;
+
+inline void test() {
+  int a = f<int>().a;
+  int b = g<TestTTP>().b;
+}
+
+// { dg-final { scan-lang-dump {Read:-[0-9]*'s partial merge key \(new\) template_decl:'::template A'} module } }
+// { dg-final { scan-lang-dump {Read:-[0-9]*'s partial merge key \(new\) template_decl:'::template B'} module } }
+
+// Don't need to write the partial specialisations
+// { dg-final { scan-lang-dump-not {Wrote declaration entity:[0-9]* template_decl:'::template A<#null#>'} module } }
+// { dg-final { scan-lang-dump-not {Wrote declaration entity:[0-9]* template_decl:'::template B<template TT,int>'} module } }
diff --git a/gcc/testsuite/g++.dg/modules/partial-6_c.C b/gcc/testsuite/g++.dg/modules/partial-6_c.C
new file mode 100644
index 00000000000..2a34457f5af
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-6_c.C
@@ -0,0 +1,12 @@
+// PR c++/113814
+// { dg-additional-options "-fmodules-ts" }
+
+import "partial-6_b.H";
+
+template <typename>
+struct TestTTP2;
+
+int main() {
+  int a = f<double>().a;
+  int b = g<TestTTP2>().b;
+}
  

On 10/31/24 5:13 AM, Nathaniel Shead wrote:
> On Fri, Oct 25, 2024 at 11:25:15AM -0400, Jason Merrill wrote:
>> On 10/24/24 5:10 PM, Nathaniel Shead wrote:
>>> On Thu, Oct 24, 2024 at 12:05:18PM -0400, Jason Merrill wrote:
>>>> On 10/24/24 3:25 AM, Nathaniel Shead wrote:
>>>>> I wasn't sure whether I should include the ambiguity checking logic from
>>>>> process_partial_specialization; we don't do this anywhere else in the
>>>>> modules handling code that I could see so I left it out for now.
>>>>
>>>> The relevant bit in the standard seems to be
>>>> https://eel.is/c++draft/temp#spec.partial.general-1
>>>> "A partial specialization shall be reachable from any use of a template
>>>> specialization that would make use of the partial specialization as the
>>>> result of an implicit or explicit instantiation; no diagnostic is required."
>>>>
>>>> So we aren't required to diagnose an imported partial spec after an
>>>> instantiation.  It's not entirely clear whether the ambiguity checking in
>>>> https://eel.is/c++draft/temp#spec.partial.match-1.2 qualifies as "make use",
>>>> but that seems a natural interpretation.
>>>>
>>>> So checking these on import seems optional, but could also be helpful in
>>>> finding subtle bugs.
>>>>
>>>
>>> Makes sense.  I might add this as a follow-up patch then, since if we're
>>> doing this here it would make sense to do in add_mergeable_specialization
>>> as well now that I think about it.
>>>
>>>>> I could also rework process_partial_specialization to call the new
>>>>> create_mergeable_partial_spec function if you prefer, to reduce code
>>>>> duplication, though the early exit for VAR_DECL and the call to
>>>>> associate_classtype_constraints that relies on global state complicated
>>>>> it enough I didn't bother for this patch.
>>>>
>>>> That sounds desirable, if we need to create the mergeable partial spec at
>>>> all.
>>>>
>>>>> Finally, I'm not entirely sure whether the choice of DECL_TEMPLATE_PARMS
>>>>> that I'm providing is correct, especially considering this hunk:
>>>>>
>>>>>      /* Give template template parms a DECL_CONTEXT of the template
>>>>>         for which they are a parameter.  */
>>>>>      for (i = 0; i < ntparms; ++i)
>>>>>        {
>>>>>          tree parm = TREE_VALUE (TREE_VEC_ELT (inner_parms, i));
>>>>>          if (TREE_CODE (parm) == TEMPLATE_DECL)
>>>>> 	DECL_CONTEXT (parm) = tmpl;
>>>>>        }
>>>>>
>>>>> I haven't been able to produce any testcase that relies on this context
>>>>> setting to cause a failure, though, and there doesn't seem to be one in
>>>>> the testsuite for the matching hunk in process_partial_specialization.
>>>>> Any hints on where this might be needed?
>>>>
>>>> I believe that's for the benefit of coerce_template_args_for_ttp.
>>>>
>>>
>>> OK thanks, I'll do some exploring to see if I can cause a failure here
>>> then.
>>>
>>>>> In some cases, when we go to import a partial specialisation there might
>>>>> already be an incomplete implicit instantiation in the specialisation
>>>>> table.  This causes ICEs described in the linked PR as we now have two
>>>>> separate matching specialisations for this same arguments.
>>>>>
>>>>> This issue doesn't appear to happen for variable templates as it doesn't
>>>>> appear that we can create incomplete specialisations in the same way.
>>>>>
>>>>> This patch attempts to solve the issue by retrofitting the existing
>>>>> implicit instantiation as a partial specialisation (similarly to how
>>>>> maybe_process_partial_specialization operates) and then relying on the
>>>>> existing merging logic to fill in the definition of the type.  This
>>>>> works because for types almost all relevant details are streamed and
>>>>> merged in 'read_class_def'.
>>>>
>>>> I find it surprising that we would need to build a partial specialization to
>>>> merge with, when in a single TU we have no trouble merging a partial
>>>> specialization with an existing implicit instantiation.  Why can't we do
>>>> that in this case as well?
>>>>
>>>
>>> As far as I can tell, this is the normal behaviour for declarations;
>>> when we do
>>>
>>>     template <typename T> struct S;
>>>
>>>     template <typename T> struct S<T*>
>>>                                  ^~~~~
>>>
>>> This registers an implicit instantiation in the specialisation table
>>> already; the parser then calls 'maybe_process_partial_specialization'
>>> which overwrites the implicit instantiation as a partial specialization
>>> instead, see:
>>>
>>>         /* This is for ordinary explicit specialization and partial
>>> 	 specialization of a template class such as:
>>>
>>> 	   template <> class C<int>;
>>>
>>> 	 or:
>>>
>>> 	   template <class T> class C<T*>;
>>>
>>> 	 Make sure that `C<int>' and `C<T*>' are implicit instantiations.  */
>>>
>>>         if (maybe_new_partial_specialization (type))
>>> 	{
>>> 	  if (!check_specialization_namespace (CLASSTYPE_TI_TEMPLATE (type))
>>> 	      && !at_namespace_scope_p ())
>>> 	    return error_mark_node;
>>> 	  SET_CLASSTYPE_TEMPLATE_SPECIALIZATION (type);
>>> 	  DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)) = input_location;
>>> 	  if (processing_template_decl)
>>> 	    {
>>> 	      tree decl = push_template_decl (TYPE_MAIN_DECL (type));
>>> 	      if (decl == error_mark_node)
>>> 		return error_mark_node;
>>> 	      return TREE_TYPE (decl);
>>> 	    }
>>> 	}
>>>
>>> The difference with modules is that rather than being able to overwrite
>>> the existing instantiation with a definition in-place, we instead
>>> currently get given a new tree with the entire template/decl/type of a
>>> partial specialisation, but there is an existing template/decl/type of
>>> an implicit instantiation in the specialisation table (and pointed to by
>>> other by decls, in the case of the PR via the return type of a function).
>>
>> Yes.
>>
>>> So we need to somehow update in-place the existing type to be the type
>>> of the partial specialisation so that we don't break any decls that
>>> refer to it already.  My approach here is to attempt to copy the
>>> relevant parts of what maybe_process_partial_specialization is doing so
>>> we can pretend that we're merging an existing partial specialisation
>>> (and use the rest of the existing logic) rather than doing part of the
>>> in-place update of the implicit instantiation from within
>>> add_mergeable_specialization, and then again once we get to
>>> read_class_def.
>>>
>>> Happy to try another route if you think there's something I've missed
>>> here though.
>>
>> Basically I was unclear why we don't automatically merge the types already.
>>
>> But actually, it seems just as well that the types aren't merged; the
>> primary template pattern and its equivalent specialization are separate
>> types even though they compare identical, and with concepts we can have
>> multiple partial specializations of the same template/args with different
>> constraints; it seems like it might be better to consistently distinguish
>> the partial specialization pattern from the matching instance of the
>> template, even when there's only a single partial specialization of that
>> type.
>>
>> That way, I think we only need to make sure that the A<T*> in the partial
>> specialization and the A<T*> in the declaration of f have the same
>> TYPE_CANONICAL.  The attached seems to be enough to make the testcase
>> compile, anyway.
>>
>> Jason
> 
> Interesting; I'm not sure I fully understand how TYPE_CANONICAL
> interacts with templates, but I think that makes sense; it also seems to
> line up with this comment in 'maybe_new_partial_specialization':
> 
>        /* We only need a separate type node for storing the definition of this
> 	 partial specialization; uses of S<T*> are unconstrained, so all are
> 	 equivalent.  So keep TYPE_CANONICAL the same.  */
>        TYPE_CANONICAL (t) = TYPE_CANONICAL (type);
> 
> I bootstrapped and regtested on x86_64-pc-linux-gnu a variant of your
> patch which does indeed seem to work properly.  OK for trunk?

OK.

> -- >8 --
> 
> Subject: [PATCH] c++/modules: Propagate TYPE_CANONICAL for partial
>   specialisations [PR113814]
> 
> In some cases, when we go to import a partial specialisation there might
> already be an incomplete implicit instantiation in the specialisation
> table.  This causes ICEs described in the linked PR as we now have two
> separate matching specialisations for this same arguments with different
> TYPE_CANONICAL.
> 
> We already support multiple specialisations with the same args however,
> as they may be differently constrained.  So we can solve this by simply
> ensuring that the TYPE_CANONICAL of the new partial specialisation
> matches the existing specialisation.
> 
> 	PR c++/113814
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (add_mergeable_specialization): Propagate
> 	TYPE_CANONICAL.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/partial-6.h: New test.
> 	* g++.dg/modules/partial-6_a.H: New test.
> 	* g++.dg/modules/partial-6_b.H: New test.
> 	* g++.dg/modules/partial-6_c.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> Co-authored-by: Jason Merrill <jason@redhat.com>
> ---
>   gcc/cp/pt.cc                               | 14 +++++++++-----
>   gcc/testsuite/g++.dg/modules/partial-6.h   |  7 +++++++
>   gcc/testsuite/g++.dg/modules/partial-6_a.H | 11 +++++++++++
>   gcc/testsuite/g++.dg/modules/partial-6_b.H | 21 +++++++++++++++++++++
>   gcc/testsuite/g++.dg/modules/partial-6_c.C | 12 ++++++++++++
>   5 files changed, 60 insertions(+), 5 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/partial-6.h
>   create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_a.H
>   create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_b.H
>   create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_c.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index e20038a81ee..fcff282db37 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -31728,12 +31728,16 @@ add_mergeable_specialization (bool decl_p, spec_entry *elt, tree decl,
>         auto *slot = type_specializations->find_slot (elt, INSERT);
>   
>         /* We don't distinguish different constrained partial type
> -	 specializations, so there could be duplicates.  Everything else
> -	 must be new.   */
> -      if (!(flags & 2 && *slot))
> +	 specializations, so there could be duplicates.  In that case we
> +	 must propagate TYPE_CANONICAL so that they are treated as the
> +	 same type.  Everything else must be new.   */
> +      if (*slot)
> +	{
> +	  gcc_checking_assert (flags & 2);
> +	  TYPE_CANONICAL (elt->spec) = TYPE_CANONICAL ((*slot)->spec);
> +	}
> +      else
>   	{
> -	  gcc_checking_assert (!*slot);
> -
>   	  auto entry = ggc_alloc<spec_entry> ();
>   	  *entry = *elt;
>   	  *slot = entry;
> diff --git a/gcc/testsuite/g++.dg/modules/partial-6.h b/gcc/testsuite/g++.dg/modules/partial-6.h
> new file mode 100644
> index 00000000000..702c9a1b7ec
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/partial-6.h
> @@ -0,0 +1,7 @@
> +// PR c++/113814
> +
> +template <typename> struct A {};
> +template <typename T> A<T*> f();
> +
> +template <template <typename> typename, typename> struct B;
> +template <template <typename> typename TT> B<TT, int> g();
> diff --git a/gcc/testsuite/g++.dg/modules/partial-6_a.H b/gcc/testsuite/g++.dg/modules/partial-6_a.H
> new file mode 100644
> index 00000000000..6e0d5ddcaf0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/partial-6_a.H
> @@ -0,0 +1,11 @@
> +// PR c++/113814
> +// { dg-additional-options "-fmodule-header" }
> +// { dg-module-cmi {} }
> +
> +#include "partial-6.h"
> +
> +template <typename T>
> +struct A<T*> { int a; };
> +
> +template <template <typename> typename TT>
> +struct B<TT, int> { int b; };
> diff --git a/gcc/testsuite/g++.dg/modules/partial-6_b.H b/gcc/testsuite/g++.dg/modules/partial-6_b.H
> new file mode 100644
> index 00000000000..569959b4f03
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/partial-6_b.H
> @@ -0,0 +1,21 @@
> +// PR c++/113814
> +// { dg-additional-options "-fmodules-ts -fdump-lang-module-alias" }
> +// { dg-module-cmi {} }
> +
> +#include "partial-6.h"
> +import "partial-6_a.H";
> +
> +template <typename, typename = void>
> +struct TestTTP;
> +
> +inline void test() {
> +  int a = f<int>().a;
> +  int b = g<TestTTP>().b;
> +}
> +
> +// { dg-final { scan-lang-dump {Read:-[0-9]*'s partial merge key \(new\) template_decl:'::template A'} module } }
> +// { dg-final { scan-lang-dump {Read:-[0-9]*'s partial merge key \(new\) template_decl:'::template B'} module } }
> +
> +// Don't need to write the partial specialisations
> +// { dg-final { scan-lang-dump-not {Wrote declaration entity:[0-9]* template_decl:'::template A<#null#>'} module } }
> +// { dg-final { scan-lang-dump-not {Wrote declaration entity:[0-9]* template_decl:'::template B<template TT,int>'} module } }
> diff --git a/gcc/testsuite/g++.dg/modules/partial-6_c.C b/gcc/testsuite/g++.dg/modules/partial-6_c.C
> new file mode 100644
> index 00000000000..2a34457f5af
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/partial-6_c.C
> @@ -0,0 +1,12 @@
> +// PR c++/113814
> +// { dg-additional-options "-fmodules-ts" }
> +
> +import "partial-6_b.H";
> +
> +template <typename>
> +struct TestTTP2;
> +
> +int main() {
> +  int a = f<double>().a;
> +  int b = g<TestTTP2>().b;
> +}