Skip to content

Implement TryFrom<&OsStr> for &str #99031

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.

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

Closed
aticu opened this issue Jun 17, 2022 · 14 comments
Closed

Implement TryFrom<&OsStr> for &str #99031

aticu opened this issue Jun 17, 2022 · 14 comments
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` A-str Area: str and String A-Unicode Area: Unicode finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@aticu
Copy link
Contributor

aticu commented Jun 17, 2022

Proposal

Problem statement

Currently it is impossible through use of the standard conversion traits to convert from an &OsStr to a &str.

Motivation, use-cases

Adding an implementation of TryFrom<&OsStr> for &str makes this conversion more discoverable to users who are already familiar with the TryFrom trait. They may even expect such an implementation to exist.
Additionally it allows the use of &OsStr in more generic methods such as those that accept impl TryInto<&str>:

fn maybe_print<'a>(arg: impl TryInto<&'a str>) {
    if let Ok(converted) = arg.try_into() {
        println!("{converted}");
    }
}

Solution sketches

Implement TryFrom<&OsStr> for &str.

Links and related work

Implementation PR: #98202

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@aticu aticu added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 17, 2022
@yaahc
Copy link
Member

yaahc commented Jun 17, 2022

@aticu can you give a more concrete example of a situation where you'd use this?

@aticu
Copy link
Contributor Author

aticu commented Jun 18, 2022

I don't remember the exact details (and I currently don't have access to the code), but as far as I remember I was trying to do some checks on the contents of an &OsStr and if they matched some predicates I wanted to further process them.
Since there is no direct way to look at the contents of an &OsStr, I was fine with just doing these checks for those &OsStr that are also valid &strs.
So naturally I tried os_str.try_into() and was surprised to learn that this implementation does not exist. Then when looking at the implementation I found the to_str method of &OsStr, which also worked in my usecase.

So for my specific situation this impl wouldn't have made anything possible that isn't already possible, but it would have saved me from confusion why the trait isn't implemented (as at least to me it seems like it should be) and it would have saved me some time looking at documentation.
With that reasoning I thought it might also be helpful for others and thus decided to try add that impl.

@thomcc
Copy link
Member

thomcc commented Jun 19, 2022

I think it's reasonable for this conversion to exist -- it's a fallible conversion that exists as an inherent method, it seems inconsistent if we're going to decide that it shouldn't exist via our trait for fallible conversions.

Or am I missing something that makes this case less straightforward than it seems? (perhaps we'd change things in a world where we guarantee the encoding?)

@yaahc
Copy link
Member

yaahc commented Jun 20, 2022

I think it's reasonable for this conversion to exist -- it's a fallible conversion that exists as an inherent method, it seems inconsistent if we're going to decide that it shouldn't exist via our trait for fallible conversions.

Or am I missing something that makes this case less straightforward than it seems? (perhaps we'd change things in a world where we guarantee the encoding?)

I agree and I don't think you're missing anything, I just wanted the concrete use case because I think its often very useful context. I think all of the complexity for this proposal is going to be in figuring out the proper Error type. Also, I just realized this change isn't unstable, it's insta-stable, so it will need an FCP not an ACP.

@yaahc yaahc added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jul 6, 2022
@yaahc
Copy link
Member

yaahc commented Jul 6, 2022

I'm retagging this since it's an insta-stable change that needs an FCP.

Along those lines, I'm not sure if rfcbot is setup to work on this repo, but there's an easy way to find out.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 6, 2022

Team member @yaahc 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
Copy link
Collaborator

rfcbot commented Jul 7, 2022

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

psst @yaahc, I wasn't able to add the final-comment-period label, please do so.

@yaahc yaahc added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Jul 7, 2022
@yaahc yaahc transferred this issue from rust-lang/libs-team Jul 7, 2022
@dwattttt
Copy link

Is this ever able to succeed on Windows? An OsStr there is not utf8, and this is known at compile time. Relegating it to a runtime failure seems a poor result.

@ChrisDenton
Copy link
Member

On Windows we use WTF-8 for OsStr. This is a superset of UTF-8 and in practice will almost always be valid UTF-8.

@ChrisDenton
Copy link
Member

Windows itself uses "WTF-16" (aka UTF-16 with unpaired surrogates) but the standard library translate from/to WTF-8 at the API boundary. We also guarantee that an &str is bit-for-bit compatible with an OsStr (see impl AsRef<OsStr> for &str) so the reverse must also be true for any string that can be represented in UTF-8.

@dwattttt
Copy link

Ah that makes sense. I had no idea an OsStr(ing) needed a copy for win32 (except maybe for Win11)

@aticu
Copy link
Contributor Author

aticu commented Jul 25, 2022

What's the status here? Shouldn't the FCP have already ended?

@yaahc yaahc added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Jul 25, 2022
@yaahc
Copy link
Member

yaahc commented Jul 25, 2022

looks like rfcbot had issues with the issue transfer from the libs-team repo.

@yaahc yaahc 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 Jul 25, 2022
@yaahc
Copy link
Member

yaahc commented Jul 25, 2022

The final comment period, with a disposition to merge, as per the #99031 (comment), is now complete.

As the automated a 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.

hopefully this is an accurate enough recreation of rfcbot's job to not mess up any other processes, 😅

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 8, 2022
@ChrisDenton ChrisDenton added the E-help-wanted Call for participation: Help is requested to fix this issue. label Mar 23, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 14, 2023
…=Amanieu

Implement `TryFrom<&OsStr>` for `&str`

Recently when trying to work with `&OsStr` I was surprised to find this `impl` missing.

Since the `to_str` method already existed the actual implementation is fairly non-controversial, except for maybe the choice of the error type. I chose an opaque error here instead of something like `std::str::Utf8Error`, since that would already make a number of assumption about the underlying implementation of `OsStr`.

As this is a trait implementation, it is insta-stable, if I'm not mistaken?
Either way this will need an FCP.
I chose "1.64.0" as the version, since this is unlikely to land before the beta cut-off.

`@rustbot` modify labels: +T-libs-api

API Change Proposal: rust-lang#99031 (accepted)
@workingjubilee workingjubilee added A-Unicode Area: Unicode A-str Area: str and String A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` labels Jul 22, 2023
@ChrisDenton ChrisDenton removed the E-help-wanted Call for participation: Help is requested to fix this issue. label Jul 22, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Aug 24, 2023
Implement `TryFrom<&OsStr>` for `&str`

Recently when trying to work with `&OsStr` I was surprised to find this `impl` missing.

Since the `to_str` method already existed the actual implementation is fairly non-controversial, except for maybe the choice of the error type. I chose an opaque error here instead of something like `std::str::Utf8Error`, since that would already make a number of assumption about the underlying implementation of `OsStr`.

As this is a trait implementation, it is insta-stable, if I'm not mistaken?
Either way this will need an FCP.
I chose "1.64.0" as the version, since this is unlikely to land before the beta cut-off.

`@rustbot` modify labels: +T-libs-api

API Change Proposal: rust-lang/rust#99031 (accepted)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` A-str Area: str and String A-Unicode Area: Unicode finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants