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.

Sign up for GitHub

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

Jump to bottom

Tracking Issue for methods to go from nul-terminated Vec<u8> to CString #73179

Closed
poliorcetics opened this issue Jun 9, 2020 · 12 comments · Fixed by #89292
Closed

Tracking Issue for methods to go from nul-terminated Vec<u8> to CString #73179

poliorcetics opened this issue Jun 9, 2020 · 12 comments · Fixed by #89292
Labels
A-FFI Area: Foreign function interface (FFI) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-feature-request Category: A feature request, i.e: not implemented / a PR. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@poliorcetics
Copy link
Contributor

poliorcetics commented Jun 9, 2020 •
edited
Loading

The feature gate for the issue is #![feature(cstring_from_vec_with_nul)].

This adds the following method to CString:

pub unsafe fn from_vec_with_nul_unchecked(v: Vec<u8>) -> Self { /* ... */ }
pub fn from_vec_with_nul(v: Vec<u8>) -> Result<Self, FromVecWithNulError> { /* ... */ }

FromVecWithNulError is a new error type, following the naming pattern of FromBytesWithNulError already existing for CStr::new.

This type, defined as:

#[derive(Clone, PartialEq, Eq, Debug)]
#[unstable(feature = "cstring_from_vec_with_nul", issue = "73179")]
pub struct FromVecWithNulError {
    error_kind: FromBytesWithNulErrorKind,
    bytes: Vec<u8>,
}

It is inspired from the FromUtf8Error type (having as_bytes and into_bytes method) that allows recuperating the input when conversion failed.

The fmt:Display implementation for the type uses the same text as the FromBytesWithNulError, without using the deprecated description method of the Error trait.

#[unstable(feature = "cstring_from_vec_with_nul", issue = "73179")]
impl fmt::Display for FromVecWithNulError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self.error_kind {
            FromBytesWithNulErrorKind::InteriorNul(pos) => {
                write!(f, "data provided contains an interior nul byte at pos {}", pos)
            }
            FromBytesWithNulErrorKind::NotNulTerminated => {
                write!(f, "data provided is not nul terminated")
            }
        }
    }
}

Unresolved Questions

  • Is this feature needed ? It was asked for issue Creating CString from nul-terminated data #73100 and CStr::from_bytes_with_nul (and its unchecked version) seems to indicates that having an owned version would be logical.
  • Names ? I just changed bytes to vec in the names (CStr::from_bytes_with_nul -> CString::from_vec_with_nul and CStr::from_bytes_with_nul_unchecked -> CString::from_vec_with_nul_unchecked).
  • Input type for the safe version: I used Vec<u8> but I see CString::new uses Into<Vec<u8>>, should I use that too ?

Implementation history

PR #73139 implements this at the moment, in a unstable form, with documentation and doc tests.

PR #89292 by @CleanCut proposes to stabilize this 🥳

Edit: this being the first time I write a tracking issue, please do not hesitate to tell me if there is something to fix.
@poliorcetics poliorcetics added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jun 9, 2020
@poliorcetics
Copy link
Contributor Author

@rustbot modify labels to T-libs, A-ffi, C-feature-request

@rustbot rustbot added A-FFI Area: Foreign function interface (FFI) C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 9, 2020
@LeSeulArtichaut
Copy link
Contributor

Couldn't there also be a TryFrom<Vec<u8>> implementation for CString, calling from_vec_with_nul? This would allow to do vec.try_into()? instead of CString::from_vec_with_nul(vec)?.

@poliorcetics
Copy link
Contributor Author

poliorcetics commented Jun 10, 2020 •
edited
Loading

Couldn't there also be a TryFrom<Vec<u8>> implementation for CString

Added in the current PR (#73139)

Edit: As Dtolnay noted in this comment, there is not one unique behaviour that can be used for a TryFrom implementation, so it has been removed.

@poliorcetics
Copy link
Contributor Author

A new error type has been added. It is explained in the original post.

@KodrAus KodrAus added B-unstable Blocker: Implemented in the nightly compiler and unstable. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@CleanCut
Copy link
Contributor

What is the path to stabilization for this feature?

@poliorcetics
Copy link
Contributor Author

No one has noted any concerns and it has been unstable for quite some time now, if you want to stabilize it you can make a PR for it (be sure to ping me so I can update the tracking issue).

@CleanCut
Copy link
Contributor

CleanCut commented Sep 26, 2021 •
edited
Loading

I found competing documentation about stabilizing a library feature in the rustc-dev-guide and the std-dev-guide. The former seems more detailed, but out of date (I can't find any trace of an @T-libs-api team.) They do seem to agree that step 0 should be completing the FCP, so...

@rust-lang/libs can we start the FCP for this feature?

I'm happy to open the stabilization PR once that has been completed.

@CleanCut CleanCut mentioned this issue Sep 27, 2021
Merged
@CleanCut
Copy link
Contributor

I'm not certain any of my attempted team @-mentions are actually functioning, so I'm going to additionally cc @joshtriplett since they are a library team member and were randomly selected on the PR I'm prepping for this.

@joshtriplett
Copy link
Member

Stabilizing this seems reasonable to me.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Oct 4, 2021 •
edited by dtolnay
Loading

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 4, 2021
@rfcbot
Copy link

rfcbot commented Oct 6, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 6, 2021
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Oct 16, 2021
@rfcbot
Copy link

rfcbot commented Oct 16, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 16, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 21, 2021
@JohnTitor
Rollup merge of rust-lang#89292 - CleanCut:stabilize-cstring_from_vec…
504d16a 
…_with_nul, r=JohnTitor

Stabilize CString::from_vec_with_nul[_unchecked]

Closes the tracking issue rust-lang#73179. I am keeping this in _draft_ mode until the FCP has ended.

This is my first time stabilizing a feature, so I would appreciate any guidance on things I should do differently.

Closes rust-lang#73179
@bors bors closed this as completed in 20687bb Oct 21, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 21, 2021
@blockedtodo blockedtodo bot mentioned this issue Oct 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-FFI Area: Foreign function interface (FFI) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-feature-request Category: A feature request, i.e: not implemented / a PR. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stabilize CString::from_vec_with_nul[_unchecked] CleanCut/rust
8 participants
@joshtriplett @CleanCut @apiraino @KodrAus @poliorcetics @rfcbot @LeSeulArtichaut @rustbot