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

Merge the Str and StrSlice? #14614

Closed
SimonSapin opened this issue Jun 3, 2014 · 21 comments
Closed

Merge the Str and StrSlice? #14614

SimonSapin opened this issue Jun 3, 2014 · 21 comments

Comments

@SimonSapin
Copy link
Contributor

Motivation: The StrSlice trait currently has a bunch of string convenience methods, implemented for &str but not the owned String. To use them on an owned string, you need e.g. s.as_slice().contains(needle) where it seems like s.contains(needle) should be enough.

It’d be nice if these methods were available on String. I’d told the answer is DST, and implementing Deref on String. But DST is not there yet. Instead…

Proposal: Move all the StrSlice methods into the Str trait and make them default methods based on the .as_slice() method.

CC @alexcrichton, @brson

@alexcrichton
Copy link
Member

This sounds pretty reasonable to me. Vectors and slices suffer a similar problem, and we should discuss how to solve them in the same way as well.

Additionally, this should have a little discussion before moving forward, I think, to ensure that it's the right direction that we want to go in.

@aturon
Copy link
Member

aturon commented Jun 3, 2014

cc me

@aturon
Copy link
Member

aturon commented Jun 3, 2014

cc #11876

@aturon
Copy link
Member

aturon commented Jun 3, 2014

@alexcrichton I wonder if you can spell out the rationale for the current design? One of the things I was most confused about when first learning Rust was the various traits around strings, vectors, and slices. In particular, do you know why the Str and Vector traits exist?

I am very much in favor of cutting down the number of traits for these core types. We should also think carefully about the naming here; we already have str and String, and the Str trait only adds to the confusion. It's also confusing that the Vector trait lives not in vec but in slice. Unfortunately, I don't have a great proposal offhand, but I'll think about it.

Does anyone see any downsides to doing the proposed merger?

@huonw
Copy link
Member

huonw commented Jun 3, 2014

I believe I actually tried this a while ago, but could not get the lifetimes to work correctly (I may be misremembering).

@aturon
Copy link
Member

aturon commented Jun 3, 2014

Hmm, maybe one of us should take a crack at doing this and see what goes wrong? I'd be happy to do it, unless @huonw wants to dig up an old checkout/branch?

@huonw
Copy link
Member

huonw commented Jun 3, 2014

I can't find the branch, sorry.

@alexcrichton
Copy link
Member

These traits originally existed, to the best of my knowledge, to allow for impl<T: Str> StrSlice for T. Note that when these were written, default methods did not work, which is likely why default methods were not chosen. I view these traits as details that should never be thought about. No program should ever type the name of these traits, except for the libstd implementation itself. I think that rustdoc has gotten better in this respect, but it was lacking when these were written.

I don't really see a reason for the duplication any more (as opposed to leveraging default methods). I also agree that the names should be chosen wisely. StrSlice and Slice seem like reasonable choices. Currently there is a distinction between ImmutableSlice and MutableSlice, but this will likely go away with DST. I don't think the name Vector is appropriate for a trait in std::slice any more.

@aturon
Copy link
Member

aturon commented Jun 4, 2014

Thanks @alexcrichton. I'll work on a patch soon and see if I run into lifetime troubles.

My worry about Slice in particular is that, to a newcomer, it's very confusing that &[T] is called a slice but then in the slice module there's also a trait called Slice, which in the proposed design is also implemented by Vec! The best alternative I can think of would be Sliceable (not a slice, but closely related), but it's not great.

Some of these worries would be mitigated by having rustdoc pages for the actual slice and str types, though. (Didn't you recently land a patch for that? I don't see those in the docs, but maybe I'm missing something?)

@huonw
Copy link
Member

huonw commented Jun 4, 2014

Yes, it landed recently: e.g. http://doc.rust-lang.org/master/std/primitive.slice.html

@aturon
Copy link
Member

aturon commented Jun 4, 2014

Oh, I see, it's linked from the main rustdoc page, rather than from within the modules defining these types.

@alexcrichton I still find it confusing that if I head to the rustdoc for the relevant module, I don't see the docs for the key type it defines. Is there any way that e.g. the slice module could actually list the slice type as something it defines, with a link to the primitive page?

@aturon
Copy link
Member

aturon commented Jun 4, 2014

@alexcrichton wow these pages are great!

@alexcrichton
Copy link
Member

Ah yes, of course! I meant to do that but I forgot. The std::slice module can definitely link to the slice primitive.

Also, you can actually click on the slice type to go to the slice page, it's just that most of the time the only clickable thing is the left-bracket because the actual type inside the slice is itself clickable.

@aturon
Copy link
Member

aturon commented Jun 4, 2014

tl;dr: the proposed implementation strategy is probably not viable.

@huonw You're right: there are lifetime problems.

The current StrSlice trait takes a lifetime parameter and does some funny things with it:

trait StrSlice<'a> {
    fn slice(&self, uint, uint) -> &'a str;
    ...
}

