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

Let chain temporaries should drop before else #103248

Closed
est31 opened this issue Oct 19, 2022 · 5 comments
Closed

Let chain temporaries should drop before else #103248

est31 opened this issue Oct 19, 2022 · 5 comments
Labels
F-let_chains `#![feature(let_chains)]`

Comments

@est31
Copy link
Member

est31 commented Oct 19, 2022

Due to #103034, now the implementation of let chains has changed, to drop temporaries created by let after the else block terminates. Prior behaviour has mirrored the pre-dropping bheaviour of if as well as bool conditionals in the let chain, while current behaviour mirrors if let. However, post-dropping is suboptimal (#103108). In particular, before #103034 this was compiling:

#![feature(let_chains)]
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;

    // compiles fine on rustc 1.66.0-nightly (a24a020e6 2022-10-18)
    // but fails on rustc 1.66.0-nightly (4b8f43199 2022-10-19)
    if true && let Foo(0) = Foo(&mut foo) {
    } else {
        *&mut foo = 1;
    }

    // compiles fine
    let Foo(0) = Foo(&mut foo) else {
        *&mut foo = 1;
        panic!()
    };
    
    // compile error - unfortunate but might not be fixed
    if let Foo(0) = Foo(&mut foo) {
    } else {
        *&mut foo = 1;
    }
    
    if true && matches!(Foo(&mut foo), Foo(0)) { // Compiles
    } else {
        *&mut foo = 1;
    }
}

But now it doesn't compile any more. I am not adding regression labels because it hasn't affected any of my own code, and it's a nightly feature.

For if let it might not be changed any more due to backwards compatibility concerns, but for let chains there are no such concerns as it is a new feature: code is only broken when developers edit it.

@rustbot label F-let_chains

@Alexendoo
Copy link
Member

Ran into a case that stopped compiling for me after the nightly update, in this case it also didn't compile with the else removed

playground

#![feature(let_chains)]

use syn::{Attribute, Meta};

fn f(attr: &Attribute) -> bool {
    if let Meta::NameValue(name_value) = attr.parse_meta().ok().unwrap()
        && let path_idents = name_value.path.segments.iter()
        /* other stuff */
    {
        
    }
    
    true
}

@est31
Copy link
Member Author

est31 commented Oct 24, 2022

@Alexendoo that's interesting, and definitely a bug (if you replace it with nested let's it works). I think that's a different issue though than this one. This issue is only about else. Can you file a bug report about it and cc the author of #103034? I confirm that on nightly-2022-10-17 it compiles fine, and on recent nightlies it fails, so it's very likely caused by that PR.

@Alexendoo
Copy link
Member

👍 opened #103476

@est31
Copy link
Member Author

est31 commented Oct 27, 2022

Okay it seems that lang team members are not a fan of this, so I think I'll close this. If it is fixed, then it should also be fixed for if let, see #103108 .

@est31 est31 closed this as completed Oct 27, 2022
@joshtriplett
Copy link
Member

@est31 FWIW, given that the let binding isn't in scope in the else branch, it seems hard to imagine a scenario in which an ordinary if let needs to wait until after the else branch to drop temporaries. We could potentially change both if-let and let-chains to consistently drop before the else branch, given a crater run. Might be worth considering, if it doesn't actually break anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-let_chains `#![feature(let_chains)]`
Projects
None yet
Development

No branches or pull requests

4 participants