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

prefer From/TryFrom to Into/TryInto #1254

Open
rnbguy opened this issue Jun 10, 2024 · 2 comments
Open

prefer From/TryFrom to Into/TryInto #1254

rnbguy opened this issue Jun 10, 2024 · 2 comments
Assignees
Labels
O: code-hygiene Objective: aims to improve code hygiene rust Pull requests that update Rust code

Comments

@rnbguy
Copy link
Collaborator

rnbguy commented Jun 10, 2024

Feature Summary

Currently, we have some Into/TryInto trait bounds which can be rewritten in From/TryFrom.

<C::ClientState as TryFrom<Any>>::Error: Into<ClientError>,
<C::ConsensusState as TryFrom<Any>>::Error: Into<ClientError>,

From is preferred over Into, as A: From<B> implies B: Into<A>, but not the other way around.

Proposal

- B: Into<A>
+ A: From<B>

- D: TryInto<C>
+ C: TryFrom<D>

Note that, Into in super traits may require Sized.

- trait Foo: Into<Bar> { }
+ trait Foo: Sized where Bar: From<Self> { }
@rnbguy rnbguy added this to ibc-rs Jun 10, 2024
@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Jun 10, 2024
@seanchen1991 seanchen1991 self-assigned this Jun 13, 2024
@seanchen1991
Copy link
Contributor

In the code you linked, are you thinking of strengthening the constraints, i.e., re-writing the constraints that are currently written with Into to instead use From?

- <C::ClientState as TryFrom<Any>>::Error: Into<ClientError>,
+ <C::ClientState as TryFrom<Any>>::Error: From<ClientError>,
- <C::ConsensusState as TryFrom<Any>>::Error: From<ClientError>,
+ <C::ConsensusState as TryFrom<Any>>::Error: From<ClientError>,

If that's not what you mean, I'm not actually sure how to re-write these lines to use From, as we can't replace them with

ClientError: From<<C::ClientState as TryFrom<Any>>::Error>,
ClientError: From<<C::ConsensusState as TryFrom<Any>>::Error>,

@rnbguy
Copy link
Collaborator Author

rnbguy commented Jun 14, 2024

are you thinking of strengthening the constraints, i.e., re-writing the constraints that are currently written with Into to instead use From?

I want to relax them. Rewriting the (Try)Into bounds with (Try)From.

ClientError: From<<C::ClientState as TryFrom>::Error>,

I believe this is alright - at least on latest Rust. (Check my comment at rollkit-ibc and the commit.) I guess, I have to check with the MSRV.

@rnbguy rnbguy added O: code-hygiene Objective: aims to improve code hygiene rust Pull requests that update Rust code labels Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: code-hygiene Objective: aims to improve code hygiene rust Pull requests that update Rust code
Projects
Status: 📥 To Do
Development

No branches or pull requests

2 participants