c++: wrong error with MVP and pushdecl [PR64679]

This patch fixes the second half of 64679.  Here we issue a wrong
"redefinition of 'int x'" for the following:

  struct Bar {
    Bar(int, int, int);
  };

  int x = 1;
  Bar bar(int(x), int(x), int{x}); // #1

cp_parser_parameter_declaration_list does pushdecl every time it sees
a named parameter, so the second "int(x)" causes the error.  That's
premature, since this turns out to be a constructor call after the
third argument!

If the first parameter is parenthesized, we can't push until we've
established we're looking at a function declaration.  Therefore this
could be fixed by some kind of lookahead.  I thought about introducing
a lightweight variant of cp_parser_parameter_declaration_list that would
not have any side effects and would return as soon as it figures out
whether it's looking at a declaration or expression.  Since that would
require fairly nontrivial changes, I wanted something simpler.  Something
like delaying the pushdecl until we've reached the ')' following the
parameter-declaration-clause.  But that doesn't quite cut it: we must
have pushed the parameters before processing a default argument, as in:

  Bar bar(int(a), int(b), int c = sizeof(a));  // valid

After more trial and error than I'd be willing to admit I came up with
the following, which stashes parameters into a vector when parsing
tentatively, and only pushes when we're about to commit to any tentative
parse (that handles the default argument case) or we're at the end of
a parameter-declaration-clause followed by a ')' (so that we still diagnose
redefining a parameter).

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

	PR c++/64679

gcc/cp/ChangeLog:

	* parser.cc (push_pending_decls): New.
	(cp_parser_parameter_declaration_clause): Maintain a vector of
	parameters that haven't been pushed yet.  Push them at the end of
	a valid parameter-declaration-clause
	(cp_parser_parameter_declaration_list): Take a new auto_vec parameter.
	Pass it down to cp_parser_parameter_declaration.  Do not pushdecl
	while parsing tentatively.
	(cp_parser_parameter_declaration): Take a new auto_vec parameter.
	push_pending_decls when we're about to commit.
	(cp_parser_cache_defarg): Adjust the call to
	cp_parser_parameter_declaration_list.

gcc/testsuite/ChangeLog:

	* g++.dg/parse/ambig11.C: New test.
	* g++.dg/parse/ambig12.C: New test.
	* g++.dg/parse/ambig13.C: New test.
---
 gcc/cp/parser.cc                     | 76 ++++++++++++++++++++++++----
 gcc/testsuite/g++.dg/parse/ambig11.C | 39 ++++++++++++++
 gcc/testsuite/g++.dg/parse/ambig12.C | 12 +++++
 gcc/testsuite/g++.dg/parse/ambig13.C | 32 ++++++++++++
 4 files changed, 148 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/parse/ambig11.C
 create mode 100644 gcc/testsuite/g++.dg/parse/ambig12.C
 create mode 100644 gcc/testsuite/g++.dg/parse/ambig13.C


base-commit: c2e846b539bb932d7f68f7e6b3e401c361cc3bf3

On 5/4/22 16:03, Marek Polacek wrote:
> This patch fixes the second half of 64679.  Here we issue a wrong
> "redefinition of 'int x'" for the following:
> 
>    struct Bar {
>      Bar(int, int, int);
>    };
> 
>    int x = 1;
>    Bar bar(int(x), int(x), int{x}); // #1
> 
> cp_parser_parameter_declaration_list does pushdecl every time it sees
> a named parameter, so the second "int(x)" causes the error.  That's
> premature, since this turns out to be a constructor call after the
> third argument!
> 
> If the first parameter is parenthesized, we can't push until we've
> established we're looking at a function declaration.  Therefore this
> could be fixed by some kind of lookahead.  I thought about introducing
> a lightweight variant of cp_parser_parameter_declaration_list that would
> not have any side effects and would return as soon as it figures out
> whether it's looking at a declaration or expression.  Since that would
> require fairly nontrivial changes, I wanted something simpler.  Something
> like delaying the pushdecl until we've reached the ')' following the
> parameter-declaration-clause.  But that doesn't quite cut it: we must
> have pushed the parameters before processing a default argument, as in:
> 
>    Bar bar(int(a), int(b), int c = sizeof(a));  // valid

I wondered how this would affect

   void f(int (i), decltype(i) j = 42);

interestingly, clang and EDG both reject this, but they accept

   void f(int (i), bool b = true, decltype(i) j = 42);

which suggests a similar implementation strategy.  MSVC accepts both.

> After more trial and error than I'd be willing to admit I came up with
> the following, which stashes parameters into a vector when parsing
> tentatively, and only pushes when we're about to commit to any tentative
> parse (that handles the default argument case) or we're at the end of
> a parameter-declaration-clause followed by a ')' (so that we still diagnose
> redefining a parameter).
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> 	PR c++/64679
> 
> gcc/cp/ChangeLog:
> 
> 	* parser.cc (push_pending_decls): New.
> 	(cp_parser_parameter_declaration_clause): Maintain a vector of
> 	parameters that haven't been pushed yet.  Push them at the end of
> 	a valid parameter-declaration-clause
> 	(cp_parser_parameter_declaration_list): Take a new auto_vec parameter.
> 	Pass it down to cp_parser_parameter_declaration.  Do not pushdecl
> 	while parsing tentatively.
> 	(cp_parser_parameter_declaration): Take a new auto_vec parameter.
> 	push_pending_decls when we're about to commit.
> 	(cp_parser_cache_defarg): Adjust the call to
> 	cp_parser_parameter_declaration_list.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/parse/ambig11.C: New test.
> 	* g++.dg/parse/ambig12.C: New test.
> 	* g++.dg/parse/ambig13.C: New test.
> ---
>   gcc/cp/parser.cc                     | 76 ++++++++++++++++++++++++----
>   gcc/testsuite/g++.dg/parse/ambig11.C | 39 ++++++++++++++
>   gcc/testsuite/g++.dg/parse/ambig12.C | 12 +++++
>   gcc/testsuite/g++.dg/parse/ambig13.C | 32 ++++++++++++
>   4 files changed, 148 insertions(+), 11 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/parse/ambig11.C
>   create mode 100644 gcc/testsuite/g++.dg/parse/ambig12.C
>   create mode 100644 gcc/testsuite/g++.dg/parse/ambig13.C
> 
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index 5fa743b5a8e..dd51b3a7b79 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -2391,9 +2391,9 @@ static void cp_parser_type_specifier_seq
>   static tree cp_parser_parameter_declaration_clause
>     (cp_parser *, cp_parser_flags);
>   static tree cp_parser_parameter_declaration_list
> -  (cp_parser *, cp_parser_flags);
> +  (cp_parser *, cp_parser_flags, auto_vec<tree> *);
>   static cp_parameter_declarator *cp_parser_parameter_declaration
> -  (cp_parser *, cp_parser_flags, bool, bool *);
> +  (cp_parser *, cp_parser_flags, bool, bool *, auto_vec<tree> * = nullptr);
>   static tree cp_parser_default_argument
>     (cp_parser *, bool);
>   static void cp_parser_function_body
> @@ -24438,6 +24438,27 @@ function_being_declared_is_template_p (cp_parser* parser)
>   	  (current_class_type));
>   }
>   
> +/* Push any pending named parameters we have seen in this function declaration
> +   and stashed into PENDING_DECLS.  This mechanism is used for cases like
> +
> +     S foo(int(x), int(x), int{x});
> +
> +   where it's not clear if we're dealing with a constructor call or a function
> +   declaration until we've seen the last argument which breaks it up.  Pushing
> +   immediately would result in the redefinition error on the second 'x'.
> +
> +   We only have to do this if the first parameter is parenthesized.  */
> +
> +static void
> +push_pending_decls (auto_vec<tree> *pending_decls)
> +{
> +  if (!pending_decls)
> +    return;
> +  for (tree p : pending_decls)
> +    pushdecl (p);
> +  pending_decls->truncate (0);
> +}
> +
>   /* Parse a parameter-declaration-clause.
>   
>      parameter-declaration-clause:
> @@ -24494,8 +24515,12 @@ cp_parser_parameter_declaration_clause (cp_parser* parser,
>         return explicit_void_list_node;
>       }
>   
> +  /* A vector of parameters that haven't been pushed yet.  */
> +  auto_vec<tree> pending_decls;
> +
>     /* Parse the parameter-declaration-list.  */
> -  parameters = cp_parser_parameter_declaration_list (parser, flags);
> +  parameters = cp_parser_parameter_declaration_list (parser, flags,
> +						     &pending_decls);
>     /* If a parse error occurred while parsing the
>        parameter-declaration-list, then the entire
>        parameter-declaration-clause is erroneous.  */
> @@ -24525,6 +24550,14 @@ cp_parser_parameter_declaration_clause (cp_parser* parser,
>     else
>       ellipsis_p = false;
>   
> +  /* A valid parameter-declaration-clause can only be followed by a ')'.
> +     So it's time to push all the parameters we have seen now that we
> +     know we have a valid declaration.  Note that here we may not have
> +     committed yet, nor should we.  Pushing here will detect the error
> +     of redefining a parameter.  */
> +  if (cp_lexer_next_token_is (parser->lexer, CPP_CLOSE_PAREN))
> +    push_pending_decls (&pending_decls);
> +
>     /* Finish the parameter list.  */
>     if (!ellipsis_p)
>       parameters = chainon (parameters, void_list_node);
> @@ -24539,23 +24572,25 @@ cp_parser_parameter_declaration_clause (cp_parser* parser,
>        parameter-declaration-list , parameter-declaration
>   
>      The parser flags FLAGS is used to control type-specifier parsing.
> +   PENDING_DECLS is a vector of parameters that haven't been pushed yet.
>   
>      Returns a representation of the parameter-declaration-list, as for
>      cp_parser_parameter_declaration_clause.  However, the
>      `void_list_node' is never appended to the list.  */
>   
>   static tree
> -cp_parser_parameter_declaration_list (cp_parser* parser, cp_parser_flags flags)
> +cp_parser_parameter_declaration_list (cp_parser* parser,
> +				      cp_parser_flags flags,
> +				      auto_vec<tree> *pending_decls)
>   {
>     tree parameters = NULL_TREE;
>     tree *tail = &parameters;
> -  bool saved_in_unbraced_linkage_specification_p;
>     int index = 0;
>   
>     /* The special considerations that apply to a function within an
>        unbraced linkage specifications do not apply to the parameters
>        to the function.  */
> -  saved_in_unbraced_linkage_specification_p
> +  bool saved_in_unbraced_linkage_specification_p
>       = parser->in_unbraced_linkage_specification_p;
>     parser->in_unbraced_linkage_specification_p = false;
>   
> @@ -24570,7 +24605,8 @@ cp_parser_parameter_declaration_list (cp_parser* parser, cp_parser_flags flags)
>         parameter
>   	= cp_parser_parameter_declaration (parser, flags,
>   					   /*template_parm_p=*/false,
> -					   &parenthesized_p);
> +					   &parenthesized_p,
> +					   pending_decls);
>   
>         /* We don't know yet if the enclosing context is unavailable or deprecated,
>   	 so wait and deal with it in grokparms if appropriate.  */
> @@ -24602,7 +24638,16 @@ cp_parser_parameter_declaration_list (cp_parser* parser, cp_parser_flags flags)
>   			       parameter->decl_specifiers.attributes,
>   			       0);
>         if (DECL_NAME (decl))
> -	decl = pushdecl (decl);
> +	{
> +	  /* We cannot pushdecl while parsing tentatively because that may
> +	     have side effects and we cannot be sure yet if we are parsing
> +	     a declaration.  */
> +	  if (pending_decls
> +	      && cp_parser_uncommitted_to_tentative_parse_p (parser))
> +	    pending_decls->safe_push (decl);
> +	  else
> +	    decl = pushdecl (decl);
> +	}
>   
>         if (decl != error_mark_node)
>   	{
> @@ -24713,6 +24758,8 @@ cp_parser_parameter_declaration_list (cp_parser* parser, cp_parser_flags flags)
>      token encountered during the parsing of the assignment-expression
>      is not interpreted as a greater-than operator.)
>   
> +   PENDING_DECLS is a vector of parameters that haven't been pushed yet.
> +
>      Returns a representation of the parameter, or NULL if an error
>      occurs.  If PARENTHESIZED_P is non-NULL, *PARENTHESIZED_P is set to
>      true iff the declarator is of the form "(p)".  */
> @@ -24721,7 +24768,8 @@ static cp_parameter_declarator *
>   cp_parser_parameter_declaration (cp_parser *parser,
>   				 cp_parser_flags flags,
>   				 bool template_parm_p,
> -				 bool *parenthesized_p)
> +				 bool *parenthesized_p,
> +				 auto_vec<tree> *pending_decls)
>   {
>     int declares_class_or_enum;
>     cp_decl_specifier_seq decl_specifiers;
> @@ -24823,7 +24871,12 @@ cp_parser_parameter_declaration (cp_parser *parser,
>   		cp_parser_simulate_error (parser);
>   	    }
>   	  else
> -	    cp_parser_commit_to_tentative_parse (parser);
> +	    {
> +	      /* OK, so this *is* a declaration.  Push any parameters we have
> +		 seen; they may be used in default arguments, noexcept, etc.  */
> +	      push_pending_decls (pending_decls);
> +	      cp_parser_commit_to_tentative_parse (parser);
> +	    }
>   	}
>         /* Parse the declarator.  */
>         declarator_token_start = token;
> @@ -34049,7 +34102,8 @@ cp_parser_cache_defarg (cp_parser *parser, bool nsdmi)
>   		  cp_lexer_consume_token (parser->lexer);
>   		  begin_scope (sk_function_parms, NULL_TREE);
>   		  tree t = cp_parser_parameter_declaration_list
> -			    (parser, CP_PARSER_FLAGS_NONE);
> +			    (parser, CP_PARSER_FLAGS_NONE,
> +			     /*pending_decls*/nullptr);
>   		  if (t == error_mark_node)
>   		    error = true;
>   		  pop_bindings_and_leave_scope ();
> diff --git a/gcc/testsuite/g++.dg/parse/ambig11.C b/gcc/testsuite/g++.dg/parse/ambig11.C
> new file mode 100644
> index 00000000000..51a586f40b0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/parse/ambig11.C
> @@ -0,0 +1,39 @@
> +// PR c++/64679
> +// { dg-do run { target c++11 } }
> +
> +struct Bar {
> +  int a, b, c;
> +  Bar(int a, int b, int c) : a(a), b(b), c(c) { }
> +};
> +
> +void
> +f ()
> +{
> +  Bar fn1(int(a), int(b), int c = sizeof(a));
> +  Bar fn2(int(x), int(y), int(z)); // { dg-warning "function declaration" }
> +  Bar fn3(int(x), int(y), int);
> +  Bar fn4(int (*p)(int(x), int(y))); // { dg-warning "function declaration" }
> +  Bar fn5(int (x), int (*p)(int(x), int(y)), int);
> +}
> +
> +int
> +main ()
> +{
> +  int x = 1;
> +  // This ain't a decl.
> +  Bar v1(int(x), int(x), int{x});
> +  if (v1.a != 1 || v1.b != v1.a || v1.c != v1.a)
> +    __builtin_abort ();
> +  Bar v2(int(x), int(x), 1);
> +  if (v2.a != 1 || v2.b != v2.a || v2.c != 1)
> +    __builtin_abort ();
> +  Bar v3(int(x), int(x), int(1));
> +  if (v3.a != 1 || v3.b != v3.a || v3.c != 1)
> +    __builtin_abort ();
> +  Bar v4(int(1), int(x), int{x});
> +  if (v4.a != 1 || v4.b != 1 || v4.c != 1)
> +    __builtin_abort ();
> +  Bar v5(int{x}, int(x), int{x});
> +  if (v5.a != 1 || v5.b != v5.a || v5.c != v5.a)
> +    __builtin_abort ();
> +}
> diff --git a/gcc/testsuite/g++.dg/parse/ambig12.C b/gcc/testsuite/g++.dg/parse/ambig12.C
> new file mode 100644
> index 00000000000..981f35f32a2
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/parse/ambig12.C
> @@ -0,0 +1,12 @@
> +// PR c++/64679
> +
> +struct Bar {
> +  Bar (int, int, int);
> +};
> +
> +void
> +g ()
> +{
> +  Bar e1(int(x), int(x), int); // { dg-error "redefinition" }
> +  Bar e2(int (*p)(int(x), int(x)), int); // { dg-error "redefinition" }
> +}
> diff --git a/gcc/testsuite/g++.dg/parse/ambig13.C b/gcc/testsuite/g++.dg/parse/ambig13.C
> new file mode 100644
> index 00000000000..5f6a3b934c2
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/parse/ambig13.C
> @@ -0,0 +1,32 @@
> +// PR c++/64679
> +// { dg-do compile { target c++11 } }
> +
> +struct Bar {
> +  Bar (int, int, int);
> +};
> +
> +template<typename T>
> +void
> +g ()
> +{
> +  int x = 1;
> +  Bar v1(T(x), T(x), T{x});
> +  Bar v2(T(x), T(x), T(1));
> +}
> +
> +void
> +invoke (Bar (*p)) noexcept(noexcept(*p))
> +{
> +}
> +
> +auto
> +pmf (int (Bar::*p)) -> decltype(p)
> +{
> +  return nullptr;
> +}
> +
> +void
> +f ()
> +{
> +  g<int>();
> +}
> 
> base-commit: c2e846b539bb932d7f68f7e6b3e401c361cc3bf3

On Wed, May 04, 2022 at 05:44:45PM -0400, Jason Merrill wrote:
> On 5/4/22 16:03, Marek Polacek wrote:
> > This patch fixes the second half of 64679.  Here we issue a wrong
> > "redefinition of 'int x'" for the following:
> > 
> >    struct Bar {
> >      Bar(int, int, int);
> >    };
> > 
> >    int x = 1;
> >    Bar bar(int(x), int(x), int{x}); // #1
> > 
> > cp_parser_parameter_declaration_list does pushdecl every time it sees
> > a named parameter, so the second "int(x)" causes the error.  That's
> > premature, since this turns out to be a constructor call after the
> > third argument!
> > 
> > If the first parameter is parenthesized, we can't push until we've
> > established we're looking at a function declaration.  Therefore this
> > could be fixed by some kind of lookahead.  I thought about introducing
> > a lightweight variant of cp_parser_parameter_declaration_list that would
> > not have any side effects and would return as soon as it figures out
> > whether it's looking at a declaration or expression.  Since that would
> > require fairly nontrivial changes, I wanted something simpler.  Something
> > like delaying the pushdecl until we've reached the ')' following the
> > parameter-declaration-clause.  But that doesn't quite cut it: we must
> > have pushed the parameters before processing a default argument, as in:
> > 
> >    Bar bar(int(a), int(b), int c = sizeof(a));  // valid
> 
> I wondered how this would affect
> 
>   void f(int (i), decltype(i) j = 42);
> 
> interestingly, clang and EDG both reject this, but they accept
> 
>   void f(int (i), bool b = true, decltype(i) j = 42);
> 
> which suggests a similar implementation strategy.  MSVC accepts both.

Sigh, C++.

So the former would be rejected with this patch because decltype(i)
is parsed as the declspec.  And I can't play any cute games with decltype
because a decltype doesn't necessarily mean a parameter:

struct F {
  F(int, int);
};

void
g ()
{
  int x = 42;

  F v1(int(x), decltype(x)(42));

  F f1(int(i), decltype(i) j = 42);
  F f2(int(i), decltype(i) j);
  F f3(int(i), decltype(i)(j));
  F f4(int(i), decltype(i)(j) = 42);
  F f5(int (i), bool b = true, decltype(i) j = 42);
  F f6(int(i), decltype(x)(x));
}


But I think there's a way out: we could pushdecl the parameters as we
go and only stash when there would be a clash, if parsing tentatively.
And then push the pending parameters only at the end of the clause, solely
to get the redefinition/redeclaration error.  That is:

  Bar b(int(x), int(x), int{x});

would mean:
push x
store x
it's not a decl -> discard it, parse as an expression

  Bar b(int(x), int(x), int);

would mean:
push x
store x
it's a decl -> push pending parameters -> error

And then I don't need to push when about to commit, avoiding the need to
change cp_parser_parameter_declaration.  WDYT?

Marek

On 5/4/22 19:20, Marek Polacek wrote:
> On Wed, May 04, 2022 at 05:44:45PM -0400, Jason Merrill wrote:
>> On 5/4/22 16:03, Marek Polacek wrote:
>>> This patch fixes the second half of 64679.  Here we issue a wrong
>>> "redefinition of 'int x'" for the following:
>>>
>>>     struct Bar {
>>>       Bar(int, int, int);
>>>     };
>>>
>>>     int x = 1;
>>>     Bar bar(int(x), int(x), int{x}); // #1
>>>
>>> cp_parser_parameter_declaration_list does pushdecl every time it sees
>>> a named parameter, so the second "int(x)" causes the error.  That's
>>> premature, since this turns out to be a constructor call after the
>>> third argument!
>>>
>>> If the first parameter is parenthesized, we can't push until we've
>>> established we're looking at a function declaration.  Therefore this
>>> could be fixed by some kind of lookahead.  I thought about introducing
>>> a lightweight variant of cp_parser_parameter_declaration_list that would
>>> not have any side effects and would return as soon as it figures out
>>> whether it's looking at a declaration or expression.  Since that would
>>> require fairly nontrivial changes, I wanted something simpler.  Something
>>> like delaying the pushdecl until we've reached the ')' following the
>>> parameter-declaration-clause.  But that doesn't quite cut it: we must
>>> have pushed the parameters before processing a default argument, as in:
>>>
>>>     Bar bar(int(a), int(b), int c = sizeof(a));  // valid
>>
>> I wondered how this would affect
>>
>>    void f(int (i), decltype(i) j = 42);
>>
>> interestingly, clang and EDG both reject this, but they accept
>>
>>    void f(int (i), bool b = true, decltype(i) j = 42);
>>
>> which suggests a similar implementation strategy.  MSVC accepts both.
> 
> Sigh, C++.
> 
> So the former would be rejected with this patch because decltype(i)
> is parsed as the declspec.  And I can't play any cute games with decltype
> because a decltype doesn't necessarily mean a parameter:
> 
> struct F {
>    F(int, int);
> };
> 
> void
> g ()
> {
>    int x = 42;
> 
>    F v1(int(x), decltype(x)(42));
> 
>    F f1(int(i), decltype(i) j = 42);
>    F f2(int(i), decltype(i) j);
>    F f3(int(i), decltype(i)(j));
>    F f4(int(i), decltype(i)(j) = 42);
>    F f5(int (i), bool b = true, decltype(i) j = 42);
>    F f6(int(i), decltype(x)(x));
> }
> 
> 
> But I think there's a way out: we could pushdecl the parameters as we
> go and only stash when there would be a clash, if parsing tentatively.
> And then push the pending parameters only at the end of the clause, solely
> to get the redefinition/redeclaration error.  That is:
> 
>    Bar b(int(x), int(x), int{x});
> 
> would mean:
> push x
> store x
> it's not a decl -> discard it, parse as an expression
> 
>    Bar b(int(x), int(x), int);
> 
> would mean:
> push x
> store x
> it's a decl -> push pending parameters -> error
> 
> And then I don't need to push when about to commit, avoiding the need to
> change cp_parser_parameter_declaration.  WDYT?

Sounds good to me.

Jason