C++ PATCH to fix a part of c++/70513 (ICE-on-invalid with enums)

C++ PATCH to fix a part of c++/70513 (ICE-on-invalid with enums)

[Marek Polacek]

This is my attempt to fix at least a part of this PR.  I haven't been able to
come up with a fix that fixes the other part involving templates.

We were ICEing on code such as

struct S
{
  enum E : int;
  enum S::E : int { foo } e;
};

Clang rejects this with "extra qualification" error.  When I modified the test
to use structs rather than enums...

struct T
{
  struct U;
  struct T::U {};
};

...I found out that we reject this with "extra qualification not allowed".  So
I think the enum case is missing a similar check that this patch adds.

By the template part of this PR I mean that we ICE on

template <typename T>
class D
{
  enum D::A { foo } c;
};

where clang++ says
error: template specialization or definition requires a template parameter list
       corresponding to the nested type 'D<T>'
which I guess means that a valid code would have "<T>" after "D".  I thought
num_template_headers_for_class and cp_parser_check_template_parameters would
do the job here, but apparently something else needs to be used for this case.
But I'm at my wits' end here.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-04-08  Marek Polacek  <polacek@redhat.com>

	PR c++/70513
	* parser.c (cp_parser_enum_specifier): Check for extra qualification.

	* g++.dg/cpp0x/forw_enum12.C: New test.

diff --git gcc/cp/parser.c gcc/cp/parser.c
index 28e01af..dc0d1c8 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -17231,6 +17231,15 @@ cp_parser_enum_specifier (cp_parser* parser)
 			  type, prev_scope, nested_name_specifier);
 	      type = error_mark_node;
 	    }
+	  /* If that scope is the scope where the declaration is being placed
+	     the program is invalid.  */
+	  else if (nested_name_specifier == prev_scope)
+	    {
+	      permerror (type_start_token->location,
+			 "extra qualification not allowed");
+	      type = error_mark_node;
+	      nested_name_specifier = NULL_TREE;
+	    }
 	}
 
       if (scoped_enum_p)
diff --git gcc/testsuite/g++.dg/cpp0x/forw_enum12.C gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
index e69de29..906ba68 100644
--- gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
+++ gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
@@ -0,0 +1,29 @@
+// PR c++/70513
+// { dg-do compile { target c++11 } }
+
+struct S1
+{
+  enum E : int;
+  enum S1::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S2
+{
+  enum class E : int;
+  enum class S2::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S3
+{
+  enum struct E : int;
+  enum struct S3::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S4
+{
+  struct S5
+  {
+    enum E : char;
+    enum S4::S5::E : char { X } e; // { dg-error "extra qualification not allowed" }
+  };
+};

	Marek

Re: C++ PATCH to fix a part of c++/70513 (ICE-on-invalid with enums)

[Marek Polacek]

Ping.

On Fri, Apr 08, 2016 at 01:51:02PM +0200, Marek Polacek wrote:
> This is my attempt to fix at least a part of this PR.  I haven't been able to
> come up with a fix that fixes the other part involving templates.
> 
> We were ICEing on code such as
> 
> struct S
> {
>   enum E : int;
>   enum S::E : int { foo } e;
> };
> 
> Clang rejects this with "extra qualification" error.  When I modified the test
> to use structs rather than enums...
> 
> struct T
> {
>   struct U;
>   struct T::U {};
> };
> 
> ...I found out that we reject this with "extra qualification not allowed".  So
> I think the enum case is missing a similar check that this patch adds.
> 
> By the template part of this PR I mean that we ICE on
> 
> template <typename T>
> class D
> {
>   enum D::A { foo } c;
> };
> 
> where clang++ says
> error: template specialization or definition requires a template parameter list
>        corresponding to the nested type 'D<T>'
> which I guess means that a valid code would have "<T>" after "D".  I thought
> num_template_headers_for_class and cp_parser_check_template_parameters would
> do the job here, but apparently something else needs to be used for this case.
> But I'm at my wits' end here.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2016-04-08  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/70513
> 	* parser.c (cp_parser_enum_specifier): Check for extra qualification.
> 
> 	* g++.dg/cpp0x/forw_enum12.C: New test.
> 
> diff --git gcc/cp/parser.c gcc/cp/parser.c
> index 28e01af..dc0d1c8 100644
> --- gcc/cp/parser.c
> +++ gcc/cp/parser.c
> @@ -17231,6 +17231,15 @@ cp_parser_enum_specifier (cp_parser* parser)
>  			  type, prev_scope, nested_name_specifier);
>  	      type = error_mark_node;
>  	    }
> +	  /* If that scope is the scope where the declaration is being placed
> +	     the program is invalid.  */
> +	  else if (nested_name_specifier == prev_scope)
> +	    {
> +	      permerror (type_start_token->location,
> +			 "extra qualification not allowed");
> +	      type = error_mark_node;
> +	      nested_name_specifier = NULL_TREE;
> +	    }
>  	}
>  
>        if (scoped_enum_p)
> diff --git gcc/testsuite/g++.dg/cpp0x/forw_enum12.C gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
> index e69de29..906ba68 100644
> --- gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
> +++ gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
> @@ -0,0 +1,29 @@
> +// PR c++/70513
> +// { dg-do compile { target c++11 } }
> +
> +struct S1
> +{
> +  enum E : int;
> +  enum S1::E : int { X } e; // { dg-error "extra qualification not allowed" }
> +};
> +
> +struct S2
> +{
> +  enum class E : int;
> +  enum class S2::E : int { X } e; // { dg-error "extra qualification not allowed" }
> +};
> +
> +struct S3
> +{
> +  enum struct E : int;
> +  enum struct S3::E : int { X } e; // { dg-error "extra qualification not allowed" }
> +};
> +
> +struct S4
> +{
> +  struct S5
> +  {
> +    enum E : char;
> +    enum S4::S5::E : char { X } e; // { dg-error "extra qualification not allowed" }
> +  };
> +};
> 
> 	Marek

	Marek

Re: C++ PATCH to fix a part of c++/70513 (ICE-on-invalid with enums)

[Jason Merrill]

On 04/08/2016 07:51 AM, Marek Polacek wrote:
> This is my attempt to fix at least a part of this PR.  I haven't been able to
> come up with a fix that fixes the other part involving templates.
>
> We were ICEing on code such as
>
> struct S
> {
>    enum E : int;
>    enum S::E : int { foo } e;
> };
>
> Clang rejects this with "extra qualification" error.  When I modified the test
> to use structs rather than enums...
>
> struct T
> {
>    struct U;
>    struct T::U {};
> };
>
> ...I found out that we reject this with "extra qualification not allowed".  So
> I think the enum case is missing a similar check that this patch adds.
>
> By the template part of this PR I mean that we ICE on
>
> template <typename T>
> class D
> {
>    enum D::A { foo } c;
> };
>
> where clang++ says
> error: template specialization or definition requires a template parameter list
>         corresponding to the nested type 'D<T>'
> which I guess means that a valid code would have "<T>" after "D".

No, this is misleading; adding the template args wouldn't make the extra 
qualification valid.  We should just give the extra qualification error 
in this case, too.

It might help to move your added check to before we push_scope.

> I thought
> num_template_headers_for_class and cp_parser_check_template_parameters would
> do the job here, but apparently something else needs to be used for this case.
> But I'm at my wits' end here.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2016-04-08  Marek Polacek  <polacek@redhat.com>
>
> 	PR c++/70513
> 	* parser.c (cp_parser_enum_specifier): Check for extra qualification.
>
> 	* g++.dg/cpp0x/forw_enum12.C: New test.
>
> diff --git gcc/cp/parser.c gcc/cp/parser.c
> index 28e01af..dc0d1c8 100644
> --- gcc/cp/parser.c
> +++ gcc/cp/parser.c
> @@ -17231,6 +17231,15 @@ cp_parser_enum_specifier (cp_parser* parser)
>   			  type, prev_scope, nested_name_specifier);
>   	      type = error_mark_node;
>   	    }
> +	  /* If that scope is the scope where the declaration is being placed
> +	     the program is invalid.  */
> +	  else if (nested_name_specifier == prev_scope)
> +	    {
> +	      permerror (type_start_token->location,
> +			 "extra qualification not allowed");
> +	      type = error_mark_node;
> +	      nested_name_specifier = NULL_TREE;
> +	    }
>   	}
>
>         if (scoped_enum_p)
> diff --git gcc/testsuite/g++.dg/cpp0x/forw_enum12.C gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
> index e69de29..906ba68 100644
> --- gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
> +++ gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
> @@ -0,0 +1,29 @@
> +// PR c++/70513
> +// { dg-do compile { target c++11 } }
> +
> +struct S1
> +{
> +  enum E : int;
> +  enum S1::E : int { X } e; // { dg-error "extra qualification not allowed" }
> +};
> +
> +struct S2
> +{
> +  enum class E : int;
> +  enum class S2::E : int { X } e; // { dg-error "extra qualification not allowed" }
> +};
> +
> +struct S3
> +{
> +  enum struct E : int;
> +  enum struct S3::E : int { X } e; // { dg-error "extra qualification not allowed" }
> +};
> +
> +struct S4
> +{
> +  struct S5
> +  {
> +    enum E : char;
> +    enum S4::S5::E : char { X } e; // { dg-error "extra qualification not allowed" }
> +  };
> +};
>
> 	Marek
>

Re: C++ PATCH to fix a part of c++/70513 (ICE-on-invalid with enums)

[Marek Polacek]

On Wed, Apr 20, 2016 at 11:33:55AM -0400, Jason Merrill wrote:
> On 04/08/2016 07:51 AM, Marek Polacek wrote:
> >By the template part of this PR I mean that we ICE on
> >
> >template <typename T>
> >class D
> >{
> >   enum D::A { foo } c;
> >};
> >
> >where clang++ says
> >error: template specialization or definition requires a template parameter list
> >        corresponding to the nested type 'D<T>'
> >which I guess means that a valid code would have "<T>" after "D".
> 
> No, this is misleading; adding the template args wouldn't make the extra
> qualification valid.  We should just give the extra qualification error in
> this case, too.

Oh, I see.  In that case...

> It might help to move your added check to before we push_scope.

This wouldn't help: nested_name_specifier and prev_scope are both "struct D",
but prev_scope contains TYPE_FIELDS, so comparison with == wouldn't work.  But
I wonder if we can't simply use same_type_p then, as in the below.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-04-20  Marek Polacek  <polacek@redhat.com>

	PR c/70513
	* parser.c (cp_parser_enum_specifier): Check and possibly error for
	extra qualification.

	* g++.dg/cpp0x/forw_enum12.C: New test.
	* g++.dg/cpp0x/forw_enum13.C: New test.

diff --git gcc/cp/parser.c gcc/cp/parser.c
index 0a1ed1a..e9d1995 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -17233,6 +17233,17 @@ cp_parser_enum_specifier (cp_parser* parser)
 			  type, prev_scope, nested_name_specifier);
 	      type = error_mark_node;
 	    }
+	  /* If that scope is the scope where the declaration is being placed
+	     the program is invalid.  */
+	  else if (CLASS_TYPE_P (nested_name_specifier)
+		   && CLASS_TYPE_P (prev_scope)
+		   && same_type_p (nested_name_specifier, prev_scope))
+	    {
+	      permerror (type_start_token->location,
+			 "extra qualification not allowed");
+	      type = error_mark_node;
+	      nested_name_specifier = NULL_TREE;
+	    }
 	}
 
       if (scoped_enum_p)
