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

Consider implementing From<T> where T: Into<#inner_type>? #124

Closed
schneiderfelipe opened this issue Feb 4, 2024 · 12 comments
Closed

Consider implementing From<T> where T: Into<#inner_type>? #124

schneiderfelipe opened this issue Feb 4, 2024 · 12 comments

Comments

@schneiderfelipe
Copy link

Currently derive(From) produces

pub fn gen_impl_trait_from(type_name: &TypeName, inner_type: impl ToTokens) -> TokenStream {
quote! {
impl ::core::convert::From<#inner_type> for #type_name {
#[inline]
fn from(raw_value: #inner_type) -> Self {
Self::new(raw_value)
}
}
}
}

Is there anything holding back the production of something like the following?

quote! {
    impl<T: Into<#inner_type>> ::core::convert::From<T> for #type_name {
        #[inline]
        fn from(raw_value: T) -> Self {
            Self::new(raw_value.into())
        }
    }
}
@greyblake
Copy link
Owner

@schneiderfelipe Hi, thanks for bringing this.

I guess I had a thought about it, the reason I've decided not to implement it is clarity / explicitness.
But you may make me reconsider this.

What's you real life use case, where you think it would make things a bit easier?

@schneiderfelipe
Copy link
Author

Well, it would make newtypes behave more like the inner type. For instance, something like the following would work, but currently doesn't:

#[newtype(derive(From))]
struct MyVec(Vec<isize8>);

// This works
let my_vec: Vec<isize8> = [0, 1, 2].into();

// This does not work, requires a separate From impl
let my_vec: MyVec = [0, 1, 2].into();

@greyblake
Copy link
Owner

@schneiderfelipe Sold! :)

@danma3x Would you be interested in addressing this one?

@schneiderfelipe
Copy link
Author

Sure!

schneiderfelipe added a commit to schneiderfelipe/nutype that referenced this issue Feb 14, 2024
@schneiderfelipe
Copy link
Author

@schneiderfelipe Sold! :)

@danma3x Would you be interested in addressing this one?

There's a problem with my proposed change: it produces conflicting implementations of the trait From, e.g.,

error[E0119]: conflicting implementations of trait `From<_>` for type `derives::test_trait_from_string::__nutype_Name__::Name`
   --> test_suite/tests/string.rs:357:9
    |
357 |         #[nutype(derive(From))]
    |         ^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         first implementation here
    |         conflicting implementation for `derives::test_trait_from_string::__nutype_Name__::Name`
    |
    = note: this error originates in the attribute macro `nutype` (in Nightly builds, run with -Z macro-backtrace for more info)

In particular, for the case of integers, things seem much worse:

error[E0277]: the trait bound `u32: From<i32>` is not satisfied
   --> test_suite/tests/integer.rs:532:22
    |
532 |         let amount = Amount::from(350);
    |                      ^^^^^^ the trait `From<i32>` is not implemented for `u32`
    |
    = help: the following other types implement trait `From<T>`:
              <u32 as From<bool>>
              <u32 as From<char>>
              <u32 as From<u8>>
              <u32 as From<u16>>
              <u32 as From<NonZeroU32>>
              <u32 as From<Ipv4Addr>>
    = note: required for `i32` to implement `Into<u32>`
note: required for `traits::test_trait_from::__nutype_Amount__::Amount` to implement `From<i32>`
   --> test_suite/tests/integer.rs:529:9
    |
529 |         #[nutype(derive(From))]
    |         ^^^^^^^^^^^^^^^^^^^^^^^ unsatisfied trait bound introduced here
530 |         pub struct Amount(u32);
    |                    ^^^^^^
    = note: this error originates in the attribute macro `nutype` (in Nightly builds, run with -Z macro-backtrace for more info)

So the compiler seems to figure out that 350 should be a u32 just fine if there is only a single From implementation, but "falls back" to i32 and relies on a separate From<i32> implementation otherwise, which should not exist obviously.

I'm afraid my proposal can't be done 😞 @greyblake what do you think?

@greyblake
Copy link
Owner

this one indeed looks bad:

error[E0119]: conflicting implementations of trait `From<_>`

And this one is kind of expected:

error[E0277]: the trait bound `u32: From<i32>` is not satisfied

Because the type for new is generic now and Rust cannot derive type of 350 and in such situations it assumes usually i32, but i32 cannot be converted into u32. That can be gone if type is explicitly specified, for example 350u32.

Could point to you changes in a branch?

@schneiderfelipe
Copy link
Author

The changes I made are in the master branch of my fork schneiderfelipe@cca7047

@asasine
Copy link

asasine commented Feb 22, 2024

Considering nutypes with validation may be fallible, and From shouldn't support fallible conversions, shouldn't the trait implementation be TryFrom, not From?

@schneiderfelipe
Copy link
Author

@asasine I think you're correct. If this ever gets implemented, one would consider extending TryFrom as well (and only that in fallible cases as you mentioned). @greyblake thoughts?

@greyblake
Copy link
Owner

Considering nutypes with validation may be fallible, and From shouldn't support fallible conversions, shouldn't the trait implementation be TryFrom, not From?

Conversions are fallible when there is validation. When there is no validation set, conversions are infallible.

If there is validation, nutype won't allow you to derive From.
Though you can still derive TryFrom if there is no validation.

@greyblake
Copy link
Owner

Ok I gave a try for a generic From implementation, but it's not gonna work the way we want.

I you want to reproduce the example, I pushed my changes in generic-from-impl-demo, see also d73ddeb commit.

Consider the following example:

#[nutype(derive(Into, From))]
pub struct Amount(i32);

This won't compile:

error[E0119]: conflicting implementations of trait `From<Amount>` for type `Amount`

The expands into the following:

    impl ::core::convert::From<Amount> for i32 {
        #[inline]
        fn from(value: Amount) -> Self {
            value.into_inner()
        }
    }

    impl<T> ::core::convert::From<T> for Amount
    where
        i32: ::core::convert::From<T>,
    {
        #[inline]
        fn from(raw_value: T) -> Self {
            let inner_value = i32::from(raw_value);
            Self::new(inner_value)
        }
    }

Note, that Into<i32> for Amount is actually defined as From<Amount> for i32, to keep the symmetry. Into<i32> for Amount will got implemented automatically through blanket implementation. This is common practice / idiom of the Rust language.

But in this case it becomes also a source of a problem.

It's not very obvious to see, but Rust finds 2 conflicting implementation for for From<Amount> for Amount.
The std has reasonable implementation of impl<T> From<T> for T, meaning converting from type T to T should just return self.

Other path Rust could take is: Amount -> i32 ->Amount, because our generic implementation of From enable this.

This could be addressed by implementing Into trait as solely Into<i32> from Amount,
meaning we lose From<Amount> for i32 implementation.

Between these 2 available options, there is no clear winner, a trade off needs to be make.
In this regard, my personal preference would be the current status quo.

With that I am closing the issue.

Thanks for you attention, if you followed me!

@schneiderfelipe
Copy link
Author

@greyblake nicely explained! I'm afraid this won't ever be possible, except maybe if specialisation gets stabilised (rust-lang/rust#31844). Thank you for taking the time though, really appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants