Stabilize let_chains in Rust 1.64

[c410-f3r]

Stabilization proposal

This PR proposes the stabilization of #![feature(let_chains)] in a future-compatibility way that will allow the possible addition of the EXPR is PAT syntax.

Tracking issue: #53667
Version: 1.64 (beta => 2022-08-11, stable => 2022-10-22).

What is stabilized

The ability to chain let expressions along side local variable declarations or ordinary conditional expressions. For example:

pub enum Color {
    Blue,
    Red,
    Violet,
}

pub enum Flower {
    Rose,
    Tulip,
    Violet,
}

pub fn roses_are_red_violets_are_blue_printer(
    (first_flower, first_flower_color): (Flower, Color),
    (second_flower, second_flower_color): (Flower, Color),
    pick_up_lines: &[&str],
) {
    if let Flower::Rose = first_flower
        && let Color::Red = first_flower_color
        && let Flower::Violet = second_flower
        && let Color::Blue = second_flower_color
        && let &[first_pick_up_line, ..] = pick_up_lines
    {
        println!("Roses are red, violets are blue, {}", first_pick_up_line);
    }
}

fn main() {
    roses_are_red_violets_are_blue_printer(
        (Flower::Rose, Color::Red),
        (Flower::Violet, Color::Blue),
        &["sugar is sweet and so are you"],
    );
}

Motivation

The main motivation for this feature is improving readability, ergonomics and reducing paper cuts.

For more examples, see the RFC.

What isn't stabilized

• Let chains in match guards (if_let_guard)

• Resolution of divergent non-terminal matchers

• The EXPR is PAT syntax

History

• On 2017-12-24, RFC: if- and while-let-chains
• On 2018-07-12, eRFC: if- and while-let-chains, take 2
• On 2018-08-24, Tracking issue for eRFC 2497, "if- and while-let-chains, take 2
• On 2019-03-19, Run branch cleanup after copy prop
• On 2019-03-26, Generalize diagnostic for x = y where bool is the expected type
• On 2019-04-24, Introduce hir::ExprKind::Use and employ in for loop desugaring
• On 2019-03-19, [let_chains, 1/6] Remove hir::ExprKind::If
• On 2019-05-15, [let_chains, 2/6] Introduce Let(..) in AST, remove IfLet + WhileLet and parse let chains
• On 2019-06-20, [let_chains, 3/6] And then there was only Loop
• On 2020-11-22, Reintroduce hir::ExprKind::If
• On 2020-12-24, Introduce hir::ExprKind::Let - Take 2
• On 2021-02-19, Lower condition of if expression before it's "then" block
• On 2021-09-01, Fix drop handling for if let expressions
• On 2021-09-04, Formally implement let chains
• On 2022-01-19, Add tests to ensure that let_chains works with if_let_guard
• On 2022-01-18, Introduce enhanced_binary_op feature
• On 2022-01-22, Fix let_chains and if_let_guard feature flags
• On 2022-02-25, Initiate the inner usage of let_chains
• On 2022-01-28, [WIP] Introduce ast::StmtKind::LetElse to allow the usage of let_else with let_chains
• On 2022-02-26, 1 - Make more use of let_chains
• On 2022-02-26, 2 - Make more use of let_chains
• On 2022-02-27, 3 - Make more use of let_chains
• On 2022-02-28, 4 - Make more use of let_chains
• On 2022-02-28, 5 - Make more use of let_chains
• On 2022-02-28, 6 - Make more use of let_chains
• On 2022-03-01, 7 - Make more use of let_chains
• On 2022-03-01, 8 - Make more use of let_chains
• On 2022-03-01, 9 - Make more use of let_chains
• On 2022-03-08, Warn users about || in let chain expressions

From the first RFC (2017-12-24) to the theoretical future stabilization day (2022-10-22), it can be said that this feature took 4 years, 9 months and 28 days of research, development, discussions, agreements and headaches to be settled.

Divergent non-terminal matchers

More specifically, #86730.

macro_rules! mac {
    ($e:expr) => {
        if $e {
            true
        } else {
            false
        }
    };
}

