# Migration guide for shorter `if let` scope

# Abstract

This guide proposes mechanical Edition 2024 migration strategies that we would like to implement in `cargo fix`.

In summary, there are at least two ways to rewrite the rejected `if let`s each with its own merits and demerits.

# Lifetime rules under impact

The most relevant rule under the impact that is currently running, even at Edition 2024, is that the temporaries created in evaluation are only dropped up until the end the most nested enclosing block or end of the statement in a block.

One example would look like the following.

call1(
    temp1(),
    {
        let x = &temp2();
        x.compute()
    }
);

In the above example the temporary value of `temp1()` is dropped at the end of the semicolon at Line 7; the temporary value of `temp2()` is dropped at the end of the block due to the fact that this block is enclosing the `let x = &temp2();` statement.

Up until Edition 2021, use of `if let $pattern = $expr { .. } else { .. }` either as an expression or bare statement is treated as a simple expression, so that the temporaries generated by evaluating `$expr` are assigned a temporary lifetime that is as long as the parent `if let` (sub-)expression. In other words, if we substitute `temp1()` with a `if let` in the previous example, `$expr` receives a temporary lifetime that ends at the last semicolon, because that was where `temp1()` would have been dropped.

Unfortunately that means that `temp1()` is also alive when `else` branch is evaluated, creating problems that the RFC would like to solve with an Edition change by shortening its temporary lifetime to only cover the `if let` pattern matching and the consequent block and only up to right before `else` branch. More examples from the last crater run are illustrated in the following section.

# Crater run with this Edition change

According to the earlier crater run, we have identified these most common issues. The more complete report is in this [separate document](https://hackmd.io/@dingxf/Hk9PAnmEC).

1. `ref` in pattern binding when by value binding is possible.

let home = if let Ok(ref value) = std::env::var("HOME") {
            //                    ^~~~~~~~~~~~~~~~~~~~~ does not live long enough
    value
} else {
    "$HOME"
}; // `std::env::var(..)` used to be dropped here

2. In a nested expression, the use of `if let` generates a temporary that the borrow checker rejects now because it is the temporary is used as part of a call argument.

call1(
    call2(
        if let Some(value) = items.as_ref().call() {
            //               ^~~~~~~~~~~~~~ does not live long enough
            value
        } // <- edition 2024 drops here
        else {
            0
        }
    )
); // <- `items.as_ref()` used to be dropped here

3. The whole initializer to `if let` is immediately behind a borrow.

call(
    if let Some(value) = &compute() {
        //                ^~~~~~~~~ does not live long enough
        value
    } // <- edition 2024 drops here
    else {
        &DEFAULT
    }
); // <- used to be dropped here

# Migration suggestion

This proposal will suggest two re-write strategies that will apply to these three scenarios.

## The `match` rewrite

`match` as of edition 2024 has a lifetime semantics that the scrutinee value lives as long as the parent expression unless the parent is a block or a statement. Several cases of our own internal AST desugering into HIR, including `for` loops and the upcoming `for await` loops and macros like `assert_eq`, heavily rely on this, unfortunately undocumented, property.

The suggested rewrite is that, whenever a sub-expression is rejected, the `if let` is rewritten so that the rejected sub-expression is `match`ed against and bounded to a new name, so that lifetime is extended and compatible with pre-2024 semantics.

In a nutshell, the rewrite rule is the following.

// before
(
    ...
    if let <pat> = (... <rejected> ...) {
        ...
    } else {
        ...
    }
    ...
)
// after
(
    ...
    match <rejected> {
        new_name => if let <pat> = (... new_name ...) {
            ...
        } else {
            ...
        }
    }
    ...
)
// or `rejected` is the whole initializer, fuse the `<pat>` pattern into `match`

### Commentary

I think this is the most straight-forward rewrite to implement and it recovers the original semantics perfectly. The major downside is that it looks complicated and `clippy` may consider it unnecessary and lint against it after the fact.

In short, to implement this, the rewrite rule is clear, but one existing `clippy` lint on `match` with only one non-trivial pattern needs a lot more tweaking.

## The lifting rewrite

It is a strategy that we already have been emitting occasionally today, which is to lift the the rejected sub-expression into its own dedicated variable binding, into a new block `{...}` surrounding the original `if let` expression.

// before
(
    ...
    if let <pat> = (... <rejected> ...) {
        ...
    } else {
        ...
    }
    ...
)
// after
(
    {
        let new_name = <rejected>;
        if let <pat> = (... new_name ...) {
            ...
        } else {
            ...
        }
    }
)

### Commentary

I consider this as a more human-readable and natural rewrite. However, there is a different de-merit to this strategy. It is possible that some sub-expression in the `rejected` expression needs to get its lifetime extended as well, which would have been handled by the `match` rewrite.

Even then, it is not guaranteed to work. For instance, to fix Case 2. the rewrite needs to lift `items.as_ref()` all the way before `call1`.

The further hazard is that the order of evaluation of expressions needs to be change. This is unfortunately not something we can reason on behalf of our users.

// after rewrite that actually works
{
    let items = items.as_ref();
    call1(
        call2(
            if let Some(value) = items.call() {
                ...
            } else {
                ...
            }
        )
    )
}

# Argument against migration at all

I would present an argument against an automated migration at all. The crater run shows that the impact is farely contained. Furthermore, the affected configurations are clearly broken down to three categories. There is a possibility that presenting the lifetime error as is can prompt users to restructure their code for better readability, reduced complexity by breaking down the nested expression that we have seen in the crater run. The suggestions and rewrite rules that are presented earlier seem to, on the contrary, introduce even more cognitive complexity to the code only for the sake of making it compiling again.

# The middle ground

I believe that there is a compromise here that we can suggest these rewrites as notes attached to the original lifetime errors. We can further lint on the expression complexity and provide further justification for this edition change wherever `if let` is involved. If there is an option in `cargo fix` to apply the suggestions anyway in spite of the warning about the change in code semantics, we can comply and apply it anyway.

<summary> My personal pick...
<details>
As an implementor, I personally suggest `match` rewrite because it preserves the evaluation order and lifetime assignment perfectly. If someone needs to upgrade the crate to Edition 2024, `match` rewrite is almost perfect. We may or may not need to address how to resolve the conflict with `clippy`.
</details>
</summary>