[C++] PR 89875 ("[7/8/9/10 Regression] invalid typeof reference to a member of an incomplete struct accepted at function scope")

Hi,

a while ago Martin noticed that an unintended consequence of an old 
tweak of mine - which avoided redundant error messages emitted from 
cp_parser_init_declarator - is that, in some cases, we started accepting 
ill-formed typeofs. Luckily, decltype isn't affected and that points to 
the real issue: by the time that place in cp_parser_init_declarator is 
reached, for a decltype version we already emitted a correct error 
message. Thus I think the right way to fix the problem is simply 
committing to tentative parse when, toward the end of 
cp_parser_sizeof_operand we know that we must be looking at a (possibly 
ill-formed) expression. Tested x86_64-linux.

Thanks, Paolo.

///////////////////////////
/cp
2019-05-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/89875
	* parser.c (cp_parser_sizeof_operand): When the type-id production
	did not work out commit to the tentative parse.

/testsuite
2019-05-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/89875
	* g++.dg/cpp0x/decltype-pr66548.C: Remove xfail.
	* g++.dg/template/sizeof-template-argument.C: Adjust expected error.

Hi,

gently pinging this...

On 10/05/19 16:29, Paolo Carlini wrote:
> Hi,
>
> a while ago Martin noticed that an unintended consequence of an old 
> tweak of mine - which avoided redundant error messages emitted from 
> cp_parser_init_declarator - is that, in some cases, we started 
> accepting ill-formed typeofs. Luckily, decltype isn't affected and 
> that points to the real issue: by the time that place in 
> cp_parser_init_declarator is reached, for a decltype version we 
> already emitted a correct error message. Thus I think the right way to 
> fix the problem is simply committing to tentative parse when, toward 
> the end of cp_parser_sizeof_operand we know that we must be looking at 
> a (possibly ill-formed) expression. Tested x86_64-linux.

     https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00501.html

Thanks, Paolo.

PS: To be honest, not knowing the exact rules, I would not consider this 
issue a P2 - in particular considering that typeof is legacy and is 
affected but all sorts of weird issues - but that's what it is ;)

On 5/10/19 10:29 AM, Paolo Carlini wrote:
> Hi,
> 
> a while ago Martin noticed that an unintended consequence of an old 
> tweak of mine - which avoided redundant error messages emitted from 
> cp_parser_init_declarator - is that, in some cases, we started accepting 
> ill-formed typeofs. Luckily, decltype isn't affected and that points to 
> the real issue: by the time that place in cp_parser_init_declarator is 
> reached, for a decltype version we already emitted a correct error 
> message. Thus I think the right way to fix the problem is simply 
> committing to tentative parse when, toward the end of 
> cp_parser_sizeof_operand we know that we must be looking at a (possibly 
> ill-formed) expression. Tested x86_64-linux.

The problem with calling cp_parser_commit_to_tentative_parse here is 
that the tentative parse you're committing to is for the enclosing 
scope, which is trying to decide whether e.g. we're parsing a 
declaration or expression.  If the operand of typeof is a well-formed 
expression, and the larger context is an expression, this will break.

Better, I think, to commit and re-parse only if you actually encounter 
an error.

Alternately, cp_parser_decltype_expr deals with this by using a 
tentative firewall and CPP_DECLTYPE; cp_parser_sizeof_operand could do 
the same, but that seems like a bigger hammer.

Jason

Hi,

On 28/05/19 16:47, Jason Merrill wrote:
> On 5/10/19 10:29 AM, Paolo Carlini wrote:
>> Hi,
>>
>> a while ago Martin noticed that an unintended consequence of an old 
>> tweak of mine - which avoided redundant error messages emitted from 
>> cp_parser_init_declarator - is that, in some cases, we started 
>> accepting ill-formed typeofs. Luckily, decltype isn't affected and 
>> that points to the real issue: by the time that place in 
>> cp_parser_init_declarator is reached, for a decltype version we 
>> already emitted a correct error message. Thus I think the right way 
>> to fix the problem is simply committing to tentative parse when, 
>> toward the end of cp_parser_sizeof_operand we know that we must be 
>> looking at a (possibly ill-formed) expression. Tested x86_64-linux.
>
> The problem with calling cp_parser_commit_to_tentative_parse here is 
> that the tentative parse you're committing to is for the enclosing 
> scope, which is trying to decide whether e.g. we're parsing a 
> declaration or expression.  If the operand of typeof is a well-formed 
> expression, and the larger context is an expression, this will break.
>
> Better, I think, to commit and re-parse only if you actually encounter 
> an error.
>
> Alternately, cp_parser_decltype_expr deals with this by using a 
> tentative firewall and CPP_DECLTYPE; cp_parser_sizeof_operand could do 
> the same, but that seems like a bigger hammer.