fn main() {
    // OK!
    assert_eq!(mac!(true && let 1 = 1), true);

    // ERROR! Anything starting with `let` is not considered an expression
    assert_eq!(mac!(let 1 = 1 && true), true);
}

To the best of my knowledge, such error or divergence is orthogonal, does not prevent stabilization and can be tackled independently in the near future or effectively in the next Rust 2024 edition. If not, then https://github.com/c410-f3r/rust/tree/let-macro-blah contains a set of changes that will consider let an expression.

It is possible that none of the solutions above satisfies all applicable constraints but I personally don't know of any other plausible answers.

Alternative syntax

Taking into account the usefulness of this feature and the overwhelming desire to use both now and in the past, let PAT = EXPR will be utilized for stabilization but it doesn't or shall create any obstacle for a possible future addition of EXPR is PAT.

The introductory snippet would then be written as the following.

if first_flower is Flower::Rose 
    && first_flower_color is Color::Red
    && second_flower is Flower::Violet
    && second_flower_color is Color::Blue
    && pick_up_lines is &[first_pick_up_line, ..]
{
    println!("Roses are red, violets are blue, {}", first_pick_up_line);
}

Just to reinforce, this PR only unblocks a possible future road for EXPR is PAT and does emphasize what is better or what is worse.

Tests

• Verifies the drop order of let chains and ensures it won't change in the future in an unpredictable way

• AST lowering does not wrap let chains in an DropTemps expression

• Checks pretty printing output

• Verifies uninitialized variables due to MIR modifications

• A collection of statements where let expressions are forbidden

• All or at least most of the places where let chains are allowed

• Ensures that irrefutable lets are allowed in let chains

• issue-88498.rs, issue-90722.rs, issue-92145.rs and issue-93150.rs were bugs found by third parties and fixed overtime.

• Indexing was triggering a ICE due to a wrongly constructed MIR graph

• Protects the precedence of && in relation to other things

• let_chains, as well as if_let_guard, has a valid MIR graph that evaluates conditional expressions correctly

Most of the infra-structure used by let chains is also used by if expressions in stable compiler versions since #80357 and #88572. As a result, no bugs were found since the integration of #88642.

Possible future work

• Let chains in match guards is implemented and working but stabilization is blocked by if_let_guard.

• The usage of let_chains with let_else is possible but not implemented. Regardless, one attempt was introduced and closed in [WIP] Introduce ast::StmtKind::LetElse to allow the usage of let_else with let_chains #93437.

Thanks @Centril for creating the RFC and huge thanks (again) to @matthewjasper for all the reviews, mentoring and MIR implementations.

Fixes #53667

[rust-highfive]

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

[rust-highfive]

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

[est31]

I wonder about linting for irrefutable assignments. This code gives you a irrefutable_let_patterns warning:

if let v = 4 + 4 { /*...*/ }
//^ WARNING: irrefutable `if let` pattern

On a similar note, I expect this code to lint:

if let v = 4 + 4 && let w = 8 + 8 { ... }

But there is no warning at all. It's a bit confusing to have an if here despite it never visiting the else clause.

Generally, it makes sense to not lint for irrefutable patterns inside if let chains, because often you might need to create a binding that you then use later. I've done this a few times for my clippy PR rust-lang/rust-clippy#8437 . And even if the binding is never used by later patterns, it might still create some side effects that you want.

However, I think both trailing and leading irrefutable patterns should be linted, as well as the combination of both cases, let chains made up of only irrefutable assignments. Leading assignments can be made part of the body the if is part of, and trailing assignments can be made part of the body, and in the case of chains made up only of irrefutable patterns one doesn't need an if at all.

I'm not sure if this lint needs to be in rust, or it is better present in clippy though. As the irrefutable_let_patterns lint is builtin, one can make the case that this one should be builtin, too.

[joshtriplett]

@est31 I agree. Technically we can always introduce that lint later, but I think we should have it in place before we ship let chains in stable, since it will affect how people start writing let chains.

[estebank]

@joshtriplett, should a PR like this one should be approved by consensus of t-lang, or should t-compiler take on that responsibility once we receive a go ahead? Either way, I don't currently have the bandwidth to shepherd this myself and would hate to be the cause of it stalling.

