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

propose if let guard #2294

Merged
merged 3 commits into from
May 27, 2018
Merged

propose if let guard #2294

merged 3 commits into from
May 27, 2018

Conversation

strake
Copy link
Contributor

@strake strake commented Jan 16, 2018

Rendered
Tracking issue

I find myself wanting this often (the latest use case being parsing terminal escape codes).

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jan 16, 2018
@Centril
Copy link
Contributor

Centril commented Jan 16, 2018

Linking other control-flow proposals with varying degrees of relatedness:

@glaebhoerl
Copy link
Contributor

(For reference: in Haskell 2010, this same feature is called "pattern guards".)

@Ixrec
Copy link
Contributor

Ixrec commented Jan 16, 2018

Just going by the examples in the RFC, I find it very difficult to tell where the match block, match arms, guards, etc all start and end (without stopping to consciously reason about it, which imo you shouldn't have to do for "basic" syntax like let/match/if/else).

I think that's because these if let guards are significantly longer than everything else in the match block, and maybe that's not representative of whatever "real world usage" of this feature would be like, but it does give me a very strong first impression that this is feature is "going too far" and code would only very rarely be improved by it.

@strake do you have any (presumably longer) code samples where the this feature would enable a clear improvement?

@leoyvens
Copy link

This would close #2214 which is exactly this feature request. It's a natural extension to if guards and it's a common pattern, examples can be found by searching a large codebase like rustc for => if let.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 20, 2018

It is ultimately syntactic sugar, but the transformation to present Rust is potentially non-obvious.

I don't see any way to desugar it into existing Rust constructions (at AST/HIR level), unfortunately.
It requires something new at lower level, namely goto 'arm construction.
Below I'll assume that chaining (#2260) is supported as well.

(First, every non-pattern condition cond can be transformed to a pattern condition cond is true, so I'll use only pattern conditions further.)
Condition chains in "free" ifs can be lowered like this (recursively):

if expr1 is pat1(bindings1) && ... && exprN is patN(bindingsN) {
    stmts1
} else {
    stmts2
}

=>

match expr1 {
    pat1(bindings1) => {
        if expr2 is pat2(bindings2) && ... && exprN is patN(bindingsN) {
            stmts1
        } else {
            goto ELSE
        }
    }
    _ => goto ELSE
}
goto END:

ELSE:
stmts2

END:

In this case gotos and labels can be represented in existing Rust using loops with labeled breaks.

For condition chains in "guard" ifs we have the next desugaring:

match expr {
    pat if expr1 is pat1(bindings1) && ... && exprN is patN(bindingsN) => {
        stmts
    }
    _ => {}
}

=>

match expr {
    pat => match expr1 {
        pat1(bindings1) => {
            if expr2 is pat2(bindings2) && ... && exprN is patN(bindingsN) {
                stmts
            } else {
                goto NEXT_ARM
            }
        }
        _ => goto NEXT_ARM
    }
    NEXT_ARM:
    _ => {}
}

In this case we can't do it in existing Rust because we cannot attach labels to match arms.

@petrochenkov
Copy link
Contributor

I wonder if labeled match arms and breaks from match arms could be useful if directly available from source code.

I can imagine them being potentially useful in the "common tail" situation for example:

'end: match expr {
    pat1 => {
        stmts1;
        break 'common
    }
    pat2 => {
        stmts2;
        break 'common
    }
    pat3 => {
        stmts3;
        // no common
    }
    _ => break 'end,
    
    'common: _ => {
        common_stmts
    }
}

or with complex arm conditions

match expr {
    pat1 => {
        complex_stmts;
        if complex_condition {
            // It was a mistake to go into this arm after all, let's try something else
            break: 'next
        }
    }
    'next: pat2 => {}
    _ => {}
}

@petrochenkov
Copy link
Contributor

cc #680

@strake
Copy link
Contributor Author

strake commented Jan 20, 2018

@Ixrec i have been writing code to avoid needing this feature, so i have none ready, but could try to write one.

@petrochenkov on desugaring: my thought was we could rewrite an if let guard as an expression, and copy the rest of the outer match expression into an inner expression:

match x {
    A(a) if let Some(y) = f_a(a) => g(y),
    B(b) => f_b(b),
    C(c) => f_c(c),
}

vs

match x {
    A(a) => if let Some(y) = f_a(a) { g(y) } else match A(a) {
        B(b) => f_b(b),
        C(c) => f_c(c),
    },
    B(b) => f_b(b),
    C(c) => f_c(c),
}

@strake
Copy link
Contributor Author

strake commented Jan 21, 2018

@Ixrec I made another example; is it better?

# Motivation
[motivation]: #motivation

This feature would greatly simplify some logic where we must match a pattern iff some value computed from the `match`-bound values has a certain form, where said value may be costly or impossible (due to affine semantics) to recompute in the match arm.
Copy link

Choose a reason for hiding this comment

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

„iff“ -> „if“ typo or is this some sort of abbreviation?

Copy link
Contributor

@Centril Centril Feb 13, 2018

Choose a reason for hiding this comment

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

iff means if and only if to distinguish between logical implication p -> q or logical equivalence p <-> q (which is equivalent to p -> q && q -> p). Could be written out to make it clearer for those not familiar with this particular mathematical jargon.

@aturon aturon self-assigned this Mar 15, 2018
@aturon
Copy link
Member

aturon commented Mar 15, 2018

The Lang Team discussed this RFC today, with the following take-aways:

  • This seems like a natural syntactic extension, under the guise of "if you can use if as a guard, it's reasonable to expect if let to work as well".
  • At least some members of the team expressed running into situations where such syntax would've been useful, and they instead used something like matching against a pair.
  • Generally there were no real objections. However, there's a concern of "syntax creep": we've been getting (and accepting) a number of such RFCs to "complete" various parts of our grammar, and we need to make sure that at the end we're left with something that sits together well.

Overall, the consensus was that we should move forward with this RFC, but with a clearly provisional flavor (as with other recent syntactic additions): when it comes time to stabilize, we want to take a holistic look at the way the syntax is evolving across these RFCs.

@rfcbot fcp merge

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Mar 15, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 15, 2018

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

Concerns:

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.

@Centril
Copy link
Contributor

Centril commented Mar 15, 2018

I have 2 concerns:

  1. (category: "formatting") This encourages rightward drift in arms and the tokens = and => are hard to distinguish at glance.
    We can mitigate this with rustfmt by formatting as:
match x {
    'C' if let &[n] = parms =>
        self.screen.move_x( n as _),  // note the linebreak!
    ...
}

or if the bit before if let is very long:

match ui.wait_event() {
    KeyPress(mod_, key, datum)
        if let Some(action) = intercept(mod_, key) => act(action, datum),
    ev => accept!(ev),
}
  1. (category: "syntax creep") match and if let (and while let) become more inconsistent; It seems to me that the natural extension of this RFC is to permit:
if let A(x) = foo()
if let B(y) = bar()
if cond(x, y) {
    ...
}

This ^ particular syntax was not (iirc) very intelligible in the survey done at #2260.

The RFC does write that you can't chain if let and if in match, so maybe if let chaining isn't the logical conclusion, but it is something to consider.

@strake
Copy link
Contributor Author

strake commented Mar 23, 2018

@Centril What consistency between match and if let would this break?

@Centril
Copy link
Contributor

Centril commented Mar 23, 2018

@strake you would not be permitted to chain an if let with an if let, but you are allowed to chain a match with an if let. That is not necessarily a problem; I'd just like us to be aware of it =)

@strake
Copy link
Contributor Author

strake commented Apr 12, 2018

I see a majority of boxes checked — what is holding back FCP?

@nikomatsakis
Copy link
Contributor

@rfcbot concern borrowck

We are currently working through how to rationalize our existing patterns in MIR borrowck, and in particular addressing a number of soundness complications. It's not entirely obvious to me how this RFC is going to interact with that -- for example, the current treatment that we are moving towards is that -- in a guard expression -- the bindings from the pattern are compiled into refrences into the value being matched, with an implicit deref. So e.g. in a match like this:

match foo {
    Some(x) if x > 5 => {..}
}

although the type of x in the binding is i32, the type of the value representing x in the MIR is an &i32, and the x > 5 becomes *x > 5. (This is similar to what we do for closures.) The goal here is to prevent arms from mutating through x (e.g., if it is a ref mut binding) or from moving out of x.

I feel like before we accept this RFC, we ought to talk through what its MIR desugaring will be and how it will interact with the borrow check.

(cc @pnkfelix)

@pnkfelix
Copy link
Member

@nikomatsakis and @pnkfelix need to discuss more.

But the executive summary is: @pnkfelix currently believes that there is a relatively straight-forward semantics under the match changes that are proposed (see rust-lang/rust#50783) as part of the NLL work.

It might be a good idea to try to work through those semantics, perhaps via a series of examples, within this RFC text.

Or it might be a good idea to just accept this RFC, with the understanding that we will need to work out the exact semantics (in terms of when the various bindings occur in the MIR desugaring) as part of the implementation effort.

@nikomatsakis
Copy link
Contributor

@rfcbot resolve borrowck

OK, so it seems I killed the conversation with my comment. I apologize that the comment didn't make it particularly clearly what sort of follow-up would satisfy me. After some consideration, I've decided to withdraw my concern. It's not that I'm not still worried, it's that I think it's ok to merge this RFC basically as our "stated intention" to specify this more clearly.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels May 17, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented May 17, 2018

🔔 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 May 17, 2018
@anirudhb
Copy link

I think that this sounds good:

match keyinfo {
  /* Separate 'if let' with 'if' using '&&'
  note: && is parsed regularly after initial && separator */
  Some(keyinfo) if let Some(key) = get_key(keyinfo) && key == K_ENTER && key == K_UP || key == K_DOWN => {
    // stuff
  },
  _ => {
    // other stuff
  }
}

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label May 27, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented May 27, 2018

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

@rfcbot rfcbot removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 27, 2018
@Centril Centril merged commit e518d9d into rust-lang:master May 27, 2018
@Centril
Copy link
Contributor

Centril commented May 27, 2018

Huzzah! This RFC is merged.

Tracking issue: rust-lang/rust#51114

@strake strake deleted the if-let-guard branch May 27, 2018 23:28
@Centril Centril added A-syntax Syntax related proposals & ideas A-expressions Term language related proposals & ideas A-control-flow Proposals relating to control flow. labels Nov 23, 2018
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 16, 2020
…thewjasper

Implement if-let match guards

Implements rust-lang/rfcs#2294 (tracking issue: rust-lang#51114).

I probably should do a few more things before this can be merged:
- [x] Add tests (added basic tests, more advanced tests could be done in the future?)
- [x] Add lint for exhaustive if-let guard (comparable to normal if-let statements)
- [x] Fix clippy

However since this is a nightly feature maybe it's fine to land this and do those steps in follow-up PRs.

Thanks a lot `@matthewjasper` ❤️ for helping me with lowering to MIR! Would you be interested in reviewing this?
r? `@ghost` for now
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 17, 2020
…thewjasper

Implement if-let match guards

Implements rust-lang/rfcs#2294 (tracking issue: rust-lang#51114).

I probably should do a few more things before this can be merged:
- [x] Add tests (added basic tests, more advanced tests could be done in the future?)
- [x] Add lint for exhaustive if-let guard (comparable to normal if-let statements)
- [x] Fix clippy

However since this is a nightly feature maybe it's fine to land this and do those steps in follow-up PRs.

Thanks a lot `@matthewjasper` ❤️ for helping me with lowering to MIR! Would you be interested in reviewing this?
r? `@ghost` for now
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Dec 20, 2020
Implement if-let match guards

Implements rust-lang/rfcs#2294 (tracking issue: #51114).

I probably should do a few more things before this can be merged:
- [x] Add tests (added basic tests, more advanced tests could be done in the future?)
- [x] Add lint for exhaustive if-let guard (comparable to normal if-let statements)
- [x] Fix clippy

However since this is a nightly feature maybe it's fine to land this and do those steps in follow-up PRs.

Thanks a lot `@matthewjasper` ❤️ for helping me with lowering to MIR! Would you be interested in reviewing this?
r? `@ghost` for now
@kennytm kennytm mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-control-flow Proposals relating to control flow. A-expressions Term language related proposals & ideas A-syntax Syntax related proposals & ideas disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. 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.

None yet