[PATCH] c++: bogus error w/ variadic concept-id as if cond [PR98394]

[PATCH] c++: bogus error w/ variadic concept-id as if cond [PR98394]

[Patrick Palka]

Here when tentatively parsing the if condition as a declaration, we try
to treat C<1> as the start of a constrained placeholder type, which we
quickly reject because C doesn't accept a type as its first argument.
But since we're parsing tentatively, we shouldn't emit an error in this
case.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk/11?

	PR c++/98394

gcc/cp/ChangeLog:

	* parser.c (cp_parser_placeholder_type_specifier): Don't emit
	a "does not constrain a type" error when parsing tentatively.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-pr98394.C: New test.
---
 gcc/cp/parser.c                               |  7 +++++--
 gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 4c2075742d6..f1498e28da4 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -19909,8 +19909,11 @@ cp_parser_placeholder_type_specifier (cp_parser *parser, location_t loc,
       if (!flag_concepts_ts
 	  || !processing_template_parmlist)
 	{
-	  error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
-	  inform (DECL_SOURCE_LOCATION (con), "concept defined here");
+	  if (!tentative)
+	    {
+	      error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
+	      inform (DECL_SOURCE_LOCATION (con), "concept defined here");
+	    }
 	  return error_mark_node;
 	}
     }
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C b/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
new file mode 100644
index 00000000000..c8407cdf7cd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
@@ -0,0 +1,14 @@
+// PR c++/98394
+// { dg-do compile { target c++20 } }
+
+template<int...>
+concept C = true;
+
+template<int, int>
+concept D = true;
+
+int main() {
+  if (C<1>); // { dg-bogus "does not constrain a type" }
+  if (D<1>); // { dg-error "wrong number of template arguments" }
+	     // { dg-bogus "does not constrain a type" "" { target *-*-* } .-1 }
+}
-- 
2.34.0.rc1.14.g88d915a634

Re: [PATCH] c++: bogus error w/ variadic concept-id as if cond [PR98394]

[Jason Merrill]

