C++ PATCH for c++/88325 - ICE with invalid out-of-line template member definition

[Jason Merrill]

On Mon, Feb 4, 2019 at 4:52 PM Marek Polacek <polacek@redhat.com> wrote:
> On Mon, Feb 04, 2019 at 09:44:01AM -0500, Jason Merrill wrote:
> > On 2/1/19 2:55 PM, Marek Polacek wrote:
> > > On Fri, Feb 01, 2019 at 12:02:44PM -0500, Jason Merrill wrote:
> > > > On 2/1/19 11:26 AM, Marek Polacek wrote:
> > > > > On Wed, Jan 30, 2019 at 01:39:11PM -0500, Jason Merrill wrote:
> > > > > > On 1/28/19 9:46 PM, Marek Polacek wrote:
> > > > > > > This patch fixes an ICE-on-invalid (becase out-of-line constructors can't have
> > > > > > > template arguments and also because function templates can't be partially
> > > > > > > specialized) in C++2a: when we're parsing
> > > > > > >
> > > > > > >      template<typename T> template<typename U> A<T>::A<U> ()
> > > > > > >
> > > > > > > in the attached test we end up parsing "A<T>::A<U>" as a type name, and first we
> > > > > > > try a class-name.  First we process "A<T>::" as the nested name specifier and then
> > > > > > > we parse "A<U>".  In this test that results in a BASELINK.  Because in this context
> > > > > > > we're supposed to treat it as a typename ([temp.res]/6), we call make_typename_type,
> > > > > > > but that crashes.
> > > > > >
> > > > > > Hmm.  If we've done an actual lookup (that gave us a BASELINK), we aren't
> > > > > > dealing with a member of an unknown specialization anymore, so we should
> > > > > > just use the result of the lookup rather than speculate about what the name
> > > > > > might mean.  Why are we still trying to treat it as a typename?
> > > > >
> > > > > Good point.  It's because cp_parser_class_name has:
> > > > >
> > > > > 23095   /* Any name names a type if we're following the `typename' keyword
> > > > > 23096      in a qualified name where the enclosing scope is type-dependent.  */
> > > > > 23097   typename_p = (typename_keyword_p && scope && TYPE_P (scope)
> > > > > 23098                 && dependent_type_p (scope));
> > > > >
> > > > > and scope is in this case "A<T>" which is dependent.  Then there's this
> > > > > "identifier, but not template-id" case which only performs name lookup when
> > > > > typename_p is false.  But we're parsing "A<U>" so we call
> > > > > cp_parser_template_id.  It sees CPP_TEMPLATE_ID -- an already parsed
> > > > > template-id, so it just returns it, which is a BASELINK.  So even messing
> > > > > with tag_type wouldn't help.
> > > > >
> > > > > Does this answer your question?
> > > >
> > > > Mostly, though I'm still curious how we got the previous parse of the
> > > > template-id and yet continued to try to parse it as a class-name.
> > >
> > > So we have
> > > template<typename T> template<typename U>
> > > A<T>::A<U> ()
> > >
> > > and we're in cp_parser_single_declaration.  We're trying to parse the
> > > decl-specifier-seq, and that first tries to parse variou RID_* keywords
> > > and if that doesn't work it tries to parse a constructor:
> > >
> > >        /* Constructors are a special case.  The `S' in `S()' is not a
> > >      decl-specifier; it is the beginning of the declarator.  */
> > >        constructor_p
> > >     = (!found_decl_spec
> > >        && constructor_possible_p
> > >        && (cp_parser_constructor_declarator_p
> > >            (parser, decl_spec_seq_has_spec_p (decl_specs, ds_friend))));
> > >
> > > cp_parser_constructor_declarator_p calls cp_parser_nested_name_specifier_opt
> > > and that's where we parse the two template-ids, A<T> and A<U>.  But
> > > cp_parser_constructor_declarator_p isn't successful so we go to parsing
> > > A<T>::A<U> as a type-specifier, and that's where the above happens.
> >
> > Aha.  That seems like a bug in cp_parser_constructor_declarator.
>
> Sorry, what specifically, that cp_parser_constructor_declarator_p thinks it's
> not a ctor?  I thought it was an invalid ctor, since "out-of-line constructor
> for 'A' cannot have template arguments".

True, good point, though it is still more a constructor declarator
than anything else.  It would be nice for the diagnostic to suggest
removing the redundant template arguments aren't allowed, not just the
current confusing message about a partial specialization.  The
diagnostic for a similar declaration of a nested class template is
"partial specialization ‘struct A<T>::B<U>’ does not specialize any
template arguments; to define the primary template, remove the
template argument list".

We also shouldn't mention the internal name "__ct".

> Once again, we're parsing "A<T>::A<U> ()".  "A<T>::" is parsed as a nested-
> name-specifier = struct A.  Then:
>
> 27377   /* If we have a class scope, this is easy; DR 147 says that S::S always
> 27378      names the constructor, and no other qualified name could.  */
> 27379   if (constructor_p && nested_name_specifier
> 27380       && CLASS_TYPE_P (nested_name_specifier))
> 27381     {
> 27382       tree id = cp_parser_unqualified_id (parser,
> 27383                                           /*template_keyword_p=*/false,
> 27384                                           /*check_dependency_p=*/false,
> 27385                                           /*declarator_p=*/true,
> 27386                                           /*optional_p=*/false);
> 27387       if (is_overloaded_fn (id))
> 27388         id = DECL_NAME (get_first_fn (id));
> 27389       if (!constructor_name_p (id, nested_name_specifier))
> 27390         constructor_p = false;
> 27391     }
>
> 'id' is a BASELINK, we extract its name which is "__ct".  And constructor_name_p
> returns false because __ct != A.  What would you like we do here?

__ct is ctor_identifier, which certainly names a constructor, and we
got this BASELINK by looking up the name of the class, so I think
returning true would be more appropriate.

Jason