Tracking issue for CString, OsString, PathBuf extra methods

[clarfon]

This includes the methods:

• CString::into_boxed_c_str
• OsString::into_boxed_os_str
• PathBuf::into_boxed_path
• <Box<CStr>>::into_c_string
• <Box<OsStr>>::into_os_string
• <Box<Path>>::into_path_buf
• CString::as_c_str

The into_boxed_* methods coming from #39594, the other into_* methods from #40009, and the as_c_str method from #41095.

The features for the methods are into_boxed_c_str, into_boxed_os_str, into_boxed_path, and as_c_str.

[clarfon]

ping @brson / @alexcrichton to tag this, also ping @jonhoo because you were specifically interested in stabilising these for something for serde.

[alexcrichton]

Oh it looks like in #39594 we forgot to update the tracking issue. @clarcharr could you update those tracking issues to point here?

[clarfon]

I included that as part of the second PR, so, already covered. If for some reason that PR isn't accepted, I'll make a separate PR for just updating the tracking issues.

[nagisa]

The into_boxed_* methods do not have associated tracking issue set.

Also, I really dislike the _ between c/os and str in the method name. I mistyped the method about three times in a row.

[clarfon]

Currently this is to be consistent with methods that already exist, like as_os_str. We could rename them and deprecate the methods with the old naming scheme, though.

[clarfon]

Also I'm confused on what you mean about them lacking a tracking issue; all of them link to here.

[nagisa]

Also I'm confused on what you mean about them lacking a tracking issue; all of them link to here.

There’s no tracking issue linked in the stable docs, which is why I was confused.

[clarfon]

I initially forgot to add a tracking issue before the cut, so, they're probably on the beta docs but not stable. I thought that I had gotten it in before the changes were merged but I guess not.

[clarfon]

Is there a way we could get this on track for stabilisation? This is necessary to make some serde impls safe on stable.

(I am not sure how to tag the libs team in general)

[alexcrichton]

Sure yeah, let's see what others think:

@rfcbot fcp merge

[rfcbot]

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

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @dtolnay
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, 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.

[dtolnay]

So far I have only looked at CString::as_c_str, will look at the other ones next.

It looks like the implementation could be simpler:

pub fn as_c_str(&self) -> &CStr {
    self
}

This is what String::as_str does.

Also I noticed there is String::as_mut_str but not CString::as_mut_c_str. Any reason or are we just punting that to a future change?

What are the reasons that people use String::as_str in practice? Do we expect all of those to apply to CString::as_c_str? Are there any additional reasons someone might need as_c_str?

[clarfon]

I personally use it a lot via map, e.g. iterator.map(String::as_str). It also helps with type inference because as_ref is ambiguous and as_str is not.

There's no as_mut_c_str currently because there are no methods for &mut CStr and currently no way to construct one outside transmuting one out of thin air. I think that this should be saved for the future when we actually have a need for such a thing, if ever.

[dtolnay]

This was in the insta-stable part of #40009 but this impl has a stray lifetime that should be removed. EDIT: also this impl.

The attribute on that impl says since = "1.17.0" but from what I can tell it was not in 1.17.0. Where in the process do those get updated to the right version? Is that something we need to track in this issue?

I'm sure this has been discussed but there is no way to do any of the Into impls in #40009 as From impls instead, right? It would be great to get a comment on each Into impl with a brief explanation or a link to an explanation or thread. That way future maintainers aren't confused. I'm sure there is a reason but it isn't obvious.

Changing them to From impls in the future if it becomes possible is backward compatible right?

These are the very first Into impls we have added to std (other than the blanket impl involving From). In the past, somebody could look at the list of implementations and understand: oh, I'm probably not supposed to implement this. Now we have a nice long list of implementations and one less way to convince people "do as we say, not as we do." Are there any ways that we could cancel out this effect or is it not worth worrying about? In particular, I would prefer to explain this in the API guidelines before the Into impls become stable. rust-lang-nursery/api-guidelines#30

[clarfon]

@dtolnay First, thank you for looking into this so much!

So, sort-of in order:

1. I filed #41960 because we should be warning on unused lifetimes. I made a PR to remove them: #42127.
2. I'm not actually sure. Usually there's a tag added to PRs to indicate that they need to be added to release notes, but no one actually modified this tag. I can modify it to say 1.18.0 and then request a beta backport.
3. I made a PR (#43126) that modifies the docs for Into to reflect this. There are issues floating around about softening the rules so that we can turn these into Froms, but I can't seem to find them.
4. They're not actually the first Into impls; there used to be three others until actually I actually mentioned in #37561 that they should be changed, and then they were removed. :)

[rfcbot]

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

[rfcbot]

The final comment period is now complete.