RFC: Raw Reform

[Gankro]

• Introduce RawSlice and RawMutSlice extension traits to be implemented on
  *const [T] and *mut [T] that provide parts of the slice and ptr API to better bridge the
  gap between the two.
• Provide unsafe versions of the slicing and indexing operators on raw slices to provide ergonomic
  unchecked slice manipulation.
• Provide unsafe versions of the addition, subtraction, and indexing operators on raw ptrs to
  provide more ergonomic ptr manipulation.
• Deprecate some of the free functions in ptr, and duplicate the rest as convenience methods on
  the RawPtr extension traits.
• Deprecate all unsafe methods on slices and slice::raw in favour of raw slices.
• Add conversion methods to the three families of extension traits for conveniently moving between
  them.

Rendered

[glaebhoerl]

I don't believe this works as written. The lifetime of the resulting slice has to come from somewhere.

Two options I see: use &self (and then lifetime elision fills them in); or, because it's an unsafe fn after all, as_slice<'a>(self) -> &'a [T], meaning that it will be valid for any lifetime of the caller's choosing, so the caller has to be very careful.

(Same thing in several other places.)

[tbu-]

Given x: &[u8], y: *[u8], shouldn't x as *const [u8] and &*y work already?

[aturon]

@tbu-

Given x: &[u8], y: *[u8], shouldn't x as *const [u8] and &*y work already?

Yes, though at least for the first one, x.as_raw() is definitely nicer for method chaining.

[tbu-]

Do we have this for normal raw pointers?

(I. e. x: &u8, x.as_raw() or similar.)

[arthurprs]

Unsafe addition, subtraction, and indexing of *const T and *mut T should also be provided with ints rather than uints. Unsafe addition and subtraction should be equivalent to calling offset on the ptr today. Indexing should have the same behaviour as indexing into a raw slice.

Doesn't this requires bounds checking? Thus effectively defeating the raw access advantage?

[Gankro]

@glaebhoerl Good catch! I think I caught them all.

[Gankro]

@arthurprs Perhaps I miscommunicated the intended behaviour, why do you expect bounds checking is required? Perhaps I didn't sufficiently clarify that raw slices are the *const [T]'s, which are unchecked.

[arthurprs]

@Gankro nevermind, when I read the uint -> int change I had python indexes in mind (so -1 is the last element, -2 the before last ...)

[Gankro]

I've started implementing these changes in a branch as a sanity check, and to demonstrate the simplicity. One thing I've run into:

Where previously you could call

ptr::copy_nonoverlapping_memory(&mut foo, &bar, 1)

Now you would need to do:

(&mut foo as *mut T).copy_nonoverlapping(&bar, 1)

Hmm... perhaps some of these would be better of as free fn's in mem, rather than methods on RawPtr.

[Gankro]

Ack, and ptr::swap is actually subtly different, it allows the ptrs to point to the same location to permit some tricks to avoid branching. The free fn moving in ptr needs to be completely rethought.

[reem]

Generally I think it's a bad idea for pointers to have inherent methods if they can avoid it, and that those should usually be free functions in ptr or equivalent.

[mahkoh]

👍

More concise unsafe code means fewer bugs.

[arielb1]

insertion_sort could actually be done more cleanly as:

/// Rotates a slice one element to the right,
/// moving the last element to the first one and all other elements one place
/// forward. (i.e., [0,1,2,3] -> [3,0,1,2])
fn rotate_right<T>(s: &mut [T]) {
    let len = s.len();
    let s = s.as_raw_mut();
    if len == 0 { return; }

    unsafe {
        let first = s.read(len-1);
        s[1..].copy(s[..len-1]);
        s.write(0, first);
    }
}

fn insertion_sort<T>(v: &mut [T], compare: |&T, &T| -> Ordering) {
    let len = v.len();

    // 1 <= i < len;
    for i in range(1, len) {
        // j satisfies: 0 <= j <= i;
        let mut j = i;

        // `i` is in bounds.
        let read_ptr = unsafe { v.unsafe_get(i) };

        // find where to insert, we need to do strict <,
        // rather than <=, to maintain stability.
        // 0 <= j - 1 < len, so j - 1 is in bounds.
        while j > 0 && compare(read_ptr,
                               unsafe { v.unsafe_get(j - 1) }) == Less {
            j -= 1;
        }

        // `i` and `j` are in bounds, so [j, i+1) = [j, i] is valid.
        rotate_right(unsafe { v.unsafe_slice_mut(j, i+1) });
    }
}

– we should support this style (although unsafe { v.unsafe_XYZ(..) } is ugly).

[Gankro]

@reem When I drafted this up I was trying to apply the lesson's I'd learned working on collections stuff. Namely, that it's generally easier and safer to work and reason with objects in "modes". With collections we have the "iterator" mode, and with maps we now have "entry" mode.

Unfortunately this translates poorly to references and rawptrs because we don't (and probably shouldn't ever) have .as_raw() on references to get raw ptrs, so you either have to do an awkward explicit cast to "as *const _" which then needs to be wrapped in parens to chain off of, or rely on automatic coercions, which of course don't (and probably shouldn't ever) work on method calls.

I still think it would be nicer if when you have a ptr foo, you can call foo.bar() on it rather than passing it in as bar(foo). Chaining is better than nesting IMO. That said, a lot of the time you have a reference, or even value, that you want to transiently convert into a ptr to do something unsafe real-quick.

I'm leaning towards having the methods and the free fns, so that people can just do whatever is most natural. Possibly moving most if not all of the free functions to mem instead, since these really aren't ptr functions, so much as raw memory manipulation functions. ptr::swap is an awkward one, though, since it has subtly different semantics to mem::swap. mem::swap_overlapping 😩 ?

[Gankro]

@arielb1 I've definitely mulled over the possibility of offering safe rotate_left, and rotate_right methods, as well as possibly some shift_*_and_replace methods on mutable slices before. Where the _and_replace methods would pop the element that would be "rotated" and inserts a new value where it would have been rotated to. This would be useful for handling the wrapping case in e.g. RingBuf. If you're interested, I could help you write up a proposal for such methods if you want. I'm a bit swamped though, and I'm not sure if the win is big enough to justify it

Otherwise, I'm not clear what you're suggesting. Every single one of your slicing operations is unchecked, and consequently riddled with unsafe's. It seems much cleaner at that point to simply say "okay, this is all unsafe" and just use raw slices. It doesn't seem very helpful to say exactly this subexpression is unsafe. But maybe that's just me?

[aturon]

@Gankro

Unfortunately this translates poorly to references and rawptrs because we don't (and probably shouldn't ever) have .as_raw() on references to get raw ptrs, so you either have to do an awkward explicit cast to "as *const _" which then needs to be wrapped in parens to chain off of, or rely on automatic coercions, which of course don't (and probably shouldn't ever) work on method calls.

I think the main reason not to have .as_raw() as a method on references is just that it would shadow any other method with that name on the pointed-to data.

Generally, we freely coerce from &T to *const T -- it's the deref that's unsafe. So I don't see why applying this coercion when calling a method taking *const self on a &T is problematic: assuming that the method actually derefs, it has to be unsafe. But maybe I'm missing something here?

[aturon]

