# Crater [run](https://crater-reports.s3.amazonaws.com/pr-125151/index.html) #125151 report

The crater run is executed by assuming the `iflet_rescope` feature gate throughout in order to assess the impact and proposals for lints, compiler assistance and migration support.

## Abstract

This report serves as a response to [this comment](https://github.com/rust-lang/rust/pull/107251#issuecomment-1779644448).

The compile error in the ecosystem due to introduction of this rule share the same trait that it leverages the behaviour that temporary values stays alive until the end of the statement, when the `if let` is nested in. It occurs at places that it is more convenient and waives the necessity of extra variable bindings. Still given that, the crater only observed code under Case *c*, or embedding complex decision-making into a much bigger expression, that can be fixed by lifting the expiring expression into a binding because the type is not *significant* by the definition of the comment linked above.

We hoped to catch violation that fails at runtime, or use and abuse of `Mutex<()>` so to speak. This crater run has not catched an instance of such in the wild. We do not exclude this possibility that between now and possible landing of this rule. One may attempt to do so right after publication of this piece but we probably should develop a lint against this practice.

## Build failure cases

### Case a. `ref`-borrowing in `if let` in a nested expression, mostly function calls.

I somehow agree with the use of it because of apparent convenience.

[sisterm](https://crates.io/crates/sisterm/3.0.2)

error[E0716]: temporary value dropped while borrowed
   --> src/main.rs:343:31
    |
343 |         if let Ok(ref home) = env::var("HOME") { home } else { "$HOME" } );
    |         ----------------------^^^^^^^^^^^^^^^^--------------------------
    |         |                     |                       |
    |         |                     |                       temporary value is freed at the end of this statement
    |         |                     creates a temporary value which is freed while still in use
    |         borrow later used here
    |
    = note: consider using a `let` binding to create a longer lived value

We have similar code from [haddock v0.2.1]

### Case b. In nested function calls, implicitly borrowing via pattern
[psi-lang 0.3.0](https://crates.io/crates/psi-lang/0.3.0)

error[E0716]: temporary value dropped while borrowed
   --> src/interpreter.rs:115:38
    |
113 |                 self.env.raw_expr(
    |                          -------- borrow later used by call
114 |                     &id[1],
115 |                     if let Some(i) = items.values().get(iteration as usize) {
    |                                      ^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
116 |                         i
117 |                     } else {
    |                     - temporary value is freed at the end of this statement
    |
    = note: consider using a `let` binding to create a longer lived value

### Case c. The whole value being pattern-matched is behind a borrow right away

They take a form of `if let $pat = & $expr`.
[haddock 0.2.1](https://crates.io/crates/haddock/0.2.1)

error[E0716]: temporary value dropped while borrowed
  --> src/commands/stop.rs:85:42
   |
83 | ...                   .chain(
   |                        ----- borrow later used by call
84 | ...                       if let Some(timeout) =
85 | ...                           &args.timeout.map(|timeout| timeout.to_string())
   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
...
88 | ...                       } else {
   |                           - temporary value is freed at the end of this statement
   |
   = note: consider using a `let` binding to create a longer lived value

We have this from [toipe](https://github.com/BabyMelvin/typeman/tree/0cc2bc6c211aa36b6bf1a60c6abbfda3b6304a96) and one unpublished [Rust repo](https://github.com/HaNaK0/project_manager/tree/54dcca71e2cb780c9ab30f98fcf9d9745e67eeca).

### Ergonomic issues from confusing error messages involving (proc-)macros

This is raised by @estebank.

[smodel v1.0.13](https://crates.io/crates/smodel/1.0.13)

error[E0716]: temporary value dropped while borrowed
   --> src/lib.rs:37:9
    |
37  | /         smodel! {
38  | |             mod smodel = crate;
39  | |
40  | |             type Arena = Arena;
...   |
121 | |             }
122 | |         }
    | |         ^
    | |         |
    | |         temporary value is freed at the end of this statement
    | |_________creates a temporary value which is freed while still in use
    |           borrow later used here
    |
    = note: consider using a `let` binding to create a longer lived value
    = note: this error originates in the macro `smodel` (in Nightly builds, run with -Z macro-backtrace for more info)

[hd5file 0.1.1](https://crates.io/crates/hdf5file/0.1.1)

error[E0716]: temporary value dropped while borrowed
  --> examples/object.rs:35:40
   |
33 | /                 println!(
34 | |                     "{}",
35 | |                     track_assert_some!(track!(object_path)?.to_str(), trackable::error::Failed)
   | |                     -------------------^^^^^^^^^^^^^^^^^^^^------------------------------------
   | |                     |                  |
   | |                     |                  creates a temporary value which is freed while still in use
   | |                     temporary value is freed at the end of this statement
36 | |                 );
   | |_________________- borrow later used here
   |
   = note: consider using a `let` binding to create a longer lived value

## Runtime failure cases

I am a bit overwhelmed by the amount of flaky tests, so my coverage may not be satisfactory.

I am covering most of the published crates in this section, but unfortunately those test failures don't seem to be related to use of `if let`.

What this analysis explicitly looked out for, was to determine if a test case failed because of a use of `if let`, *with a non-trivial `else` block*, that produces temporary values with a *significant `Drop` implementation*. Fortunately for us in this case, most failing test cases are located in crates that only involved types without significant `Drop` such as `String`, `Vec` and other standard containers.

:::spoiler
##### Racy tests
Some tests have been written so that the states overwrite each other when they are run in parallel. This applies to several Advant of Code repositories.
##### Crater container running out of space
:::

## Applicability of this change to macro code

This is not explicitly spelled in the original RFC, but I think this change shall be contained only in code span generated by Edition 2024 proc-macros or macros.

As of writing, 4 Jun 2024, the original PR does not implement a check on the edition of the span covering the `if` node. It is now under work.

## Lint proposals

I would like to put forth the following possible migration lints and fixes.

### `ref (mut)` pattern in matching

I propose to lint on Case a. when it leads to compile error, and we would say that `ref` shall be either dropped, or the initializer should be lifted into a block statement. How specifically this can be done is found in the migration section below.

### borrows on the initializer expr

I propose to lint on Case c. when it leads to compile error, and we would leave a note that the appropriate fix is to review if the borrow is necessary or a `let`-binding should be made. Similarly, we will try our best to locate a location to lift the initializer into as a `let`-binding.

### complex, nested expression in function call arguments

I propose to leave a note that having nested `if let` expression in function call arguments is hurting readability.

Unfortunately, other than suggesting lifting the appropriate span of code into a variable binding, we have to leave the choice to the user.

## Migration proposals
We can in general apply this procedure to approximately fix the error for migration user.

1. Identify the expression that requires a lifetime extension, which we have been doing.
2. Traverse the expression tree up from the reported expression node, until we encounter
- `if`, `loop`, `for`, `match`-arm or just a plain `{}` block
- the immediate parent that is a statement in block ending with a semi-colon
3. Suggest a variable binding to hold the expiring expression.

For instance, we would suggest a re-write of the following...

{
    call(
        nested_call(
            if let Some(ref value) = please_extend_me() {
                value
            } else {
                something_else()
            }
        )
    );
}

into...

{
    let extended = please_extend_me();
    call(
        nested_call(
            if let Some(ref value) = extended {
                value
            } else {
                something_else()
            }
        )
    )
}