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

Implement From for FromBytesWithNulError #96104

Closed
wants to merge 3 commits into from

Conversation

SUPERCILEX
Copy link
Contributor

Needed for #93668 and generally useful.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 16, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 16, 2022
@SUPERCILEX
Copy link
Contributor Author

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

@rustbot rustbot 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 Apr 16, 2022
@SUPERCILEX
Copy link
Contributor Author

r? @joshtriplett

@rust-log-analyzer

This comment has been minimized.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@joshtriplett
Copy link
Member

Trait impls can't be marked unstable; I posted a suggestion to address that. With that applied, r=me.

Co-authored-by: Josh Triplett <josh@joshtriplett.org>
@SUPERCILEX
Copy link
Contributor Author

Sounds good, done!

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 19, 2022

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

Concerns:

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 Apr 19, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Apr 19, 2022

@rfcbot concern Which ErrorKind?

I'm not sure if InvalidInput is always the correct error kind for FromBytsWithNulError. I can imagine situations where InvalidData or UnexpectedEof or InvalidFilename would make more sense.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 19, 2022

Also note that io::Error::new(io::ErrorKind::InvalidInput, e) will do two allocations as it effectively allocates a Box<Box<dyn Error>>.

@SUPERCILEX
Copy link
Contributor Author

@rfcbot concern Which ErrorKind?

I'm not sure if InvalidInput is always the correct error kind for FromBytsWithNulError. I can imagine situations where InvalidData or UnexpectedEof or InvalidFilename would make more sense.

Ah, so are you suggesting that From shouldn't be implemented at all and you should instead always choose the ErrorKind and message? If so that makes sense and I'll close this PR.

@dtolnay
Copy link
Member

dtolnay commented Apr 20, 2022

Independent of the question of which ErrorKind is correct, I think I would prefer not to add this impl. From what I've seen in serde, folks looking for impl From<?> for std::io::Error are most often doing it because they are cutting corners by using std::io::Error as an error type in code that has at most a little to do with i/o. If they value that concise ability to use ?, they should instead use an error type that has a generic impl<E: Error> From<E> instead of needing each implementor of std::error::Error to provide its own individual conversion to std::io::Error.

@SUPERCILEX
Copy link
Contributor Author

Lol, that's literally what I'm trying to do. :) (Though in my case it's justified because I do have to return io::Error.) Anyway, I think this is enough feedback in the negative direction to close this PR.

@SUPERCILEX SUPERCILEX closed this Apr 20, 2022
@rfcbot rfcbot removed 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 Apr 20, 2022
@SUPERCILEX SUPERCILEX deleted the nul-err-from-impl branch October 16, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

8 participants