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 an optional vert at the beginning of a match branch #1925

Merged
merged 12 commits into from
Aug 26, 2017

Conversation

mdinger
Copy link
Contributor

@mdinger mdinger commented Feb 24, 2017

This was written to provide a formal proposal to allow a leading | at the beginning of a match as was discussed in #1745. In that thread, it was previously stated the language team is in favor of this change.

Some of the sections I don't know anything about but I tried to fill in what I knew. For such a small topic, this form seems excessive and so a lot of the text seems fairly brief to me. Also, I'm not actually sure all the examples are needed but I thought it wouldn't hurt to add them.

In brief, this is a proposal that the following match syntax be legal:

enum E { A, B, C, D}
use E::*;

match foo {
| A
| B => (),
| C
| D => (),
}

Rendered

@joshtriplett
Copy link
Member

@mdinger One nit: in example B, why not include the vert to make all the branches match, rather than calling attention to the lack of one with a comment?

More significantly, the "detailed design" section needs some details. In that section, you should discuss modifying the grammar for match arms to allow an optional single vertical bar | before the pattern.

@mdinger
Copy link
Contributor Author

mdinger commented Feb 24, 2017 via email

@mdinger
Copy link
Contributor Author

mdinger commented Feb 24, 2017 via email

@joshtriplett
Copy link
Member

@mdinger In that case, you may want to show a "before" and "after" for example B: the inconsistent version, and the consistent version.

@est31
Copy link
Member

est31 commented Feb 24, 2017

I think for consistency it should also allow leading ,, like it was suggested in the issue. Still, I think neither of the two notations should become official rust style.

@keeperofdakeys
Copy link

keeperofdakeys commented Feb 24, 2017

I'm finding it a bit hard to read the examples when I glance over them. The indenting makes it hard see where each pattern starts or ends, and to spot where each match occurs.

So personally I don't think it would fit with the language unless it used a style like this:

match A {
    | A
    | B => println!("Give me A | B!"),

    | C
    | D => println!("Give me C | D!"),
}

@nikomatsakis
Copy link
Contributor

I very much appreciated the ability to have leading | in Ocaml (I've never used F#, though I've often wanted to) and I've also wondered why we don't support them in Rust from time to time, but never bothered to propose it. So I'm basically 👍 on this proposal.

That said, if we're going to accept leading | but not recommend you use them, it seems less exciting. I'd still be in favor, because macros and so on, but less so. Therefore, I'm also interested in the feedback of the "format" team -- I don't know best to cc them, though, so I'll just cc a few members that I can think of off the top of my head: @nrc, @solson, @joshtriplett. I'm not really following a lot of the discussions there, but last time I checked in, it seemed like we were aiming for a "less visual" more "block-oriented" formatting strategy. In that case, I feel like leading | might fit in there. (Also, I remember that rustfmt has some "issues" around match arms, maybe this helps to resolve them.)

@joshtriplett
Copy link
Member

joshtriplett commented Feb 24, 2017

@keeperofdakeys Allowing leading vert doesn't specify whether match arms should have a leading indent in addition to the | or not.

@nikomatsakis @rust-lang-nursery/style, because for some reason we put fmt-rfcs under rust-lang-nursery. Also, for some reason that doesn't seem to actually highlight and work; do cross-organization team references not work? If they don't, all the more reason to consider moving the style team and fmt-rfcs ro rust-lang.

