add CString::from_vec_until_nul

[ericseppanen]

This adds a fn that converts a Vec into a CString, without requiring the nul byte to be at the end of the Vec. It is intended for the case when an FFI function call wrote a string of unknown length, which from_vec_with_nul doesn't handle easily.

For example,

let mut buffer = vec![0u8; 32];
unsafe {
    some_c_function(buffer.as_mut_ptr(), buffer.len());
}
let result = CString::from_vec_until_nul(buffer).unwrap();

This is a companion to CStr::from_bytes_until_nul (#94984) , and shares the feature gate: cstr_from_bytes_until_nul (tracking issue #95027).

Unresolved questions:

I couldn't decide whether this fn should use its own unique error type, or whether it should share the error type with from_vec_with_nul. In this version I reused the existing error type, but I'm happy to change if that's the better choice.

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[ericseppanen]

(Second attempt at this; the first attempt was #95160 which was closed by github during force-push shenanigans, and I don't seem to have the powers to re-open it).

[ericseppanen]

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

[ericseppanen]

This PR seems to have fallen between the cracks, so I will ping a few people who were helpful on the earlier PR (#94984): @m-ou-se @AzureMarker @mbartlett21

Please let me know if there's anything I can do to improve this change or push it forward.

I think it makes sense to have an owned (CString) companion to the CStr method added in #94984. It's not a complicated change, but I desire feedback on:

• Should this exist?
• Does it seem weird that it calls .truncate on the input Vec? That seemed surprising to me, but I don't see any other way.
• Should it share an error type with from_vec_with_nul? The resulting name situation seems awkward, but I'm not sure if that's better or worse than having two very similar methods that don't share an error type.
• Docs. Always nice to see better docs :)

[mbartlett21]

[...] but I desire feedback on:

• Should this exist?

It seems like a good idea to match the borrowed version.

• Does it seem weird that it calls .truncate on the input Vec? That seemed surprising to me, but I don't see any other way.

That's fine. It won't actually reallocate the Vec, so the cost of this method is just a scan for a nul. EDIT: Actually it reallocates into a Box anyway, so that is incorrect.

• Should it share an error type with from_vec_with_nul? The resulting name situation seems awkward, but I'm not sure if that's better or worse than having two very similar methods that don't share an error type.

In my opinion, it would be worse having the matching methods for CStr not share an error type but the methods for CString sharing one.

[m-ou-se]

It might be good to mention that this doesn't reduce the capacity of the Vec/CString. (Like the Vec::truncate docs.)

[m-ou-se]

Oh wait, never mind: CString stores a Box<[u8]>, not a Vec, so it does shrink the capacity.

[m-ou-se]

That does raise the question of how useful this method is, though, since it most likely ends up reallocating anyway.

[ericseppanen]

I suppose the same thing could be accomplished with Cstr::from_bytes_with_nul() following by .into_c_string(). But being able to do the same thing from a CString context would be a kindness. Particularly if the user doesn't think to check the docs for both places when looking for this functionality.

[ericseppanen]

I had to tweak the unit test, as some time in the last month support for alloc::Vec::from(b"foo") got reverted (95098).

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[ericseppanen]

I rebased onto a recent master, just to check that this still applies. No changes were made since the last version.

[ericseppanen]

It's probably my fault this went stale, but perhaps reviewers would be willing to have another look?

[m-ou-se]

I am not fully convinced we should have this method, as its use cases are limited and it will still have to reallocate in all cases that are not already covered by from_vec_with_nul.

When would you use this function? Are those use cases already already covered by CString::from_vec_with_nul() or CStr::from_vec_until_nul().to_owned()?

[ericseppanen]

I am not fully convinced we should have this method, as its use cases are limited and it will still have to reallocate in all cases that are not already covered by from_vec_with_nul.

When would you use this function? Are those use cases already already covered by CString::from_vec_with_nul() or CStr::from_vec_until_nul().to_owned()?

To me it's mostly about consistency: CString::from_vec_until_nul is to CString::from_vec_with_nul as CStr::from_bytes_until_nul is to CStr::from_bytes_with_nul. Leaving two with_nul methods in place while only adding one until_nul method seems like it would be confusing to someone reading the CString docs. Adding both is a kindness to the reader, as it's much more obvious how to do Vec->CString without the annoying restriction with_nul imposes.

But this PR has been open a long time, so I'm looking forward to seeing it resolved, one way or the other.

[Stargateur]

I am not fully convinced we should have this method, as its use cases are limited and it will still have to reallocate in all cases that are not already covered by from_vec_with_nul.

I kind of agree, since the implementation of CString force the reallocation (boxed slice) I see no much interest in this. There seem to be no demand for it. I advice use CStr::from_bytes_until_nul(bytes).map(ToOwned::to_owned). Maybe add a documentation note to CString::from_vec_with_nul.

[tgross35]

@ericseppanen if you've addressed the concerns, comment @rustbot ready to mark it as ready for review again

I would put the justification from your previous comment into the issue description. Since this would need FCP, it probably wouldn't be bad to further update it to match the ACP template https://github.com/rust-lang/libs-team/issues/new?assignees=&labels=api-change-proposal%2C+T-libs-api&template=api-change-proposal.md&title=%28My+API+Change+Proposal%29

Also, make sure you update the mentioned gate cstr_from_bytes_until_nul to cstring_from_bytes_until_nul

[ericseppanen]

As far as I can tell, this has already been reviewed and isn't going anywhere. I don't see any point in asking for more reviews when nothing has changed since the last one.