Conversion of String to Vector{UInt8}

[bkamins]

Consider the following code:

s = "123"
v = Vector{UInt8}(s)

Now changing v mutates s.
Since strings should be immutable I would it find it more natural to:

• define reinterpret(UInt8, s::String) that would perform such in-place conversion (as this is what one would expect from reinterpret);
• make convert(Vector{UInt8}, s::String) allocate a new vector (so that users do not accidentally mutate strings).

Additionally this would be consistent with the general behavior that Vector{UInt8}(s) for AbstractString other than String would create a copy.

A non-breaking alternative would be to add an information to the documentation that this conversion is in-place.

[StefanKarpinski]

It does seem like observing the mutation of a string should need to involve something unsafe_: all of these are normal and apparently safe operations, which would seem to violate string immutability.

[bkamins]

If we want to be on a safe side then String(::Vector{UInt8}) also should make a copy by default. The good thing is that it is documented that it does not do it now (as opposed to Vector{UInt8}(::String)), but changing this would be consistent with the above line of thinking.

Regarding the names - there are probably people with more experience with naming conventions in Base than me. Maybe we can use unsafe_string_to_array and unsafe_array_to_string as the underlying C functions are jl_string_to_array and jl_array_to_string. However, I am open to suggestions.

In summary the proposal would be to:

• change Vector{UInt8}(::String) so that it performs a copy;
• change String(::Vector{UInt8}) so that it performs a copy;
• define two new functions performing unsafe conversions between String and Vector{UInt8}; the proposed names are unsafe_string_to_array and unsafe_array_to_string.

If the above are agreed I can make an appropriate PR.

[vtjnash]

That does sound good. Another possible name suggestion would be to make this a method of unsafe_wrap

[malmaud]

Maybe we should add this to triage, since this would be a potentially breaking change.

[JeffBezanson]

This is going to be a performance disaster. The main problem is that String(take!(buf)) is the idiom for converting an IOBuffer (used as a string builder) to a String in-place. reinterpret(String, take!(buf)) is too long and weird to take its place. Changing String( ) requires a solution to this.

[bkamins]

Changing String( ) was less of a priority in my opinion as it is well documented - I have added it later to the discussion to have a complete picture.

From my point of view the key problem is to make Vector{UInt8}(s) conversion make a copy. And if only this part would be left, and reinterpret is considered not a very good name for conversion from String to Vector{UInt8}, actually I like adding the unsafe_wrap(::Type{Vector{UInt8}},::String) method as it would not introduce a new name into Base.

[StefanKarpinski]

As long as some unsafe_ function needs to be called in order to observe string mutability, I think it's fine. The real trouble here is that it's possible to observe string mutation without calling any explicitly unsafe functions.

[JeffBezanson]

I think the following would be safe and efficient:

1. Make Vector{UInt8}(s) copy, as requested here. Internally, it should use a String for the new storage so you can go back to String without yet another copy.
2. String(vec) can continue to share memory (if the vector is String-backed), but we also set the vector's size to 0 and remove its storage so that it's impossible to mutate the string via the vector from then on.

[StefanKarpinski]

Being able to "destroy" arrays like that would be generally useful for APIs where a new object "takes control" of the array's contents and anyone else mutating it after that is generally dangerous. And not just for arrays, if we had deterministic finalization a la #7721, being able to make any usage of the finalized object an error after finalization would be ideal.

[JeffBezanson]

With this, we might want to export StringVector as well, since using it becomes safer.

[JeffBezanson]

Accepted by triage. We can go with the plan in #24388 (comment). No need to export StringVector.

It would be good to know how many uses of Vector{UInt8}(str) there are. There are a fair number in Base. I think most cases just want to access the bytes in the string, which can be done more efficiently using codeunit. The problem is that changing this will silently give worse performance, so we might want to consider deprecating it at least temporarily.