Skip to content
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

Allow use of pipe operator in patterns. #1882

Closed
wants to merge 3 commits into from
Closed

Allow use of pipe operator in patterns. #1882

wants to merge 3 commits into from

Conversation

XAMPPRocky
Copy link
Member

@XAMPPRocky XAMPPRocky commented Feb 3, 2017

@petrochenkov
Copy link
Contributor

s/sub expressions/patterns/

@petrochenkov
Copy link
Contributor

I think you also have to introduce pattern grouping with parens () to disambiguate something like a @ PAT1 | PAT2.

@Nemo157
Copy link
Member

Nemo157 commented Feb 3, 2017

As an example this currently works:

match a {
    f @ 'b' | f @ 'c' => foo(f),
    _ => (),
}

and this currently fails because of f not being bound in the second pattern:

match a {
    f @ 'b' | 'c' => foo(f),
    _ => (),
}

but if 'b' | 'c' was itself a pattern then it could be possible to do something like

match a {
    f @ ('b' | 'c') => foo(f),
    _ => (),
}

to bind f to whatever matched the pattern 'b' | 'c' without having to bind in each sub-pattern individually


## Summary
this RFC proposes allowing the `|` operator to be used within patterns in match
statements, to allow for pattern matching with less boilerplate.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should apply to all patterns, not just patterns in match statements.

}
```

The solution to this would be to allow for `|` to be used within tuples. This
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be restricted to tuple patterns.

@mbrubeck
Copy link
Contributor

mbrubeck commented Feb 3, 2017

I think a better way to modify the grammar might look something like this:

Remove the pats_or nonterminal, and replace it with pat in the match expression grammar:

nonblock_match_clause
: maybe_outer_attrs pat maybe_guard FAT_ARROW nonblock_expr
| maybe_outer_attrs pat maybe_guard FAT_ARROW full_block_expr;

block_match_clause
: maybe_outer_attrs pat maybe_guard FAT_ARROW block;

Change the top-level pat grammar to allow one or more patterns separated by |:

pat
: single_pat
| pat '|' single_pat;

And add a new single_pat rule that matches the old pat rule, with the possible addition of an alternation enclosed in parentheses:

single_pat
: UNDERSCORE
| '&' pat
| '&' MUT pat
// ...
| '(' pat '|' single_pat ')'
;

(There might be some ambiguity to resolve between that last rule and tuple patterns.)

@XAMPPRocky XAMPPRocky changed the title Allow use of pipe operator in sub expressions. Allow use of pipe operator in patterns. Feb 3, 2017
@aturon aturon added the T-lang Relevant to the language team, which will review and decide on the RFC. label Feb 3, 2017
@sgrif
Copy link
Contributor

sgrif commented Feb 8, 2017

I am in favor of this, but this was also proposed last year. Have the reasons that was postponed changed/been addressed?

(Note: WRT real world impact, I don't have links off-hand but I have run into wanting this 3-4 times in Diesel in the past 3 months or so. I'm sure I can dig up where I wanted this if needed)

@Ericson2314
Copy link
Contributor

Well, for one, the MIR is done now.

The Haskell community is discussing this right now, btw: ghc-proposals/ghc-proposals#43 (comment). http://gallium.inria.fr/%7Escherer/research/ambiguous_pattern_variables/ml_workshop_2016.abstract.pdf was especially useful and cited in that thread.

@XAMPPRocky
Copy link
Member Author

@petrochenkov @Nemo157 Why would the () be necessary? Wouldn't foo @ '\n' | '\r' => {} be ideal syntax?

@sgrif Well with the Rust roadmap, and I think this would fit well into lowering the learning curve for Rust, as it provides a much more intuitive syntax for pattern matching.

@petrochenkov
Copy link
Contributor

@Aaronepower
@Nemo157 gave an example of why introducing | as a high priority operator is a breaking change.

f @ 'b' | f @ 'c'

is currently a valid match arm and it's grouped as (f @ 'b') | (f @ 'c').
If | has high priority, then the example is parsed as an invalid pattern f @ ('b' | f @ 'c').

I've just grepped rustc and found a few examples in real code:

ref t @ TyUint(_) | ref t @ TyInt(_)
def @ None | def @ Some(Def::Local(_))
seq @ TokenTree::Delimited(..) | seq @ TokenTree::Token(_, DocComment(..))
// etc

&PAT | &PAT has this problem as well.

In general, it would be natural for | to have low priority, both for compatibility, and because binary/sequence operators have lower priority than unary operators in both expression and type grammars.

However, if | has low priority, then we cannot express foo @ ('\n' | '\r'), which can be useful. This can be fixed in the same way as it's fixed in expression and type grammars - with parens.

@solson solson mentioned this pull request Feb 11, 2017
@aturon
Copy link
Member

aturon commented Apr 29, 2017

This RFC is missing its motivation section, which is probably the most important section of an RFC!

While this feature seems potentially nice, there is often a lot of unanticipated detail work necessary to push changes like this through (e.g, @petrochenkov is already turning up some interesting issues). Before we spend time hashing out all those details, there needs to be a clear case for why this is important to prioritize right now.

@joshtriplett
Copy link
Member

One corner case in treating this as pure syntactic sugar: what if I write this:

match a_big_tuple {
(0|1, 0|1, 0|1, 0|1, 0|1, 0|1, ...) => ...

(Or anything else where you embed multiple alternations inside a pattern.) The obvious syntactic-sugar expansion would turn that into 2^n patterns. (The compiler ought to be able to optimize that in cases like this, but it could result in a large intermediate state if not handled specially.)

Does this seem like a problem?

@mglagla
Copy link
Contributor

mglagla commented May 22, 2017

Small data point: i am making a small tetris clone using the sdl2-crate. In my code i have a match expression which looks like this, if i understand it right:

for event in event_pump.poll_iter() {
    use sdl2::event::Event;
    use sdl2::keyboard::Keycode::{Escape, Q};
    match event {
        Event::KeyDown {
            keycode: Some(Escape),
            ..
        }
        | Event::KeyDown {
            keycode: Some(Q),
            ..
        } => break 'game,
        _ => {},
    }

With this RFC i would be able to express more concisely like this:

for event in event_pump.poll_iter() {
    use sdl2::event::Event;
    use sdl2::keyboard::Keycode::{Escape, Q};
    match event {
        Event::KeyDown {
            keycode: Some(Escape | Q),
            ..
        } => break 'game,
        _ => {},
    }

In my opinion, this is more clear and thus less error-prone.

@aturon
Copy link
Member

aturon commented Jul 25, 2017

I'm going to move to close this RFC as-is; the basic issues around the RFC content remain. While the lang team is potentially open to an enhancement along these lines, we need a full-blown RFC before proceeding.

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 25, 2017

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

No concerns currently listed.

Once these reviewers reach consensus, 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 rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jul 25, 2017
@joshtriplett
Copy link
Member

@Aaronepower To further clarify: if you're interested in seeing this go into Rust, that's still possible; it just needs further expansion into a full RFC that addresses all the issues raised in this thread.

I'm personally interested in seeing this.

@liigo
Copy link
Contributor

liigo commented Jul 28, 2017

I'm confused. What's the type of expression a | b? and what is its value?

@eddyb
Copy link
Member

eddyb commented Jul 28, 2017

@liigo It's not an expression, it's a pattern for "try pattern a and if that doesn't match try b".

@nrc
Copy link
Member

nrc commented Jul 31, 2017

Agree we should close. Although I do think it would be a good feature to have, I don't think it should be high priority right now.

@aturon
Copy link
Member

aturon commented Aug 9, 2017

(Checking off for @pnkfelix, who is away)

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 9, 2017

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

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Aug 9, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 19, 2017

The final comment period is now complete.

@liigo
Copy link
Contributor

liigo commented Aug 22, 2017

It's not an expression, it's a pattern for "try pattern a and if that doesn't match try b".

According to the refference:

The type of the patterns must equal the type of the head expression.

the pattern Some('a' | 'b')'s type must be Option(char). so the type of 'a' | 'b' should be char?

@Centril
Copy link
Contributor

Centril commented Aug 24, 2017

@nrc I disagree - it helps ergonomics a lot and should fit in well with the 2017 ergonomics initiative.

@liigo
Copy link
Contributor

liigo commented Aug 26, 2017

I agree it helps ergonomics, but a little.

@aturon
Copy link
Member

aturon commented Aug 26, 2017

Closing, as per FCP. Thanks @Aaronepower for the RFC!

@mbrubeck
Copy link
Contributor

New RFC for this feature: #2535.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.