Note that the lifetime of self here is arbitrary, yet the resulting subslice has the lifetime 'a (which is tied to the original slice).

Now suppose we want to add as_slice, and provide default methods that delegate to it. What should the signature be?

fn as_slice(&self) -> &'a str;
fn as_slice(&'a self) -> &'a str;
fn as_slice<'b>(&'b self) -> &'b str;

Unfortunately, none of these options work:

  • The first option is impossible to implement for String, since it's basically saying that given a borrowed &'b String you can produce a &'a str for an arbitrary lifetime 'a, which is wrong.
  • The other two options do not give sufficient lifetimes to implement the default methods, which (like slice above) often return longer lifetimes for their output than their input.

Why is StrSlice designed this way?

Concretely, it means that examples like

let a = "test".to_str();
let mut b = a.as_slice();
b = b.slice(1, 2);

work, which rely on b having the same lifetime as the subslice b.slice(1,2).

Philosophically, I think the point is that a slice already has a determined lifetime, which we want to record in the trait's type precisely to play the games above. So saying impl<'a> StrSlice<'a> for String just doesn't make sense, since it's promising that the String has any lifetime 'a that you like, prior to actually turning it into a slice.

On the other hand, a design like

trait StrSlice {
    fn slice<'a>(&'a self, uint, uint) -> &'a str;
    ...
}

can be implemented for both &str and String, but is less flexible, since it wouldn't provide the longer lifetimes that today's StrSlice trait does.

In summary, I'm not sure that &str and String can share a common interface, because the methods on &str can (and should) provide stronger lifetime bounds.

cc @nikomatsakis

@aturon aturon added the A-libs label Jun 4, 2014
@huonw
Copy link
Member

huonw commented Jun 4, 2014

Hm, does impl<'a> StrSlice<'a> for &'a String work?

@aturon
Copy link
Member

aturon commented Jun 4, 2014

@huonw Not really: it won't autoborrow, so the end result is that if foo is a String then (&foo).contains(needle) works but foo.contains(needle) doesn't. That's not a big improvement over foo.as_slice().contains(needle), sadly.

@nikomatsakis
Copy link
Contributor

On Tue, Jun 03, 2014 at 11:10:05PM -0700, Aaron Turon wrote:

In summary, I'm not sure that &str and String can share a common
interface, because the methods on &str can (and should) provide
stronger lifetime bounds.

Your analysis seems correct. However, you COULD implement the same
interface for &str and &String. Not sure if this is helpful. If
you think about it, though, it makes sense.

@aturon
Copy link
Member

aturon commented Jun 5, 2014

@nikomatsakis Agreed -- I think this is what @huonw was suggesting above. But at least in today's Rust, as I mentioned above,

if foo is a String then (&foo).contains(needle) works but foo.contains(needle) doesn't. That's not a big improvement over foo.as_slice().contains(needle), sadly.

because autoborrowing only applies to arguments of type &self, not to traits implemented for &T.

@nikomatsakis
Copy link
Contributor

On Thu, Jun 05, 2014 at 09:23:10AM -0700, Aaron Turon wrote:

@nikomatsakis Agreed -- I think this is what @huonw was suggesting above. But at least in today's Rust, as I mentioned above,

if foo is a String then (&foo).contains(needle) works but foo.contains(needle) doesn't. That's not a big improvement over foo.as_slice().contains(needle), sadly.
because autoborrowing only applies to arguments of type &self, not to traits implemented for &T.

Right. But in the new algorithm I am working on, I actually know how
to extend it to the other case. I was debating if it's worth doing or
not. This seems to push me over to one side. We'll have to experiment
a bit I guess.

@japaric
Copy link
Member

japaric commented Nov 4, 2014

String/Vec now implement Deref<str>/Deref<[T]>. I think this can be closed.

@aturon
Copy link
Member

aturon commented Nov 4, 2014

Agreed. Closing.

@aturon aturon closed this as completed Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants