[proof of concept] give String support for CoW wrapping of literals

[Gankro]

This is a proof of concept implementation for people who want to test this out. It isn't fully tested, and the RawVec changes probably need more work. In addition the implementation isn't as optimized as it could be (see FIXME's).

This adds a String::literally constructor that wraps up a literal without allocating. It in turn makes Vec partially support this (with more robust capacity checking) and RawVec partially support this (by copying the source if len > cap). String also gains some make_unique guards for methods which don't naturally do this themselves (ones which make mutable views or mutate the middle of the buffer).

[rust-highfive]

r? @sfackler

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

[Gankro]

cc @withoutboats @aturon

[Gankro]

How to test this: replace instances of:

• String::from("...")
• "...".to_string()
• "...".to_owned()

with String::literally("...")

[ghost]

Isn't it "Vecs", not "Vec's"?

You only need the apostrophe if the "Vec" is possessing something.

[Gankro]

this is a pre-existing comment, but yes

[ghost]

You might want to change this to something more informative.

[Gankro]

This isn't intended to be landed any time soon, so I opted for a joke :)

[BurntSushi]

This isn't intended to be landed any time soon

Do you say this because there is more implementation work to be done, or because it hasn't been decided that this is a good thing? (I'm pretty out of the loop.) If the former, cool. If the latter, has it been discussed somewhere?

[withoutboats]

There's a thread on internals about landing this feature as a coercion (rather than a method): https://internals.rust-lang.org/t/pre-rfc-allowing-string-literals-to-be-either-static-str-or-string-similar-to-numeric-literals/5029

[BurntSushi]

Oh hey, that's pretty cool! Thanks.

[Manishearth]

https://github.com/manishearth/rust/tree/literally-sed has it all sed-ed. The sed commands are in each commit (gsed is GNU sed, which is default on most distros but not on OSX)

There's a ^\([^\/]*\)" in it so that we don't do replacements on doc comments.

[shepmaster]

I don't really know how to triage this. You uh... hmm. Uh, what do you think we should do?

[aturon]

@shepmaster I'm gonna mark as waiting on (libs) team. cc @rust-lang/libs

[aturon]

@Gankro

The libs team discussed this PR some today, and the following points came up:

• What happened to concerns around unsafe code making assumptions about the capacity? Wasn't that the killer last time around?
• People are generally fine with the DerefMut implementation potentially allocating, but this would mean that String would not implement DerefPure, if that becomes a thing. It was unclear exactly what the implications would be.
• More generally, are we concerned about code assuming a stable address?

Modulo these concerns, no one saw a blocking objection, and we'd love to see this proceed to an RFC.

[Gankro]

We only found examples of people assuming vec's capacity. We halted progress because it was possible to use String in the same way. It still is. That's one of the reasons this PR exists: so we can test this change out on other crates.

Sadly it's hard to cargo-bomb this since nothing in the ecosystem uses it, so nothing would trip it!

I am vaguely considered about stable address assumptions, but also we could reasonably just phase it out of the ecosystem, like we've done for other implementation-defined assumptions that we've broken (heap::EMPTY).

[stjepang]

What happened to concerns around unsafe code making assumptions about the capacity? Wasn't that the killer last time around?

Unless I misunderstood something, the assumption in question is s.len() <= s.capacity().

This PR breaks the assumption, but I believe it's easy to avoid breaking it. The trick is to implement String::capacity as:

fn capacity(&self) -> usize {
    cmp::max(self.vec.capacity(), self.vec.len())
}

This way the assumption is still valid, and we can still internally disambiguate owned and literal strings by checking self.vec.capacity() == 0.

[Gankro]

No, this will break anyone using the _raw apis.

[withoutboats]

More generally, are we concerned about code assuming a stable address?

Rental assumes that string's derefmut derefs to a stable address. I spoke briefly with the author once; rental has a StableDeref trait that would just need to gain a StableDerefMut pair that isn't implemented for String. This would break rental's current assumptions - but its only relevant for the ascii mutation in-place methods, because they're the only methods using &mut str. In other words, there's probably no code in existence which actually makes use of rental's broken assumption about String.

I'm really eager to see this change, but I think it needs to be epoch gated.

[rkruppe]

I thought library changes can't be epoch-specific, because crates of all epochs share the same standard library (and other libraries, for that matter)?

[withoutboats]

I thought library changes can't be epoch-specific, because crates of all epochs share the same standard library (and other libraries, for that matter)?

We can introduce new APIs only in a new epoch; in this case String::literally. So it wouldn't be possible to construct a String that violates the current assumptions on the 2015 epoch (even though the internal implementation would still support that possibility).

[CryZe]

Can't such a String get moved into a library that is written against the 2015 epoch though?

[withoutboats]

Can't such a String get moved into a library that is written against the 2015 epoch though?

Absolutely.

To be clearer, I don't think we need an epoch to change this (from a legalistic perspective). Rental is currently making an assumption that String upholds a particular invariant that we haven't really guaranteed. However, rental - and possibly other unsafe code - is making that assumption, and we need to decide what to do about it.

Often, we will say our hands are tied by the unsafe code that exists and is relying on this thing, so we now guarantee this thing we didn't originally say we would. I don't think that needs to be the case here, since the guarantee failing can only be observed in very particular edge cases - assuming that calling to_ascii_(upper|lower)case will not change the address of the string from what it was before. Rental can make a breaking change for this little detail and all will be well.

However, I'd like to very clearly and loudly message to users about the fact that these assumptions about String are now invalidated. One way to do that is have it be one of the changes we're making in the epoch transition. So I'd like to use the epoch as a social force in amplifying information about this potential incorrect assumption that as far as we know no one is actually relying on.

[bors]

☔️ The latest upstream changes (presumably #46952) made this pull request unmergeable. Please resolve the merge conflicts.

[shepmaster]

I'm going to unilaterally move this to be nominated for discussion by the team, otherwise it seems like this will just languish forever.

(or you know, what @aturon said in response to me earlier that I forgot about...)

[aturon]

@shepmaster I'm going to go a step further and close this PR -- not because we don't want it, but because the next step is an RFC.

I don't have the bandwidth to write such an RFC myself, but I'd really like to see this at least attempted. Is there anyone who would like to take on an RFC with me mentoring/serving as backup?

[Manishearth]

I can do an RFC, if Gankro doesn't want to. But if someone else wants to be mentored it's better if they do it (I can help mentor). On Jan 27, 2018 10:47 PM, "Aaron Turon" <notifications@github.com> wrote: @shepmaster <https://github.com/shepmaster> I'm going to go a step further and *close* this PR -- not because we don't want it, but because the next step is an RFC. I don't have the bandwidth to write such an RFC myself, but I'd really like to see this at least attempted. Is there anyone who would like to take on an RFC with me mentoring/serving as backup? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#46993 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSC4dRuAXJLARGdZrb8f-0I0JImLfks5tO1o1gaJpZM4RMFN5> .

[Gankro]

I don't have any interest in working on an RFC. I made this PoC so ergonomics people couldn't complain to me about implementation issues anymore. Yer problem now!

[withoutboats]

I'm interested in shepherding (but not writing) an RFC that adds this as a coercion (rather than the String::literally constructor), gated on the 2018 epoch.

[SimonSapin]

Why not both a coercion and an explicit contructor? Coercion sounds like it’d work from any &'static str value, is that what you had in mind or only from literals like integers? Why epoch-gated?

[withoutboats]

I don't really have an objection to having a constructor. I laid out an argument in favor of epoch gating a few posts up. More generally these seem like good questions for an RFC thread 😉.