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

Add 'else match' blocks to if expressions. #1712

Closed
wants to merge 8 commits into from

Conversation

phoenixenero
Copy link

@phoenixenero phoenixenero commented Aug 11, 2016

Rendered

A more specific version of my previous proposal.

baz() => do_that()
} else {
do_something();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

with single variant matches if let is definitely more readable:

enum Bazz {
    Baz,
    Bak,
}

fn foo() -> bool { unimplemented!() }
fn baz() -> Bazz { unimplemented!() }
fn do_this() -> () { unimplemented!() }
fn do_that() -> () { unimplemented!() }
fn do_something() -> () { unimplemented!() }

fn main() {
    if foo() {
        do_this();
    } else if let Bazz::Baz = baz() {
        do_that();
    } else {
        do_something();
    }
}

@louy2
Copy link

louy2 commented Aug 11, 2016

I am not convinced this is necessary unless more useful optimization can be done against it. It seems pure aesthetic now.

}
```

## Use-cases
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, in no of these cases I see a clear improvement. In fact, the opposite is true. I find it "uglier".

@ahicks92
Copy link

If I was going to do anything in this regard, it would be to let else if and else if let be valid without braces. I don't much see the utility of this either. It seems like else if would be more useful, assuming that's not allowed already (I'm assuming it's not because I've never seen it, but haven't tried).

else if let would let you match multiple arms as well, though without the exhaustion checks:

if flag {
    do_something();
}
else if let pattern {
    do_something_else();
}
else if let other_pattern {
    do_a_third_thing();
}

Etc. While this has the downside of not getting the compiler to check for you, it also looks good in the case where you need to match patterns against multiple, different values; the one the RFC proposes would require indentation for this idiom.

It might be worth expanding the RFC with this, or closing the RFC and making another along these lines, but something like the above seems much more generally useful.

expression, with one key addition:

When an arbitrary expression `exp` in `else match <exp>` fails to match any of the cases in the
`else match` block, the next block (if it exists) is run.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am in favor of else match, but I am really against this! match should work consistently.

There is an orthogonal proposal to allow match to take elses at the end, so you don't have to include the catchall branch inside the match intend. But that proposal should apply to all match statements, not only the else match ones.

@withoutboats
Copy link
Contributor

Integrating match and if let (as @camlorn notes) into the if/else construct seems obvious and normalizing. As I commented on a line note, it also make sense for match to take else in general. In my opinion these changes would make the control flow constructs of the language more consistent in how they work.

Removed an example, removed "match else"-style functionality, added some
clarification, and slight tweaks to the formatting of the grammar.
@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 12, 2016
@nrc
Copy link
Member

nrc commented Aug 12, 2016

@camlorn both else if and else if let are already accepted (although you need ... pattern = expr in your example to parse).

This RFC somewhat makes sense to me since we allow else if let, but I'm not completely convinced of the value here. if ... else if ... else seems a very common pattern. However, I can't imagine that if ... else match would be so common. It is also not clear that it makes sense since else match can't be chained, e.g., if ... else match ... else doesn't make sense unless we relax the requirement for matchs to be exhaustive.

@glaebhoerl
Copy link
Contributor

Instead of special-casing match I would rather extend this to any control-flow construct which has a block attached. This rule would naturally encompass the three specific constructs which are already allowed after an else: plain blocks { ... }, if ... { ... }, and if let ... { ... }. So instead of adding more special cases, it would result in fewer.

@withoutboats
Copy link
Contributor

@glaebhoerl you mean that this RFC should be written to support else loop, else while, else for?

@glaebhoerl
Copy link
Contributor

I think there should be a syntactic category of "things that have blocks", and anything in that category should be allowed. So yes those, and also catch once it's implemented, and unsafe, and anything else that might be slipping my mind. But the point would be to uniformly distinguish on the basis of "does it have a block or not" instead of a laundry-list of special cases.

(Not necessarily this RFC. Just stating my preference in general.)

@phoenixenero
Copy link
Author

phoenixenero commented Aug 13, 2016

Personally, for match statements I can agree on because match is basically a glorified if-else-if/switch structure. Loop, for, while, unsafe, etc. belong in a different "class" of structure that does not deal with Boolean logic (and pattern matching in general) and would be confusing.

@nikomatsakis nikomatsakis self-assigned this Aug 18, 2016
@nikomatsakis
Copy link
Contributor

I've wanted to propose this for a while. This pattern seems to come up often for me.

@withoutboats
Copy link
Contributor

@glaebhoerl I think "having blocks" is just a syntactic category and there is some semantics to this. Basically I think it should make sense as a 'branching' construct. This could make sense for loops if loops ever evaluate to values, and I have to think about catch more, but for unsafe I don't think it makes sense because unsafe doesn't have anything to do with control flow. I also think its good to make an exception for unsafe for the reason that we want that keyword to be extremely visible, and this would make it easier to miss that this else block is actually also an unsafe block.

@glaebhoerl
Copy link
Contributor

glaebhoerl commented Aug 20, 2016

I think "having blocks" is just a syntactic category and there is some semantics to this. Basically I think it should make sense as a 'branching' construct.

To be honest, I don't think so. The way I look at it is, what's the core requirement? The core requirement is to avoid the "goto fail" class of bugs, where something can be visually mistaken for being part of the else clause when it is not. What do we need to satisfy this requirement? We need that where there is the possibility to add an additional statement after a statement belong to an else clause, that that statement always be enclosed in braces. And my inclination would be to just satisfy the requirement in the simplest way possible without getting any other perceived distinctions mixed up in it. I mean, C and most C-family languages allow anything to come after an else, "branching construct" or not, block-having or not. Relative to them the only real difference we have is that we want to avoid goto fail.

I also think its good to make an exception for unsafe for the reason that we want that keyword to be extremely visible, and this would make it easier to miss that this else block is actually also an unsafe block.

This might make some sense for unsafe in particular. Though requiring unsafe and only unsafe to always use double-nesting in this case also feels a bit like the "make unsafe code unergonomic to write so people are discouraged from writing it" philosophy of design which we've tried to avoid following.

@mdinger
Copy link
Contributor

mdinger commented Sep 10, 2016

This type of idea was previously discussed here. As an alternative, if all you're trying to do is reduce rightward drift for matches, #1745 might be interesting.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

This is a small bit of syntactic sugar, but it seems harmless enough, and I know that I have found myself wanting it many times. It frequently happens because I have some guards I need to execute all the time, and then I want to do a bunch of matching in the general case. So I propose we merge it. I would start small: just else match, basically on the premise that match and if are generalizations of one another.

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 11, 2016

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

Concerns:

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.

@withoutboats
Copy link
Contributor

withoutboats commented Nov 11, 2016

@nikomatsakis how would you feel about allowing match to carry an else, which would be equivalent to a _ => arm?

match an_option {
    Some(foo) => { ... }
} else {
    ...
}

It could take an if, which would be the same as a guard:

match an_option {
    Some(foo) => { ... }
} else if a_bool {
    ...
} else {
    ...
}

I don't feel that this is necessarily good style, but if we're allowing if { } else match { } it seems more consistent to allow match { } else if { }

@nagisa
Copy link
Member

nagisa commented Nov 12, 2016

I would like to note that this RFC generally has more of the negative feedback than it has of positive. I personally find that a sign that it ought to be moving towards fcp close regardless of the harmlessness, smallness or personal experiences of the lead of most of the teams.

I’m personally fine with this getting merged provided we also investigate (now, as part of accepting this RFC) making things like else loop, else while, else for and so forth (hence the :/ reaction), even if we are not planning of including any of these as part of this RFC.


I very much dislike the idea of match { ... } else.

@scottmcm
Copy link
Member

@glandium I agree that they don't require else { if { } }; both have explicit counterexamples.

But I still don't think they allow

if (foo) {
  do_one_thing();
} else switch (bar) {
  case 1:
    do_another();
    break;
  default:
    do_that();
    break;
}

For the Mozilla one, for example, I can see no exception or counterexample to the "Always brace controlled statements" rule that would allow else switch or else while.

Hmm, DXR looks nice. Let's ask it about else switch in C++: https://dxr.mozilla.org/mozilla-central/search?q="else+switch"+ext%3Acpp In 9,130 files, it finds three uses. Two are in machine-generated code (from YACC), where two more braces wouldn't be a hardship. The third is from ICU, in code that clearly doesn't follow Mozilla's coding style.

"else while" exp:cpp? No matches. "else for" exp:cpp? 4 matches, three of which are comments. The other has a loop in the if block, so IMHO would actually look better with the braces-and-indent. And that particular use throws off my mental pattern matching, as my brain sees

  else
    ...
    return NS_OK;
  }

but the else doesn't always return OK because that return is inside a for loop over a linked list.

So overall, I can't help thinking that generalized else keyword isn't that common a case, isn't that helpful when it does come up, and might end up being considered bad style anyway.

@glaebhoerl
Copy link
Contributor

@scottmcm Thanks for bringing that up, it's definitely something that's worth investigating. Do we know what their reasons are for disallowing it? That is, is there a substantive reason why it should be considered "bad style"? If there are reasons behind it, we should very much take them into consideration. If we don't know of any, though, then it's still an interesting data point, but I think we should make the decision based on substance ("are there any problems this could conceivably cause?") rather than heuristics ("how do other people do it?").

For what it's worth, I suspect the reason for it is that, in guidelines and conventions, because you're solely reliant on people to keep them all in their head, simplicity and clarity are at a premium, and it's preferable to have few blunt, conservative, but easy to remember rules, than nuanced but more flexible ones. Having the compiler check it for you alters the balance between tradeoffs significantly: if the compiler lets you do it, then you're good to go, and you don't have to second-guess yourself nearly as much. Complexity still has a price, of course, but it's considerably less acute. (And at least in terms of how the rules are formulated, I've argued that else keyword reduces complexity -- in terms of how brains conceive of the rules, it's of course harder to tell.)

For what it's also worth, I've personally been using else for and else switch in my C++ code for a long time now, precisely because I don't see any substantive reason there'd be a problem with it.

@burdges
Copy link

burdges commented Feb 13, 2017

If else keyword is allowed, then I suggest that rustfmt should not reformat

if .. {
    foo(0);
} else for i in .. {
        foo(i);
    }

or

if .. {
    foo(0);
} else for i in .. { foo(i); }

or

if .. {
    foo(0);
} else 
    for i in .. {
        foo(i); 
    }

or even

if .. {
    foo(0);
} else 
for i in .. {
    foo(i);
}

into an obnoxious indentation that hides the loop like

if .. {
    foo(0);
} else for i in .. {
    foo(i);
}

If anything, rustfmt should convert this fifth indentation into the first or third.

I've no feelings against else match itself because match has the same time complexity as if, and match often requires double indentation anyways.

@repax
Copy link

repax commented Feb 13, 2017

@burdges: In your first and third example:

if .. {
    foo(0);
} else for i in .. {
        foo(i);
        ...
        ...
        // Imagine some more code here...
        ...
        ...
    }

//<-- The lack of an ending bracket on this indentation level
// makes me think that the brackets are unbalanced.
// I feel I'd had to go back and count them.
more_code();

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 14, 2017

@rfcbot fcp close

Honestly, I feel like this discussion has been kind of going around in circles. It seems clear that people's intuitions differ here. Therefore, I am moving to close this RFC: I think we ought to just leave things as they are for now. The status quo is reasonable and we can consider more extensions in the future.

To summarize some of the recent discussion:

  • Just support else match { ... } strikes some (ahem, at least me) as reasonable, but strikes others as a kind of surprising limitation. They would expect to be able to substitute any keyword, at minimum, in that spot.
  • But for others, supporting else match { ... } seems to imply that match { ... } else { ... } ought to work. Others find this form sort of surprising.
  • Still others find that if we support else match { ... } we might consider keywords in other places, such as loop if { ... }, but the meaning of such constructs can be quite confusing.

Some things that might help a future revision of this topic make progress:

  • Gathering data on how often various patterns arise in practice.

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 14, 2017

Team member @nikomatsakis 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.

@solson
Copy link
Member

solson commented Feb 14, 2017

While I was in favor of else <keyword> I now agree with @nikomatsakis that none of the proposals seem to have clear support. I also don't think the win from else <keyword> is very big, so it's not worth introducing if it would confuse others.

@withoutboats
Copy link
Contributor

I've also been feeling increasingly inclined to close this. None of this seems obvious enough for the learning cost to not outweigh the convenience cost.

@glaebhoerl
Copy link
Contributor

This is a bit frustrating. I've felt like throughout this thread, there has only been one proposal that has had clear support, but people have kept conflating it with other extensions that very few people think are a good idea. This had the effect of muddying the waters, and making it seem like it was a complicated, confusing and ambiguous situation when it really wasn't. So I thought of doing the straw polls to try to restore some clarity to the situation, and it seems to me that this was successful (moreso than I had expected): it's now completely clear that there is, in fact, only one proposal that has any real support. I had hoped that this would have allowed us to refocus discussion around that one proposal, and to stop getting the others mixed up in it. And now the motion is to close, because there are too many different proposals, and the water is too muddy. I... don't see how this makes any sense. The verbiage in @nikomatsakis's comment and my earlier one is remarkably similar: "I feel like this thread has been drifting around in circles"; "I feel like this discussion has been kind of going around in circles". Yes. Maybe now that it's clear that there's only one direction to go in, we could stop going around in circles?

@withoutboats
Copy link
Contributor

withoutboats commented Feb 14, 2017

I'm in favor of closing this for a slightly different set of reasons from Niko.

  1. The only proposal with any support has ambivalent support, and I have trouble finding strong motivation for it.
  2. I do not believe this will seem like a natural and comprehensible extension to the language. I think it will seem capricious & arbitrary to people when they encounter it (even in the generalized and not match-specific form).
  3. The fact that there are many other proposals, even if they have weak or no support, indicates that this is not an 'obvious' extension to language.

@aturon
Copy link
Member

aturon commented Feb 14, 2017

@glaebhoerl I agree with you that there's been only one real contender for a while; that was clear to me even before the straw poll. And I personally would still be happy to land such a feature. But I think @nikomatsakis was despairing of reaching full consensus around the proposal, since many are still opposed, and those on the lang team are a bit ambivalent; meanwhile, this RFC has been a bit of a timesink with pretty minimal benefits if we do land it.

So, for me: I'd love to land this, I'm OK with closing it, but I'm ready to reach a decision.

@nikomatsakis
Copy link
Contributor

@aturon @glaebhoerl can one of you clarify what you see as the "one true contender"? Is it extending else to permit any control-flow keyword following (i.e., in addition to if, also accept loop, for, match, while, break, continue, return, are there others?)

I could get behind this proposal but I also feel a lot of ambivalence. I am definitely ready to be done talking about it either way. The win seems relatively slim (less, e.g., than if let). The potential for surprise (in the sense of people not really understanding what will and won't work) seems, well, surprisingly high.

@aturon
Copy link
Member

aturon commented Feb 14, 2017

@nikomatsakis yes, that's the "consensus" proposal. I'm sympathetic to @withoutboats's concerns for sure, but still would be OK trying it.

@iopq
Copy link
Contributor

iopq commented Feb 14, 2017

Yes, that's the proposal that makes the most sense to me. It applies rules in a consistent manner, where we can just say else can be followed by a block or a construct that has a block attached to it.

It "back-generalizes" why else if works, since it's just another construct that has a block attached to it.

@withoutboats
Copy link
Contributor

withoutboats commented Feb 15, 2017

I was in favor of the else keyword proposal until about 2 or 3 weeks ago, as I think the discussion record indicates. But at some point I stepped back & realized I was driven by a bias for action. The original proposal seemed too specific, so I generalized it to something plausible. But just because its plausible doesn't mean its preferable to doing nothing & I realized that I would really rather do nothing.

I understand the argument that else if is a special case that this eliminates, but I don't agree. else if is an expected/understood construct from other languages. Like much of our control flow*, if/else is empowered by familiarity as much as convenience, and making it act strangely from other languages only because its more general in some abstract sense doesn't make it more intuitive or usable.

*The most glaring example of this "familiarity sugar" is while: I doubt we would have considered that use case deserving of sugar if it weren't for the fact that its a universally common construct in imperative languages.

@glaebhoerl
Copy link
Contributor

I think that's the strongest counterargument, fwiw. Finally the discussion we should be having. :)

I'm not so convinced that allowing only else if as a special case is more consistent with expectations from other languages, however. As far as I'm aware, more or less all of the mainstream statically typed C-heritage languages - C, C++, Java, C#, Objective-C, D, ... - allow anything to come after an else, if or not, keyword or not, braces-having or not. (Correct me if I'm wrong.) It's mainly the various "scripting languages" which deviate from this by having special elif or elsif constructs. So we wouldn't be out of line by generalizing, ours would just be slightly narrower. I think of this idea as providing the best of both worlds: protection against goto fail and similar mistakes, and the convenience of else match and friends where there's no potential danger.

@withoutboats
Copy link
Contributor

withoutboats commented Feb 15, 2017

@glaebhoerl But the consistency in the languages you cite is between if and else not requiring braces for a single statement, whereas if certainly would still require it in Rust. I don't find this appreciably more consistent with those languages' behavior. Rather, we have said that Rust requires braces around the result and does not require parens around the conditional, and that else if is "an exception" doesn't even need to be said, users just assume it.

@glaebhoerl
Copy link
Contributor

glaebhoerl commented Feb 15, 2017

Sure, not more consistent, but not less consistent either. 🤷

@nrc
Copy link
Member

nrc commented Feb 23, 2017

I'm also a bit sad about the circles here and I'm voting to close because I think the effort is far outweighing the potential benefit at this point.

FTR I am in favour of else match only. else keyword feels like consistency for the sake of consistency. I fear it would lead to confusing, hard to follow code with many rare combinations to trip up the reader.

And just to go off topic and risk wasting more time and energy, a note on language design philosophy: IMO, consistency is a means to an end, not a goal. If we try to make consistency an overarching goal (without considering practicality) we will end up with a language which is loved by uber-geeks, popular in academia, and abandoned in practice. As with most things there is a delicate trade off to consider.

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Feb 23, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 23, 2017

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

@iopq
Copy link
Contributor

iopq commented Feb 23, 2017

Consistency is not for the sake of itself, it's for the principle of least surprise. else if is not surprising, but else match ... is surprising unless else for ... also works

@glaebhoerl
Copy link
Contributor

Substance level: I agree this is a very minor thing, there are reasonable arguments both for and against, and in the end it's not going to make much difference either way -- it's obviously not worth wrangling over.

Process level: It kind of bothers me from an incentive-compatibility perspective that a proposal can get sidetracked to death and the official response is, "this proposal has been sidetracked to death so we're going to close it". I might've hoped there would be someone with the responsibility to help discussions stay on track.

@joshtriplett
Copy link
Member

@glaebhoerl I don't think I'd characterize this as "this has been sidetracked to death so we're going to close it". I think I'd characterize this as "this has too much pull in too many different directions to conclude that we have a reasonable consensus in favor of the change".

It's not clear whether the "consensus" proposal was "else match" or "else keyword" (multiple people described each as the consensus at various times0, and both had substantial pushback.

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 5, 2017

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Mar 6, 2017

I'm going to go ahead and close out this RFC. While there may still be some merit in ideas along these lines, it has proved too difficult to reach a lasting consensus here, and we have bigger fish to fry in the meantime. Thanks @phoenixenero for the RFC!

@aturon aturon closed this Mar 6, 2017
@phaux
Copy link

phaux commented Apr 14, 2017

How about using => or : when nesting control statements?

for x in 0..10 => for y in 0..10 {
    println!("{:?}", (x, y));
}

Or simply bring back the people's favorite if <expr> then <stmt> and for <pat> in <expr> do <stmt> keywords.

;)

@gustavolsson
Copy link

gustavolsson commented Apr 28, 2017

I don't know if one is allowed to post more comments now that this is closed, but I just wanted to chime in on the discussion: I followed the same path of reasoning for my toy language and eventually settled on two types of statements: action statements and flow control statements. This is very similar to what was discussed here but I think the concept of splitting flow control statements (expressions, in Rust's case) into their own category and separating them from blocks is worth emphasizing.

For Rust, this would become:

expr : literal | ... | flow_expr ;

flow_expr : while_expr | loop_expr | break_expr | continue_expr
     | for_expr | if_expr | match_expr | if_let_expr | while_let_expr
     | flow_block_expr | return_expr ;

flow_block_expr : '{' block '}' ;

if_expr : "if" no_struct_literal_expr flow_block_expr
          else_tail ? ;

else_tail : "else" flow_expr ;

Note how flow control expressions are broken out as flow_expr.

By allowing any flow control expression after else, a block being one type of (but not limited to) flow control expression, one generalizes the common C-like else if to other constructs without letting any pointless single statement through. (New statements or non-flow expressions would require a block making scoping clear and forcing pointless single statements to visually stand out)

Not all keywords should be considered flow control expressions (for example unsafe) and blocks need not to imply a flow control statement. loop loop would not be allowed because all loops would require explicit blocks as before. I personally like this approach but I agree that it is a minor improvement to the language.

I'm very much a beginner when it comes to Rust, so please excuse me if I've missed something important!

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.