[C++] PR 71140 ("[concepts] ill-formed nested-requirement lacking a semicolon not rejected")

Message ID d9f3ed05-06c4-166b-6ac2-2fe573c99297@oracle.com
State New
Headers show
Series
  • [C++] PR 71140 ("[concepts] ill-formed nested-requirement lacking a semicolon not rejected")
Related show

Commit Message

Paolo Carlini Oct. 3, 2018, 12:18 p.m.
Hi,

a simple issue, we weren't correctly implementing 7.5.7.4 on the 
terminating semicolon. Tested x86_64-linux.

Thanks, Paolo.

///////////////////
/cp
2018-10-03  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/71140
	* parser.c (cp_parser_compound_requirement): Require a terminating
	semicolon, per 7.5.7.4.

/testsuite
2018-10-03  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/71140
	* g++.dg/concepts/pr71140.C: New.
	* g++.dg/concepts/pr67595.C: Adjust.

Comments

Paolo Carlini Oct. 11, 2018, 3:42 p.m. | #1
Hi,

already pinging this because it seems rather straightforward to me...

On 03/10/18 14:18, Paolo Carlini wrote:
> Hi,

>

> a simple issue, we weren't correctly implementing 7.5.7.4 on the 

> terminating semicolon. Tested x86_64-linux.


     https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00173.html

Thanks! Paolo.
Jason Merrill Oct. 11, 2018, 6:36 p.m. | #2
On Wed, Oct 3, 2018 at 8:18 AM Paolo Carlini <paolo.carlini@oracle.com> wrote:
> a simple issue, we weren't correctly implementing 7.5.7.4 on the

> terminating semicolon. Tested x86_64-linux.


If the missing semicolon is followed by }, let's allow it with a pedwarn.

Jason
Paolo Carlini Oct. 11, 2018, 8:30 p.m. | #3
Hi,

On 11/10/18 20:36, Jason Merrill wrote:
> On Wed, Oct 3, 2018 at 8:18 AM Paolo Carlini <paolo.carlini@oracle.com> wrote:

>> a simple issue, we weren't correctly implementing 7.5.7.4 on the

>> terminating semicolon. Tested x86_64-linux.

> If the missing semicolon is followed by }, let's allow it with a pedwarn.


I see. Unfortunately we have yet another issue in this area: our 
requirement-list, as parsed in cp_parser_requirement_list, isn't the 
same as requirement-seq in the working draft:

    requirement-list:
        requirement
        requirement-list ';' requirement[opt]

vs

    requirement-seq
        requirement
        requirement-seq requirement

thus, in particular, we accept a single requirement either terminated 
with semicolon or not (which explains why we have c++/71139 and 
c++/71140 es accept invalid but we don't reject anything valid). We do 
this together with correctly enforcing the terminating semicolon for 
simple-requirement and type-requirement and not enforcing it for 
compound-requirement and nested-requirement (per the bugs at issue). 
Sort of a mess. I don't know how much we care about backward 
compatibility with the TS, etc, in this area (*) but it would be *so* 
nice to implement requirement-seq too correctly and simply require the 
terminating semicolon for all the 4 kinds of requirements...

Thanks, Paolo.

(*) In principle we could even imagine a legacy Concepts TS mode - by 
and large frozen, the way of the library TR1 - and a proper C++20 
concepts mode, useful for much more serious issues too. No idea if 
somebody already discussed this?!?
Jason Merrill Oct. 12, 2018, 12:55 p.m. | #4
On Thu, Oct 11, 2018 at 4:31 PM Paolo Carlini <paolo.carlini@oracle.com> wrote:
>

> On 11/10/18 20:36, Jason Merrill wrote:

> > On Wed, Oct 3, 2018 at 8:18 AM Paolo Carlini <paolo.carlini@oracle.com> wrote:

> >> a simple issue, we weren't correctly implementing 7.5.7.4 on the

> >> terminating semicolon. Tested x86_64-linux.

> > If the missing semicolon is followed by }, let's allow it with a pedwarn.