Given the availability of leading |, personally, I'd be inclined to recommend using it for any match pattern that contains an alternation. For consistency, I'd personally suggest using it on the rest of the match patterns if any pattern in the match has an alternation in it. (If every match arm consists of a single pattern, I don't think using it makes sense to prefix them all with |.)

@mdinger
Copy link
Contributor Author

mdinger commented Feb 24, 2017

To clarify, I never intended for this to be a proposal to change the recommended way matches are formatted. Stylistic topics are enormously difficult to push through to communities because everyone has their own preferences, which is okay and not problematic.

My intent was to showcase that allowing the leading vert could be a very valuable addition to some people without hindering understanding for others. If you don't like it, fine, don't use it but that is no reason to hinder others if they like it. The best I probably hoped for was for the vert to be allowed and an example in the docs somewhere include the style so that people could decide if it is good. That is, I don't always wait for rust-lang.org to tell me if I like a coding style. Sometimes, I decide for myself (it's nice to have a menu to show me what my options are though - I'd never have discovered this style if I hadn't seen it in F#).

@keeperofdakeys Styles are mutable. Your entire history in Rust and every other language colors your perception of what looks good. My history works the same way. I don't think I can categorically say that any simple style change of this sort could never look like Rust.


Taking an example from regex: can you identify which lines are match arms in less than a second in the following example?

            match c {
                '\\' => return match self.c() {
                            Some('#') => {self.advance(); Some('#')}
                            _ => Some('\\')
                        },
                '#'  => loop {
                            match self.c() {
                                Some(c) => {
                                    self.advance();
                                    if c == '\n' {
                                        break;
                                    }
                                },
                                None => return None
                            }
                        },
                _    => if !c.is_whitespace() {return Some(c);}
            }

Repeating the example, if I told you every arm started with a vert, could you do it faster? I can. I think they highlight them:

            match c {
                | '\\' => return match self.c() {
                            | Some('#') => {self.advance(); Some('#')}
                            | _ => Some('\\')
                        },
                | '#'  => loop {
                            match self.c() {
                                | Some(c) => {
                                    self.advance();
                                    if c == '\n' {
                                        break;
                                    }
                                },
                                | None => return None
                            }
                        },
                _    => if !c.is_whitespace() {return Some(c);}
            }

@keeperofdakeys
Copy link

I actually think this would be a good addition to the language, I just wanted to point out that I found the RFC examples a bit hard to read. People are free to use a feature how they want, but I think that RFC examples are a good indication of how a feature could/should be used. Ultimately a style rfc is the way to decide how it should be styled though.

@mdinger I agree, it's easier to identify match arms in the second.

@withoutboats withoutboats added the T-lang Relevant to the language team, which will review and decide on the RFC. label Feb 25, 2017
@withoutboats
Copy link
Contributor

When I have more variants than can fit on a line I write my matches like this:

match foo {
    Foo::Bar
    | Foo::Baz
    | Foo::Quux,
    => Whatever::One,
    Foo::Spam
    | Foo::Eggs,
    => Whatever::Two,
}

I would probably write this after this RFC:

match foo {
    | Foo::Bar
    | Foo::Baz
    | Foo::Quux,
    => Whatever::One,
    | Foo::Spam
    | Foo::Eggs,
    => Whatever::Two,
}

When I don't have a situation like this (which is most of the time), I make sure my matchrockets all align.

Most people write their matches differently from me, but I would still like to have the grammar be more flexible.

However, supporting | within patterns is more important, and so I want to make sure this is square with that proposal.

@durka
Copy link
Contributor

durka commented Feb 26, 2017

Unfortunately the closing braces in the same column kills it for me right away, along with the more minor issue @keeperofdakeys pointed out where it becomes less clear which lines are the beginnings of patterns and which are not.

@mdinger
Copy link
Contributor Author

mdinger commented Feb 26, 2017

@joshtriplett I added some updates to try to address your comments.
@keeperofdakeys I don't know how I can make the examples more clear. Branches always occur where the => points. They always have and they always will. This appears to be finding the syntax unfamiliar and thus feels hard (here's something which technically should be really easy but probably will be really hard due to lack of familiarity). The only example I think is somewhat difficult to parse is the first part of Example C. I find it tricky because the indentation is always changing somewhat how the line below keeps moving. I find the second part more consistent by far and much more straightforward.

wavy

To others regarding braces, considering how syntax heavy rust is, I actually find the extra braces just a slight nitpick and isn't even that bad. Compare with the fairly common ))))) which pops up at the completion of function calls. To the functional programmer, having tons of closing parenthesis is probably even more dumb than an out of place brace considering their languages don't even really require parenthesis. If Example C was translated to F#, most of the symbols (,{}():: just vaporize). Consider how clean this looks (not valid F#):

    match name
    | Name Franks name =>
        match name
        | Franks Alice
        | Franks Brenda
        | Franks Dave => FavoriteBook
            author: "alice berkley"
            title: "Name of a popular book"
            date: 1982
        | Franks Charles
        | Franks Steve => FavoriteBook
            author: "fred marko"
            title: "We'll use a different name here"
            date: 1960
    | Name Sawyer name  =>
        match name
        | Sawyer Tom => FavoriteBook
            author: "another name"
            title: "Again we change it"
            date: 1999
        | Sawyer Sid
        | Sawyer May => FavoriteBook
            author: "again another name"
            title: "here is a different title"
            date: 1972

I'm not saying all the extra symbols need to go away but when you compare it to something like this which, for the most part, should be somewhat clear (and less heavy-handed), the debate about whether 1 or 2 different braces should be aligned isn't that important. The language is syntax heavy and we do the best we can.

@durka
Copy link
Contributor

durka commented Feb 26, 2017 via email

@mdinger
Copy link
Contributor Author

mdinger commented Feb 26, 2017