@Gankro Ah, I think I understand now: you're saying that we don't (and shouldn't) automatically make the various methods of RawPtr available on arbitrary references. Definitely agree.

[Gankro]

@arielb1 Also, you can just do this, if you really want:

fn insertion_sort<T>(v: &mut [T], compare: |&T, &T| -> Ordering) {
    let len = v.len();

    // 1 <= i < len;
    for i in range(1, len) {
        // j satisfies: 0 <= j <= i;
        let mut j = i;

        // `i` is in bounds.
        let read_ptr = unsafe { &v.as_raw()[i] };

        // find where to insert, we need to do strict <,
        // rather than <=, to maintain stability.
        // 0 <= j - 1 < len, so j - 1 is in bounds.
        while j > 0 && compare(read_ptr,
                               unsafe { &v.as_raw()[j - 1] }) == Less {
            j -= 1;
        }

        // `i` and `j` are in bounds, so [j, i+1) = [j, i] is valid.
        rotate_right(unsafe { &mut*v.as_raw_mut()[j, i+1] });
    }
}

Which is basically the same code-wise. Just more sigily.

[aturon]

@Gankro

I'm leaning towards having the methods and the free fns, so that people can just do whatever is most natural. Possibly moving most if not all of the free functions to mem instead, since these really aren't ptr functions, so much as raw memory manipulation functions. ptr::swap is an awkward one, though, since it has subtly different semantics to mem::swap. mem::swap_overlapping 😩 ?

Given the above discussion, providing both free functions and methods probably makes sense (and it explains why these weren't just methods in the first place).

I believe the motivation for having the ptr module was that the operations there essentially take raw pointers as their receiver; it's like the slice module in that it provides basic operations for a built-in type. Note that, by contrast, none of the (non-deprecated) functions in mem take raw pointers.

[Gankro]

@aturon

I was discussing this with @kballard yesterday on IRC. They suggested that ptr::replace should also remain as a convenience for not doing mem::replace(&mut *foo, ...), largely for semantic reasons. Something along the lines of "you're working with raw ptrs, not references".

I'm not totally convinced by this argument, but I don't have particularly strong feelings. Removing it would reduce the API surface area with minimal ergonomic loss in my mind. It might also prevent novices from thinking ptr::replace is special, or from not realizing mem::replace exists at all.

Any thoughts?

[aturon]

@Gankro

I was discussing this with @kballard yesterday on IRC. They suggested that ptr::replace should also remain as a convenience for not doing mem::replace(&mut *foo, ...), largely for semantic reasons. Something along the lines of "you're working with raw ptrs, not references".

I'm not totally convinced by this argument, but I don't have particularly strong feelings. Removing it would reduce the API surface area with minimal ergonomic loss in my mind. It might also prevent novices from thinking ptr::replace is special, or from not realizing mem::replace exists at all.

I think I agree with @kballard on this: &mut *foo is a bit of a pain, and while providing replace in ptr does increase the API surface, if is has the same semantics and basically an equivalent signature to mem then it seems like an easy-to-remember convenience.

I think the same is true for swap, and I think it can remain in ptr with its current name. The fact that the mem version doesn't allow overlapping follows directly from its type signature, since mutable refs are unaliasable.

[Gankro]

Free fns are mostly all back in the latest draft. Fleshed out some other stuff too.

[arielb1]

@Gankro

Wrapping the operations in individual unsafe-blocks is just a style thing – I put unsafe blocks around all places where safety invariants are temporarily being violated, so each occurrence unchecked indexing (which does not violate any invariant) gets placed in its own unsafe block.

The primary point of my example is that it mostly uses unchecked indexing, rather than playing with raw pointers (other than within rotate). Raw pointers lose the aliasing guarantees, which just adds unneeded unsafety. I just noticed your proposal didn't talk about unchecked indexing, which is what we want in this case.

Having official rotations (and "rotate-through-carry"-es) would be quite nice (should I post an RFC?).

[Gankro]

@arielb1: You're totally right. I strongly alluded to the fact that these operations would be unchecked in the earlier sections, but completely forgot to state this in the detailed design. I've now fixed this.

I've also added your lifetime concern as a drawback to the proposal, as it is a very legitimate one!

You can post an RFC, or prototype it out on discuss if you aren't totally comfortable with your current design.

[aturon]

@Gankro This is one reason that I think leaving unsafe_get and unsafe_mut for slices is a good idea: avoiding a bounds check is a very common case, and these allow you to do it without jumping to a raw pointer. (With the new conventions, these will likely become get_unchecked and get_unchecked_mut -- easy to remember variants of the usual get methods.)

[Gankro]

@arielb1 @aturon I've added back unsafe_get and unsafe_mut.

[lilyball]

Instead of putting len() in RawSlice<T>, you should just implement Collection on *const [T]/*mut [T].

[Gankro]

I don't think Collection is going to survive #235, although that's a bit ambiguous.

[lilyball]

Half of slice::raw is actually operations on std::raw::Slice, not unsafe operations on slices.

[Gankro]

And that half doesn't seem to actually be used anywhere, as far as I can tell. Searching github just reveals... various versions of slice.rs.

[lilyball]

Looks like you're right, pop_ptr and shift_ptr are used inside rust only in libcore/slice.rs.

[lilyball]

If they're going to be duplicated, why not just leave them as free functions in the ptr module? Seems like ptr::read(foo) is more convenient than RawPtr::read(foo).

Edit: I thought you were saying RawPtr would have static functions on it, but reading below, you left the ptr free functions in place.

[lilyball]

RawPtr<T> can't ever be implemented on T: Unsized anyway, because the read() method returns a T, which is only valid if T is sized.

[reem]

Couldn't you have two impl blocks, for T: Sized and Sized? T?

[lilyball]

On that note, this means that raw pointers to unsized types besides [T] will implement neither RawSlice<T> nor RawPtr<T>. This seems potentially unfortunate. For example, a *const str will have this issue.

Perhaps RawPtr<T> should actually be split into two traits, RawPtr<T> which contains everything that doesn't conflict with RawSlice<T>, and then RawSizedPtr<T: Sized>.

[lilyball]

No. I'm actually somewhat inclined to say I don't agree with pointer arithmetic. Pointer arithmetic, as a + b, can be one of the most confusing aspects of dealing with pointers, partially because it looks like regular arithmetic, and partially because the actual value of the result depends not just on the value of b, but also on the size of the type of *a. For this reason, I'm leaning towards saying that we should just not have pointer arithmetic and continue to use the .offset() method.

[Gankro]

That's really good to know. @thestinger suggested to me on IRC that pointer arithmetic was highly desirable. If I understand the history correctly, it used to be there, but had to be removed because it had to be an Unsafe operation, and Add and Sub are safe. If that's true, then that implies the only reason it was removed was purely technical, and not philosophical.

Personally, I don't have a very strong opinion. offset is easier to chain, which I prefer. However arithmetic does make porting tricky C/C++ code easier.

[lilyball]

However arithmetic does make porting tricky C/C++ code easier.

Porting to offset, I would argue, makes sure you understand what the code is actually doing. Being able to copy a chunk of pointer arithmetic without modification is something I would be extremely nervous about recommending anyone do.

[mahkoh]

Being able to copy a chunk of pointer arithmetic without modification is something I would be extremely nervous about recommending anyone do.

If pointer arithmetic behaves the same in both languages then modifications are more likely to introduce new bugs. Not to mention that offset is harder to read.

[lilyball]

If pointer arithmetic behaves the same in both languages

That's a very big if. When porting code to Rust, you may modify types along the way, and that would potentially invalidate any pointer arithmetic. For example, if you took C code that used int and converted it to Rust code that used int, the size of the type is now different on 64-bit machines.

offset is harder to read

I actually think it's slightly easier to read, because it highlights the fact that this is pointer arithmetic, which is especially helpful when doing pointer arithmetic and integer arithmetic at the same time.

If you're doing enough pointer arithmetic that .offset() becomes too unwieldy, then I think you might have a problem ;)

[mahkoh]

When porting code to Rust, you may modify types along the way, and that would potentially invalidate any pointer arithmetic. For example, if you took C code that used int and converted it to Rust code that used int, the size of the type is now different on 64-bit machines.

If your pointer arithmetic depends on the size of a certain type, then choosing a different size when porting is a bug unrelated to pointer arithmetic. In this case the issues and RFCs related to the name of Rust's int type are more relevant. Either way, if

(x + 1) as uint == x as uint + size_of::<X>()

then a faithful port will behave the same way. Furthermore, pointer arithmetic will be used much more often for the inspection of arrays, and in this case only the number of elements matters, and not the contained type.

I actually think it's slightly easier to read, because it highlights the fact that this is pointer arithmetic

This has nothing to do with readability but [something whose name I don't know [the transfer of knowledge]]. Given full knowledge of the behavior of the operations involved,

x + y

is more readable than

x.add_with_possible_overflow(y).

In this case, x and y are integers. And while integer arithmetic is more frequent than pointer arithmetic in Rust code, when you port code, you want to change as little as possible in order to not introduce new bugs.

Secondly, I do not believe that the behavior of pointer arithmetic is a source of many bugs (compared to ordinary unchecked array indexing.) Most of the time you either index an array or inspect an object with c_char pointers (also because of the stricter aliasing rules in C.) In the second case, pointer arithmetic behaves just like integer arithmetic.

Thirdly, pointer arithmetic only happens in unsafe blocks where increased attention is required anyway.

Lastly, raw pointers are not arbitrary memory locations but the addresses of properly aligned objects. Nobody would expect x + 1 to create a pointer whose address is one larger than the address of the previous pointer, because that would be undefined behavior if the required alignment for *x is larger than one.

[lilyball]

I don't think raw slices should have offset(). It would be used far too rarely to justify its presence.

[lilyball]

Truncating, and it should return a uint with the number of elements it actually copied.

[Gankro]

That's a C-ism, yeah? Maybe we could have something Like Result<(), uint>?

[lilyball]

A C-ism? I don't think so. It's a common idiom in many languages when doing a copy where the number of elements copied is calculated rather than passed as a parameter.

I don't see what use Result<(), uint> would be. All it does is let you check if the copy wasn't truncated without having to get the length, but it's unclear as to whether it returns Ok(()) in the case where src.len() == dest.len() or merely in the case where src.len() <= dest.len(). It also is intentionally throwing away information that might be potentially useful if you actually do care how many elements were copied regardless of whether there was truncation.

[Gankro]

Result<uint, uint> then? I would expect only truncation would be an Err, but I could see that that really depends on context. I can live with a single uint, it just feel a bit semantically poor. I obviously don't work with those kinds of APIs much, though.

[lilyball]

No, I think null is special enough and comparing against null is common enough that .is_null() is worth preserving.

[lilyball]

I assume you mean variants that would swap/replace a value at an index? As long as it's easy to get a pointer to one of the elements, then I don't think we should add it to RawSlice.

[arielb1]

@Gankro

What about slice_unchecked and slice_mut_unchecked?

[Gankro]

@arielb1 Can you clarify?

[arielb1]

@Gankro

Just noticed that the unsafe methods on &[T] aren't really part of this RFC. I was talking about slice_unchecked(self: &[T], from: uint, len: uint) -> &[T] and a mut equivalent.

[Gankro]

Egh, just got reminded of str::raw. Might want to add some stuff about raw strs or something? That seems like a much more dubious motivation though.

[Gankro]

Discussed with @pcwalton the viability of adding any lang stuff for new unsafe operators and it looks like that's a total no-go for 1.0; not out of the question for later, though! If this is indeed the case, then we need a migration plan.

I propose adding the desired operators as unsafe named methods marked #[experimental], although that doesn't really work because as I understand it 1.0+ apis need to be 100% stable. 😕

[Gankro]

After some discussion with @aturon we've concluded that the most interesting bits can largely be hacked out in cargo while we wait for Rust to flesh out its operator overloading story. There's some useful ideas in here for stabilization, but we can revisit that with a different RFC.