Implement TryFrom for CString and CStr #44916

nvzqz · 2017-09-29T05:42:25Z

Implements TryFrom by utilizing the available conversions.

This is a subset of #33417.

Conversions

From	Into	Kind
~~`&{mut,} [u8]`~~	~~`&{mut,} str`~~	~~UTF-8~~
~~`Vec<u8>`~~	~~`String`~~	~~UTF-8~~
~~`&[u16]`~~	~~`String`~~	~~UTF-16~~
`&[u8]`	`&CStr`	UTF-8 with nul
`CString`	`String`	UTF-8
`Into<Vec<u8>>`	`CString`	UTF-8 with nul

Motivation

These types already have conversions that return Result<Self, Error>. This simply implements TryFrom for such types.

rust-highfive · 2017-09-29T05:42:37Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nvzqz · 2017-09-29T05:43:44Z

@sfackler would you like to take a look at this one? It's similar to #44764 in nature.

nvzqz · 2017-09-29T06:47:05Z

r? @sfackler

nvzqz · 2017-09-29T06:54:33Z

~~Getting this error now:~~

[00:02:28] error: use of unstable library feature 'try_from' (see issue #33417)
[00:02:28]   --> /checkout/src/liballoc/string.rs:60:5
[00:02:28]    |
[00:02:28] 60 | use core::convert::TryFrom;
[00:02:28]    |     ^^^^^^^^^^^^^^^^^^^^^^
[00:02:28]    |
[00:02:28]    = help: add #![feature(try_from)] to the crate attributes to enable
[00:02:28] 
[00:02:28] error: use of unstable library feature 'try_from' (see issue #33417)
[00:02:28]     --> /checkout/src/liballoc/string.rs:1607:5
[00:02:28]      |
[00:02:28] 1607 |     type Error = FromUtf8Error;
[00:02:28]      |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:02:28]      |
[00:02:28]      = help: add #![feature(try_from)] to the crate attributes to enable

~~Even though I marked the impl as unstable.~~

Seems to be fixed with 526de507c81062905e2206eac06308cfe63a91a3.

arthurprs · 2017-09-29T08:11:44Z

Thank you. Very useful 😄

sfackler · 2017-09-29T17:46:39Z

I'm feeling a bit unsure about the UTF8 and UTF16 conversions - for [u8] in particular, there are all kinds of text encodings, and it's not necessarily clear that it would obviously interpret it as UTF8 rather than trying to infer the type or whatever.

sfackler · 2017-09-29T18:23:41Z

cc @rust-lang/libs

bors · 2017-10-13T01:39:08Z

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

shepmaster · 2017-10-15T15:53:09Z

Yo — @rust-lang/libs — it's been 16 days since we heard from all y'all. Care to chime in?

alexcrichton · 2017-10-16T16:20:53Z

These seem fine to me, @sfackler I don't have too many thoughts about the encoding question, it seems fine to me to take a conservative route and not add them for now.

sfackler · 2017-10-18T03:14:58Z

That'd limit it to the cstr impls then.

kennytm · 2017-11-02T13:49:05Z

Hello @nvzqz @rust-lang/libs — Is there a decision for this PR, that we limit to the CStr implementations?:

// keep these:
impl<'a> TryFrom<&'a [u8]> for &'a CStr { … }
impl TryFrom<Vec<u8>> for CString { … }
impl TryFrom<CString> for String { … }

// and remove the rest?

nvzqz · 2017-11-02T15:27:23Z

Makes sense to me. I'll make the appropriate changes later today.

shepmaster · 2017-11-10T15:14:53Z

I'll make the appropriate changes later today.

Hello from triage, @nvzqz! It's been a week since we last heard from you and it appears you've got some merge conflicts! Will you have some time to address those and the most recent feedback you received?

nvzqz · 2017-11-10T16:13:09Z

@shepmaster just addressed these changes. Thanks for the reminder 😄

ollie27 · 2017-11-10T18:15:57Z

src/libstd/ffi/c_str.rs

+    type Error = IntoStringError;
+
+    fn try_from(c_str: CString) -> Result<String, IntoStringError> {
+        c_str.into_string()


This checks for valid UTF-8 so this impl should also be removed.

TryFrom<CString> for String is an obvious conversion. The underlying conversion checks valid UTF-8 but CString is a named and obvious type unlike Vec<u8>.

A CString is a sequence of none nul bytes. A Vec<u8> is a sequence of bytes. The conversion to String is exactly the same so it doesn't make sense to include one but not the other.

@kennytm @sfackler do you guys have any input on this?

I'd lean towards leaving this out for now.

kennytm

CI failed...

kennytm · 2017-11-14T08:32:44Z

src/libstd/ffi/c_str.rs

@@ -646,6 +647,24 @@ impl From<CString> for Vec<u8> {
    }
 }

+#[unstable(feature = "try_from", issue = "33417")]
+impl<T: Into<Vec<u8>>> TryFrom<T> for CString {


[00:02:22] error[E0119]: conflicting implementations of trait `core::convert::TryFrom<_>` for type `ffi::c_str::CString`: [00:02:22] --> /checkout/src/libstd/ffi/c_str.rs:651:1 [00:02:22] | [00:02:22] 651 | / impl<T: Into<Vec<u8>>> TryFrom<T> for CString { [00:02:22] 652 | | type Error = NulError; [00:02:22] 653 | | [00:02:22] 654 | | fn try_from(t: T) -> Result<CString, NulError> { [00:02:22] 655 | | CString::new(t) [00:02:22] 656 | | } [00:02:22] 657 | | } [00:02:22] | |_^ [00:02:22] | [00:02:22] = note: conflicting implementation in crate `core`

It is conflicting with impl<T, U> TryFrom<U> for T where T: From<U>, because CString: Into<Vec<u8>>, i.e. this overlaps with the identity transformation CString: TryFrom<CString>.

Try to change it to impl TryFrom<Vec<u8>> for CString and impl<'a> TryFrom<&'a [u8]> for CString.

Perhaps this should just be TryFrom<Vec<u8>> instead. Only having the slice conversion results in a potentially unwanted allocation in the case of:

let vec = vec![/* ... */]; let c_str: CString = vec.try_into()?;

kennytm · 2017-11-22T10:49:43Z

Hi @nvzqz, do you have time to fix the CI failure?

kennytm · 2017-11-23T08:08:51Z

Ok @sfackler it's your turn again.

BTW @nvzqz I strongly suggest you rebase and combine everything down to a single commit.

Conversions: - Vec<u8> to CString - CString to String - &[u8] to &CStr

carols10cents · 2017-11-27T15:15:43Z

Review ping for you @sfackler !

kennytm · 2017-11-27T19:09:49Z

@sfackler There is a question for you too on #44916 (review).

kennytm · 2017-11-29T15:42:42Z

@nvzqz I think the consensus here is to keep only the encoding-agnostic conversions [u8] → CStr and Vec<u8> → CString.

ollie27 · 2017-11-29T16:02:20Z

There is also an inconsistency between the &[u8] → &CStr and Vec<u8> → CString conversions. &[u8] → &CStr requires the slice ends in a nul byte but Vec<u8> → CString doesn't. At the very least that needs to be documented on the impls.

kennytm · 2017-12-06T13:59:38Z

I'm nominating this PR to the @rust-lang/libs team. The consensus so far is removing every conversion involving string encodings, which leaves us with [u8] → CStr and Vec<u8> → CString.

However, as #44916 (comment) pointed out, these two conversions are semantically incompatible in this current form. I can see several resolutions:

Accept the discrepancy, improve documentation
Remove [u8] → CStr and keep only Vec<u8> → CString
Remove Vec<u8> → CString and keep only [u8] → CStr
Keep [u8] → CStr and change Vec<u8> → CString to require a trailing zero
Give up, don't add any TryFrom implementations.
Some other creative solution?

No matter which is picked, I don't think the author can proceed without further decisions.

sfackler · 2017-12-06T17:11:10Z

The fact that there is any debate over what the semantics of these impls may indicate we shouldn't add them.

kennytm · 2017-12-20T14:50:33Z

Triage ping @rust-lang/libs @sfackler! Any update on this PR?

dtolnay

We discussed these impls with the libs team. Regarding &[u8] ⟶ &CStr and Vec<u8> ⟶ CString we agreed on option 5 of Kenny's #44916 (comment) -- give up and don't add these as TryFrom implementations. We prefer for code to use the verbose but unambiguous named constructors like CStr::from_bytes_with_nul if there is any question of what the impl does.

Feeling the need to document what the impl does is a sure sign that there is a question of what the impl does. From and TryFrom impls should never need explanatory documentation.

The &[u8] ⟶ &CStr conversion is the one that comes closest to meeting this bar. If someone understand the invariants of CStr and the implications of & ⟶ &, it really narrows it down to one possible thing the TryFrom impl could reasonably do. But even for that we felt it involves too much background knowledge and in practice the impl would need to be documented, so the conversion should not be handled by a TryFrom impl.

About CString ⟶ String we felt less strongly, but from reading the discussion here again I agree with the consensus of ruling this one out for the same reason as Vec<u8> ⟶ String.

As a secondary concern, we were skeptical about the motivation for adding any of these TryFrom impls. In general we felt that the existence of a Result<Self, Error> method does not necessary mean that an equivalent TryFrom impl needs to exist. We would have preferred for this to be motivated by a concrete API that someone wanted to write, some function where it makes sense to take T: TryInto<&'a CStr> that cannot be expressed any other way that is as ergonomic for callers.

Thanks anyway for the PR and for the insightful discussion -- this is what helps figure out the role we want for TryFrom.

rust-highfive assigned BurntSushi Sep 29, 2017

nvzqz changed the title ~~Implement TryFrom<&[u8]> for &str~~ Implement TryFrom for String Types Sep 29, 2017

nvzqz force-pushed the str-try-from branch from 3afebb1 to dc0ec3b Compare September 29, 2017 06:13

rust-highfive assigned sfackler and unassigned BurntSushi Sep 29, 2017

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2017

sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 29, 2017

aidanhs added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2017

sfackler added the I-needs-decision Issue: In need of a decision. label Oct 15, 2017

jonhoo mentioned this pull request Nov 3, 2017

No support for non-UTF8 parsing djc/imap-proto#5

Open

carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 6, 2017

ollie27 reviewed Nov 10, 2017

View reviewed changes

kennytm requested changes Nov 14, 2017

View reviewed changes

kennytm approved these changes Nov 23, 2017

View reviewed changes

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 23, 2017

nvzqz force-pushed the str-try-from branch from f44a07d to 26c0efe Compare November 23, 2017 16:56

nvzqz changed the title ~~Implement TryFrom for String Types~~ Implement TryFrom for CString and CStr Nov 23, 2017

Add TryFrom impls for CString and CStr

1eefda6

Conversions: - Vec<u8> to CString - CString to String - &[u8] to &CStr

nvzqz force-pushed the str-try-from branch from 26c0efe to 1eefda6 Compare November 23, 2017 17:01

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-needs-decision Issue: In need of a decision. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2017

kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-nominated and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 6, 2017

dtolnay reviewed Dec 21, 2017

View reviewed changes

dtolnay closed this Dec 21, 2017

nvzqz deleted the str-try-from branch December 25, 2017 19:37

dtolnay self-assigned this Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement TryFrom for CString and CStr #44916

Implement TryFrom for CString and CStr #44916

nvzqz commented Sep 29, 2017 •

edited

Loading

rust-highfive commented Sep 29, 2017

nvzqz commented Sep 29, 2017

nvzqz commented Sep 29, 2017

nvzqz commented Sep 29, 2017 •

edited

Loading

arthurprs commented Sep 29, 2017

sfackler commented Sep 29, 2017

sfackler commented Sep 29, 2017

bors commented Oct 13, 2017

shepmaster commented Oct 15, 2017

alexcrichton commented Oct 16, 2017

sfackler commented Oct 18, 2017

kennytm commented Nov 2, 2017

nvzqz commented Nov 2, 2017

shepmaster commented Nov 10, 2017

nvzqz commented Nov 10, 2017

ollie27 Nov 10, 2017

nvzqz Nov 11, 2017

ollie27 Nov 11, 2017

nvzqz Nov 23, 2017

sfackler Nov 29, 2017

kennytm left a comment

kennytm Nov 14, 2017

nvzqz Nov 15, 2017

kennytm commented Nov 22, 2017

kennytm commented Nov 23, 2017

carols10cents commented Nov 27, 2017

kennytm commented Nov 27, 2017

kennytm commented Nov 29, 2017

ollie27 commented Nov 29, 2017

kennytm commented Dec 6, 2017

sfackler commented Dec 6, 2017 •

edited

Loading

kennytm commented Dec 20, 2017

dtolnay left a comment

Implement TryFrom for CString and CStr #44916

Implement TryFrom for CString and CStr #44916

Conversation

nvzqz commented Sep 29, 2017 • edited Loading

Conversions

Motivation

rust-highfive commented Sep 29, 2017

nvzqz commented Sep 29, 2017

nvzqz commented Sep 29, 2017

nvzqz commented Sep 29, 2017 • edited Loading

arthurprs commented Sep 29, 2017

sfackler commented Sep 29, 2017

sfackler commented Sep 29, 2017

bors commented Oct 13, 2017

shepmaster commented Oct 15, 2017

alexcrichton commented Oct 16, 2017

sfackler commented Oct 18, 2017

kennytm commented Nov 2, 2017

nvzqz commented Nov 2, 2017

shepmaster commented Nov 10, 2017

nvzqz commented Nov 10, 2017

ollie27 Nov 10, 2017

Choose a reason for hiding this comment

nvzqz Nov 11, 2017

Choose a reason for hiding this comment

ollie27 Nov 11, 2017

Choose a reason for hiding this comment

nvzqz Nov 23, 2017

Choose a reason for hiding this comment

sfackler Nov 29, 2017

Choose a reason for hiding this comment

kennytm left a comment

Choose a reason for hiding this comment

kennytm Nov 14, 2017

Choose a reason for hiding this comment

nvzqz Nov 15, 2017

Choose a reason for hiding this comment

kennytm commented Nov 22, 2017

kennytm commented Nov 23, 2017

carols10cents commented Nov 27, 2017

kennytm commented Nov 27, 2017

kennytm commented Nov 29, 2017

ollie27 commented Nov 29, 2017

kennytm commented Dec 6, 2017

sfackler commented Dec 6, 2017 • edited Loading

kennytm commented Dec 20, 2017

dtolnay left a comment

Choose a reason for hiding this comment

nvzqz commented Sep 29, 2017 •

edited

Loading

nvzqz commented Sep 29, 2017 •

edited

Loading

sfackler commented Dec 6, 2017 •

edited

Loading