Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

if let should pre-drop wrt else #103108

Closed
est31 opened this issue Oct 16, 2022 · 3 comments
Closed

if let should pre-drop wrt else #103108

est31 opened this issue Oct 16, 2022 · 3 comments
Labels
F-if_let_guard `#![feature(if_let_guard)]` F-let_chains `#![feature(let_chains)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@est31
Copy link
Member

est31 commented Oct 16, 2022

Consider this snippet:

struct Foo<'a>(&'a mut u32);

impl<'a> Drop for Foo<'a> {
    fn drop(&mut self) {
        *self.0 = 0;
    }
}

fn main() {
    let mut foo = 0;
    
    if let Foo(0) = Foo(&mut foo) { // Doesn't compile
    } else {
        *&mut foo = 1;
    }
    
    if matches!(Foo(&mut foo), Foo(0)) { // Compiles
    } else {
        *&mut foo = 1;
    }
}

Here, if let complains about an access of foo in the else branch, while if is okay with this.

This is due to if let dropping its temporaries after the else block. This is not neccessary, it could also drop them before.

Similar to #103107

cc @Nilstrieb

@rustbot label T-lang

@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 16, 2022
@Noratrieb
Copy link
Member

This is probably a leftover from the time when if let used to desugar to a match, which keeps the scrutinee temporaries alive until the end of the match.

Dropping them early would be more consistent with if (it cannot be quite consistent as if let needs to keep them alive during the if block) but less consistent with match. Since if let is closer to if than match, I agree that it should behave more like it.

But I don't think we can change this. It could break code in extremely subtle undetectable cases (think of some kind of lock being held in the condition which is then required in the else branch). This also makes it hardly editionable, from a migration and also from a future rustc maintenance perspective.

@est31
Copy link
Member Author

est31 commented Oct 17, 2022

Good point about the consistency. There are analogous drop order optimization for matches too: You can drop the scrutinee temporaries before entering the body of any arm that is binding free and only followed by other binding-free arms, so cases like:

match foo {
    Enum::ValA(a) => {},
    Enum::ValB => {},
    _ => {},
}

for example, you would drop the temporaries before entering the bodies of the ValB and _ arms. Applied to if let, this would mean always dropping before else, and iff the pattern is binding free, also dropping before the then body.

But even without that extension, I'd be okay with the inconsistency with match if it makes more programs compile and drops resources more early on.

You are right, code might depend on the current behaviour and it might cause slight bugs. OTOH, one could also argue that if you rely on that specific quirk of drop order, then maybe your program was buggy from the start. <insert spacebar heating xkcd> :).

@matthewjasper matthewjasper added the F-if_let_guard `#![feature(if_let_guard)]` label Sep 11, 2023
@matthewjasper matthewjasper added the F-let_chains `#![feature(let_chains)]` label Sep 27, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 12, 2024
…r=jieyouxu

Rescope temp lifetime in if-let into IfElse with migration lint

Tracking issue rust-lang#124085

This PR shortens the temporary lifetime to cover only the pattern matching and consequent branch of a `if let`.

At the expression location, means that the lifetime is shortened from previously the deepest enclosing block or statement in Edition 2021. This warrants an Edition change.

Coming with the Edition change, this patch also implements an edition lint to warn about the change and a safe rewrite suggestion to preserve the 2021 semantics in most cases.

Related to rust-lang#103108.
Related crater runs: rust-lang#129466.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 13, 2024
…r=jieyouxu

Rescope temp lifetime in if-let into IfElse with migration lint

Tracking issue rust-lang#124085

This PR shortens the temporary lifetime to cover only the pattern matching and consequent branch of a `if let`.

At the expression location, means that the lifetime is shortened from previously the deepest enclosing block or statement in Edition 2021. This warrants an Edition change.

Coming with the Edition change, this patch also implements an edition lint to warn about the change and a safe rewrite suggestion to preserve the 2021 semantics in most cases.

Related to rust-lang#103108.
Related crater runs: rust-lang#129466.
@est31
Copy link
Member Author

est31 commented Nov 7, 2024

The given example compiles now on edition 2024, I suppose since #131984?

Also, #131154 contains a lot of useful discussion about this question.

@est31 est31 closed this as completed Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-if_let_guard `#![feature(if_let_guard)]` F-let_chains `#![feature(let_chains)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants