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
Closed
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
245 changes: 245 additions & 0 deletions text/0000-else-match.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,245 @@
- Feature Name: Else Match
- Start Date: 2016-07-26
- RFC PR:
- Rust Issue:

# Summary
[summary]: #summary

Extend the `if` expression to accept an `else match` block.

# Motivation
[motivation]: #motivation

This proposal is meant to reduce the verbosity of writing `if ... else { match ... } ` style
statements.

It also makes code similar to:

```rust
let flag = false;

if foo() {
do_this();
} else {
match bar() {
baz() => do_that(),
_ => flag = true
}
}

if flag {
do_something();
}
```

simpler and more concise:

```rust
if foo() {
do_this();
} else match bar() {
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();
    }
}

```

## 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".


Though rare, this pattern does exist in several Rust projects.

### [Servo](https://github.com/servo/servo)

<https://github.com/servo/servo/blob/master/components/layout/model.rs#L557>

Before:

```rust
/// Clamp the given size by the given `min` and `max` constraints.
pub fn clamp(&self, other: Au) -> Au {
if other < self.min {
self.min
} else {
match self.max {
Some(max) if max < other => max,
_ => other
}
}
}
```

After:

```rust
/// Clamp the given size by the given `min` and `max` constraints.
pub fn clamp(&self, other: Au) -> Au {
if other < self.min {
self.min
} else match self.max {
Some(max) if max < other => max,
_ => other
}
}
Copy link
Member

Choose a reason for hiding this comment

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

No, I'm sorry, to me, that doesn't look good at all. If you really want to get rid of that additional level of indentation, just use an early return in the if branch (works for the next example as well).

```

### [xsv](https://github.com/BurntSushi/xsv)

<https://github.com/BurntSushi/xsv/blob/master/src/cmd/stats.rs#L311>:

Before:

```rust
if !self.typ.is_number() {
pieces.push(empty()); pieces.push(empty());
} else {
match self.online {
Some(ref v) => {
pieces.push(v.mean().to_string());
pieces.push(v.stddev().to_string());
}
None => { pieces.push(empty()); pieces.push(empty()); }
}
}
```

After:

```rust
if !self.typ.is_number() {
pieces.push(empty()); pieces.push(empty());
} else match self.online {
Some(ref v) => {
pieces.push(v.mean().to_string());
pieces.push(v.stddev().to_string());
}
None => { pieces.push(empty()); pieces.push(empty()); }
}
```

### [trust-dns](https://github.com/bluejekyll/trust-dns)

<https://github.com/bluejekyll/trust-dns/blob/master/src/authority/authority.rs#L558>:

Before:

```rust
if class == self.class {
match rr.get_rr_type() {
RecordType::ANY | RecordType::AXFR | RecordType::IXFR => return Err(ResponseCode::FormErr),
_ => (),
}
} else {
match class {
DNSClass::ANY => {
if rr.get_ttl() != 0 { return Err(ResponseCode::FormErr) }
if let &RData::NULL(..) = rr.get_rdata() { () }
else { return Err(ResponseCode::FormErr) }
match rr.get_rr_type() {
RecordType::AXFR | RecordType::IXFR => return Err(ResponseCode::FormErr),
_ => (),
}
},
DNSClass::NONE => {
if rr.get_ttl() != 0 { return Err(ResponseCode::FormErr) }
match rr.get_rr_type() {
RecordType::ANY | RecordType::AXFR | RecordType::IXFR => return Err(ResponseCode::FormErr),
_ => (),
}
},
_ => return Err(ResponseCode::FormErr),
}
}
```

After:

```rust
if class == self.class {
match rr.get_rr_type() {
RecordType::ANY | RecordType::AXFR | RecordType::IXFR => return Err(ResponseCode::FormErr),
_ => (),
}
} else match class {
DNSClass::ANY => {
if rr.get_ttl() != 0 { return Err(ResponseCode::FormErr) }
if let &RData::NULL(..) = rr.get_rdata() { () }
else { return Err(ResponseCode::FormErr) }
match rr.get_rr_type() {
RecordType::AXFR | RecordType::IXFR => return Err(ResponseCode::FormErr),
_ => (),
}
},
DNSClass::NONE => {
if rr.get_ttl() != 0 { return Err(ResponseCode::FormErr) }
match rr.get_rr_type() {
RecordType::ANY | RecordType::AXFR | RecordType::IXFR => return Err(ResponseCode::FormErr),
_ => (),
}
},
} else {
return Err(ResponseCode::FormErr);
}
```

# Detailed design
[design]: #detailed-design

## Grammar

See the following document for an (incomplete) guide to the grammar used in Rust:
[Rust Documentation → Grammar](https://doc.rust-lang.org/grammar.html).

This proposal modifies the
[if expression grammar](https://doc.rust-lang.org/grammar.html#if-expressions).

```
else_tail : "else" [ if_expr | if_let_expr
+ | match_expr
| '{' block '}' ] ;
```

## Execution

An `else match` block should be treated similar to an `else` block with a single `match`
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.


### Dead code

An `else match` block with a `_ => ...` match means the next clause (if exists) will never run.
There might be more complicated cases to optimize for and is outside the scope of this document.

# Drawbacks
[drawbacks]: #drawbacks

- Slight maintainability problems whenever you need to add additional logic to an `else` block.

# Alternatives
[alternatives]: #alternatives

Not an alternative but an addition to the proposal: `if match` expressions. This would add an
additional grammar rule and modify an existing one:

```
expr : literal | path | tuple_expr | unit_expr | struct_expr
| block_expr | method_call_expr | field_expr | array_expr
| idx_expr | range_expr | unop_expr | binop_expr
| paren_expr | call_expr | lambda_expr | while_expr
| loop_expr | break_expr | continue_expr | for_expr
| if_expr | match_expr | if_let_expr | while_let_expr
~ | if_match_expr | return_expr ;

...

+ if_match_expr : "if" match_expr else_tail ? ;
```

Should work nearly the same as `else match`.

# Unresolved questions
[unresolved]: #unresolved-questions

None.