false positive for if-let-rescope on enums when the pattern exhaustively matches all cases with significant-drop fields

[steffahn]

Coming from #133167 (comment) and from this new forum thread, my takeaway of some remaining clear false positives is:

• if the if let matches on an enum and only some enum variant(s) have fields with potentially-significant destructor, but those are exhaustively matched in the non-else case of the if let, then there can be no actual change in behavior, because the else case is only reached in case the drop is not significant

• in the case of cargo-edit, this was if let Some(…irrefutable…) = expr on an Option<T>

• in the urlo thread, I’ve identified the case of if let Err(…irrefutable…) on a Result<(), E>

Here’s a simple repro for an Option case:

Code

#![allow(unused)]
#![warn(if_let_rescope)]

struct Struct;
impl Drop for Struct {
    fn drop(&mut self) {}
}

fn f(option_s: Option<Struct>) {
    if let Some(s) = option_s {
        // …
    } else {
        // …
    }
}

Here’s the code in the playground.

Current output

warning: `if let` assigns a shorter lifetime since Edition 2024
  --> src/lib.rs:10:8
   |
10 |     if let Some(s) = option_s {
   |        ^^^^^^^^^^^^^^--------
   |                      |
   |                      this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-if-let-scope.html>
help: the value is now dropped here in Edition 2024
  --> src/lib.rs:12:5
   |
12 |     } else {
   |     ^
note: the lint level is defined here
  --> src/lib.rs:2:9
   |
2  | #![warn(if_let_rescope)]
   |         ^^^^^^^^^^^^^^
help: a `match` with a single arm can preserve the drop order up to Edition 2021
   |
10 ~     match option_s { Some(s) => {
11 |         // …
12 ~     } _ => {
13 |         // …
14 ~     }}
   |

@rustbot label A-edition-2024, A-lints, L-if_let_rescope, C-enhancement

[traviscross]

cc @dingxiangfei2009 @nikomatsakis

[steffahn]

Looking back through the actual cargo-edit cases, I might have misinterpreted those somewhat, actually.

It looks like none of these 4 warnings really apply to this issue desciption

warning: `if let` assigns a shorter lifetime since Edition 2024
  --> src/version.rs:60:12
   |
60 |         if let Some((pre_ext, pre_ext_ver)) = prerelease_id_version(self)? {
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------------------
   |                                               |
   |                                               this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-if-let-scope.html>
help: the value is now dropped here in Edition 2024
  --> src/version.rs:72:9
   |
72 |         } else {
   |         ^
note: the lint level is defined here
  --> src/lib.rs:19:5
   |
19 |     if_let_rescope,
   |     ^^^^^^^^^^^^^^
help: a `match` with a single arm can preserve the drop order up to Edition 2021
   |
60 ~         match prerelease_id_version(self)? { Some((pre_ext, pre_ext_ver)) => {
61 |             if pre_ext == VERSION_BETA || pre_ext == VERSION_RC {
...
71 |             }
72 ~         } _ => {
73 |             self.increment_patch();
74 |             self.pre = semver::Prerelease::new(&format!("{VERSION_ALPHA}.1",))?;
75 |             Ok(())
76 ~         }}
   |

warning: `if let` assigns a shorter lifetime since Edition 2024
  --> src/version.rs:80:12
   |
80 |         if let Some((pre_ext, pre_ext_ver)) = prerelease_id_version(self)? {
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------------------
   |                                               |
   |                                               this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-if-let-scope.html>
help: the value is now dropped here in Edition 2024
  --> src/version.rs:92:9
   |
92 |         } else {
   |         ^
help: a `match` with a single arm can preserve the drop order up to Edition 2021
   |
80 ~         match prerelease_id_version(self)? { Some((pre_ext, pre_ext_ver)) => {
81 |             if pre_ext == VERSION_RC {
...
91 |             }
92 ~         } _ => {
93 |             self.increment_patch();
94 |             self.pre = semver::Prerelease::new(&format!("{VERSION_BETA}.1"))?;
95 |             Ok(())
96 ~         }}
   |

warning: `if let` assigns a shorter lifetime since Edition 2024
   --> src/version.rs:100:12
    |
100 |         if let Some((pre_ext, pre_ext_ver)) = prerelease_id_version(self)? {
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------------------
    |                                               |
    |                                               this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
    |
    = warning: this changes meaning in Rust 2024
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-if-let-scope.html>
help: the value is now dropped here in Edition 2024
   --> src/version.rs:108:9
    |
108 |         } else {
    |         ^
help: a `match` with a single arm can preserve the drop order up to Edition 2021
    |
100 ~         match prerelease_id_version(self)? { Some((pre_ext, pre_ext_ver)) => {
101 |             let new_ext_ver = if pre_ext == VERSION_RC {
...
107 |             Ok(())
108 ~         } _ => {
109 |             self.increment_patch();
110 |             self.pre = semver::Prerelease::new(&format!("{VERSION_RC}.1"))?;
111 |             Ok(())
112 ~         }}
    |

warning: `if let` assigns a shorter lifetime since Edition 2024
   --> src/version.rs:131:12
    |
131 |         if let Some((alpha, numeric)) = version.pre.as_str().split_once('.') {
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^^^^^^^^^^^^^^^^^^^^^^
    |                                         |
    |                                         this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
    |
    = warning: this changes meaning in Rust 2024
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-if-let-scope.html>
help: the value is now dropped here in Edition 2024
   --> src/version.rs:136:9
    |
136 |         } else {
    |         ^
help: a `match` with a single arm can preserve the drop order up to Edition 2021
    |
131 ~         match version.pre.as_str().split_once('.') { Some((alpha, numeric)) => {
132 |             let alpha = alpha.to_owned();
...
135 |             Ok(Some((alpha, Some(numeric))))
136 ~         } _ => {
137 |             Ok(Some((version.pre.as_str().to_owned(), None)))
138 ~         }}
    |

warning: `cargo-edit` (lib) generated 4 warnings (run `cargo fix --lib -p cargo-edit` to apply 4 suggestions)

Instead, these feature 2 different things:

• the last one with if let Some((alpha, numeric)) = version.pre.as_str().split_once('.') is a case where version has 2 fields, one of them the .pre field, that contain types using some

      pub(crate) struct Identifier {
      head: NonNull<u8>,
      tail: [u8; TAIL_BYTES],
  }

  struct (i.e. simply a custom Drop impl for memory management)… but here actually this version and version.pre as well are not actually in a temporary, but simply being borrowed. This is purely #137411 territory!

• the other 3 are also not matching the case this issue was trying to identify
  here, Some((pre_ext, pre_ext_ver)) = prerelease_id_version(self)? is working with
  this prerelease_id_version function returning CargoResult<Option<(String, Option<u64>)>>

  but this means that the Option being matched is actually just an Option<(String, Option<u64>)>, which are all known insignificant-drop types!

  instead, we look at CargoResult<T> which is a type synonym for anyhow::Result<T> – the problem is thus an Err value with custom destructor, anyhow::Error in particular

  the interesting question here instead if whether perhaps somehow one can manage to get ?-operator usage like that better "supported"…

I have no idea if that’s a common case though… (false positives involving ?, that is).

Because this example, too, is still a true false positive (the custom drop impl is never called at a different time between editions). At least I’m pretty sure it isn’t. The specific circumstances that might be easy to catch, and common, might be:

• the if let … else … it itself a statement-expression, or final block value (analogous to the considerations in Improve behavior of IF_LET_RESCOPE around temporaries and place expressions #137444 for if let … without else), so the only effect to pay attention to in the first place would be the question of "does this code change behavior if the else clause is reached?"
• the Result<T, E> type in question is produced by-value from a function call, and immediately propagated with the ? operator. This case seems both common and reasonably easy to detect. In this case one could disregard the temporary for the whole Result<T, E> and only consider the drop effects of a temporary for T instead. The E isn’t relevant because the early-returns means that when a E was produced, the else is never entered!

[compiler-errors]

For the record, the false positives that come from ? should be fixed by #137444, at least it's fixed for:

#![warn(if_let_rescope)]

use std::any::Any;

fn a() -> Result<Option<i32>, Box<dyn Any>> { todo!() }

fn foo() -> Result<(), Box<dyn Any>> {
    if let None = a()? {} else {}
    Ok(())
}