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

Temporaries in match conditions should be freed if not required to stick around #38355

Closed
Manishearth opened this Issue Dec 13, 2016 · 17 comments

Comments

Projects
None yet
8 participants
@Manishearth
Member

Manishearth commented Dec 13, 2016

Currently, the following code works:

    let x = RefCell::new(true);
    
    if *x.borrow() {
        x.borrow_mut();
    }

but this panics:

    let x = RefCell::new(true);
    
    match *x.borrow() {
        _ => { x.borrow_mut(); }
    }

(playpen)

Due to #12033 (implemented in #36029) we free temporaries from if conditions.

We should also free temporaries in match subjects if there are no active borrows from them.

This would be semantically a [breaking change]. I do not think it will change how things compile. I am not sure if we need an rfc for this because IMO the fact that this doesn't work is a bug.

cc @eddyb @nikomatsakis

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Dec 13, 2016

Member

I also feel like this used to work.

Member

Manishearth commented Dec 13, 2016

I also feel like this used to work.

@durka

This comment has been minimized.

Show comment
Hide comment
@durka

durka Dec 13, 2016

Contributor

Now wait a minute, match feels very different from the if example because the pattern could bind to the expression, in which case the borrow would definitely need to be kept alive. It doesn't seem right that changing the pattern(s) should change the lifetime semantics of the match expression.

Contributor

durka commented Dec 13, 2016

Now wait a minute, match feels very different from the if example because the pattern could bind to the expression, in which case the borrow would definitely need to be kept alive. It doesn't seem right that changing the pattern(s) should change the lifetime semantics of the match expression.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Dec 13, 2016

Member

It doesn't seem right that changing the pattern(s) should change the lifetime semantics of the match expression.

That's not what I'm proposing.

I'm saying that if the match expression returns something that does not have a lifetime tied to that of the temporary, we should destroy it immediately. Changing the patterns will not change anything here.

In this case, we return a boolean, with no lifetime. I can imagine that the following match would work:

let x = RefCell::new(vec![1,2,3]);

match x.borrow().len() {
 ...
}

but this would not:

let x = RefCell::new(vec![1,2,3]);

match x.borrow().index(1) { // returns an &u8, lifetime tied to borrow
 ...
}
Member

Manishearth commented Dec 13, 2016

It doesn't seem right that changing the pattern(s) should change the lifetime semantics of the match expression.

That's not what I'm proposing.

I'm saying that if the match expression returns something that does not have a lifetime tied to that of the temporary, we should destroy it immediately. Changing the patterns will not change anything here.

In this case, we return a boolean, with no lifetime. I can imagine that the following match would work:

let x = RefCell::new(vec![1,2,3]);

match x.borrow().len() {
 ...
}

but this would not:

let x = RefCell::new(vec![1,2,3]);

match x.borrow().index(1) { // returns an &u8, lifetime tied to borrow
 ...
}
@durka

This comment has been minimized.

Show comment
Hide comment
@durka

durka Dec 13, 2016

Contributor

Oh I see, it was *x.borrow() not x.borrow(). I don't see how this can be changed without (silently!) breaking lots of stuff, similar to the idea of dropping things after last use instead of at end of scope.

Contributor

durka commented Dec 13, 2016

Oh I see, it was *x.borrow() not x.borrow(). I don't see how this can be changed without (silently!) breaking lots of stuff, similar to the idea of dropping things after last use instead of at end of scope.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Dec 13, 2016

Member

Note that we already changed it for if in August.

Member

Manishearth commented Dec 13, 2016

Note that we already changed it for if in August.

@durka

This comment has been minimized.

Show comment
Hide comment
@durka

durka Dec 13, 2016

Contributor

I've even seen match held up as a "trick" for extending temporary lifetimes in an expression.

Contributor

durka commented Dec 13, 2016

I've even seen match held up as a "trick" for extending temporary lifetimes in an expression.

@durka

This comment has been minimized.

Show comment
Hide comment
@durka

durka Dec 13, 2016

Contributor

Hmm, it's a bit more complicated -- if was changed in 2014, if/else was changed in August to be consistent with that (in case you're going to call me on it, I'm not sure that was a good idea). I agree that changing match would make things more consistent but I don't like the silent breakage after all this time.

Contributor

durka commented Dec 13, 2016

Hmm, it's a bit more complicated -- if was changed in 2014, if/else was changed in August to be consistent with that (in case you're going to call me on it, I'm not sure that was a good idea). I agree that changing match would make things more consistent but I don't like the silent breakage after all this time.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Dec 13, 2016

Member

if was changed in 2014

Oh, that's why I think this used to work. It has worked for if for two years.

Member

Manishearth commented Dec 13, 2016

if was changed in 2014

Oh, that's why I think this used to work. It has worked for if for two years.

@durka

This comment has been minimized.

Show comment
Hide comment
@durka

durka Dec 13, 2016

Contributor

Workaround:

use std::cell::RefCell;

macro_rules! drop_temps {
    ($e:expr) => {{
        let x = $e;
        x
    }}
}

fn main() {
    let x = RefCell::new(true);
    
    match drop_temps!(*x.borrow()) {
        _ => x.borrow_mut()
    };
}
Contributor

durka commented Dec 13, 2016

Workaround:

use std::cell::RefCell;

macro_rules! drop_temps {
    ($e:expr) => {{
        let x = $e;
        x
    }}
}

fn main() {
    let x = RefCell::new(true);
    
    match drop_temps!(*x.borrow()) {
        _ => x.borrow_mut()
    };
}
@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Dec 13, 2016

