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

@rustbot

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

@dingxiangfei2009

cc @dingxiangfei2009 @nikomatsakis

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!

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(())
}

Navigation Menu

Repository navigation

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

Code

Current output

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Code

Current output

Activity

traviscross commented on Feb 22, 2025

compiler-errors commented on Feb 22, 2025

steffahn commented on Feb 23, 2025

compiler-errors commented on Feb 23, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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