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.

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

Impl TryFrom<Vec<u8>> for String #132268

Merged
merged 1 commit into from
Feb 20, 2025
Merged

Conversation

elichai
Copy link
Contributor

@elichai elichai commented Oct 28, 2024

I think this is useful enough to have :)
As a general question, is there any policy around adding "missing" trait implementations? (like adding AsRef<T> for T for std types), I mostly stumble upon them when using a lot of "impl Trait in argument position" like (foo: impl Into<String>)

@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2024

r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 28, 2024
@clubby789 clubby789 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 28, 2024
@workingjubilee
Copy link
Member

r? lips-api

@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2024

Failed to set assignee to lips-api: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@workingjubilee
Copy link
Member

...
r? libs-api

@rustbot rustbot assigned Amanieu and unassigned thomcc Oct 28, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Oct 28, 2024

As a general question, is there any policy around adding "missing" trait implementations? (like adding AsRef for T for std types), I mostly stumble upon them when using a lot of "impl Trait in argument position" like (foo: impl Into)

In general, libs-api tends to be wary of implementations for From and AsRef because they can, due to nuances of the type system, significantly weaken type inference in ways that can have significant ecosystem-affecting impacts.

TryFrom may prove slightly less of a concern because its type inference is "naturally weaker"... it implicitly includes all valid implementations of From, after all, as well as TryFrom. So as every implementation tends to have more possibilities, any existing call-sites have to infer against a much wider spread of possibilities, and it is thus somewhat less likely that any individual trait added is a problem.

Usually libs-api has been happy to add implementations of obviously-missing traits in general, though, especially on traits which aren't as... open-ended. Things like missing Iterator extension traits on iterators, for example. And the impact of a new implementation can always be evaluated via crater runs, though those require careful reading.

@elichai
Copy link
Contributor Author

elichai commented Oct 29, 2024

@workingjubilee Hmm I see, I'll say that the "open-ended" ones are the ones I miss the most, for example I've been using Borrow instead of AsRef a lot because primitives don't implement any useful AsRef (i.e AsRef<u64> for u64)

@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 25, 2024
@Amanieu
Copy link
Member

Amanieu commented Nov 26, 2024

This makes sense since we already have impl From<String> for Vec<u8>.

@rfcbot fcp merge

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 26, 2024
@rfcbot
Copy link

rfcbot commented Nov 26, 2024

Team member @Amanieu 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. labels Nov 26, 2024
@Amanieu
Copy link
Member

Amanieu commented Nov 26, 2024

This does need a crater run though.

@bors try

@bors
Copy link
Contributor

bors commented Nov 26, 2024

⌛ Trying commit 0cd8694 with merge 88e773b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
Impl TryFrom<Vec<u8>> for String

I think this is useful enough to have :)
As a general question, is there any policy around adding "missing" trait implementations? (like adding `AsRef<T> for T` for std types), I mostly stumble upon them when using a lot of "impl Trait in argument position" like (`foo: impl Into<String>`)
@bors
Copy link
Contributor

bors commented Nov 26, 2024

☀️ Try build successful - checks-actions
Build commit: 88e773b (88e773be5b0a68392bd25d47edd909e9b2d64a6a)

@Amanieu
Copy link
Member

Amanieu commented Nov 28, 2024

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-132268 created and queued.
🤖 Automatically detected try build 88e773b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2024
@compiler-errors
Copy link
Member

@craterbot cancel

Crater is currently bugging out and running at 155% completed on another job. I'm gonna clear the queue and re-queue all the jobs in order.

@craterbot
Copy link
Collaborator

🚧 Experiment pr-132268 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 25, 2024
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 7, 2025
@rfcbot
Copy link

rfcbot commented Jan 7, 2025

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

@scottmcm
Copy link
Member

scottmcm commented Jan 9, 2025

This makes sense since we already have impl From<String> for Vec<u8>.

It's not obvious to me that this follows in this case, since once could expect TryFrom<Vec<u8>> to be ascii-checked or something as well.

Can you say when you'd encourage people to use this over String::from_utf8?

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. 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 Jan 17, 2025
@rfcbot
Copy link

rfcbot commented Jan 17, 2025

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.