diff --git gcc/testsuite/g++.dg/cpp0x/forw_enum12.C gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
index e69de29..906ba68 100644
--- gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
+++ gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
@@ -0,0 +1,29 @@
+// PR c++/70513
+// { dg-do compile { target c++11 } }
+
+struct S1
+{
+  enum E : int;
+  enum S1::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S2
+{
+  enum class E : int;
+  enum class S2::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S3
+{
+  enum struct E : int;
+  enum struct S3::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S4
+{
+  struct S5
+  {
+    enum E : char;
+    enum S4::S5::E : char { X } e; // { dg-error "extra qualification not allowed" }
+  };
+};
diff --git gcc/testsuite/g++.dg/cpp0x/forw_enum13.C gcc/testsuite/g++.dg/cpp0x/forw_enum13.C
index e69de29..b8027f0 100644
--- gcc/testsuite/g++.dg/cpp0x/forw_enum13.C
+++ gcc/testsuite/g++.dg/cpp0x/forw_enum13.C
@@ -0,0 +1,47 @@
+// PR c++/70513
+// { dg-do compile { target c++11 } }
+
+template <typename T>
+class D1
+{
+  enum A : int;
+  enum D1::A : int { foo } c; // { dg-error "extra qualification not allowed" }
+};
+
+template <typename T>
+class D2
+{
+  enum A : int;
+  enum D2<T>::A : int { foo } c; // { dg-error "extra qualification not allowed" }
+};
+
+template <typename T>
+class D3
+{
+  enum D3::A { foo } c; // { dg-error "extra qualification not allowed" }
+};
+
+template <typename T>
+class D4
+{
+  enum D4<T>::A { foo } c; // { dg-error "extra qualification not allowed" }
+};
+
+template <typename T>
+class D5
+{
+  class D6
+  {
+    enum D6::A { foo } c; // { dg-error "extra qualification not allowed" }
+  };
+};
+
+template <typename T>
+class D7
+{
+  class D8
+  {
+    enum A : int;
+    enum D8::A : int { foo } c; // { dg-error "extra qualification not allowed" }
+  };
+};

	Marek

Re: C++ PATCH to fix a part of c++/70513 (ICE-on-invalid with enums)

[Jason Merrill]

On 04/21/2016 07:35 AM, Marek Polacek wrote:
> +	      permerror (type_start_token->location,
> +			 "extra qualification not allowed");
> +	      type = error_mark_node;

If we're using permerror, we shouldn't set type to error_mark_node; if 
we do that, -fpermissive won't make it work.

Jason

Re: C++ PATCH to fix a part of c++/70513 (ICE-on-invalid with enums)

[Marek Polacek]

On Thu, Apr 21, 2016 at 09:57:30AM -0400, Jason Merrill wrote:
> On 04/21/2016 07:35 AM, Marek Polacek wrote:
> >+	      permerror (type_start_token->location,
> >+			 "extra qualification not allowed");
> >+	      type = error_mark_node;
> 
> If we're using permerror, we shouldn't set type to error_mark_node; if we do
> that, -fpermissive won't make it work.

Yikes, that makes sense.  I removed the assignment.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-04-21  Marek Polacek  <polacek@redhat.com>

	PR c++/70513
	* parser.c (cp_parser_enum_specifier): Check and possibly error for
	extra qualification.

	* g++.dg/cpp0x/forw_enum12.C: New test.
	* g++.dg/cpp0x/forw_enum13.C: New test.

diff --git gcc/cp/parser.c gcc/cp/parser.c
index 0a1ed1a..feb8de7 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -17233,6 +17233,16 @@ cp_parser_enum_specifier (cp_parser* parser)
 			  type, prev_scope, nested_name_specifier);
 	      type = error_mark_node;
 	    }
+	  /* If that scope is the scope where the declaration is being placed
+	     the program is invalid.  */
+	  else if (CLASS_TYPE_P (nested_name_specifier)
+		   && CLASS_TYPE_P (prev_scope)
+		   && same_type_p (nested_name_specifier, prev_scope))
+	    {
+	      permerror (type_start_token->location,
+			 "extra qualification not allowed");
+	      nested_name_specifier = NULL_TREE;
+	    }
 	}
 
       if (scoped_enum_p)
diff --git gcc/testsuite/g++.dg/cpp0x/forw_enum12.C gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
index e69de29..906ba68 100644
--- gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
+++ gcc/testsuite/g++.dg/cpp0x/forw_enum12.C
@@ -0,0 +1,29 @@
+// PR c++/70513
+// { dg-do compile { target c++11 } }
+
+struct S1
+{
+  enum E : int;
+  enum S1::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S2
+{
+  enum class E : int;
+  enum class S2::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S3
+{
+  enum struct E : int;
+  enum struct S3::E : int { X } e; // { dg-error "extra qualification not allowed" }
+};
+
+struct S4
+{
+  struct S5
+  {
+    enum E : char;
+    enum S4::S5::E : char { X } e; // { dg-error "extra qualification not allowed" }
+  };
+};
diff --git gcc/testsuite/g++.dg/cpp0x/forw_enum13.C gcc/testsuite/g++.dg/cpp0x/forw_enum13.C
index e69de29..b8027f0 100644
--- gcc/testsuite/g++.dg/cpp0x/forw_enum13.C
+++ gcc/testsuite/g++.dg/cpp0x/forw_enum13.C
@@ -0,0 +1,47 @@
+// PR c++/70513
+// { dg-do compile { target c++11 } }
+
+template <typename T>
+class D1
+{
+  enum A : int;
+  enum D1::A : int { foo } c; // { dg-error "extra qualification not allowed" }
+};
+
+template <typename T>
+class D2
+{
+  enum A : int;
+  enum D2<T>::A : int { foo } c; // { dg-error "extra qualification not allowed" }
+};
+
+template <typename T>
+class D3
+{
+  enum D3::A { foo } c; // { dg-error "extra qualification not allowed" }
+};
+
+template <typename T>
+class D4
+{
+  enum D4<T>::A { foo } c; // { dg-error "extra qualification not allowed" }
+};
+
+template <typename T>
+class D5
+{
+  class D6
+  {
+    enum D6::A { foo } c; // { dg-error "extra qualification not allowed" }
+  };
+};
+
+template <typename T>
+class D7
+{
+  class D8
+  {
+    enum A : int;
+    enum D8::A : int { foo } c; // { dg-error "extra qualification not allowed" }
+  };
+};

	Marek

Re: C++ PATCH to fix a part of c++/70513 (ICE-on-invalid with enums)

[Jason Merrill]

OK.

Jason