On 11/8/21 10:35, Patrick Palka wrote:
> Here when tentatively parsing the if condition as a declaration, we try
> to treat C<1> as the start of a constrained placeholder type, which we
> quickly reject because C doesn't accept a type as its first argument.
> But since we're parsing tentatively, we shouldn't emit an error in this
> case.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk/11?
> 
> 	PR c++/98394
> 
> gcc/cp/ChangeLog:
> 
> 	* parser.c (cp_parser_placeholder_type_specifier): Don't emit
> 	a "does not constrain a type" error when parsing tentatively.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-pr98394.C: New test.
> ---
>   gcc/cp/parser.c                               |  7 +++++--
>   gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C | 14 ++++++++++++++
>   2 files changed, 19 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
> 
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 4c2075742d6..f1498e28da4 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -19909,8 +19909,11 @@ cp_parser_placeholder_type_specifier (cp_parser *parser, location_t loc,
>         if (!flag_concepts_ts
>   	  || !processing_template_parmlist)
>   	{
> -	  error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
> -	  inform (DECL_SOURCE_LOCATION (con), "concept defined here");
> +	  if (!tentative)
> +	    {
> +	      error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
> +	      inform (DECL_SOURCE_LOCATION (con), "concept defined here");
> +	    }

We probably want to cp_parser_simulate_error in the tentative case?

I also wonder why this code uses a "tentative" parameter instead of 
checking cp_parser_parsing_tentatively itself.

Jason

Re: [PATCH] c++: bogus error w/ variadic concept-id as if cond [PR98394]

[Patrick Palka]

On Mon, 8 Nov 2021, Jason Merrill wrote:

> On 11/8/21 10:35, Patrick Palka wrote:
> > Here when tentatively parsing the if condition as a declaration, we try
> > to treat C<1> as the start of a constrained placeholder type, which we
> > quickly reject because C doesn't accept a type as its first argument.
> > But since we're parsing tentatively, we shouldn't emit an error in this
> > case.
> > 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk/11?
> > 
> > 	PR c++/98394
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* parser.c (cp_parser_placeholder_type_specifier): Don't emit
> > 	a "does not constrain a type" error when parsing tentatively.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp2a/concepts-pr98394.C: New test.
> > ---
> >   gcc/cp/parser.c                               |  7 +++++--
> >   gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C | 14 ++++++++++++++
> >   2 files changed, 19 insertions(+), 2 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
> > 
> > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > index 4c2075742d6..f1498e28da4 100644
> > --- a/gcc/cp/parser.c
> > +++ b/gcc/cp/parser.c
> > @@ -19909,8 +19909,11 @@ cp_parser_placeholder_type_specifier (cp_parser
> > *parser, location_t loc,
> >         if (!flag_concepts_ts
> >   	  || !processing_template_parmlist)
> >   	{
> > -	  error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
> > -	  inform (DECL_SOURCE_LOCATION (con), "concept defined here");
> > +	  if (!tentative)
> > +	    {
> > +	      error_at (loc, "%qE does not constrain a type", DECL_NAME
> > (con));
> > +	      inform (DECL_SOURCE_LOCATION (con), "concept defined here");
> > +	    }
> 
> We probably want to cp_parser_simulate_error in the tentative case?

It seems the only caller, cp_parser_simple_type_specifier, is
responsible for that currently.

> 
> I also wonder why this code uses a "tentative" parameter instead of checking
> cp_parser_parsing_tentatively itself.

During the first call to cp_parser_placeholder_type_specifier from
cp_parser_simple_type_specifier, we're always parsing tentatively so
cp_parser_parsing_tentatively would always be true whereas 'tentative'
could be false if there's also an outer tentative parse.  So some
reworking of the caller would also be needed in order to avoid the
'tentative' parameter.  I tried, but I wasn't able to make it work
without introducing diagnostic regressions :/

While poking at the idea though, I was reminded of the related PR85846
which is about the concept-id C<> being rejected due to a stray error
being emitted during the tentative type parse.  The below patch
additionally fixes this PR since it just requires a one-line change in
cp_parser_placeholder_type_specifier.

-- >8 --

Subject: [PATCH] c++: bogus error w/ variadic concept-id as if cond [PR98394]

Here when tentatively parsing the if condition as a declaration, we try
to treat C<1> as the start of a constrained placeholder type, which we
quickly reject because C doesn't accept a type as its first argument.
But since we're parsing tentatively, we shouldn't emit an error in this
case.

In passing, also fix PR85846 by only overriding tentative to false when
given a concept-name, and not also when given a concept-id with empty
argument list.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk/11?

	PR c++/98394
	PR c++/85846

gcc/cp/ChangeLog:

	* parser.c (cp_parser_placeholder_type_specifier): Declare
	static.  Don't override tentative to false when tmpl is a
	concept-id with empty argument list.  Don't emit a "does not
	constrain a type" error when tentative.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-pr98394.C: New test.
	* g++.dg/cpp2a/concepts-pr85846.C: New test.
---
 gcc/cp/parser.c                               | 11 +++++++----
 gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C |  5 +++++
 gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 4c2075742d6..71f782468ef 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -19855,7 +19855,7 @@ cp_parser_simple_type_specifier (cp_parser* parser,
   Note that the Concepts TS allows the auto or decltype(auto) to be
   omitted in a constrained-type-specifier.  */
 
-tree
+static tree
 cp_parser_placeholder_type_specifier (cp_parser *parser, location_t loc,
 				      tree tmpl, bool tentative)
 {
@@ -19871,7 +19871,7 @@ cp_parser_placeholder_type_specifier (cp_parser *parser, location_t loc,
       args = TREE_OPERAND (tmpl, 1);
       tmpl = TREE_OPERAND (tmpl, 0);
     }
-  if (args == NULL_TREE)
+  else if (args == NULL_TREE)
     /* A concept-name with no arguments can't be an expression.  */
     tentative = false;
 
@@ -19909,8 +19909,11 @@ cp_parser_placeholder_type_specifier (cp_parser *parser, location_t loc,
       if (!flag_concepts_ts
 	  || !processing_template_parmlist)
 	{
-	  error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
-	  inform (DECL_SOURCE_LOCATION (con), "concept defined here");
+	  if (!tentative)
+	    {
+	      error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
+	      inform (DECL_SOURCE_LOCATION (con), "concept defined here");
+	    }
 	  return error_mark_node;
 	}
     }
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C b/gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C
new file mode 100644
index 00000000000..1507c604920
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C
@@ -0,0 +1,5 @@
+// PR c++/85846
+// { dg-do compile { target c++20 } }
+
+template<int = 0> concept Foo = true;
+bool f(Foo<>);
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C b/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
new file mode 100644
index 00000000000..c8407cdf7cd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
@@ -0,0 +1,14 @@
+// PR c++/98394
+// { dg-do compile { target c++20 } }
+
+template<int...>
+concept C = true;
+
+template<int, int>
+concept D = true;
+
+int main() {
+  if (C<1>); // { dg-bogus "does not constrain a type" }
+  if (D<1>); // { dg-error "wrong number of template arguments" }
+	     // { dg-bogus "does not constrain a type" "" { target *-*-* } .-1 }
+}
-- 
2.34.0.rc1.14.g88d915a634

Re: [PATCH] c++: bogus error w/ variadic concept-id as if cond [PR98394]

[Jason Merrill]

On 11/8/21 19:54, Patrick Palka wrote:
> On Mon, 8 Nov 2021, Jason Merrill wrote:
> 
>> On 11/8/21 10:35, Patrick Palka wrote:
>>> Here when tentatively parsing the if condition as a declaration, we try
>>> to treat C<1> as the start of a constrained placeholder type, which we
>>> quickly reject because C doesn't accept a type as its first argument.
>>> But since we're parsing tentatively, we shouldn't emit an error in this
>>> case.
>>>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
>>> trunk/11?
>>>
>>> 	PR c++/98394
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* parser.c (cp_parser_placeholder_type_specifier): Don't emit
>>> 	a "does not constrain a type" error when parsing tentatively.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/cpp2a/concepts-pr98394.C: New test.
>>> ---
>>>    gcc/cp/parser.c                               |  7 +++++--
>>>    gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C | 14 ++++++++++++++
>>>    2 files changed, 19 insertions(+), 2 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
>>>
>>> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
>>> index 4c2075742d6..f1498e28da4 100644
>>> --- a/gcc/cp/parser.c
>>> +++ b/gcc/cp/parser.c
>>> @@ -19909,8 +19909,11 @@ cp_parser_placeholder_type_specifier (cp_parser
>>> *parser, location_t loc,
>>>          if (!flag_concepts_ts
>>>    	  || !processing_template_parmlist)
>>>    	{
>>> -	  error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
>>> -	  inform (DECL_SOURCE_LOCATION (con), "concept defined here");
>>> +	  if (!tentative)
>>> +	    {
>>> +	      error_at (loc, "%qE does not constrain a type", DECL_NAME
>>> (con));
>>> +	      inform (DECL_SOURCE_LOCATION (con), "concept defined here");
>>> +	    }
>>
>> We probably want to cp_parser_simulate_error in the tentative case?
> 
> It seems the only caller, cp_parser_simple_type_specifier, is
> responsible for that currently.
> 
>>
>> I also wonder why this code uses a "tentative" parameter instead of checking
>> cp_parser_parsing_tentatively itself.
> 
> During the first call to cp_parser_placeholder_type_specifier from
> cp_parser_simple_type_specifier, we're always parsing tentatively so
> cp_parser_parsing_tentatively would always be true whereas 'tentative'
> could be false if there's also an outer tentative parse.  So some
> reworking of the caller would also be needed in order to avoid the
> 'tentative' parameter.  I tried, but I wasn't able to make it work
> without introducing diagnostic regressions :/
> 
> While poking at the idea though, I was reminded of the related PR85846
> which is about the concept-id C<> being rejected due to a stray error
> being emitted during the tentative type parse.  The below patch
> additionally fixes this PR since it just requires a one-line change in
> cp_parser_placeholder_type_specifier.
> 
> -- >8 --
> 
> Subject: [PATCH] c++: bogus error w/ variadic concept-id as if cond [PR98394]
> 
> Here when tentatively parsing the if condition as a declaration, we try
> to treat C<1> as the start of a constrained placeholder type, which we
> quickly reject because C doesn't accept a type as its first argument.
> But since we're parsing tentatively, we shouldn't emit an error in this
> case.
> 
> In passing, also fix PR85846 by only overriding tentative to false when
> given a concept-name, and not also when given a concept-id with empty
> argument list.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk/11?

OK for both.

> 	PR c++/98394
> 	PR c++/85846
> 
> gcc/cp/ChangeLog:
> 
> 	* parser.c (cp_parser_placeholder_type_specifier): Declare
> 	static.  Don't override tentative to false when tmpl is a
> 	concept-id with empty argument list.  Don't emit a "does not
> 	constrain a type" error when tentative.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-pr98394.C: New test.
> 	* g++.dg/cpp2a/concepts-pr85846.C: New test.
> ---
>   gcc/cp/parser.c                               | 11 +++++++----
>   gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C |  5 +++++
>   gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C | 14 ++++++++++++++
>   3 files changed, 26 insertions(+), 4 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
> 
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 4c2075742d6..71f782468ef 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -19855,7 +19855,7 @@ cp_parser_simple_type_specifier (cp_parser* parser,
>     Note that the Concepts TS allows the auto or decltype(auto) to be
>     omitted in a constrained-type-specifier.  */
>   
> -tree
> +static tree
>   cp_parser_placeholder_type_specifier (cp_parser *parser, location_t loc,
>   				      tree tmpl, bool tentative)
>   {
> @@ -19871,7 +19871,7 @@ cp_parser_placeholder_type_specifier (cp_parser *parser, location_t loc,
>         args = TREE_OPERAND (tmpl, 1);
>         tmpl = TREE_OPERAND (tmpl, 0);
>       }
> -  if (args == NULL_TREE)
> +  else if (args == NULL_TREE)
>       /* A concept-name with no arguments can't be an expression.  */
>       tentative = false;
>   
> @@ -19909,8 +19909,11 @@ cp_parser_placeholder_type_specifier (cp_parser *parser, location_t loc,
>         if (!flag_concepts_ts
>   	  || !processing_template_parmlist)
>   	{
> -	  error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
> -	  inform (DECL_SOURCE_LOCATION (con), "concept defined here");
> +	  if (!tentative)
> +	    {
> +	      error_at (loc, "%qE does not constrain a type", DECL_NAME (con));
> +	      inform (DECL_SOURCE_LOCATION (con), "concept defined here");
> +	    }
>   	  return error_mark_node;
>   	}
>       }
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C b/gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C
> new file mode 100644
> index 00000000000..1507c604920
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-pr85846.C
> @@ -0,0 +1,5 @@
> +// PR c++/85846
> +// { dg-do compile { target c++20 } }
> +
> +template<int = 0> concept Foo = true;
> +bool f(Foo<>);
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C b/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
> new file mode 100644
> index 00000000000..c8407cdf7cd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-pr98394.C
> @@ -0,0 +1,14 @@
> +// PR c++/98394
> +// { dg-do compile { target c++20 } }
> +
> +template<int...>
> +concept C = true;
> +
> +template<int, int>
> +concept D = true;
> +
> +int main() {
> +  if (C<1>); // { dg-bogus "does not constrain a type" }
> +  if (D<1>); // { dg-error "wrong number of template arguments" }
> +	     // { dg-bogus "does not constrain a type" "" { target *-*-* } .-1 }
> +}
>