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

The From<T> trait should not assume a valid into implementation can be provided #3637

Open
4 tasks
mitchmindtree opened this issue Dec 19, 2022 · 1 comment
Open
4 tasks
Labels
blocked lib: std Standard library

Comments

@mitchmindtree
Copy link
Contributor

mitchmindtree commented Dec 19, 2022

Currently, our From trait looks like this:

pub trait From<T> {
    fn from(b: T) -> Self;
    fn into(self) -> T;
}

This has the problem that it assumes both directions of conversion are infallible, which is not always the case!

We should consider following Rust's conventions for From and TryFrom here.

Ideally, From implementations should always be lossless (never lose information) and should always be infallible (can never revert). Fallible implementations should use a TryFrom alternative that returns a Result so that the user decide whether they want to revert on Err or handle it in some other way.

We can see this convention in Rust's std lib for the primitive types, e.g. it has impl From<u16> for u32, but does not have impl From<u32> for u16. Instead, it offers impl TryFrom<u32> for u16 which checks if the value is out of range.

TODO

  • Remove the into method from the From<T> trait.
  • Add a TryFrom<T> trait to support fallible conversions. Note that this requires associated types to ergonomically infer Self::Error type from implementations. Add associated items to impls and traits. #610. In the meantime, we could unblock ourselves from this by returning an Option rather than a Result and use None to imply conversion failure, and then switch to Result in the future once associated types land?
  • Review From implementations in std to ensure all are infallible (i.e. don't revert). Replace fallible implementations with TryFrom and produce an error instead.
  • Omit Into<T> until we support blanket implementations i.e. Implement trait Into<T> in lib-std #3399.
@mitchmindtree mitchmindtree added lib: std Standard library blocked labels Dec 19, 2022
@mohammadfawaz
Copy link
Contributor

mohammadfawaz commented Dec 20, 2022

That's fair. I suppose omitting Into for now altogether (until we're able to implement it properly) is better than having the bad implementation that we have today.

@nfurfaro nfurfaro self-assigned this Jan 5, 2023
bitzoic added a commit to FuelLabs/sway-libs that referenced this issue Apr 20, 2023
## Type of change

<!--Delete points that do not apply-->

- New feature

## Changes

The following changes have been made:

- Added `StorageString` type
- Added `from_raw_slice()` and `as_raw_slice()` to the `String` type
- Updated src README wording, links, and added `StorageString`
- Moved `String` into `sway-libs/libs/strings` folder with the
`StorageString` type
- Updated `String` README and SPECIFICATION to remove outdated info
- Updated `String` to use non-mutable types in `from_utf8()`

## Notes

- The `From<raw_slice>` implementation for the `String` type is
commented out until FuelLabs/sway#3637 is
resolved. The `from_raw_slice()` shall be removed when this is
reimplemented
- Tests for the `from_raw_slice()` and `as_raw_slice()` functions have
been commented out until FuelLabs/sway#4408 is
resolved
- Until FuelLabs/fuels-rs#940 is resolved,
developers must return any `StorageString`s from storage as a `Bytes`
type
- This should unblock NFT URIs, Fuel Name Service, token standards, and
more

## Related Issues

<!--Delete everything after the "#" symbol and replace it with a number.
No spaces between hash and number-->

Closes #40

---------

Co-authored-by: bitzoic <cameron.carstens@fuel.sh>
@nfurfaro nfurfaro removed their assignment May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked lib: std Standard library
Projects
None yet
Development

No branches or pull requests

3 participants