Today I spent quite a bit of time on this and eventually decided to 
follow the example of decltype as closely as possible. Then I started 
tweaking those drafts which laready passed the testsuite and after a 
while ended up with the below, rather close to the current code, in 
fact. Testing !cp_parser_error_occurred and in case calling 
cp_parser_abort_tentative_parse by hand (closer to the decltype example) 
also works. What do you think? Thanks, Paolo.

///////////////////////
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 271692)
+++ cp/parser.c	(working copy)
@@ -28942,6 +28942,8 @@ cp_parser_sizeof_operand (cp_parser* parser, enum
     {
       tree type = NULL_TREE;
 
+      tentative_firewall firewall (parser);
+
       /* We can't be sure yet whether we're looking at a type-id or an
 	 expression.  */
       cp_parser_parse_tentatively (parser);
@@ -28969,11 +28971,15 @@ cp_parser_sizeof_operand (cp_parser* parser, enum
       /* If all went well, then we're done.  */
       if (cp_parser_parse_definitely (parser))
 	expr = type;
+      else
+	{
+	  /* Commit to the tentative_firewall so we get syntax errors.  */
+	  cp_parser_commit_to_tentative_parse (parser);
+
+	  expr = cp_parser_unary_expression (parser);
+	}
     }
-
-  /* If the type-id production did not work out, then we must be
-     looking at the unary-expression production.  */
-  if (!expr)
+  else
     expr = cp_parser_unary_expression (parser);
 
   /* Go back to evaluating expressions.  */
Index: testsuite/g++.dg/cpp0x/decltype-pr66548.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype-pr66548.C	(revision 271692)
+++ testsuite/g++.dg/cpp0x/decltype-pr66548.C	(working copy)
@@ -11,7 +11,7 @@ struct Meow {};
 
 void f ()
 {
-  decltype (Meow.purr ()) d;   // { dg-error "expected primary-expression" "pr89875" { xfail c++98_only } }
+  decltype (Meow.purr ()) d;   // { dg-error "expected primary-expression" }
   (void)&d;
 }
 
Index: testsuite/g++.dg/template/sizeof-template-argument.C
===================================================================
--- testsuite/g++.dg/template/sizeof-template-argument.C	(revision 271692)
+++ testsuite/g++.dg/template/sizeof-template-argument.C	(working copy)
@@ -3,9 +3,9 @@
 
 template<int> struct A {};
 
-template<typename> struct B : A <sizeof(=)> {}; /* { dg-error "template argument" } */
+template<typename> struct B : A <sizeof(=)> {}; /* { dg-error "expected primary-expression" } */
 
-template<typename> struct C : A <sizeof(=)> {}; /* { dg-error "template argument" } */
+template<typename> struct C : A <sizeof(=)> {}; /* { dg-error "expected primary-expression" } */
 
 int a;

On 5/28/19 4:44 PM, Paolo Carlini wrote:
> Hi,
> 
> On 28/05/19 16:47, Jason Merrill wrote:
>> On 5/10/19 10:29 AM, Paolo Carlini wrote:
>>> Hi,
>>>
>>> a while ago Martin noticed that an unintended consequence of an old 
>>> tweak of mine - which avoided redundant error messages emitted from 
>>> cp_parser_init_declarator - is that, in some cases, we started 
>>> accepting ill-formed typeofs. Luckily, decltype isn't affected and 
>>> that points to the real issue: by the time that place in 
>>> cp_parser_init_declarator is reached, for a decltype version we 
>>> already emitted a correct error message. Thus I think the right way 
>>> to fix the problem is simply committing to tentative parse when, 
>>> toward the end of cp_parser_sizeof_operand we know that we must be 
>>> looking at a (possibly ill-formed) expression. Tested x86_64-linux.
>>
>> The problem with calling cp_parser_commit_to_tentative_parse here is 
>> that the tentative parse you're committing to is for the enclosing 
>> scope, which is trying to decide whether e.g. we're parsing a 
>> declaration or expression.  If the operand of typeof is a well-formed 
>> expression, and the larger context is an expression, this will break.
>>
>> Better, I think, to commit and re-parse only if you actually encounter 
>> an error.
>>
>> Alternately, cp_parser_decltype_expr deals with this by using a 
>> tentative firewall and CPP_DECLTYPE; cp_parser_sizeof_operand could do 
>> the same, but that seems like a bigger hammer.
> 
> Today I spent quite a bit of time on this and eventually decided to 
> follow the example of decltype as closely as possible. Then I started 
> tweaking those drafts which laready passed the testsuite and after a 
> while ended up with the below, rather close to the current code, in 
> fact. Testing !cp_parser_error_occurred and in case calling 
> cp_parser_abort_tentative_parse by hand (closer to the decltype example) 
> also works. What do you think? Thanks, Paolo.

OK.

Jason