[joshtriplett]

@estebank Historically, I think the way we've handled that is to have two separate aspects of approval: the feature stabilization, and the PR content.

• T-lang reviews and approves "do we want to stabilize this", which doesn't depend on the exact content of the PR. (That review and approval would be based on input about the state of the available feature; if T-compiler tells us a feature isn't ready to consider stabilization, we're not going to approve a stabilization.)
• Anyone who can review the actual content of the PR does so. For small PRs that review process might consist of one reviewer and an r+; for larger or more invasive PRs that could potentially include an FCP or MCP.

In this particular case, while the PR is large, it looks like a straightforward stabilization of the feature gate, plus the extensive updating of usages of the feature gate. Seems like any compiler reviewer could reasonably r+ this, once the stabilization is approved?

[joshtriplett]

@rust-lang/lang, shall we stabilize let chains?

@rfcbot merge

@rfcbot concern irrefutable-lint

[rfcbot]

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

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

Concerns:

• add-test-to-confirm-no-let-else-in-let-chain resolved by Stabilize let_chains in Rust 1.64 #94927 (comment)
• doc-pr resolved by Stabilize let_chains in Rust 1.64 #94927 (comment)
• irrefutable-lint resolved by Stabilize let_chains in Rust 1.64 #94927 (comment)
• let-expressions-in-macros resolved by Stabilize let_chains in Rust 1.64 #94927 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

[flip1995]

I think both trailing and leading irrefutable patterns should be linted

FWIW Clippy has an internal lint for exactly this inside the if_chain! macro.

rust/src/tools/clippy/clippy_lints/src/utils/internal_lints.rs

Lines 1184 to 1198 in 95561b3

	if is_first_if_chain_expr(cx, block.hir_id, if_chain_span) {
	span_lint(
	cx,
	IF_CHAIN_STYLE,
	if_chain_local_span(cx, local, if_chain_span),
	"`let` expression should be above the `if_chain!`",
	);
	} else if local.span.ctxt() == block.span.ctxt() && is_if_chain_then(after, block.expr, if_chain_span) {
	span_lint(
	cx,
	IF_CHAIN_STYLE,
	if_chain_local_span(cx, local, if_chain_span),
	"`let` expression should be inside `then { .. }`",
	);
	}

This lint could be used as a basis for implementing the same for let_chains.

[est31]

I've filed a PR to extend the irrefutable patterns lint to the prefixes/suffixes of let chains: #94951

@flip1995 thanks for the suggestion about the clippy lint. Maybe that lint is even the cause for my suggestion above, as it's possible I got warned by it back when I wrote my clippy PR, I'm not sure any more. I've checked out the implementation to see if I can do something differently in #94951. Mostly I note that it's more compact, probably due to the rich set of utilities that clippy has. Also, maybe the error messages can be improved a little.

[flip1995]

I've checked out the implementation to see if I can do something differently in #94951.

Yeah the clippy utils really help with writing lints. It might also just be that internal Clippy lints usually don't really care about producing good suggestions/messages, because they're not user facing and we have a lower bar there.

[nikomatsakis]

@rfcbot fcp reviewed

+1 in general

[nikomatsakis]

@rfcbot concern doc-pr

Do we have a pending update to the Rust reference?

[nikomatsakis]

Big thank you to @c410-f3r and others involved in pushing this feature forward!

[estebank]

@rfcbot concern let_else-interaction

Do we have existing tests verifying that this feature interacts nicely with let_else, both in parsing and semantics? It might be enough to extend https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs with a single let/else expression.

[joshtriplett]

@estebank When that has come up previously, our general conclusion of the interaction between let chains and let else was "don't": they don't mix at all.

Adding a failing test to ensure that the compiler does not allow attaching an else to a let that's part of an if or while condition would help.

[c410-f3r]

Rebased due to recent code conflict.

[joshtriplett]

@bors r+

[bors]

📌 Commit 3266460 has been approved by joshtriplett

It is now in the queue for this repository.

[WiSaGaN]

Just for reference, this was reverted in #100538, thus will likely not be in 1.64 release.

[jhpratt]

Because of this ^^

@rustbot label -relnotes