@teor2345
Copy link
Contributor

Can you say when you'd encourage people to use this over String::from_utf8?

If you depend on code that already requires a generic TryFrom bound, this impl avoids having to write a wrapper type, and then manage bounds or conversions for that wrapper. (Which can unfortunately be infectious across a whole codebase, if you have a lot of dependent generics.)

I can’t come up with a simple example where you have to have a TryFrom impl, but I’ve run into it before in generic-heavy code like tokio Services, and other generic component architectures.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 13, 2025
@Amanieu
Copy link
Member

Amanieu commented Feb 18, 2025

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2025

📌 Commit 0cd8694 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2025
…nieu

Impl TryFrom<Vec<u8>> for String

I think this is useful enough to have :)
As a general question, is there any policy around adding "missing" trait implementations? (like adding `AsRef<T> for T` for std types), I mostly stumble upon them when using a lot of "impl Trait in argument position" like (`foo: impl Into<String>`)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2025
…nieu

Impl TryFrom<Vec<u8>> for String

I think this is useful enough to have :)
As a general question, is there any policy around adding "missing" trait implementations? (like adding `AsRef<T> for T` for std types), I mostly stumble upon them when using a lot of "impl Trait in argument position" like (`foo: impl Into<String>`)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#127793 (Added project-specific Zed IDE settings)
 - rust-lang#132268 (Impl TryFrom<Vec<u8>> for String)
 - rust-lang#134995 (Stabilize const_slice_flatten)
 - rust-lang#136985 (Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited))
 - rust-lang#137155 (Organize `OsString`/`OsStr` shims)
 - rust-lang#137227 (docs(dev): Update the feature-gate instructions)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2025
…nieu

Impl TryFrom<Vec<u8>> for String

I think this is useful enough to have :)
As a general question, is there any policy around adding "missing" trait implementations? (like adding `AsRef<T> for T` for std types), I mostly stumble upon them when using a lot of "impl Trait in argument position" like (`foo: impl Into<String>`)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2025
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#120580 (Add `MAX_LEN_UTF8` and `MAX_LEN_UTF16` Constants)
 - rust-lang#132268 (Impl TryFrom<Vec<u8>> for String)
 - rust-lang#136093 (Match Ergonomics 2024: update old-edition behavior of feature gates)
 - rust-lang#136344 (Suggest replacing `.` with `::` in more error diagnostics.)
 - rust-lang#136690 (Use more explicit and reliable ptr select in sort impls)
 - rust-lang#136815 (CI: Stop /msys64/bin from being prepended to PATH in msys2 shell)
 - rust-lang#136923 (Lint `#[must_use]` attributes applied to methods in trait impls)
 - rust-lang#137155 (Organize `OsString`/`OsStr` shims)
 - rust-lang#137225 (vectorcall ABI: require SSE2)

Failed merges:

 - rust-lang#136780 (std: move stdio to `sys`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#120580 (Add `MAX_LEN_UTF8` and `MAX_LEN_UTF16` Constants)
 - rust-lang#132268 (Impl TryFrom<Vec<u8>> for String)
 - rust-lang#136093 (Match Ergonomics 2024: update old-edition behavior of feature gates)
 - rust-lang#136344 (Suggest replacing `.` with `::` in more error diagnostics.)
 - rust-lang#136690 (Use more explicit and reliable ptr select in sort impls)
 - rust-lang#136815 (CI: Stop /msys64/bin from being prepended to PATH in msys2 shell)
 - rust-lang#136923 (Lint `#[must_use]` attributes applied to methods in trait impls)
 - rust-lang#137155 (Organize `OsString`/`OsStr` shims)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7b7b1d4 into rust-lang:master Feb 20, 2025
7 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 20, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2025
Rollup merge of rust-lang#132268 - elichai:string_try_from_vec, r=Amanieu

Impl TryFrom<Vec<u8>> for String

I think this is useful enough to have :)
As a general question, is there any policy around adding "missing" trait implementations? (like adding `AsRef<T> for T` for std types), I mostly stumble upon them when using a lot of "impl Trait in argument position" like (`foo: impl Into<String>`)
@elichai elichai deleted the string_try_from_vec branch February 20, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.