Okay. I think I've done my part trying to defend the stylistic part of the proposal and people who don't like it still won't like it. Having said that, I don't know where the consensus stands as far as the technical part of this being supported by the compiler. I haven't seen any comment which states making this legal is problematic in any fashion aside from something stylistic.

Let me know if I need to update the RFC in any other ways.

@dashed
Copy link

dashed commented Feb 26, 2017

If this gets approved, I hope there will be an associated rustfmt RFC to helps folks that may either approve or disapprove this style: https://github.com/rust-lang-nursery/fmt-rfcs

@nrc
Copy link
Member

nrc commented Feb 26, 2017

I don't think we would recommend them - we don't recommend a leading comma style, for example. I know @joshtriplett favours leading newlines with punctuation, e.g.,

a + b
+ c + d

// c.f.

a + b +
c + d

But that is not what is being proposed here. I can't think of anywhere we are proposing actually leading with punctuation.

In general, I would be in favour of this RFC - I usually prefer more flexibility in our syntax rather than less, but I don't think we would make this the default style. In particular, the needing a | in the single pattern case seems very weird to me.

(Do we allow trailing |? That seems useful for macros, but I don't think we'd recommend a trailing | either).

@joshtriplett
Copy link
Member

@mdinger For the sake of demonstrating the smallest incremental change, I'd suggest changing the examples so they still indent match arms as normal (four spaces in front), rather than outdenting them and treating the vert-and-space as their indent. We could evaluate the latter as a potential style change, but that leads to the issue with trailing braces. Let's not conflate that style change and associated brace indentation issue with the minimal grammar proposal.

You could add a section towards the end mentioning that potential style change, move the discussion of the brace problem from the drawbacks to that new section, and punt the actual style, arguing that the grammar change is useful independently.

@mdinger
Copy link
Contributor Author

mdinger commented Feb 27, 2017

I don't mind doing that. I might get around to it a day or so but if not then, it might be a few weeks. Might be pretty busy and don't know what my free time will be like.

@nikomatsakis
Copy link
Contributor

I think the reason to make this a recommended style would be to avoid the double indentation and so forth. But I agree it's a separate thing-- still it seems like it'd be good to take this idea into account when settling on the best indentation style for matches

@vitiral
Copy link

vitiral commented Mar 5, 2017

In elm you would do something like

match Z
    { A
    | B =>... 
    , C
    | D
   ... 
} 

Which would be legal rust syntax and make everything align.

@joshtriplett
Copy link
Member

joshtriplett commented Mar 9, 2017

In the interests of moving this forward:

I'm not seeing any objections to the idea of supporting this in the grammar. I do see some comments about what our recommended style should look like, and in particular whether or not this should replace indentation of match arms. The thing that's causing those two to be conflated is that the RFC's samples all assume a particular style when writing, and not everyone is a fan of that style. As far as I can tell, every objection I've seen is objecting to the indentation style as shown in the RFC, which I wouldn't argue with.

Does anyone object to allowing the optional vert at all? If not, I would propose that we move to accept this, and then separately determine how it might affect the default style.

(And with my style team hat on, I'd be inclined to not change the recommended indentation, but instead just add the starting vert for alignment purposes when writing a series of patterns one per line.)

@eddyb
Copy link
Member

eddyb commented Aug 28, 2017

@est31 I meant to disable them. Not someone removing their reaction.

@est31
Copy link
Member

est31 commented Aug 28, 2017

@eddyb ah right, that seems impossible indeed.

@withoutboats
Copy link
Contributor

withoutboats commented Aug 28, 2017

We definitely don't make decisions on the basis of thumb counts. Obviously they can have an impact, but we try to make decisions in a more nuanced manner. Here are reasons that I personally thought the RFC should be merged:

  • Despite the thumb count, the comments felt much more positive. What seemed clear is that while many users didn't want this syntax (and maybe wouldn't allow it in their code base), there were some users who very strongly did want this and were willing to press on it. I weighed these strong positive feelings more heavily than the mild negative feelings.

  • We tend to support a flexible grammar when possible, including things we don't consider idiomatic at all. Philosophically, this is an RFC we are inclined to accept, and there'd probably have to be a pretty strong, practical reason for us not to accept it.

  • Sort of along the lines of both of the previous points, I have difficulty imagining this having a serious negative impact on any user. You might see some match arms you don't like, maybe find them a little confusing for a moment, but I have difficulty valuing this impact over the positive impact on people who want to write their code this way.

Speaking more general, RFCs are a deliberative process, not an up or down vote. The point of the process is to seek out useful commentary from community members, and while thumbs provide some feedback for decision making, they're limited in their impact in proportion to the amount of information they contain.

@steveklabnik
Copy link
Member

@liigo Small procedural note:

To the core team, are you still keep open?

This is not a core team RFC, but a lang team one.

@jansegre
Copy link

jansegre commented Aug 31, 2017

I realize I'm a bit late, but I just saw this on TWiR. I haven't seen the following point: from what I understand one of the barriers for people to get into Rust is the syntax, and those people would probably be even more put off if they encounter this kind of style disagreement upfront (this syntax is being approved because people want to use it, and when it is stable it may end up on popular posts and even on official docs) and since they are not into Rust yet, most won't participate in the RFC process.

To summarize that, I think this has some (potentially) negative impact on people outside of Rust who don't participate on the RFC process.

Personally I just find it a bit ugly and confusing, my instinct is to expect something before a | so it looks like a condition is missing. I also think that reasonable style (placing | on the end of the previous line) was already proposed that makes the code align well. I get the flexibility argument, one could use leading semicolons for example, but I also feel that this is a stretch.

@lilianmoraru
Copy link

I just saw this in TWiR - this is a really weird syntax.....

@chris-morgan
Copy link
Member

(Somehow I missed this one in the last couple of weeks of TWiR or I would have commented earlier. I don’t really get the motivation for this and I feel the drawbacks section being empty is unreasonable, but let those pass for now.)

One concern I have about this is the impact on macro rules. As it stands, we have the pat fragment specifier which dovetails well with repeating groups to cover most of match_pat in a very simple pattern—​granted, it doesn’t support the guard ([ "if" expr ] ?), but at least it supports multiple patterns (pat [ '|' pat ] *):

$($patterns:pat)|+

If you want to support the [ "if" expr ] ? as well, it’s not too bad: you need to tack on

$(if $guard:expr)*

(Side note: we really need $(…)?.)

After this change, people writing macros that want to accept match patterns will need to prefix $(|)* to make it support the full match_pat grammar. It’s not a big deal, but it’s just one more thing that stands in the way of writing DSLs that behave like proper Rust.

You may say that trailing commas already have this problem (hence the pattern $(x:expr),* $(,)*, the $(,)* part of which I freely admit many don’t know about—so it is already a problem), but I think it’s more severe (though less common) with this one because I don’t expect many people to write match patterns with leading pipes (I never intend to), and so people writing macro libraries are much less likely to think of coping with the optional leading pipe, which will make people that like this pattern grouchy.

One of the main problems with increasing grammar complexity is in macros that wish to support or emulate the full grammar with certain variations of behaviour; unless we have a fragment specifier which handles that part of the grammar, macro rules become steadily more complex, or become incomplete (leading to frustration when the macro doesn’t work with otherwise normal Rust code).

After this RFC is implemented, full support for match patterns in a macro are going to be

$(|)* $($patterns:pat)|+ $(if $guard:expr)*

@juggle-tux
Copy link

i really dislike the idea to have many syntax options for the same think. It makes it imo harder for a beginner to understand the language since it raises questions like
“With syntax to use and why?”, “Are there any differences and if not why have the different syntax in the first place?”, “With way will run faster?” and so on

@SoniEx2
Copy link

SoniEx2 commented Mar 29, 2018

Ppl don't use

match x {
    Hello     |
    World     |
    AndThings => ...,
}

?

You know we have this same "issue" with || and && and so on, right?

Wonders how long until Rust decides to deprecate this

The only nice thing about this is that now you can "convert" C into Rust with a simple s/case [ident]:/| [ident]/ but if that's the only nice thing and everything else sucks then it's not really a good feature now is it?

@warlord500
Copy link

warlord500 commented Mar 29, 2018

@SoniEx2 i am curious as to what issue you are referring to when you said

You know we have this same "issue" with || and && and so on, right?

the second comment about converting C to rust has been true even with out this addition.
the only real improvement is it makes vert optional at match branch.

@SoniEx2
Copy link

SoniEx2 commented Mar 29, 2018

if
x
|| y
|| z
{

Are we gonna allow this next, for consistency?

if
|| x
|| y
|| z
{

I still prefer the apparently much dreaded

if
x ||
y ||
z {

I have absolutely no idea why this is so dreaded. It's even more compact!

@warlord500
Copy link

I seriously hope that no one would consider the additional bars in if as okay. also I am pretty sure
that would lead to some ambiguity in the grammar somewhere
I actually like the third better than the other two!
I wonder often that many or chains happen in code?

the best i can do now against this feature is make a Clippy lint against it.
which is better than giving support for it in most code bases

@SoniEx2
Copy link

SoniEx2 commented Mar 29, 2018

I'll take the clippy lint tbh

@H2CO3
Copy link

H2CO3 commented Apr 12, 2018

@warlord500 indeed, inside an expression, leading || means the beginning of a closure with no arguments. So if that's allowed, Rust (and we) will have a bad (as in C++ context-sensitivity level bad) grammar problem.

@liigo
Copy link
Contributor

liigo commented Apr 14, 2018

@ubsan

if people do not make their voice heard, we can't help that. The thumbs up/thumbs down thing is not a voting system, and expecting us to treat it that way seems unreasonable. The only objection I can see is "it looks weird", which is completely subjective? Otherwise, testing it out seems like a good thing. If people don't like it, we can remove it.

It seams like more and more people don't like it, from time to time.

I just give you three results:

[2017-08-21]

15 👍 VS 28 👎 comment

[2017-08-28]

comment #are_u_still_open_and_listening

[2018-04-14] THE LATEST

bad

And forgive me, I'll repeat my old comment, for the last time:

It's too sad that, many people dislike this one (-31 VS +15), but it was still approved.
To the core team, are you still keep open? are you still listening to the community?

rust-lang/rust#44101

@est31
Copy link
Member

est31 commented Apr 14, 2018

The feature was stabilized and shipped in the 1.25.0 version of the Rust compiler/language. You won't be able to remove it now due to the stability promise, except of removing it from the 2018 edition but I'd say that is very unlikely... It'd require a separate RFC and good reasoning (not just thumbs). So fuming is a bit pointless I'd say.

@mgeisler
Copy link

@est31

So fuming is a bit pointless I'd say.

I think the fuming should be seen as a signal to the language team that they might want to be more careful about adding such features in the future.

Perhaps the down votes should be seen as a "Hey, let's stop and reevaluate this — out of the people who voted, a vast majority didn't like this idea". A good next step would be to wait until someone presents proper arguments against the idea. Accepting an idea by default seems bad to me.

@yarrow
Copy link

yarrow commented May 10, 2018

@mgeisler

Perhaps the down votes should be seen as a "Hey, let's stop and reevaluate this — out of the people who voted, a vast majority didn't like this idea".

That's what @withoutboats did in a comment last year after the first call for FCP:

There are a lot of thumbs down on the opening comment, but little expressed negative feedback. I wonder if anyone wants to give feedback explaining why they think this shouldn't be accepted?

That resulted in re-opening the discussion. Posts between the first and the second FCP mostly addressed concerns about trailing vertical bars. Trailing vertical bars were removed from the RFC.

In the second FCP itself, two people said they were against it. One gave no reason. The second gave two: that it was weird syntax, and that there were more thumbs down than thumbs up. The thumbs up/down is not a voting mechanism, and in this case it was also hard to know whether the thumbs down were primarily about leading or trailing vertical bars.

As for the growing number of thumbs down, I can't know whether a new thumbs-down means "I'm against leading vertical bars" or "I'm annoyed with the language team" or something else. And neither can the language team. A comment conveys a lot more information than a thumbs-down.

I think the language team did a fine job of listening to the community on this PR. It may be that

  • it was the wrong decision;
  • Rust should make syntax changes much more slowly than it has in the past; or
  • the process for involving the community could be improved.

I don't know the answer to the first two points. The answer to the third point is of course "Yes, the process could be improved." Because anything can be improved. And I do think the process is currently excellent. (I also believe that treating thumbs up/down as a vote would be a move in the wrong direction: online polls are not a good way to count votes, let alone to reach a rough consensus.)

@est31
Copy link
Member

est31 commented May 10, 2018

@mgeisler like any kind of feedback, complaining and thumbs down have the most effect the earlier on you are in the process. After a feature has been stabilized and even shortly before it is being stabilized, complaints have the least effect.

You didn't even define what "such features" means... how should the language team know that?

What I suggest for you in the future is that you read the RFCs opened and voice your concerns, or even just 👎 the RFCs you don't like. The try RFC #2388 is on a similar course as this one: the number of 👎 is increasing more rapidly than the number of 👍 's. But better is to 👎 earlier on in the process.

@SoniEx2
Copy link

SoniEx2 commented May 10, 2018

try / #2388 can still be pulled back, at least, since it's nowhere near stable

@BurntSushi
Copy link
Member

I think this thread has outlived its usefulness.

@rust-lang rust-lang locked as resolved and limited conversation to collaborators May 10, 2018
@Centril Centril added A-syntax Syntax related proposals & ideas A-patterns Pattern matching related proposals & ideas A-control-flow Proposals relating to control flow. labels Nov 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-control-flow Proposals relating to control flow. A-patterns Pattern matching related proposals & ideas A-syntax Syntax related proposals & ideas 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.