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

Tracking issue for the `SliceIndex` trait #35729

Closed
alexcrichton opened this Issue Aug 16, 2016 · 37 comments

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Aug 16, 2016

Tracking issue for rust-lang/rfcs#1679

The implementations are stable, but the SliceIndex trait is not.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Nov 28, 2016

This is now implemented and will land in the 1.15 release!

@sfackler sfackler closed this Nov 28, 2016

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Nov 28, 2016

Oh right, this should be the tracking issue for the unstable helper traits...

@sfackler sfackler reopened this Nov 28, 2016

@sfackler sfackler added the B-unstable label Nov 28, 2016

@sfackler sfackler changed the title Tracking issue for adding panic safe slicing methods Tracking issue for the `SliceIndex` trait Nov 28, 2016

@briansmith

This comment has been minimized.

Copy link

briansmith commented Feb 18, 2017

Could we please stabilize this for the next release (or the next next release)? It seems like it is possible to use the new functionality, but it isn't possible to write (transparent) wrappers around it because any reference to the SliceIndex trait triggers "use of unstable library feature 'slice_get_slice' (see issue #35729)".

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Feb 21, 2017

@sfackler I forget, was this an intended "never to be stabilized" trait or did we want this on the track to stabilization?

@briansmith note that the trait can be vendored locally for now (if necessary) as it's not unstable to implement it, just to use the libstd version. Now that's a huge PITA (especially in multiple crates), but it at least gets the ergonomics immediately!

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Feb 21, 2017

I wouldn't call it a "never stabilize" kind of thing, but it's definitely a bit gross in its current form. One suggestion on the RFC was to split out a trait per method which could be a bit cleaner and more extensible in the future.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Feb 21, 2017

@alexcrichton I don't know what you mean by "can be vendored." Here's what I'm trying to write:

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub struct Slice<'a> {
    bytes: &'a [u8]
}

impl<'a> Slice<'a> {
    #[inline]
    pub fn new(bytes: &'a [u8]) -> Slice<'a> {
        Slice { bytes: bytes }
    }

    #[inline]
    pub fn get<I>(&self, i: I) -> Option<&I::Output>
                  where I: std::slice::SliceIndex<u8> {
        self.bytes.get(i)
    }
}

The result is "use of unstable library feature 'slice_get_slice' (see issue #35729))."

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Feb 22, 2017

@briansmith oh I just mean that you can add your own SliceIndex trait in your own local crate, add the various impls/methods you need, and then use it locally.

That'd work for one crate to get ergonomics, but it wouldn't work at large to continue abstracting over it.

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Apr 13, 2017

It'd be nice to see this trait combined with RangeArgument somehow.

@jonhoo

This comment has been minimized.

Copy link
Contributor

jonhoo commented May 11, 2017

@alexcrichton I'm not sure what you mean in your recommendation to @briansmith? I want to take a generic slice index argument (i.e., may be a range), and then use it to index into a slice. If I copy the trait into my own crate, I can't then take something implementing that trait and pass to slice::get?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented May 11, 2017

@jonhoo oh mean basically just copy/pasting the trait in libstd into your own crate. You wouldn't call <[T]>::get, you'd call the local trait's method. Again, good for library consumer ergonomics, bad for library developer ergonomics and generics

jonhoo added a commit to jonhoo/rust-ibverbs that referenced this issue May 11, 2017

Now works on beta/stable
Until rust-lang/rust#35729 stabilizes, we directly import and implement
the `SliceIndex` trait, which lets us work correctly on stable!

ndusart added a commit to ndusart/nix that referenced this issue Jul 24, 2017

use usize as underlying type for SpecialCharacterIndices
This allows to use it as slice indice waiting for the `SliceIndex` trait to be stabilized (rust-lang/rust#35729)
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Apr 20, 2018

The final comment period is now complete.

tmccombs added a commit to tmccombs/rust that referenced this issue May 29, 2018

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented May 30, 2018

#51147 implements #35729 (comment) except #[doc(hidden)], because of #13698.

kennytm added a commit to kennytm/rust that referenced this issue May 30, 2018

Rollup merge of rust-lang#51147 - tmccombs:sliceindex, r=SimonSapin
Stabilize SliceIndex trait.

CC rust-lang#35729

According to recommendations in
rust-lang#35729 (comment)

kennytm added a commit to kennytm/rust that referenced this issue May 30, 2018

Rollup merge of rust-lang#51147 - tmccombs:sliceindex, r=SimonSapin
Stabilize SliceIndex trait.

CC rust-lang#35729

According to recommendations in
rust-lang#35729 (comment)

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jun 2, 2018

Rollup merge of rust-lang#51147 - tmccombs:sliceindex, r=SimonSapin
Stabilize SliceIndex trait.

CC rust-lang#35729

According to recommendations in
rust-lang#35729 (comment)

@bors bors closed this in 9d770e9 Jun 3, 2018

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jun 3, 2018

Would it be possible to add a blanket impl for RangeBounds? It's been discussed in this thread, but all that ended up going in was a series of impls for the particular Range types (RangeTo, RangeFull, etc).

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 4, 2018

@joshlf Seems reasonable to me, wanna send a PR? With #51147 landed it would need to happen in this release cycle.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 4, 2018

Oh wait, I just remembered: that would replace specialized code with enum-based dispatch. I don’t know if that ends up being reliably optimized away.

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jun 4, 2018

Couldn't we use impl specialization?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 10, 2018

If this doesn’t simplify the code, why is this blanker impl useful?

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jun 10, 2018

It's useful because otherwise you can't write the following code (which I tried to do, which motivated my commenting here):

fn foo<R: RangeBounds<usize>>(buf: &mut [u8], range: R) {
    bar(&mut buf[range]);
}
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 10, 2018

Can you have a bound on SliceIndex instead? That’s exactly why we stabilized the trait without stabilizing the methods.

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jun 10, 2018

Unfortunately not - some of the code actually makes use of the methods on RangeBounds. E.g., this function, which needs the ability to query what the bounds are:

Click to expand
use zerocopy::ByteSlice;

/// Extract a range from a slice of bytes.
///
/// `extract_slice_range` extracts the given range from the given slice of
/// bytes. It also returns the byte slices before and after the range.
///
/// If the provided range is out of bounds of the slice, or if the range
/// itself is nonsensical (if the upper bound precedes the lower bound),
/// `extract_slice_range` returns `None`.
pub fn extract_slice_range<B: ByteSlice, R: RangeBounds<usize>>(
    bytes: B, range: R,
) -> Option<(B, B, B)> {
    let lower = resolve_lower_bound(range.start_bound());
    let upper = resolve_upper_bound(bytes.len(), range.end_bound())?;
    if lower > upper {
        return None;
    }
    let (a, rest) = bytes.split_at(lower);
    let (b, c) = rest.split_at(upper - lower);
    Some((a, b, c))
}

// return the inclusive equivalent of the bound
fn resolve_lower_bound(bound: Bound<&usize>) -> usize {
    match bound {
        Bound::Included(x) => *x,
        Bound::Excluded(x) => *x + 1,
        Bound::Unbounded => 0,
    }
}

// return the exclusive equivalent of the bound, verifying that it is in
// range of len
fn resolve_upper_bound(len: usize, bound: Bound<&usize>) -> Option<usize> {
    let bound = match bound {
        Bound::Included(x) => *x + 1,
        Bound::Excluded(x) => *x,
        Bound::Unbounded => len,
    };
    if bound > len {
        return None;
    }
    Some(bound)
}
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 11, 2018

… then have two bounds?

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jun 11, 2018

That doesn't work because generic code needs to call extract_slice_range. The only way to get it to work (which I do now) is to have a function which converts any pair of R: RangeBounds<usize> and a slice length into a Range. It works, but it's a real hack, and it requires code that looks like slice_mut(slice, range) rather than slice[range], where slice_mut is itself pretty annoying, and has to be used everywhere.

Click to expand
    pub fn slice_mut<'a, T, R: RangeBounds<usize>>(slc: &'a mut [T], range: &R) -> &'a mut [T] {
        let lower = resolve_lower_bound(range.start_bound());
        let upper = resolve_upper_bound(slc.len(), range.end_bound()).unwrap();
        &mut slc[lower..upper]
    }

    // return the inclusive equivalent of the bound
    fn resolve_lower_bound(bound: Bound<&usize>) -> usize {
        match bound {
            Bound::Included(x) => *x,
            Bound::Excluded(x) => *x + 1,
            Bound::Unbounded => 0,
        }
    }

    // return the exclusive equivalent of the bound, verifying that it is in
    // range of len
    fn resolve_upper_bound(len: usize, bound: Bound<&usize>) -> Option<usize> {
        let bound = match bound {
            Bound::Included(x) => *x + 1,
            Bound::Excluded(x) => *x,
            Bound::Unbounded => len,
        };
        if bound > len {
            return None;
        }
        Some(bound)
    }
@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jun 11, 2018

Oh, sorry, I guess you mean have the bound R: RangeBounds<usize> + SliceIndex? That works I suppose. It's just ugly, and I suspect other Rust programmers are going to run into this sharp corner a lot. It'd be better if it Just Worked. If we can't get the default impl working, then so be it, but so long as we can get it to work, I feel like it will round off an otherwise sharp corner.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 11, 2018

Yes, if you want to do with a generic type two things that each require a different trait, have your own code require both traits.

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jun 11, 2018

Oh, and one other issue. Since SliceIndex takes a type parameter, and we don't support HKT, there's no way to just have R: RangeBounds<usize> + SliceIndex, so the caller would have to know the concrete type T in SliceIndex<T>.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 11, 2018

This higher-ranked trait bounds rather than types, but yeah, good point.

F001 added a commit to F001/rust that referenced this issue Jun 24, 2018

est31 added a commit to est31/balloc that referenced this issue Mar 4, 2019

Implement indexing
This requires Rust 1.28.0,
due to usage of [1],
so I guess we'll have to
gate it on the rustc version.

[1]: rust-lang/rust#35729
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.