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 slice::{ref_slice, mut_ref_slice} #27774

Closed
alexcrichton opened this issue Aug 12, 2015 · 39 comments · Fixed by #29254
Closed

Tracking issue for slice::{ref_slice, mut_ref_slice} #27774

alexcrichton opened this issue Aug 12, 2015 · 39 comments · Fixed by #29254
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@alexcrichton
Copy link
Member

This is a tracking issue for the unstable ref_slice feature in the standard library. These functions seem fairly innocuous but it's also somewhat questionable how useful they are. They're probably ready for either stabilization or deprecation as-is.

cc @nikomatsakis

@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2015
@nikomatsakis
Copy link
Contributor

Speaking personally, I love these functions. I don't need 'em often, but when I do, I'm happy I have them. =)

@alexcrichton alexcrichton added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Aug 13, 2015
@SimonSapin
Copy link
Contributor

Although they are unsafe, the implementations of these functions are one-liners (based on slice::from_raw_parts{,_mut}) that can be easily replicated outside of std. So I wouldn’t miss them too much.

(Compare with functionality that may be rarely used but is impossibly to reasonably implement outside of std because e.g. it requires accessing private fields of std types.)

@alexcrichton
Copy link
Member Author

Nominating for 1.5 resolution (I'd personally want to deprecate)

@tbu-
Copy link
Contributor

tbu- commented Sep 17, 2015

It's a safe abstraction of unsafe code, I think it would be suited for the standard library.

@nikomatsakis
Copy link
Contributor

Just to be clear for where I use this (in case you don't know), it's in a situation like this. Often I have an enum where the variants have different numbers of things associated with them (let's say integers here). Sometimes it's handy to match and only have to deal with the exact right number of integers for each case, but other times I want to just walk over all the integers. For some reason, it often happens that the variants have either 0, 1, or N such things. This works out quite nicely:

enum Foo {
    A, // no integers
    B(u32), // 1 integer
    C(Vec<u32>), // >1 integer
}

impl Foo {
    fn get_integers(&self) -> &[u32] {
        match *self {
            Foo::A => &[],
            Foo::B(ref i) => ref_slice(i),
            Foo::C(ref v) => v,
        }
    }
}

This seems to come up semi-regularly for me. (Sadly, the methods that let you go from &(T, T) to &[T] were removed, or it would work even for an arbitrary number of integers.)

The other situation where this fn comes in handy for me is when you have some generic helper that takes a &[T], but you happen to have an &T lying around. You can easily reuse this helper.

I think it's ok for this to live in libstd. It's not really duplicating anything else; I mean it's true you can write them in terms of other APIs, but only by dropping into unsafe code and going into a whole 'nother level of abstraction.

(That said, if there were to be another crate that ALSO included the ability to convert tuples into slices and so forth, that's more interesting. Though that's probably something we can't "safely" do until we agree to commit to the way tuples are represented in memory, though I could never imagine us changing this.)

@alexcrichton
Copy link
Member Author

This issue is now entering its cycle-long FCP for resolution in 1.5

The libs team was a little up in the air about deprecation or stabilization here, comments are certainly welcome!

@alexcrichton alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-nominated labels Sep 24, 2015
@abonander
Copy link
Contributor

👍 It's somewhat superfluous, yes, but being that it's a safe wrapper around unsafe code it's not a bad abstraction overall. It'd just end up in some utility crate somewhere, like the Apache Commons for Rust.

@Stebalien
Copy link
Contributor

Please stabilize these. Personally, I use them for reading single bytes from a reader (with read_exact).

@briansmith
Copy link
Contributor

The documentation says "Converts a pointer to A into a slice of length 1 (without copying)." Shouldn't "pointer" be "reference"?

@SimonSapin
Copy link
Contributor

Yes.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Sep 25, 2015
@steveklabnik steveklabnik added this to the 1.5 milestone Oct 1, 2015
@Manishearth Manishearth removed the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Oct 2, 2015
@canndrew
Copy link
Contributor

Definitely stabilize. Don't force me to use unsafe for something so simple.

@photino
Copy link

photino commented Oct 21, 2015

Personally, I would like to see that they are deprecated. They are simple wrappers of the unsafe code but not marked as unsafe. This is somewhat confusing and inconsistent.

@abonander
Copy link
Contributor

@photino They are not marked unsafe because it's safe to convert any &T or &mut T where T: Sized to, respectively, &[T] or &mut [T] with a length of 1. The layout of the data in the pointed-to area of memory is the same, it doesn't extend the lifetime of the reference, and it doesn't change an &T to an &mut [T].

This is absolutely consistent and is actually a very concise example of a core Rust idiom: that safe wrappers can be created around unsafe code if they can guarantee certain invariants. Here, the invariants are just very easy to enforce.

@photino
Copy link

photino commented Oct 21, 2015

@cybergeek94 Thanks for your explanation. I can't figure out any other reasons for not providing such safe wrappers.

@alexcrichton
Copy link
Member Author

The libs team discussed this during triage today and the decision was to deprecate these functions. The standard library currently doesn't house many nuggets of functionality like this (e.g. see the recent discussions around Box::leak), and we've also deprecated similar functionality (Option::as_slice).

While certainly useful from time to time, it's not clear that these functions belong in the standard library right now. There can always be a helper crate implementing all sorts of conversions like this, however!

@canndrew
Copy link
Contributor

That seems like a really weird decision. This is such a fundamental conversion between primitive datatypes it probably belongs at the language level (ie. &x as &[T]). In C they are the same type. The standard library has stuff for doing networking, hashing, spawning processes and threads all of which could be spun off into separate libraries but if I want to treat a value as an array of length one I need an auxiliary crate?

What does it matter if people only need it from time to time? It's such a trivial function it's not like it's adding bloat.

@canndrew
Copy link
Contributor

I mean it's like saying "Oh sure you can parse the file table of a fat32 drive out-of-the-box but if you want to upcast a u32 to a u64 you'll need to download the int_conversion crate". That kinda weird.

@tbu-
Copy link
Contributor

tbu- commented Oct 23, 2015

@huonw Do you have a link to that decision?

@huonw
Copy link
Member

huonw commented Oct 23, 2015

That crate and its documentation. (You might be able to get the actual things that were written down at the time in the lang team meeting minutes somewhere in July or August, but I don't think they'll offer anything more than that link.)

@tbu-
Copy link
Contributor

tbu- commented Oct 23, 2015

@steveklabnik Thanks for the crate.

@notriddle
Copy link
Contributor

@tbu-
Copy link
Contributor

tbu- commented Oct 24, 2015

@notriddle Thanks.

@huonw
Copy link
Member

huonw commented Oct 24, 2015

NB. that discussion isn't the lang team (I believe it was an independent invention of the same idea). The notes that were taken are at the end of https://github.com/rust-lang/meeting-minutes/blob/master/lang-team/2015-07-23.md

@tbu-
Copy link
Contributor

tbu- commented Oct 24, 2015

@huonw Thank you, I tried finding them, but I didn't.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Oct 25, 2015
This commit stabilizes and deprecates library APIs whose FCP has closed in the
last cycle, specifically:

Stabilized APIs:

* `fs::canonicalize`
* `Path::{metadata, symlink_metadata, canonicalize, read_link, read_dir, exists,
   is_file, is_dir}` - all moved to inherent methods from the `PathExt` trait.
* `Formatter::fill`
* `Formatter::width`
* `Formatter::precision`
* `Formatter::sign_plus`
* `Formatter::sign_minus`
* `Formatter::alternate`
* `Formatter::sign_aware_zero_pad`
* `string::ParseError`
* `Utf8Error::valid_up_to`
* `Iterator::{cmp, partial_cmp, eq, ne, lt, le, gt, ge}`
* `<[T]>::split_{first,last}{,_mut}`
* `Condvar::wait_timeout` - note that `wait_timeout_ms` is not yet deprecated
  but will be once 1.5 is released.
* `str::{R,}MatchIndices`
* `str::{r,}match_indices`
* `char::from_u32_unchecked`
* `VecDeque::insert`
* `VecDeque::shrink_to_fit`
* `VecDeque::as_slices`
* `VecDeque::as_mut_slices`
* `VecDeque::swap_remove_front` - (renamed from `swap_front_remove`)
* `VecDeque::swap_remove_back` - (renamed from `swap_back_remove`)
* `Vec::resize`
* `str::slice_mut_unchecked`
* `FileTypeExt`
* `FileTypeExt::{is_block_device, is_char_device, is_fifo, is_socket}`
* `BinaryHeap::from` - `from_vec` deprecated in favor of this
* `BinaryHeap::into_vec` - plus a `Into` impl
* `BinaryHeap::into_sorted_vec`

Deprecated APIs

* `slice::ref_slice`
* `slice::mut_ref_slice`
* `iter::{range_inclusive, RangeInclusive}`
* `std::dynamic_lib`

Closes rust-lang#27706
Closes rust-lang#27725
cc rust-lang#27726 (align not stabilized yet)
Closes rust-lang#27734
Closes rust-lang#27737
Closes rust-lang#27742
Closes rust-lang#27743
Closes rust-lang#27772
Closes rust-lang#27774
Closes rust-lang#27777
Closes rust-lang#27781
cc rust-lang#27788 (a few remaining methods though)
Closes rust-lang#27790
Closes rust-lang#27793
Closes rust-lang#27796
Closes rust-lang#27810
cc rust-lang#28147 (not all parts stabilized)
bors added a commit that referenced this issue Oct 25, 2015
This commit stabilizes and deprecates library APIs whose FCP has closed in the
last cycle, specifically:

Stabilized APIs:

* `fs::canonicalize`
* `Path::{metadata, symlink_metadata, canonicalize, read_link, read_dir, exists,
   is_file, is_dir}` - all moved to inherent methods from the `PathExt` trait.
* `Formatter::fill`
* `Formatter::width`
* `Formatter::precision`
* `Formatter::sign_plus`
* `Formatter::sign_minus`
* `Formatter::alternate`
* `Formatter::sign_aware_zero_pad`
* `string::ParseError`
* `Utf8Error::valid_up_to`
* `Iterator::{cmp, partial_cmp, eq, ne, lt, le, gt, ge}`
* `<[T]>::split_{first,last}{,_mut}`
* `Condvar::wait_timeout` - note that `wait_timeout_ms` is not yet deprecated
  but will be once 1.5 is released.
* `str::{R,}MatchIndices`
* `str::{r,}match_indices`
* `char::from_u32_unchecked`
* `VecDeque::insert`
* `VecDeque::shrink_to_fit`
* `VecDeque::as_slices`
* `VecDeque::as_mut_slices`
* `VecDeque::swap_remove_front` - (renamed from `swap_front_remove`)
* `VecDeque::swap_remove_back` - (renamed from `swap_back_remove`)
* `Vec::resize`
* `str::slice_mut_unchecked`
* `FileTypeExt`
* `FileTypeExt::{is_block_device, is_char_device, is_fifo, is_socket}`
* `BinaryHeap::from` - `from_vec` deprecated in favor of this
* `BinaryHeap::into_vec` - plus a `Into` impl
* `BinaryHeap::into_sorted_vec`

Deprecated APIs

* `slice::ref_slice`
* `slice::mut_ref_slice`
* `iter::{range_inclusive, RangeInclusive}`
* `std::dynamic_lib`

Closes #27706
Closes #27725
cc #27726 (align not stabilized yet)
Closes #27734
Closes #27737
Closes #27742
Closes #27743
Closes #27772
Closes #27774
Closes #27777
Closes #27781
cc #27788 (a few remaining methods though)
Closes #27790
Closes #27793
Closes #27796
Closes #27810
cc #28147 (not all parts stabilized)
bors added a commit that referenced this issue Nov 2, 2017
Bring back slice::ref_slice as slice::from_ref.

These functions were deprecated and removed in 1.5, but such simple
functionality shouldn't require using unsafe code, and it isn't
cluttering libstd too much.

The original removal was quite contentious (see #27774), since then
we've had precedent for including such nuggets of functionality (see rust-lang/rfcs#1789),
and @nikomatsakis has provided a lot of use cases in rust-lang/rfcs#1789 (comment).
Hence this PR.

I'm not too sure what to do with stability, feel free to correct me.
It seems pointless to go through stabilization for these functions though.

cc @aturon
@nox
Copy link
Contributor

nox commented Sep 19, 2019

Note that with the recent effort to reduce the amount of dependencies in some projects, we end up downstream copying the functions in their own crate rather than having one more dependency.

This is IMO quite unfortunate and that's more unsafe code to review for everyone.

https://github.com/CraneStation/cranelift/blob/c47ca7bafc8fc48358f1baa72360e61fc1f7a0f2/cranelift-codegen/src/ref_slice.rs#L1-L8

@Marwes
Copy link
Contributor

Marwes commented Sep 19, 2019

@nox These were brought back to life quite some time ago https://doc.rust-lang.org/std/slice/fn.from_ref.html https://doc.rust-lang.org/std/slice/fn.from_mut.html

@nox
Copy link
Contributor

nox commented Sep 19, 2019

OH WOW, I never saw that. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.