Next steps for the unsafe code guidelines

[nikomatsakis]

@nikomatsakis

Maybe we can meet again on March 30th with some observations? (1 week)

So, I wrote contradictory things here. It's been 1 week, but it's not March 30th. Do we think we want to try and meet today?

I think people should go ahead and leave notes on their observations in the thread for discussion. In fact, maybe I should start a new thread (just so it has a more appropriate subject line)? I think I'll do that.

[nikomatsakis]

@nikomatsakis

So, I wrote contradictory things here. It's been 1 week, but it's not March 30th. Do we think we want to try and meet today?

I realize now that (a) I cannot do March 30th, owing to the GNOME coding sprint, and (b) in general the time we elected (2pm EST on Thursdays) is not especially good for me (I have another meeting at that time). Still, I think that synchronous meetings are maybe still of value. We certainly don't have much of a rhythm yet. Perhaps we could establish a more regular time (earlier on Thu seemed like it would generally work).

I could probably do a meeting today but I'd love to hear from others -- has anyone been able to do the research etc? (If so, I encourage you to leave a comment on this thread I spawned.

[stebalien]

Responding to your "unsafe code in the wild" thread here so as not to contaminate it:

Canvas unsafe code in the wild

I didn’t get to the most interesting parts of rayon yet, which would be rayon-core, but I did some investigation into the parallel iterators. These are mostly safe code (yay safe abstractions!) but there is a bit of unsafe code dealing with uninitialized memory. Specifically, when constructing a vector, we will initialize it to the desired length and then fill in parts of it in parallel (with some assertions to make sure that everything gets written): The collect() code. The main interface bou…

This looks like a case of missing abstractions and I'm sure it's not the only one. In this case, a write-only UninitializedVec<T> abstraction would probably help. Something like:

#![feature(untagged_unions)]

use std::ops::{Index, IndexMut};
use std::ptr::drop_in_place;

pub struct UninitializedVec<T: Sized> {
    data: *mut T,
    len: usize,
    capacity: usize,
}

impl<T: Sized> UninitializedVec<T> {
    // All the normal methods except that they operate over `Uninit<T>` instead of `T`...
    // And:
    pub unsafe fn into_vec(self) -> Vec<T> {
        Vec::from_raw_parts(self.data, self.len, self.capacity)
    }
}

impl<T: Sized> Index<usize> for UninitializedVec<T> {
    type Output = Uninit<T>;
    fn index(&self, idx: usize) -> &Uninit<T> {
        unsafe {
            assert!(idx < self.len, "out of bounds");
            &*(self.data.offset(idx as isize) as *const Uninit<T>)
        }
    }
}

impl<T: Sized> IndexMut<usize> for UninitializedVec<T> {
    fn index_mut(&mut self, idx: usize) -> &mut Uninit<T> {
        unsafe {
            assert!(idx < self.len, "out of bounds");
            &mut *(self.data.offset(idx as isize) as *mut Uninit<T>)
        }
    }
}

// I assume the alignment/size will match T...
#[allow(unions_with_drop_fields)]
pub union Uninit<T> {
    inited: T,
}

impl<T> Uninit<T> {
    /// Set to `value`. This will forget the existing value, if any.
    pub fn set(&mut self, value: T) {
        unsafe {
            self.inited = value;
        }
    }
    /// Set to `value` dropping the existing one.
    pub unsafe fn replace(&mut self, value: T) {
        drop_in_place(&mut self.inited);
        self.inited = value;
    }
    pub unsafe fn get_unchecked(&self) -> &T {
        &self.inited
    }
    pub unsafe fn get_mut_unchecked(&mut self) -> &mut T {
        &mut self.inited
    }
}

[nikomatsakis]

@stebalien

This looks like a case of missing abstractions and I'm sure it's not the only one. In this case, a write-only UninitializedVec<T> abstraction would probably help. Something like:

Actually that's pretty unclear to me. That is, the existing code...is fairly clean, it's not obvious to me that it would add much to have an "uninitialized vector". Put another way, that's kind of what we wrote. =)

[stebalien]

Unless I misunderstood your post, you're creating references to uninitialized types which may be UB (depending on the final the rules) and is definitely a bit dangerous (you're producing safe references to types with "safe" methods that are actually unsafe to use). Isn't that why you initially started out by using *mut T pointers everywhere? The fact that you had to make this choice leads me to believe that there's something missing so I'm trying to propose a less "unsafe" middle ground ("unsafe" code doesn't have to be all or nothing).

After thinking about it, you don't actually need a special vector to deal with this; a simple Uninit<T> wrapper and a Vec<Uninit<T>> should suffice. I've forked off a new topic as it doesn't really seem to belong here:

Less Unsafe

I’m starting this topic to discuss ways to make unsafe code less “unsafe”. The basic idea is to introduce semi-safe abstractions to: Create “guard rails” that limit unsafety. Make unsafe code more ergonomic. The harder it is to write and understand code, the harder it is to get right. Is this something people are interested in? My first proposal would be an Uninit<T> wrapper to make dealing with uninitialized memory less unsafe: #![feature(untagged_unions)] use std::{mem, ptr}; // Assumi…

[nikomatsakis]

@stebalien

Unless I misunderstood your post, you're creating references to uninitialized types which may be UB (depending on the final the rules) and is definitely a bit dangerous

Indeed. My point was merely that a more direct fix might be for us to use &mut Uninit<T> (where union Uninit<T> { value: T, uninitialized: () }), but otherwise keep working with slices as we have been doing. I guess that's what you're saying in your thread though. =)

[RalfJung]

Sorry, meeting today will not work out for me. And I haven’t yet had the time to do my share of the survey, either…

[nikomatsakis]

Yeah, let’s try to communciate over the thread for now. We can meet again in a few weeks – my schedule is a bit crazy for next two weeks, between the GNOME thing, some Mozilla obligations, and the ECOOP PC meeting.