[C++] PR 88986 ("[7/8/9 Regression] ICE: tree check: expected tree that contains 'decl minimal' structure, have 'error_mark' in member_vec_binary_search, at cp/name-lookup.c:1136")

Hi,

I think that this ICE on invalid (and valid, for c++17+) can be in fact 
avoided by accepting in make_typename_type a TYPE_PACK_EXPANSION as 
context, thus by not triggering the "‘T ...’ is not a class" error. Not 
sure if a better fix would be something more general. Note, anyway, that 
we are asserting TYPE_P (context) thus TYPE_PACK_EXPANSIONs definitely 
get through beyond MAYBE_CLASS_TYPE_P.

Tested x86_64-linux.

Thanks, Paolo.

///////////////////////////
/cp
2019-02-01  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/88986
	* decl.c (make_typename_type): Allow for TYPE_PACK_EXPANSION as
	context (the first argument).

/testsuite
2019-02-01  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/88986
	* g++.dg/cpp1z/using4.C: New.

On 2/1/19 3:52 PM, Paolo Carlini wrote:
> Hi,
> 
> I think that this ICE on invalid (and valid, for c++17+) can be in fact 
> avoided by accepting in make_typename_type a TYPE_PACK_EXPANSION as 
> context, thus by not triggering the "‘T ...’ is not a class" error. Not 
> sure if a better fix would be something more general. Note, anyway, that 
> we are asserting TYPE_P (context) thus TYPE_PACK_EXPANSIONs definitely 
> get through beyond MAYBE_CLASS_TYPE_P.

The testcase should test that the using actually works, i.e. imports a 
type from a base class.

Jason

Hi,

On 04/02/19 15:47, Jason Merrill wrote:
> On 2/1/19 3:52 PM, Paolo Carlini wrote:
>> Hi,
>>
>> I think that this ICE on invalid (and valid, for c++17+) can be in 
>> fact avoided by accepting in make_typename_type a TYPE_PACK_EXPANSION 
>> as context, thus by not triggering the "‘T ...’ is not a class" 
>> error. Not sure if a better fix would be something more general. 
>> Note, anyway, that we are asserting TYPE_P (context) thus 
>> TYPE_PACK_EXPANSIONs definitely get through beyond MAYBE_CLASS_TYPE_P.
> The testcase should test that the using actually works, i.e. imports a 
> type from a base class.

Uhm, if I change the testcase to something like:

struct B { typedef int type; };

template<typename ...T> struct C : T... {
   using typename T::type ...;
   void f() { type value; }
};

template class C<B>;

we get a "sorry, unimplemented: use of ‘type_pack_expansion’ in 
template" for value, which, arguably, is better than the current ICE, 
but I'm not sure if we are close to completing the implementation of 
this / we even want to attempt that at this Stage?!?

Thanks, Paolo.

On Mon, Feb 4, 2019 at 11:00 AM Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 04/02/19 15:47, Jason Merrill wrote:
> > On 2/1/19 3:52 PM, Paolo Carlini wrote:
> >> Hi,
> >>
> >> I think that this ICE on invalid (and valid, for c++17+) can be in
> >> fact avoided by accepting in make_typename_type a TYPE_PACK_EXPANSION
> >> as context, thus by not triggering the "‘T ...’ is not a class"
> >> error. Not sure if a better fix would be something more general.
> >> Note, anyway, that we are asserting TYPE_P (context) thus
> >> TYPE_PACK_EXPANSIONs definitely get through beyond MAYBE_CLASS_TYPE_P.
> > The testcase should test that the using actually works, i.e. imports a
> > type from a base class.
>
> Uhm, if I change the testcase to something like:
>
> struct B { typedef int type; };
>
> template<typename ...T> struct C : T... {
>    using typename T::type ...;
>    void f() { type value; }
> };
>
> template class C<B>;
>
> we get a "sorry, unimplemented: use of ‘type_pack_expansion’ in
> template" for value

I suspected that would happen.

> which, arguably, is better than the current ICE,
> but I'm not sure if we are close to completing the implementation of
> this / we even want to attempt that at this Stage?!?

Well, I'd look at how we implement the equivalent for e.g. a variable:

struct A { static int i; };
template <class ...T> struct B: T... {
  using T::i ...;
};
auto x = B<A>::i; // works

and try to use that code path for the typename case as well.

Jason

Hi,

On 04/02/19 17:48, Jason Merrill wrote:
> On Mon, Feb 4, 2019 at 11:00 AM Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> On 04/02/19 15:47, Jason Merrill wrote:
>>> On 2/1/19 3:52 PM, Paolo Carlini wrote:
>>>> Hi,
>>>>
>>>> I think that this ICE on invalid (and valid, for c++17+) can be in
>>>> fact avoided by accepting in make_typename_type a TYPE_PACK_EXPANSION
>>>> as context, thus by not triggering the "‘T ...’ is not a class"
>>>> error. Not sure if a better fix would be something more general.
>>>> Note, anyway, that we are asserting TYPE_P (context) thus
>>>> TYPE_PACK_EXPANSIONs definitely get through beyond MAYBE_CLASS_TYPE_P.
>>> The testcase should test that the using actually works, i.e. imports a
>>> type from a base class.
>> Uhm, if I change the testcase to something like:
>>
>> struct B { typedef int type; };
>>
>> template<typename ...T> struct C : T... {
>>     using typename T::type ...;
>>     void f() { type value; }
>> };
>>
>> template class C<B>;
>>
>> we get a "sorry, unimplemented: use of ‘type_pack_expansion’ in
>> template" for value
> I suspected that would happen.
>
>> which, arguably, is better than the current ICE,
>> but I'm not sure if we are close to completing the implementation of
>> this / we even want to attempt that at this Stage?!?
> Well, I'd look at how we implement the equivalent for e.g. a variable:
>
> struct A { static int i; };
> template <class ...T> struct B: T... {
>    using T::i ...;
> };
> auto x = B<A>::i; // works
>
> and try to use that code path for the typename case as well.

Yes. Note that, AFAICS, the code you added to implement P0195R2 works 
fine for typename too. The problem happens when we actually encounter 
inside the class something like:

     void f() { type value; }

which, so to speak, appears to expand to a set of types: 'type' is a 
TYPENAME_TYPE which has a TYPE_PACK_EXPANSION as TYPE_CONTEXT. What 
should we do here, in general? Should we expand it to a set a VAR_DECLs? 
Note that in practice when the pack is tsubst-ed to more than one type 
we error out before seeing the function 'f', we say that we have a 
redeclaration:

struct B1 { typedef int type; };
struct B2 { typedef int type; };

template<typename ...T> struct C : T... {
   using typename T::type ...;
   //void f() { type value; }
};

template class C<B1, B2>;

we do exactly the same in the non-variadic using case, thus at least we 
are consistent.Therefore I can't imagine cases where potentially it 
would make sense to inject *a set* of VAR_DECLs and see what happens. 
See the attached for something more concrete: in hindsight it's obvious 
that if we change make_typename_type to let TYPE_PACK_EXPANSION through 
then we have to handle more than aggregates in tsubst...

Thanks, Paolo.
Index: decl.c
===================================================================
--- decl.c	(revision 268513)
+++ decl.c	(working copy)
@@ -3816,7 +3816,9 @@ make_typename_type (tree context, tree name, enum
   gcc_assert (identifier_p (name));
   gcc_assert (TYPE_P (context));
 
-  if (!MAYBE_CLASS_TYPE_P (context))
+  if (TREE_CODE (context) == TYPE_PACK_EXPANSION)
+    /* This can happen for C++17 variadic using (c++/88986).  */;
+  else if (!MAYBE_CLASS_TYPE_P (context))
     {
       if (complain & tf_error)
 	error ("%q#T is not a class", context);
Index: pt.c
===================================================================
--- pt.c	(revision 268513)
+++ pt.c	(working copy)
@@ -14895,8 +14895,18 @@ tsubst (tree t, tree args, tsubst_flags_t complain
 
     case TYPENAME_TYPE:
       {
-	tree ctx = tsubst_aggr_type (TYPE_CONTEXT (t), args, complain,
-				     in_decl, /*entering_scope=*/1);
+	tree ctx = TYPE_CONTEXT (t);
+	if (TREE_CODE (ctx) == TYPE_PACK_EXPANSION)
+	  {
+	    ctx = tsubst_pack_expansion (ctx, args, complain, in_decl);
+	    if (ctx == error_mark_node
+		|| TREE_VEC_LENGTH (ctx) != 1)
+	      return error_mark_node;
+	    ctx = TREE_VEC_ELT (ctx, 0);
+	  }
+	else
+	  ctx = tsubst_aggr_type (ctx, args, complain, in_decl,
+				  /*entering_scope=*/1);
 	if (ctx == error_mark_node)
 	  return error_mark_node;

... the below passes testing on x86_64-linux.

Note, we have to handle separately the empty pack case, otherwise we 
don't emit any diagnostics and we crash pretty soon.

Thanks, Paolo.

/////////////////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 268513)
+++ cp/decl.c	(working copy)
@@ -3816,7 +3816,9 @@ make_typename_type (tree context, tree name, enum
   gcc_assert (identifier_p (name));
   gcc_assert (TYPE_P (context));
 
-  if (!MAYBE_CLASS_TYPE_P (context))
+  if (TREE_CODE (context) == TYPE_PACK_EXPANSION)
+    /* This can happen for C++17 variadic using (c++/88986).  */;
+  else if (!MAYBE_CLASS_TYPE_P (context))
     {
       if (complain & tf_error)
 	error ("%q#T is not a class", context);
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 268513)
+++ cp/pt.c	(working copy)
@@ -14895,8 +14895,24 @@ tsubst (tree t, tree args, tsubst_flags_t complain
 
     case TYPENAME_TYPE:
       {
-	tree ctx = tsubst_aggr_type (TYPE_CONTEXT (t), args, complain,
-				     in_decl, /*entering_scope=*/1);
+	tree ctx = TYPE_CONTEXT (t);
+	if (TREE_CODE (ctx) == TYPE_PACK_EXPANSION)
+	  {
+	    ctx = tsubst_pack_expansion (ctx, args, complain, in_decl);
+	    if (ctx == error_mark_node
+		|| TREE_VEC_LENGTH (ctx) > 1)
+	      return error_mark_node;
+	    if (TREE_VEC_LENGTH (ctx) == 0)
+	      {
+		error ("%qD is instantiated for an empty pack",
+		       TYPENAME_TYPE_FULLNAME (t));
+		return error_mark_node;
+	      }
+	    ctx = TREE_VEC_ELT (ctx, 0);
+	  }
+	else
+	  ctx = tsubst_aggr_type (ctx, args, complain, in_decl,
+				  /*entering_scope=*/1);
 	if (ctx == error_mark_node)
 	  return error_mark_node;
 
Index: testsuite/g++.dg/cpp1z/using4.C
===================================================================
--- testsuite/g++.dg/cpp1z/using4.C	(nonexistent)
+++ testsuite/g++.dg/cpp1z/using4.C	(working copy)
@@ -0,0 +1,12 @@
+// PR c++/88986
+// { dg-do compile { target c++11 } }
+// { dg-options "" }
+
+struct B { typedef int type; };
+
+template<typename ...T> struct C : T... {
+  using typename T::type ...;  // { dg-warning "pack expansion" "" { target c++14_down } }
+  void f() { type value; }
+};
+
+template struct C<B>;
Index: testsuite/g++.dg/cpp1z/using5.C
===================================================================
--- testsuite/g++.dg/cpp1z/using5.C	(nonexistent)
+++ testsuite/g++.dg/cpp1z/using5.C	(working copy)
@@ -0,0 +1,12 @@
+// PR c++/88986
+// { dg-do compile { target c++11 } }
+// { dg-options "" }
+
+struct B { typedef int type; };
+
+template<typename ...T> struct C : T... {
+  using typename T::type ...;  // { dg-warning "pack expansion" "" { target c++14_down } }
+  void f() { type value; }  // { dg-error "empty pack" }
+};
+
+template struct C<>;

On 2/5/19 4:39 AM, Paolo Carlini wrote:
> Hi,
> 
> On 04/02/19 17:48, Jason Merrill wrote:
>> On Mon, Feb 4, 2019 at 11:00 AM Paolo Carlini 
>> <paolo.carlini@oracle.com> wrote:
>>> On 04/02/19 15:47, Jason Merrill wrote:
>>>> On 2/1/19 3:52 PM, Paolo Carlini wrote:
>>>>> Hi,
>>>>>
>>>>> I think that this ICE on invalid (and valid, for c++17+) can be in
>>>>> fact avoided by accepting in make_typename_type a TYPE_PACK_EXPANSION
>>>>> as context, thus by not triggering the "‘T ...’ is not a class"
>>>>> error. Not sure if a better fix would be something more general.
>>>>> Note, anyway, that we are asserting TYPE_P (context) thus
>>>>> TYPE_PACK_EXPANSIONs definitely get through beyond MAYBE_CLASS_TYPE_P.
>>>> The testcase should test that the using actually works, i.e. imports a
>>>> type from a base class.
>>> Uhm, if I change the testcase to something like:
>>>
>>> struct B { typedef int type; };
>>>
>>> template<typename ...T> struct C : T... {
>>>     using typename T::type ...;
>>>     void f() { type value; }
>>> };
>>>
>>> template class C<B>;
>>>
>>> we get a "sorry, unimplemented: use of ‘type_pack_expansion’ in
>>> template" for value
>> I suspected that would happen.
>>
>>> which, arguably, is better than the current ICE,
>>> but I'm not sure if we are close to completing the implementation of
>>> this / we even want to attempt that at this Stage?!?
>> Well, I'd look at how we implement the equivalent for e.g. a variable:
>>
>> struct A { static int i; };
>> template <class ...T> struct B: T... {
>>    using T::i ...;
>> };
>> auto x = B<A>::i; // works
>>
>> and try to use that code path for the typename case as well.
> 
> Yes. Note that, AFAICS, the code you added to implement P0195R2 works 
> fine for typename too. The problem happens when we actually encounter 
> inside the class something like:
> 
>      void f() { type value; }
> 
> which, so to speak, appears to expand to a set of types: 'type' is a 
> TYPENAME_TYPE which has a TYPE_PACK_EXPANSION as TYPE_CONTEXT.

I wonder about replacing uses of 'type' with a TYPENAME_TYPE with the 
current class as TYPE_CONTEXT?  But your approach is OK, too.

Jason