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 as_slice stabilization #27729

Closed
aturon opened this issue Aug 12, 2015 · 41 comments · Fixed by #30943
Closed

Tracking issue for as_slice stabilization #27729

aturon opened this issue Aug 12, 2015 · 41 comments · Fixed by #30943
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.

Comments

@aturon
Copy link
Member

aturon commented Aug 12, 2015

Over time, the way that you convert e.g. a vector to a slice has transformed repeatedly. With deref coercions, it's usually implicit, but there are still some cases where you need to do it explicitly.

The as_slice method follows the usual conversion conventions, but has been somewhat controversial due to offering "yet another way" to perform the conversion (in addition to as_ref and &*).

@aturon aturon 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
@aturon
Copy link
Member Author

aturon commented Aug 12, 2015

FWIW, "there is only one way to do it" has never been a hard design rule for std, especially when it comes to convenience methods. I personally think there is little cost in having additional conversion methods that follow a strong convention.

@jethrogb
Copy link
Contributor

There's also &vec[..].

@aturon
Copy link
Member Author

aturon commented Sep 23, 2015

Nominating for 1.5 FCP discussion.

@alexcrichton
Copy link
Member

This issue is now entering its cycle-long FCP for conclusion at 1.5.

The libs team was a little up in the air about whether to deprecate or stabilize these methods. On one hand some are already stable (e.g. as_path) and these new methods follow clear existing conventiosn and don't come with much cognitive overhead. On the other hand these methods are very rarely called and it'll be another method to perform a similar task as the slicing syntax, for example.

@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
@steveklabnik
Copy link
Member

I'm generally 👎 on this, personally, for the "yet another way to do it" angle.

I haven't heard of anyone who is actually using as_slice() over slicing syntax, which indicates to me that it shouldn't exist.

@aturon
Copy link
Member Author

aturon commented Sep 24, 2015

cc @carllerche

@FranklinChen
Copy link
Contributor

I was using as_slice() all the time before slicing syntax came along, then I switched, but Vec documentation https://doc.rust-lang.org/std/vec/struct.Vec.html could be improved, to explicitly mention slicing syntax early and up front and with examples.

@bstrie
Copy link
Contributor

bstrie commented Sep 24, 2015

-1, unless you can present some damning case where all of the current three methods is insufficient. I'm already salty about having both &*x and &x[..]. Even if TIMTOWTDI isn't a hard rule, it's a very important guideline.

@jethrogb
Copy link
Contributor

@bstrie do you mean TOOWTDI?

@steveklabnik
Copy link
Member

steveklabnik commented Sep 24, 2015 via email

@ruuda
Copy link
Contributor

ruuda commented Sep 24, 2015

Multiple ways of doing the same thing enable inconsistencies and disagreement about superficial issues. I think we should avoid multiple ways of doing the same things when possible. That said, a method is better for discoverability than syntactic sugar. Deprecating as_slice with a message pointing to &vec[..] can help point new users in the right direction.

@abonander
Copy link
Contributor

👎 It's superfluous to AsRef<[T]> which is already in the prelude and can also be used as a trait bound that accepts [T; N], &[T; N], &[T], Vec<T>, &Vec<T>, Box<[T]>, etc. I'm not a strong proponent of TOOWTDI, but limiting the number of intersecting approaches is probably a good idea--otherwise people just tend to get confused.

Edit: actually, I don't see that Box<T> implements AsRef<T>. Is there a particular reason for this or is it just an oversight?

@nagisa
Copy link
Member

nagisa commented Sep 24, 2015

I don’t see myself using as_slice very often either. I’ll either go for autoderef or &x[..]/&* or .as_ref() in generic code.

@briansmith
Copy link
Contributor

As a recent learner of Rust, I've found the different ways of slicing to be confusing, and as_slice just added to that confusion. So, not only would I like to see as_slice removed, but it would be nice if we could do a sweep of the documentation that mentions it (on blogs, Stack Overflow, and elsewhere) and help people understand that the slicing syntax should be used and not as_slice.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Sep 30, 2015
Fixes rust-lang#28359

I'm not doing more here because it's unclear that `as_slice()` is even going to stick around, see rust-lang#27729
@steveklabnik steveklabnik added this to the 1.5 milestone Oct 1, 2015
@brson
Copy link
Contributor

brson commented Oct 21, 2015

I'm sympathetic to limiting the proliferation of conversion functions.

@alexcrichton
Copy link
Member

The libs team discussed this during triage today and the decision was punt this to a future FCP.

@alexcrichton alexcrichton removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 22, 2015
@bluss
Copy link
Member

bluss commented Oct 29, 2015

This should be removed.

@bluss
Copy link
Member

bluss commented Dec 6, 2015

Nominating for discussion. This unstable method makes more harm than good by misleading people into thinking String -> &str itself is unstable in all forms(!) (As seen in user questions).

It should be removed or stabilized ASAP.

Edit: I'm primarily talking about .as_str(), which shares this tracking issue. as_slice may have the same concerns.

@SimonSapin
Copy link
Contributor

I’ve ran sed commands to mass-remove .as_slice() calls (fixing things up afterwards by adding derefs as necessary per build errors) when deref on Vec and String was added.

+1 for deprecation. There are many replacements. (Which themselves are arguably redundant with each other already.)

@SimonSapin
Copy link
Contributor

also, String has as_bytes() to convert to slice

I don’t think this is comparable. String and &str are both dealing with text, we only have separate types because of how ownership and borrowing work in Rust. Similarly, Vec<u8> and &[u8] are both bytes. str::as_bytes (which really should have been called as_utf8 IMO) however is going from one world to the other.

@arcnmx
Copy link
Contributor

arcnmx commented Dec 29, 2015

another random opinion... I like the explicit conversion methods because it allows one to write Some(&string).map(String::as_slice) or similar. Slicing or deref syntax like Some(&string).map(|s| &*s) is just plain uglier. Borrow::borrow and Deref::deref are often options but aren't included in the default prelude (which I really disagree with for a variety of reasons).

Slicing syntax or the &* magic deref spell is often less readable in some contexts, and doesn't really scream conversion to me. Operators and special syntax are at odds with method chaining as mentioned in a comment above. Having a conversion method that isn't ambiguous to typeck (generic-bound as_ref is out, and so are & coersions) is a very useful thing to me.

@aturon
Copy link
Member Author

aturon commented Dec 31, 2015

I'm with @alexcrichton here. What is the downside of keeping the conversion method in place, given that it follows all of the standard conventions? The answer can't be "there should only be a single way to do it" because (1) there are already many other ways to do it and (2) in general, that's not been tops on the list of concerns with the standard library; we violate that principle all over the place, and for good reason.

My standard argument here is that there is little effective burden in providing the method, because it follows an expected convention -- if you see its name, you know immediately what it's doing. Moreover, offering such a conventional method has ergonomic benefits, not just for the scenarios @alexcrichton described, but because ad hoc as_foo methods are in general the conventional way of doing such conversions.

Put differently, I think "all expected conventional methods/impls are present" significantly outweighs "there's more than one way to do it" in terms of ergonomic/understandability concerns.

@bluss
Copy link
Member

bluss commented Jan 1, 2016

It's an unstable feature, with plenty of alternatives. In that light, we're not keeping them in place, as much as adding the methods to the language.

The "not used very much" arguments are weak for the same reason: you're never supposed to use these, there are stable equivalents. But would you use them, and is this addition a net benefit to Rust?

On balance, I think these methods (Vec::as_slice and String::as_str) are ok, but I'm very much against there being more than one obvious way to do it in general. In particular do not want us to continue adding conversion methods as a standard solution.

These methods are consistent with

  • PathBuf::as_path
  • Iter::as_slice
  • Chars::as_str

These methods are also strictly better than generic conversion methods .as_ref(), since conversion methods are for argument conversion and not supposed to be called in free standing code IMO. (Conversion methods might even infer differently in a new rust, as users have experienced, if you rely on them being unambiguous today.)

These methods possibly heighten confusion. Deref and slicing remains the tools that most experienced rusties will reach for. Deref for method dispatch is simply the natural way, and deref and slicing are both terser (and ingrained!) ways to coerce a Vec into a slice or String into a str. Note however, that the current state of in the docs, unstable and unusable is even more confusing than either choice of stabilizing or removing. So stalling is not an option 😄

@aturon
Copy link
Member Author

aturon commented Jan 5, 2016

@bluss

I'm very much against there being more than one obvious way to do it in general

I would love for someone to lay out the concrete benefits of following this dictum, rather than just asserting the principle. As I mentioned in my comment, we've never given this principle much credence in the standard library; we've valued things like consistent API surfaces more highly. And I've tried to argue why it matters: that once you know the convention around conversion methods, you can find and understand them very easily -- an ergonomic win with no maintenance downside.

These methods possibly heighten confusion.

What is the confusion you anticipate about? I believe our conversion method conventions are pretty clear.

Deref and slicing remains the tools that most experienced rusties will reach for.

As you argued at the beginning of your comment, it's hard to really say that until everything under discussion is stable :)

For myself, I'd definitely use v.as_slice() over &v[..] -- despite being longer, it's much easier to type and a bit easier to read, and works with method chaining, as others have pointed out.

@bluss
Copy link
Member

bluss commented Jan 5, 2016

@aturon

To make sure that we don't end up adding lots of redundant ways to do everything. Isn't .size() more logical than .len() on a HashMap? In that example, it's untenable to add the method, it creates a second standard that's just busy work to expand to cover all collections (including user-defined such), and you risk that some API copies & follows one "standard "and some the other. So we should withstand the urge to add methods just to be nice. (That's kind of the worst case scenario of adding more ways to do something.)

The example when expanded to as_slice (and potentially as_str) has some minor fallout: user collections like ArrayVec, SmallVec (and others I guess)
will need to add .as_slice() to be consistent with Vec. They could go without it, but the precedent is there. Their current Deref to slice impls are then not enough (and AsRef, AsMut, Borrow, and BorrowMut, only arrayvec does those four though, smallvec instead goes for an ensemble of Index impls. We can say the multiple standards for what a "veclike" implements are already showing.). The multiple standards issue would be if ArrayVec chose to only implement as_slice() and SmallVec only Deref. Unlikely, but it could happen in the wider rust ecosystem.

For myself, I'd definitely use v.as_slice() over &v[..] -- despite being longer, it's much easier to type and a bit easier to read, and works with method chaining, as others have pointed out.

Yes, but I think that you will not use v.as_slice().binary_search(...), but instead rely on Deref. I will continue using &v and have it coerce to slice, so I'll continue to use Deref. So in that sense, these less discoverable(?) non-method approaches will remain as the tools that most experienced rusties will reach for.

@bluss bluss closed this as completed Jan 5, 2016
@bluss bluss reopened this Jan 5, 2016
@arcnmx
Copy link
Contributor

arcnmx commented Jan 5, 2016

I wouldn't even really consider it redundant because there is no similar way already. size vs len is redundant because they're both methods. as_slice vs (&_[..] or &*_) is not because they require different ways and syntax to call into them that don't fit all situations. I suppose I'd consider Deref to be equivalent (though kind of ugly) if it were included in the std prelude...

The multiple standards issue would be if ArrayVec chose to only implement as_slice() and SmallVec only Deref. Unlikely, but it could happen in the wider rust ecosystem.

And if SmallVec and ArrayVec choose to implement as_slice() while Vec doesn't? It can happen both ways. In your situation they're both out of sync with the interface they're copying (Vec) and simply need to change. Collection traits would make this mostly a non-issue, but of course we don't have those...

@ruuda
Copy link
Contributor

ruuda commented Jan 5, 2016

I would love for someone to lay out the concrete benefits of following this dictum, rather than just asserting the principle.

After this discussion, I am now slightly in favour of keeping as_slice. But let me elaborate on why multiple ways of doing things are generally bad:

  1. It forces people to make a choice. (Do I use &v[..] or v.as_slice()?) People will have opinions. They will have different opinions. Every time a reviewer asks you to change option A to equivalent option B is time that could have been spent doing useful stuff. To avoid this we introduce additional rules and style guides. Now we need to keep even more things in mind. For instance, the Google C++ Style Guide generally bans iostream in favour of printf. If there was no choice in the first place, such a rule would not be necessary.

  2. It might not be clear that different constructs are doing the same thing. The following Haskell functions are exactly the same function. Who would have guessed?

    liftM :: Monad m => (a1 -> r) -> m a1 -> m r
    fmap :: Functor f => (a -> b) -> f a -> f b
    liftA :: Applicative f => (a -> b) -> f a -> f b
    

    Providing an equivalent for something that exists already can be more convenient in some contexts, but on the other hand, people reading the code will now need to deal with an additional construct. In the example above, if all surrounding code is dealing with monads, it can be more consistent to use liftM there. But somebody who reads the code later might be familiar with fmap and not with liftM. It takes an extra look in the documentation to realise that it is just fmap. And the programmer writing the code will be forced to make the trade off: consistency, or familiarity?

  3. Constructs that appear the same introduce confusion. When you are done with your StreamReader in C#, do you call Close on it or Dispose?

In the case of as_slice, I think the strong conventions around as_* functions, and the convenience of having an actual function, may outweigh the above concerns.

@arcnmx
Copy link
Contributor

arcnmx commented Jan 5, 2016

Good points. The language enforcing a style certainly helps with the ambiguity, and having the choice (and settling on a style) is a bit of a burden. While I obviously feel that it's important to have as_* conversion methods, I'd be conflicted on using them vs slicing syntax when the option isn't clear. I'd probably fall back on slicing syntax just because it's been the stable standard for so long but it's just... so ugly and awkward to type :(

One situation where it comes up somewhat often for me is match &string[..] { "etc" => ... }

@bluss
Copy link
Member

bluss commented Jan 8, 2016

After this discussion, I am now slightly in favour of keeping as_slice

I am too. I have many regrets about there being so many alternatives, but most of them are actually worse than .as_slice(). Adding .as_slice() is a consistency win.

@BurntSushi
Copy link
Member

I am also relatively swayed by this discussion in favor of keeping as_slice (only in the sense that it doesn't do much harm, and it is occasionally useful with more gnarly types), but I personally wouldn't miss it if it were removed.

@briansmith
Copy link
Contributor

@bluss

I'm very much against there being more than one obvious way to do it in general

I would love for someone to lay out the concrete benefits of following this dictum, rather than just asserting the principle

It has the same benefits that justify rustfmt and the Rust Guidelines. In fact, rustfmt and the Rust Guidelines are only necessary because Rust violates this principle in so many ways already. When somebody uses as_slice I'm forced to think about why they didn't use the slicing syntax. Why make it so easy to distract people with trivia like that?

Every email that this thread generates costs more time than adding as_slice will ever add back to anybody's life. Since it's an unstable feature, it's hard to see how any resolution other than removing it and moving on could possibly be more productive.

@alexcrichton
Copy link
Member

The libs team discussed this in triage recently, and the decision was to stabilize

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 16, 2016
This commit stabilizes and deprecates the FCP (final comment period) APIs for
the upcoming 1.7 beta release. The specific APIs which changed were:

Stabilized

* `Path::strip_prefix` (renamed from `relative_from`)
* `path::StripPrefixError` (new error type returned from `strip_prefix`)
* `Ipv4Addr::is_loopback`
* `Ipv4Addr::is_private`
* `Ipv4Addr::is_link_local`
* `Ipv4Addr::is_multicast`
* `Ipv4Addr::is_broadcast`
* `Ipv4Addr::is_documentation`
* `Ipv6Addr::is_unspecified`
* `Ipv6Addr::is_loopback`
* `Ipv6Addr::is_unique_local`
* `Ipv6Addr::is_multicast`
* `Vec::as_slice`
* `Vec::as_mut_slice`
* `String::as_str`
* `String::as_mut_str`
* `<[T]>::clone_from_slice` - the `usize` return value is removed
* `<[T]>::sort_by_key`
* `i32::checked_rem` (and other signed types)
* `i32::checked_neg` (and other signed types)
* `i32::checked_shl` (and other signed types)
* `i32::checked_shr` (and other signed types)
* `i32::saturating_mul` (and other signed types)
* `i32::overflowing_add` (and other signed types)
* `i32::overflowing_sub` (and other signed types)
* `i32::overflowing_mul` (and other signed types)
* `i32::overflowing_div` (and other signed types)
* `i32::overflowing_rem` (and other signed types)
* `i32::overflowing_neg` (and other signed types)
* `i32::overflowing_shl` (and other signed types)
* `i32::overflowing_shr` (and other signed types)
* `u32::checked_rem` (and other unsigned types)
* `u32::checked_neg` (and other unsigned types)
* `u32::checked_shl` (and other unsigned types)
* `u32::saturating_mul` (and other unsigned types)
* `u32::overflowing_add` (and other unsigned types)
* `u32::overflowing_sub` (and other unsigned types)
* `u32::overflowing_mul` (and other unsigned types)
* `u32::overflowing_div` (and other unsigned types)
* `u32::overflowing_rem` (and other unsigned types)
* `u32::overflowing_neg` (and other unsigned types)
* `u32::overflowing_shl` (and other unsigned types)
* `u32::overflowing_shr` (and other unsigned types)
* `ffi::IntoStringError`
* `CString::into_string`
* `CString::into_bytes`
* `CString::into_bytes_with_nul`
* `From<CString> for Vec<u8>`
* `From<CString> for Vec<u8>`
* `IntoStringError::into_cstring`
* `IntoStringError::utf8_error`
* `Error for IntoStringError`

Deprecated

* `Path::relative_from` - renamed to `strip_prefix`
* `Path::prefix` - use `components().next()` instead
* `os::unix::fs` constants - moved to the `libc` crate
* `fmt::{radix, Radix, RadixFmt}` - not used enough to stabilize
* `IntoCow` - conflicts with `Into` and may come back later
* `i32::{BITS, BYTES}` (and other integers) - not pulling their weight
* `DebugTuple::formatter` - will be removed
* `sync::Semaphore` - not used enough and confused with system semaphores

Closes rust-lang#23284
cc rust-lang#27709 (still lots more methods though)
Closes rust-lang#27712
Closes rust-lang#27722
Closes rust-lang#27728
Closes rust-lang#27735
Closes rust-lang#27729
Closes rust-lang#27755
Closes rust-lang#27782
Closes rust-lang#27798
Manishearth added a commit to Manishearth/rust that referenced this issue Jan 17, 2016
This commit stabilizes and deprecates the FCP (final comment period) APIs for
the upcoming 1.7 beta release. The specific APIs which changed were:

Stabilized

* `Path::strip_prefix` (renamed from `relative_from`)
* `path::StripPrefixError` (new error type returned from `strip_prefix`)
* `Ipv4Addr::is_loopback`
* `Ipv4Addr::is_private`
* `Ipv4Addr::is_link_local`
* `Ipv4Addr::is_multicast`
* `Ipv4Addr::is_broadcast`
* `Ipv4Addr::is_documentation`
* `Ipv6Addr::is_unspecified`
* `Ipv6Addr::is_loopback`
* `Ipv6Addr::is_unique_local`
* `Ipv6Addr::is_multicast`
* `Vec::as_slice`
* `Vec::as_mut_slice`
* `String::as_str`
* `String::as_mut_str`
* `<[T]>::clone_from_slice` - the `usize` return value is removed
* `<[T]>::sort_by_key`
* `i32::checked_rem` (and other signed types)
* `i32::checked_neg` (and other signed types)
* `i32::checked_shl` (and other signed types)
* `i32::checked_shr` (and other signed types)
* `i32::saturating_mul` (and other signed types)
* `i32::overflowing_add` (and other signed types)
* `i32::overflowing_sub` (and other signed types)
* `i32::overflowing_mul` (and other signed types)
* `i32::overflowing_div` (and other signed types)
* `i32::overflowing_rem` (and other signed types)
* `i32::overflowing_neg` (and other signed types)
* `i32::overflowing_shl` (and other signed types)
* `i32::overflowing_shr` (and other signed types)
* `u32::checked_rem` (and other unsigned types)
* `u32::checked_shl` (and other unsigned types)
* `u32::saturating_mul` (and other unsigned types)
* `u32::overflowing_add` (and other unsigned types)
* `u32::overflowing_sub` (and other unsigned types)
* `u32::overflowing_mul` (and other unsigned types)
* `u32::overflowing_div` (and other unsigned types)
* `u32::overflowing_rem` (and other unsigned types)
* `u32::overflowing_neg` (and other unsigned types)
* `u32::overflowing_shl` (and other unsigned types)
* `u32::overflowing_shr` (and other unsigned types)
* `ffi::IntoStringError`
* `CString::into_string`
* `CString::into_bytes`
* `CString::into_bytes_with_nul`
* `From<CString> for Vec<u8>`
* `From<CString> for Vec<u8>`
* `IntoStringError::into_cstring`
* `IntoStringError::utf8_error`
* `Error for IntoStringError`

Deprecated

* `Path::relative_from` - renamed to `strip_prefix`
* `Path::prefix` - use `components().next()` instead
* `os::unix::fs` constants - moved to the `libc` crate
* `fmt::{radix, Radix, RadixFmt}` - not used enough to stabilize
* `IntoCow` - conflicts with `Into` and may come back later
* `i32::{BITS, BYTES}` (and other integers) - not pulling their weight
* `DebugTuple::formatter` - will be removed
* `sync::Semaphore` - not used enough and confused with system semaphores

Closes rust-lang#23284
cc rust-lang#27709 (still lots more methods though)
Closes rust-lang#27712
Closes rust-lang#27722
Closes rust-lang#27728
Closes rust-lang#27735
Closes rust-lang#27729
Closes rust-lang#27755
Closes rust-lang#27782
Closes rust-lang#27798
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.