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

Finally blocks for safer, faster, and clearer unsafe code #1010

Open
Gankra opened this issue Mar 24, 2015 · 11 comments
Open

Finally blocks for safer, faster, and clearer unsafe code #1010

Gankra opened this issue Mar 24, 2015 · 11 comments
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@Gankra
Copy link
Contributor

Gankra commented Mar 24, 2015

BinaryHeap currently has the following impl of sift_up that relies on the drop flag and LLVM being smart to avoid some swaps while being panic-safe:

    fn sift_up(&mut self, start: usize, mut pos: usize) {
        unsafe {
            let new = replace(&mut self.data[pos], zeroed());

            while pos > start {
                let parent = (pos - 1) >> 1;

                if new <= self.data[parent] { break; }

                let x = replace(&mut self.data[parent], zeroed());
                ptr::write(&mut self.data[pos], x);
                pos = parent;
            }
            ptr::write(&mut self.data[pos], new);
        }
    }

This code is quite unclear, and relies on a lot of things going right. However if we had a finally block, we could do the following:

    fn sift_up(&mut self, start: usize, mut pos: usize) {
        unsafe {
            let new = ptr::read(&self.data[pos]);

            while pos > start {
                let parent = (pos - 1) >> 1;

                if new <= self.data[parent] { break; }

                ptr::copy_nonoverlapping(&mut self.data[pos], &self.data[parent], 1);
                pos = parent;
            }

            finally {
                ptr::write(&mut self.data[pos], new);
            }
        }
    }

Which clearly captures the actual semantics we want. This functionality can be emulated by creating a struct with a drop impl, but it requires "weakly capturing" all the values through *const's. However that is noisy, cumbersome, confusing, and needlessly unsafe. A first class finally block can be verified to correctly run no matter where unwinding occurs (possibly through verifying that nothing it "closes" over is ever conditionally moved out).

I've constructed similar code while working on some trusted_len iterator problems (using the *const weak closing drop impl).

It would be fine with me if finally was considered unsafe, since its value to me is for cleanly failing in transient unsafe states.

I am logging this as only an issue because I don't have the knowledgebase to flesh out the precise rules, and because there's no way this can land for 1.0.

@Gankra
Copy link
Contributor Author

Gankra commented Mar 24, 2015

@pnkfelix this may be relevant to your drop flag work

@aidancully
Copy link

What about a Drop-implementing structure that can run an Fn on drop? Then the closure-capturing rules will be less confusing than a *const weak capture:

#![feature(unsafe_destructor)]
use std::ops;

pub struct Finally<T: FnMut() -> ()>(Option<T>);

impl<T: FnMut() -> ()> Finally<T> {
  fn new(t: T) -> Finally<T> {
    Finally(Some(t))
  }
}

#[unsafe_destructor]
impl<T: FnMut() -> ()> ops::Drop for Finally<T> {
  fn drop(&mut self) {
    self.0.take().unwrap()();
  }
}

Then the example looks like:

    fn sift_up(&mut self, start: usize, mut pos: usize) {
        unsafe {
            let new = Some(ptr::read(&self.data[pos]));
            let finally = Finally::new(|| ptr::write(&mut self.data[pos], new.take().unwrap());

            while pos > start {
                let parent = (pos - 1) >> 1;

                if new.as_ref().unwrap() <= self.data[parent] { break; }

                ptr::copy_nonoverlapping(&mut self.data[pos], &self.data[parent], 1);
                pos = parent;
            }
        }
    }

Has the advantage that you get good control over when the "finally" should be invoked, but the disadvantages that a) it must use the Option dance to avoid moving new to the closure, b) uses an unsafe_destructor (though I think that should be OK for this purpose?), and c) the control flow is kind of strange.

@kennytm
Copy link
Member

kennytm commented Mar 25, 2015

@aidancully Looks similar to the now-deprecated finally module

@aidancully
Copy link

@kennytm Yes, you're right, and what I wrote won't work either since new is borrowed mutably by the closure, preventing the new.as_ref() in the main body. :-( Hard to see how to get this to work reasonably with the borrow-checker, maybe by allowing a borrow from the closure's captured environment? In the Finally trait style:

let new = ptr::read(&self.data[pos]);
// `env` is the dtor `FnOnce` passed to the `finally` function.
(|env| {
  ...
  // allow borrows from `env`'s captured environment
  if env.new <= self.data[parent] { break; }
}).finally(|| ptr::write(&mut self.data[pos], new));

This would rely on blanket Deref and DerefMut impls on FnOnce, to allow borrows from the captured environment prior to closure execution.

@Gankra
Copy link
Contributor Author

Gankra commented Mar 25, 2015

See also Jonathan Blow's defer keyword which specifies code to be run at the end of scope (in reverse order like destructors).

{
  let ptr = allocate();
  defer free(ptr);

  // do lots of stuff with ptr

  // free(ptr) gets executed here
}

@bluss
Copy link
Member

bluss commented Mar 25, 2015

We do have exception safety problematics. Rust has exceptions but no exception handling, that sounds like a match to try clauses with finally but no catch clauses!

@bluss
Copy link
Member

bluss commented Mar 26, 2015

@gankro We can do that in Rust with an "owning scope guard",, but it can only offer Deref & DerefMut access to its protected value.

@lilyball
Copy link
Contributor

@gankro That defer keyword looks exactly like golang's defer.

I don't think we need to introduce defer like golang does, but I am partial to the idea of having a finally construct available. The problem with doing this at the library level is it does not interact well with borrowck, since the RAII guard has to take a reference / ownership of the value when it's created (whereas the desired behavior is to take a mutable reference only when the finally clause is going to be executed).

I think it's fine to make this construct only available inside of unsafe blocks, as safe code should still be safe in the presence of unwinding. It's certainly possible to write code such that you have "safe" code outside of an unsafe block that is nevertheless necessary to maintain safe use of shared structures, but this "safe" code is generally paired with unsafe code and I think it's reasonable to say the user has to expand the scope of the unsafe block in order to use finally in those instances.

All that said, I'm not quite sure about the syntax. Having a finally { ... } at the end of an unsafe suggests that you should be able to put it elsewhere too, and doesn't make it obvious what scope it's actually protecting. But using a construct that introduces a new scope for the "guarded" code can also be problematic. And doing something like unsafe { ... } finally { ... } puts the finally code outside of the unsafe scope entirely, which I don't think is feasible either.

@theemathas
Copy link

I believe that in this specific case of sift_up, the speed gain is best achieved by having some guarantees about comparisons, such as discussed on #956

@ticki
Copy link
Contributor

ticki commented Nov 16, 2015

👍

I like the idea, but I'm not sure about thee syntax. Is it necessary to introduce a new keyword?

@jFransham
Copy link

macro_rules! finally {
    ($contents:block) => {
        struct A;

        impl Drop for A {
            fn drop(&mut self) {
                { $contents };
            }
        }

        let a = A;
    };
}


fn main() {
    finally! {{
        println!("hello, world!");
    }}

    println!("but first...");
}

I tried to allow single braces by using $( $contents:tt )* but it complained about using macros inside it. If you were willing to do more duplication of Rust syntax you could probably get that working.

EDIT: this won't work if you try to access data in the surrounding scope, but all you need to do to fix that is make A take a closure, wrap $contents in ||{ $contents }, and then call it from inside drop().

EDIT2: possibly a good idea to somehow wrap everything in a RefCell so that it doesn't interfere with semantics, or do it some other way since we know values won't be used until the end of the method. Either way, this is definitely something that I would rather see done with a macro or plugin.

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests