Implement if let

[lilyball]

Implement the new desugaring if let syntax.

Fixes #16779.

[lilyball]

One thing to note is the error message for having a non-refutable pattern is kind of confusing:

fn main() {
    if let a = 1i {
        println!("irrefutable pattern");
    }
}

iflet.rs:2:5: 4:6 error: unreachable pattern [E0001] (pass `--explain E0001` to see a detailed explanation)
iflet.rs:2     if let a = 1i {
iflet.rs:3         println!("irrefutable pattern");
iflet.rs:4     }

This could perhaps be fixed by attaching an attribute to the desugared match that is used by the "unreachable pattern" to produce an error message tailored to if let. Of course, we didn't do anything like that for for, which also had the potential of being confusing (notably, if Some/None were not in scope), but it might be worth doing in this case.

[lilyball]

Another option is to put an attribute on the generated _ => () match arm that says "don't throw an error if this is unrreachable", but I would prefer to keep irrefutable if lets as an error and make the message better instead.

[lilyball]

I implemented the attribute approach, and then replaced it with an approach based on expn_info. I prefer the expn_info approach, but I left the attribute approach commit intact in case anyone wants to look at it.

Prior to merging, the discarded approach (currently the attribute approach) should be removed from the commit list.

[huonw]

If we do take this approach, I would mildly prefer that ExprMatch just has an extra field:

enum MatchSource {
    MatchNormal,
    MatchIfLetDesugar
}

[lilyball]

@huonw At first glance, it looks like that would require updating at least 19 locations across librustc and libsyntax just to account for the new parameter, and that's without actually even using it.

It's certainly doable, but is it really appropriate? Do clients of ExprMatch really need to care that it was produced from a desugared if let? To me, the desugaring is basically equivalent to macro expansion, and expn_info seems like perhaps the correct place to record that information.

Using expn_info like this is also future-compatible with any other desugarings that we might add in the future, such as the while let that I suggested today.

[huonw]

I have a feeling that some/most of those could be changed to give error diagnostics specific to if let, rather than match (even if it's just changing the word match to if let in the output text).

[lilyball]

Most of what? Those aren't errors I was talking about. Those were just uses of the token ExprMatch.

[huonw]

Those were just uses of the token ExprMatch.

They are 'just' uses that lead to compiler diagnostics, e.g.

• rust/src/librustc/lint/builtin.rs

  Line 1083 in f92015f

  ast::ExprMatch(head, _) => (head, "`match` head expression", true),

• rust/src/librustc/util/ppaux.rs

  Line 83 in 98ec85f

  ast::ExprMatch(..) => explain_span(cx, "match", expr.span),

They should not be talking about match when the problem was created with an if let. Furthermore, the majority of the uses of ExprMatch have (..), and I don't imagine if let will need special handling, so they can be left entirely unchanged.

[lilyball]

@huonw Ok I pushed a commit that uses enum MatchSource.

[lilyball]

As before, this commit obsoletes the two previous commits, but I haven't removed them, for comparison's sake. They will be removed if we go ahead with enum MatchSource.

[lilyball]

I've added the feature gate. At this point we just need to decide which approach to use for error reporting. Since it's already implemented, I'm in favor of @huonw's MatchSource approach, though I think the expr_info approach is fine as well (but the unnecessary parens lint would need to be updated for that).

@nikomatsakis Since you seemed to care strongly that if let be an AST rewrite, do you have any opinion on the error reporting approach? You can see all 3 approaches implemented in the commits on this PR.

[nikomatsakis]

@huonw's suggestion is the same as what I had in mind.

[lilyball]

Force-pushed to remove the rejected approaches from commit history.

r?

[nrc]

replace 'desugaring syntax' with 'feature' or 'statement' - the fact that it is implemented with desugaring is not relevant to the user

[nrc]

} else {
    false
}

[lilyball]

We actually have no style guidelines here, and some minor precedence for making very short else expressions into one-liners, such as https://github.com/rust-lang/rust/blob/master/src/librustdoc/html/render.rs#L885

[lilyball]

That said, I will make the change, because this isn't something worth fighting for.

[nrc]

Looks good! r=me with the nits addressed

[lilyball]

@nick29581 Thanks! I'll push the updated version as soon as I make sure it works locally

[lilyball]

Same weird error three times in a row. It's looking less and less like a weird ephemeral error. But this PR has nothing to do with any of those run-pass-fulldeps issues, or with the plugin registry.

[nrc]

A belated review comment - we should add if let to the docs, at the very least to the reference manual.

@kballard If you are going to add stuff to this PR for the error, could you do that here. Otherwise, could you file an issue for it please?

[lilyball]

@nick29581 Good point. I'm still trying to figure out what the heck is going on with the errors though.

[steveklabnik]

This needs a rebase.

[lilyball]

Yes it does. I've been really busy lately, but I'm going to try and get back to this soon. Even before the rebase it was still having weird issues though, which is what was giving me problems in the first place.

[ghost]

I chatted with @kballard on IRC and we agreed I could pick this up. I'll open a new PR.

[alexcrichton]

Closing in favor of #17634, thanks @jakub-!