New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amend RFC 2175 to support for loops and leading vert #2530

Merged
merged 1 commit into from Sep 10, 2018

Conversation

Projects
None yet
4 participants
@Centril
Contributor

Centril commented Aug 24, 2018

This PR fixes RFC 2175 to also:

  • include leading | (already in match expressions)
  • include for loops (where the or-pattern must be irrefutable as in let bindings).

This is more or less a consistency bug-fix.
Elaboration on grammatical consistency and simplification in #2530 (comment).

@Centril Centril added the T-lang label Aug 24, 2018

@Centril Centril self-assigned this Aug 24, 2018

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Aug 24, 2018

Member

I agree that this RFC extended irrefutable patterns to allow |, so it makes sense for that to be the case in for too, not just let. But that's not exactly just a typo, so let's get more eyes on it:

@rfcbot fcp merge

Member

scottmcm commented Aug 24, 2018

I agree that this RFC extended irrefutable patterns to allow |, so it makes sense for that to be the case in for too, not just let. But that's not exactly just a typo, so let's get more eyes on it:

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Aug 24, 2018

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Aug 24, 2018

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 25, 2018

Member

With my style team hat on, I don't think we should recommend using leading vert in any of these places. However, I don't have any objection allowing this if it drastically simplify the grammar. That said, @Centril, can you please show how this simplifies the grammar enough to be worthwhile?

Member

joshtriplett commented Aug 25, 2018

With my style team hat on, I don't think we should recommend using leading vert in any of these places. However, I don't have any objection allowing this if it drastically simplify the grammar. That said, @Centril, can you please show how this simplifies the grammar enough to be worthwhile?

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Aug 26, 2018

Contributor

In full agreement wrt. style recommendations :)

That said, @Centril, can you please show how this simplifies the grammar enough to be worthwhile?

Certainly! Here goes a review of grammatical simplifications:

The status quo (i.e. with RFC 2071) is that we have (formulated for maximum clarity):

// Important bits:

let_pat : pat ('|' pat)* ;
match_pat : '|'? let_pat ;
for_pat : pat ;

// Where they are used:

expr_if_let : IF LET let_pat '=' expr '{' block '}' else_tail ? ;
expr_while_let : maybe_label WHILE LET let_pat '=' expr '{' block '}' ;
let : LET let_pat maybe_ty_ascription maybe_init_expr ';' ;

expr_for : maybe_label FOR for_pat IN expr_nostruct block ;

match_arm : maybe_outer_attrs match_pat (IF expr_nostruct)? FAT_ARROW ;

// Details of `match ... { .. }`:

expr_match : MATCH expr_nostruct '{' match_clause* nonblock_match_clause? '}' ;
match_clause : nonblock_match_clause ',' | block_match_clause ','? ;
nonblock_match_clause : match_arm (nonblock_expr | block_expr_dot) ;
block_match_clause : match_arm (block | block_expr) ;

what this PR does is unify first bits of the grammar so that we get:

let_pat   : '|'? pat ('|' pat)* ;
match_pat : '|'? pat ('|' pat)* ;
for_pat   : '|'? pat ('|' pat)* ;

since these are all equivalent, our grammar becomes just:

pats_or : '|'? pat ('|' pat)* ;

expr_if_let : IF LET pats_or '=' expr '{' block '}' else_tail ? ;
expr_while_let : maybe_label WHILE LET pats_or '=' expr '{' block '}' ;
let : LET pats_or maybe_ty_ascription maybe_init_expr ';' ;

expr_for : maybe_label FOR pats_or IN expr_nostruct block ;

match_arm : maybe_outer_attrs pats_or (IF expr_nostruct)? FAT_ARROW ;

// as before...

I would not call this simplification drastic, but it is simplification that makes the language have less corner cases and thus makes it easier for users to understand. Another benefit is that macros in particular have an easier time when you avoid syntactic corner cases.

As a note about style, observe that rustfmt already will convert:

match foo {
    | A => {},
    | B => {},
    | C => {},
}

into:

match foo {
    A => {},
    B => {},
    C => {},
}

The rustfmt tool should, for consistency, apply this rule for any leading | whether it be for match, if let, while let, let, for.

Contributor

Centril commented Aug 26, 2018

In full agreement wrt. style recommendations :)

That said, @Centril, can you please show how this simplifies the grammar enough to be worthwhile?

Certainly! Here goes a review of grammatical simplifications:

The status quo (i.e. with RFC 2071) is that we have (formulated for maximum clarity):

// Important bits:

let_pat : pat ('|' pat)* ;
match_pat : '|'? let_pat ;
for_pat : pat ;

// Where they are used:

expr_if_let : IF LET let_pat '=' expr '{' block '}' else_tail ? ;
expr_while_let : maybe_label WHILE LET let_pat '=' expr '{' block '}' ;
let : LET let_pat maybe_ty_ascription maybe_init_expr ';' ;

expr_for : maybe_label FOR for_pat IN expr_nostruct block ;

match_arm : maybe_outer_attrs match_pat (IF expr_nostruct)? FAT_ARROW ;

// Details of `match ... { .. }`:

expr_match : MATCH expr_nostruct '{' match_clause* nonblock_match_clause? '}' ;
match_clause : nonblock_match_clause ',' | block_match_clause ','? ;
nonblock_match_clause : match_arm (nonblock_expr | block_expr_dot) ;
block_match_clause : match_arm (block | block_expr) ;

what this PR does is unify first bits of the grammar so that we get:

let_pat   : '|'? pat ('|' pat)* ;
match_pat : '|'? pat ('|' pat)* ;
for_pat   : '|'? pat ('|' pat)* ;

since these are all equivalent, our grammar becomes just:

pats_or : '|'? pat ('|' pat)* ;

expr_if_let : IF LET pats_or '=' expr '{' block '}' else_tail ? ;
expr_while_let : maybe_label WHILE LET pats_or '=' expr '{' block '}' ;
let : LET pats_or maybe_ty_ascription maybe_init_expr ';' ;

expr_for : maybe_label FOR pats_or IN expr_nostruct block ;

match_arm : maybe_outer_attrs pats_or (IF expr_nostruct)? FAT_ARROW ;

// as before...

I would not call this simplification drastic, but it is simplification that makes the language have less corner cases and thus makes it easier for users to understand. Another benefit is that macros in particular have an easier time when you avoid syntactic corner cases.

As a note about style, observe that rustfmt already will convert:

match foo {
    | A => {},
    | B => {},
    | C => {},
}

into:

match foo {
    A => {},
    B => {},
    C => {},
}

The rustfmt tool should, for consistency, apply this rule for any leading | whether it be for match, if let, while let, let, for.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Aug 29, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Aug 29, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 8, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

rfcbot commented Sep 8, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment