Proposal: stabilize if_let_rescope for Edition 2024

[dingxiangfei2009]

Tracked by #124085

Summary

if_let_rescope is an implementation of the #124085 through #107251. This change did not come with a RFC. This proposal will aim to describe the change to the language fully.

if let and the current unstabilized if let-chain has a caveat to the assignment of lifetime rules regarding temporary values generated from initializer expressions.

//@ edition: 2021
if let Some(value) = droppy().get() {
    // do something
} else {
    // `droppy()` is still alive here
    // but it is out of reach as well.
}

Instead, after stabilizing this, we have this effect.

//@ edition: 2024
#![feature(if_let_rescope)]
if let Some(value) = droppy().get() {
    // do something
} else {
    // `droppy()` is already dropped here
}

This will allow us to be more consistent with let $pat = $expr else { $diverge; } where temporaries from $expr are dropped before entering the $diverge branch in today's language.

Given that this is an Edition breaking change, a lint is developed and tested in the field with crater runs.

What is being proposed for stabilization

In #107251, a new kind of scope data IfThenRescope is introduced and gated behind the edition and this feature gate. Upon stabilization, this scope will work like a terminating scope for variables and temporary values generated from the let-bindings.

What is more significant is the case where if let is used as a non-trivial value-producing subexpression in a larger evaluation context, in which it will have semantic impact to lifetimes and object drop orders.

Here are some breaking changes that will be introduced by stabilization.

#![feature(if_let_rescope)]
// borrowck may reject this ...
run(if let Some(ref value) = droppy().borrow() {
    value
} else {
    something_else()
})
// ...

A marginal amount of crates are discovered by previous crater runs in which this change leads to rejection from the borrow checker, because droppy() does not live long enough.

The migration lint that is now implemented on master suggests a rewrite to restore the old semantics perfectly, but not automatically applicable due to the current limitation of machine applicable lints and our internal support for overlapping suggestion spans.

#![feature(if_let_rescope)]
// borrowck will not reject this ...
run(match droppy().borrow() {
    Some(ref value) => { value }
    _ => { something_else() }
})

A more subtle and silent breakage could be involving use of types with significant drop implementation and synchronization primitives such as Mutexes.

if let Some(value) = mutex.lock().unwrap().get() {
    // do something
} else {
    // somehow code expects the lock to be held here ...
    // or because the mutex was only improperly used as a semaphore or for signalling ...
}

Although the crater run did not find breakage in runtime behaviour through cargo test, we proceed with developing lints to detect those cases and rewrite them into matches. Here the suggestion notes are machine applicable because we can coalesce the overlapping spans in one pass.

In the latest crater run we found a marginal population of crates that are truly impacted by this lint, revealing corner cases with the lint which are now properly handled and tested.

Future interactions

Currently there is another possible use of pattern matching in control flows that complements if let chain is the is operator proposed in rust-lang/rfcs#3573. This change both runs along the same principle of no unnecessary binding and long lifetime beyond the applicable scope from a pattern-matching predicate. With this change, the implementation of is would be believably less error-prone due to the unexpectedly long lifetimes.

This leaves match being the only exception to this principle. In fact, our mitigation and migration strategy is based on it. It might remains as an acceptable exception and further be publicized and inadvertently advertised as such.

In general, this change is not expected to influence or hinder major language design.

[dingxiangfei2009]

@rustbot labels +T-lang +A-edition-2024

[jieyouxu]

Note that I'm not very good at analysing crater run results (not just what broke, but also what happened to the crates that don't build for other reasons), but I think it would be important to have a crater analysis that includes the exact number of breakages and crates that didn't build (or otherwise what the analysis did not cover), and if feasible classification of the nature of breakages. Regarding the lint, as mentioned it's not machine-applicable due to overlapping suggestions not possible in the current fix application infra, but also as is usual with proc-macro detection, any crates that forwards user spans with no distinguishing syntax context will cause Span::from_expansion to return false (technically not-our-bug, but still).

[joshtriplett]

FCP for stabilization:

@rfcbot merge

[rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

@rfcbot reviewed

[nikomatsakis]

@rfcbot reviewed

Copying my reasoning from the meeting notes for posterity:

This introduces a disparity between match and if-let, which I don't like, but it moves towards earlier drop, which I do like, and it also means that if and if let work consistently:

struct DropOrder;

impl DropOrder {
    fn test(&self) -> bool {
        false
    }
}

impl Drop for DropOrder {
    fn drop(&mut self) {
        eprintln!("drop");
    }
}

fn main() {
    if DropOrder.test() {
    // ^^^^^^^^^ dropped before entering else
        eprintln!("this body");
    } else {
        eprintln!("else body");
    }
}

You can see the behavior here, it prints "drop" first.

In short, this is moving us closer to the world I want, which is one in which match is able to drop more eagerly.

[tmandry]

@rfcbot reviewed

I'm confident in the semantics proposed, given the above reasoning, and that we can make the edition experience good enough for our users, though it might require some dialing in before we ship.

[rfcbot]

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

[joshtriplett]

Also, when we document this, we should point out that while it's possible to have surprises with Mutex<()> getting dropped early, this change avoids surprises with it being dropped late, such as surprising self-deadlocks in the else branch.

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this in the meeting today... and it's now in FCP.

[rfcbot]

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.