>

> I see. Unfortunately we have yet another issue in this area: our

> requirement-list, as parsed in cp_parser_requirement_list, isn't the

> same as requirement-seq in the working draft:

>

>     requirement-list:

>         requirement

>         requirement-list ';' requirement[opt]

>

> vs

>

>     requirement-seq

>         requirement

>         requirement-seq requirement

>

> thus, in particular, we accept a single requirement either terminated

> with semicolon or not (which explains why we have c++/71139 and

> c++/71140 es accept invalid but we don't reject anything valid). We do

> this together with correctly enforcing the terminating semicolon for

> simple-requirement and type-requirement and not enforcing it for

> compound-requirement and nested-requirement (per the bugs at issue).

> Sort of a mess. I don't know how much we care about backward

> compatibility with the TS, etc, in this area (*) but it would be *so*

> nice to implement requirement-seq too correctly and simply require the

> terminating semicolon for all the 4 kinds of requirements...


And allowing a missing semicolon at the end of the list/sequence (with
a pedwarn) smooths over the difference.

> (*) In principle we could even imagine a legacy Concepts TS mode - by

> and large frozen, the way of the library TR1 - and a proper C++20

> concepts mode, useful for much more serious issues too. No idea if

> somebody already discussed this?!?


That seems like a lot of hassle for little use; we can't just put it
in a different file like the library can, and users of concepts will
want to the up-to-date implementation.  If someone wants to compare
against the current TS implementation, they can use GCC 8.

BTW, I would discourage you from messing much with the concepts code
at this point, as a major overhaul should be coming soon.

Jason
Andrew Sutton Oct. 12, 2018, 1:32 p.m. | #5
>

> BTW, I would discourage you from messing much with the concepts code

> at this point, as a major overhaul should be coming soon.

>


Major overhaul:

https://github.com/asutton/gcc (branch is concepts; we're about 2 weeks
back from trunk).

Unfortunately, I we haven't been following GCC commit discipline, and
there's a bunch of dead/legacy code that I need to clean up. And of course
some regressions. I wanted to spend the weekend on this and forward a
cleaner version next week. But no time is as good as the present it seems.

This fork reimplements concepts as currently specified in the WD (for the
most part). It also preserves TS syntax (but not behavior), although there
are certainly going to be some new bugs.

-std=c++2a turns on concepts by default (sets -fconcepts)
-fconcepts-ts can be additionally specified to enable TS extensions
(abbreviated fn templates, etc).
-fconcepts on its own gives you (should give you) TS syntax with C++20
semantics and no C++20 features.

Here's what's changed:
- new requires clause syntax as required in the WD (-fconcepts-ts will
change this back to the TS syntax)
- concept bool is now a warning, although (IIRC) disabled with
-fconcepts-ts. Function and variable concepts live on.
- concepts are now their own kind of declaration (CONCEPT_DECL). That was a
big change.
- now only 3 kinds of constraints: conj, disj, and pred (should be
atomic, also needs a dead code cleanup).
- constraints on declarations are represented as expressions -- no
normalization until later
- associated constraints are only instantiated when checked -- no premature
substitution
- new implementaiton of satisfaction, does not require ahead-of-time
normalization
- new implementation of normalization (fewer nodes, smaller impl)
- atomic constraint comparison based on expr identity/parameter mapping
- complete rewrite of subsumption (new comparison model invalidated some
assumptions in the old impl)
- constrained decls differentiated by syntax of constraints (not
equivalence)
- moved the concepts testsuite into c++2a directory as a vetting/curating
process, new 2a tests, new ts-compatability tests

There are some bugs and regressions. I know for a fact that we've broken
partial specialization of variable templats, but I'm really not sure how.
There's also probably a bug in the constraint comparison implementation
that affects partial ordering. More testing is needed. I mostly ignored the
TS support while updating to the WD semantics, so that's been a little
buggy when I brought it back online. Also, sometimes diagnostics aren't
emitted correctly.

I'm not quite sure how to proceed with submitting this patch. Once I made
the decision to make concepts their own kind of declaration, the idea of
sending small patches went right out the window.
Paolo Carlini Oct. 12, 2018, 4:42 p.m. | #6
Hi,

On 12/10/18 14:55, Jason Merrill wrote:
> BTW, I would discourage you from messing much with the concepts code

> at this point, as a major overhaul should be coming soon.


Agreed: we have good news from Andrew!

By the way, over the last days, outside trivial parsing issues, I 
noticed a rather annoying ICE on valid which should be rather easy to 
fix and has got **tons** of duplicates, just fyi (and Andrew's 
information): c++/67147, c++/87441, c++/80746, c++/79759, c++/86269 and 
more!

Cheers, Paolo.
Jason Merrill Oct. 25, 2018, 7:09 p.m. | #7
On 10/12/18 9:32 AM, Andrew Sutton wrote:
>>

>> BTW, I would discourage you from messing much with the concepts code

>> at this point, as a major overhaul should be coming soon.

> 

> Major overhaul:

> 

> https://github.com/asutton/gcc (branch is concepts; we're about 2 weeks

> back from trunk).


Awesome!

> Unfortunately, I we haven't been following GCC commit discipline, and

> there's a bunch of dead/legacy code that I need to clean up. And of course

> some regressions. I wanted to spend the weekend on this and forward a

> cleaner version next week. But no time is as good as the present it seems.

> 

> This fork reimplements concepts as currently specified in the WD (for the

> most part). It also preserves TS syntax (but not behavior), although there

> are certainly going to be some new bugs.

> 

> -std=c++2a turns on concepts by default (sets -fconcepts)

> -fconcepts-ts can be additionally specified to enable TS extensions

> (abbreviated fn templates, etc).

> -fconcepts on its own gives you (should give you) TS syntax with C++20

> semantics and no C++20 features.

> 

> Here's what's changed:

> - new requires clause syntax as required in the WD (-fconcepts-ts will

> change this back to the TS syntax)

> - concept bool is now a warning, although (IIRC) disabled with

> -fconcepts-ts. Function and variable concepts live on.

> - concepts are now their own kind of declaration (CONCEPT_DECL). That was a

> big change.


Can you say a bit about why that was better than continuing to use VAR_DECL?

> - now only 3 kinds of constraints: conj, disj, and pred (should be

> atomic, also needs a dead code cleanup).

> - constraints on declarations are represented as expressions -- no

> normalization until later

> - associated constraints are only instantiated when checked -- no premature

> substitution

> - new implementaiton of satisfaction, does not require ahead-of-time

> normalization

> - new implementation of normalization (fewer nodes, smaller impl)

> - atomic constraint comparison based on expr identity/parameter mapping

> - complete rewrite of subsumption (new comparison model invalidated some

> assumptions in the old impl)

> - constrained decls differentiated by syntax of constraints (not

> equivalence)

> - moved the concepts testsuite into c++2a directory as a vetting/curating

> process, new 2a tests, new ts-compatability tests

> 

> There are some bugs and regressions. I know for a fact that we've broken

> partial specialization of variable templats, but I'm really not sure how.

> There's also probably a bug in the constraint comparison implementation

> that affects partial ordering. More testing is needed. I mostly ignored the

> TS support while updating to the WD semantics, so that's been a little

> buggy when I brought it back online. Also, sometimes diagnostics aren't

> emitted correctly.

> 

> I'm not quite sure how to proceed with submitting this patch. Once I made

> the decision to make concepts their own kind of declaration, the idea of

> sending small patches went right out the window.


Yeah, don't worry about trying to send small patches.  I don't mind 
reviewing what's on the branch, though at least the final patch should 
be sent to the list for archival.

What feedback are you looking for at this point?

Jason
Andrew Sutton Oct. 31, 2018, 6:34 p.m. | #8
Sorry for the slow reply. I've been stuck working on some other projects.


> Can you say a bit about why that was better than continuing to use

> VAR_DECL?

>


I wanted to make sure that we avoid normal VAR_DECL processing routines, so
we don't e.g., slip into a function where we might try to generate an
address for a concept.


Yeah, don't worry about trying to send small patches.  I don't mind
> reviewing what's on the branch, though at least the final patch should

> be sent to the list for archival.

>

> What feedback are you looking for at this point?

>


Mostly anything that would obviously prevent or cause problems merging in
the near future.

I'll try to keep the asutton/gcc fork on github rebased on trunk so there
shouldn't be too many merge issues.
Jason Merrill Dec. 18, 2018, 1:03 a.m. | #9
On Wed, Oct 31, 2018 at 2:34 PM Andrew Sutton <andrew.n.sutton@gmail.com> wrote:
>

> Sorry for the slow reply. I've been stuck working on some other projects.

>>

>> Can you say a bit about why that was better than continuing to use VAR_DECL?

>

> I wanted to make sure that we avoid normal VAR_DECL processing routines, so we don't e.g., slip into a function where we might try to generate an address for a concept.

>

>> Yeah, don't worry about trying to send small patches.  I don't mind

>> reviewing what's on the branch, though at least the final patch should

>> be sent to the list for archival.

>>

>> What feedback are you looking for at this point?

>

> Mostly anything that would obviously prevent or cause problems merging in the near future.

>

> I'll try to keep the asutton/gcc fork on github rebased on trunk so there shouldn't be too many merge issues.


Any updates?

Jason

Patch

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 264805)
+++ cp/parser.c	(working copy)
@@ -26110,10 +26114,11 @@  cp_parser_compound_requirement (cp_parser *parser)
   return finish_compound_requirement (expr, type, noexcept_p);
 }
 
-/* Parse a nested requirement. This is the same as a requires clause.
+/* Parse a nested requirement. This is the same as a requires clause followed
+   by a semicolon.
 
    nested-requirement:
-     requires-clause */
+     requires-clause ';' */
 static tree
 cp_parser_nested_requirement (cp_parser *parser)
 {
@@ -26121,6 +26126,9 @@  cp_parser_nested_requirement (cp_parser *parser)
   tree req = cp_parser_requires_clause (parser);
   if (req == error_mark_node)
     return error_mark_node;
+
+  cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
+
   return finish_nested_requirement (req);
 }
 
Index: testsuite/g++.dg/concepts/pr67595.C
===================================================================
--- testsuite/g++.dg/concepts/pr67595.C	(revision 264805)
+++ testsuite/g++.dg/concepts/pr67595.C	(working copy)
@@ -2,7 +2,7 @@ 
 
 template <class X> concept bool allocatable = requires{{new X}->X * };
 template <class X> concept bool semiregular = allocatable<X>;
-template <class X> concept bool readable = requires{requires semiregular<X>};
+template <class X> concept bool readable = requires{requires semiregular<X>;};
 template <class> int weak_input_iterator = requires{{0}->readable};
 template <class X> bool input_iterator{weak_input_iterator<X>}; // { dg-warning "narrowing conversion" }
 template <class X> bool forward_iterator{input_iterator<X>};
Index: testsuite/g++.dg/concepts/pr71140.C
===================================================================
--- testsuite/g++.dg/concepts/pr71140.C	(nonexistent)
+++ testsuite/g++.dg/concepts/pr71140.C	(working copy)
@@ -0,0 +1,8 @@ 
+// { dg-do compile { target c++14 } }
+// { dg-additional-options "-fconcepts" }
+
+template<typename T>
+concept bool C = requires(T t) {
+  requires true  // { dg-error "expected .\;. before .\}. token" }
+};
+static_assert(C<int>, "");