std: Stabilize the ascii module

[alexcrichton]

• Move the type parameter on the AsciiExt trait to an associated type named
  Owned.
• Move ascii::escape_default to using an iterator.

This is a breaking change due to the removal of the type parameter on the
AsciiExt trait as well as the modifications to the escape_default function
to returning an iterator. Manual implementations of AsciiExt (or AsciiExt
bounds) should be adjusted to remove the type parameter and using the new
escape_default should be relatively straightforward.

[breaking-change]

[alexcrichton]

cc @SimonSapin

r? @aturon

[rust-highfive]

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

Note that this does not currently mark the methods/trait as #[stable], somewhat intentionally.

[SimonSapin]

One downside of these changes is that String now has a .eq_ignore_ascii_case(&String) method, which is less flexible than the .eq_ignore_ascii_case(&str) method it has trough Deref to str (which is now shadowed). (Same for Vec<u8> and [u8].) Unfortunately I don’t see how to fix this without keeping two traits.

[SimonSapin]

Implement AsciiExt for all &T where T: AsciiExt.

When is this useful?

[huonw]

Strictly speaking &String and &str are now equally flexible via std::string::as_string although it is unclear to me if we wish to retain that forever.

[SimonSapin]

@huonw Ok, as_string makes things not impossible, but it’s not very convenient or easy to discover.

[alexcrichton]

One downside of these changes is that String now has a .eq_ignore_ascii_case(&String) method,

Hm that's a good point, I had not considered that. Having two traits isn't necessarily the end of the world, so perhaps this makes it worth it to have. Out of curiosity, do you think that having two vs one trait for this is ok?

Implement AsciiExt for all &T where T: AsciiExt.

I didn't originally have a use for it, but I've been doing it with other common traits (mainly just because it's possible), so I figured I'd throw it in. I'd be find leaving it until later though.

[SimonSapin]

Implement AsciiExt for all &T where T: AsciiExt.

I didn't originally have a use for it, but I've been doing it with other common traits (mainly just because it's possible), so I figured I'd throw it in. I'd be find leaving it until later though.

As far as I can tell, there are two uses for a trait: have it in scope to call methods, and use it as a bound in generics. This impl doesn’t help the former: any method of T can already be called on &T trough Deref. For the latter, I don’t see how having a bound on AsciiExt is useful: this trait is really about providing methods, not describing aspects of a type.

So, although having this impl doesn’t hurt, I’d go with “you ain’t gonna need it”. It can always be added later if needed.

[aturon]

Sorry for the long delay looking at this.

I like moving to an associated type and iterator, but agree with @SimonSapin on the other points.

[alexcrichton]

I've updated this PR and because the changes are so minor now I've marked all contents as #[stable] as well.

re-r? @SimonSapin and @aturon

[SimonSapin]

Looks good to me.

[aturon]

So, I'm sorry to be suggesting this so late, but something about this design has always bugged me, and I think I just put my finger on it: the use of the into_ methods at all. Why not instead have mutating variants on &self, and a single AsciiExt trait?

To make this work, we'd need a DerefMut implementation on String -- and a better convention for when we have mutating and copy-creating variants of methods :-)

Thoughts? Am I missing something?

[alexcrichton]

Hm, could you sketch that out a little more concretely? I think we could safely implement DerefMut for String but I'm not sure how it connects to this or how it would help (e.g. you can't safely modify &mut str I believe).

[SimonSapin]

Sure, mutating methods instead of by-value into_ methods would work, they’re equivalent. I don’t really have an opinion on which is preferable.

I don’t think DerefMut is required, the String impl can use .as_mut_vec()

[alexcrichton]

One slightly problem might be that if we have something like fn convert_to_ascii_uppercase(&mut self) then it's uncler to me whether we can provide an impl of AsciiExt on str. I think in terms of memory safety we should be able to, but I'm not sure that &mut str really shows up anywhere else...

[SimonSapin]

These methods would be on the OwnedAsciiExt trait, only implemented for String and Vec<u8>.

[aturon]

@SimonSapin

Sure, mutating methods instead of by-value into_ methods would work, they’re equivalent. I don’t really have an opinion on which is preferable.

They're not equivalent, because the into methods require full ownership of the data (so you can't use it with e.g. &mut [u8]). This is why these methods stick out like a sore thumb (to me).

I don’t think DerefMut is required, the String impl can use .as_mut_vec()

The idea was to not impl on String/Vec at all, but to work directly on slices. (See sketch below)

@alexcrichton

Hm, could you sketch that out a little more concretely? I think we could safely implement DerefMut for String but I'm not sure how it connects to this or how it would help (e.g. you can't safely modify &mut str I believe).

One slightly problem might be that if we have something like fn convert_to_ascii_uppercase(&mut self) then it's uncler to me whether we can provide an impl of AsciiExt on str. I think in terms of memory safety we should be able to, but I'm not sure that &mut str really shows up anywhere else...

Right, so we don't use &mut str anywhere today because there's relatively little you can do safely with it. But the type is available! And this is a safe use case.

Here's what I have in mind. We have a single trait AsciiExt in ascii, as follows:

pub trait AsciiExt {
    type Owned;
    fn is_ascii(&self) -> bool;
    fn to_ascii_uppercase(&self) -> Owned;
    fn to_ascii_lowercase(&self) -> Owned;
    fn make_ascii_uppercase(&mut self);
    fn make_ascii_lowercase(&mut self);
    fn eq_ignore_ascii_case(&self, other: &Self) -> bool;
}

and this trait would be implemented for: [u8], u8, char, and str only.

For this to work, we need String to DerefMut to str, so that dispatch will automatically deref and find the implementation above. (We could impl on String directly, of course, but that would be redundant.)

[aturon]

Note: the naming of the methods in the above sketch is a strawman, we badly need a convention here.

[SimonSapin]

Ooh, I see. Yes, that makes sense. And having a single trait is nicer.

[alexcrichton]

How would you guys feel about moving to that today? We could leave OwnedAsciiExt as #[unstable] as well as the make_* methods. That should mean that implementations of AsciiExt are unstable (due to unstable methods) and use of the make_* methods is unstable. All stable methods should be fine to use, however.

[aturon]

@alexcrichton I'd be fine with that, modulo that I'm not really fond of make_ as a prefix (but am struggling to come up with a good convention).

[alexcrichton]

Ok, updated with mentioned changes, r? @aturon

[aturon]

@bors: r+ 235f35b

[bors]

⌛ Testing commit 235f35b with merge 8a91f08...

[bors]

💔 Test failed - auto-mac-64-nopt-t

[SimonSapin]

Looks like make_ascii_{upper,lower}case landed, but DerefMut<Target=str> for String (which it requires) did not.

http://www.reddit.com/r/rust/comments/2xr7i1/how_to_call_strmake_ascii_uppercasemut_self/

[SimonSapin]

Also, should OwnedAsciiExt be deprecated?

[alexcrichton]

Looks like make_ascii_{upper,lower}case landed, but DerefMut<Target=str> for String (which it requires) did not.

Yes that is currently intentional. Implementing DerefMut for String is a bit of a bigger decision I'd like to hold off on just yet. This is also why OwnedAsciiExt is not deprecated so there's still a method of consumption during case conversion.