Member

Interestingly, your workaround doesn't make the x.borrow() case (instead of *x.borrow()) stop compiling. It makes it panic, though, which we would expect.

This means that one way of implementing this is to simply internally treat match discriminants as {let x = expr; x}. Obviously the actual implementation would be smoother.

(I say this because @eddyb thinks we need a new form of inference to implement this, but I feel like the mechanism to implement this already exists.)

Member

Manishearth commented Dec 13, 2016

Interestingly, your workaround doesn't make the x.borrow() case (instead of *x.borrow()) stop compiling. It makes it panic, though, which we would expect.

This means that one way of implementing this is to simply internally treat match discriminants as {let x = expr; x}. Obviously the actual implementation would be smoother.

(I say this because @eddyb thinks we need a new form of inference to implement this, but I feel like the mechanism to implement this already exists.)

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Dec 13, 2016

Member

@Manishearth That thought is actually not entirely my own, both @nikomatsakis and @pnkfelix have considered this before, but I believe they're wary of the user-facing complexity resulting from it.

As for reusing the let logic: that is far too limited to work in practice. You can have a go if you want to.

Member

eddyb commented Dec 13, 2016

@Manishearth That thought is actually not entirely my own, both @nikomatsakis and @pnkfelix have considered this before, but I believe they're wary of the user-facing complexity resulting from it.

As for reusing the let logic: that is far too limited to work in practice. You can have a go if you want to.

@abonander

This comment has been minimized.

Show comment
Hide comment
@abonander

abonander Dec 14, 2016

Contributor

I've come across this before, and I remember the conclusion being that non-lexical borrow scopes would fix this as well as many related issues, but I haven't heard of any movement on that feature in a long time.

Contributor

abonander commented Dec 14, 2016

I've come across this before, and I remember the conclusion being that non-lexical borrow scopes would fix this as well as many related issues, but I haven't heard of any movement on that feature in a long time.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Dec 14, 2016

Member

@abonander They're orthogonal, this is about ordering drops based on lifetimes, whereas non-lexical borrows obey the already computed drop scopes no matter what they are.

Member

eddyb commented Dec 14, 2016

@abonander They're orthogonal, this is about ordering drops based on lifetimes, whereas non-lexical borrows obey the already computed drop scopes no matter what they are.

@jucs

This comment has been minimized.

Show comment
Hide comment
@jucs

jucs Dec 29, 2016

@eddyb The worst user-facing complexity is when things don't work for no obvious reason; or, even worse, if there is inconsistency between different language constructs.

Also, from my point of view, non-lexical borrow scopes is one of the most important features Rust currently lacks.

jucs commented Dec 29, 2016

@eddyb The worst user-facing complexity is when things don't work for no obvious reason; or, even worse, if there is inconsistency between different language constructs.

Also, from my point of view, non-lexical borrow scopes is one of the most important features Rust currently lacks.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Jan 9, 2017

Contributor

@jucs

non-lexical borrow scopes and drop order are completely orthogonal. Drop order changes the meaning of correct code (e.g., if the destructor contain println!s, the order of them changes), while NLLs only change which Rust programs are valid.

I am not sure that making drop order more complicated is a good thing, and in any case it needs an RFC.

Contributor

arielb1 commented Jan 9, 2017

@jucs

non-lexical borrow scopes and drop order are completely orthogonal. Drop order changes the meaning of correct code (e.g., if the destructor contain println!s, the order of them changes), while NLLs only change which Rust programs are valid.

I am not sure that making drop order more complicated is a good thing, and in any case it needs an RFC.

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum May 19, 2017

Member

So it seems that ultimately any changes here would need an RFC, so I'm going to close in favor of that. If anyone's actively interested, please file an RFC or an issue at rust-lang/rfcs.

Member

Mark-Simulacrum commented May 19, 2017

So it seems that ultimately any changes here would need an RFC, so I'm going to close in favor of that. If anyone's actively interested, please file an RFC or an issue at rust-lang/rfcs.

@osa1

This comment has been minimized.

Show comment
Hide comment
@osa1

osa1 Oct 8, 2017

Contributor

if the match expression returns something that does not have a lifetime tied to that of the temporary, we should destroy it immediately

This actually caused deadlocks in a program I was working on. I had something like this:

use std::sync::Arc;
use std::sync::Mutex;

struct S(i32);

impl S {
    pub fn get_int(&self) -> i32 {
        self.0
    }

    pub fn bump_int(&mut self) {
        self.0 += 1;
    }
}

fn main() {
    let s = Arc::new(Mutex::new(S(0)));

    match s.lock().unwrap().get_int() {
        i => {
            s.lock().unwrap().bump_int();
        }
    };
}

This program never returns because match keeps the lock longer than necessary.

Contributor

osa1 commented Oct 8, 2017

if the match expression returns something that does not have a lifetime tied to that of the temporary, we should destroy it immediately

This actually caused deadlocks in a program I was working on. I had something like this:

use std::sync::Arc;
use std::sync::Mutex;

struct S(i32);

impl S {
    pub fn get_int(&self) -> i32 {
        self.0
    }

    pub fn bump_int(&mut self) {
        self.0 += 1;
    }
}

fn main() {
    let s = Arc::new(Mutex::new(S(0)));

    match s.lock().unwrap().get_int() {
        i => {
            s.lock().unwrap().bump_int();
        }
    };
}

This program never returns because match keeps the lock longer than necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment