Skip to content

Conversation

acrrd
Copy link
Contributor

@acrrd acrrd commented Aug 29, 2024

I have a use case where I convert to and from primitive, and I need to involve NonZero* types as well.
Currently, I have a function that uses these traits for primitive types, and I have additional functions for each NonZero* that I need. It would be nice to get rid of them and have only one.

I can create on PR per trait implementation if it's preferred. This PR has one commit per trait implementation.

@acrrd acrrd force-pushed the support_nonzero branch 4 times, most recently from 03bb963 to eee8ac0 Compare August 30, 2024 11:11
@foolishell
Copy link

@acrrd
Thank you so much for this pull request. If possible, could you also add support for AsPrimitive?

@tarcieri
Copy link
Contributor

Curious about https://doc.rust-lang.org/stable/core/num/struct.NonZero.html as well (might need an MSRV-preserving feature-gate)

@acrrd
Copy link
Contributor Author

acrrd commented Sep 17, 2024

@acrrd Thank you so much for this pull request. If possible, could you also add support for AsPrimitive?

done

@acrrd
Copy link
Contributor Author

acrrd commented Sep 17, 2024

Curious about https://doc.rust-lang.org/stable/core/num/struct.NonZero.html as well (might need an MSRV-preserving feature-gate)

This needs nightly, NonZero is only usable with types that implement ZeroablePrimitive. This trait is usable only in nightly.
When stabilized it will reduce the needed code to implement NonZero, but now is not usable.

@tarcieri
Copy link
Contributor

@acrrd though the ZeroablePrimitive trait itself is unstable, there are impls for it for the core integer types already which are usable on stable Rust:

https://doc.rust-lang.org/stable/core/num/trait.ZeroablePrimitive.html#implementors

Quite a bit of the functionality is already stable, as is noted in the feature gates in the rustdoc. Here's a small example:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9c4126921b16b781588732fd26d050a4

@cuviper
Copy link
Member

cuviper commented Sep 17, 2024

The old types are just aliases now, literally type NonZeroI32 = NonZero<i32>; etc. So implementing traits here on those aliases has exactly the same effect as implementing NonZero<T> for specific T.

@cuviper
Copy link
Member

cuviper commented Sep 17, 2024

I'm torn on AsPrimitive -- right now that is only implement for types where you could literally write x as T in non-generic settings. It's not sealed though, and the docs do mention how newtypes should behave too.

So if we add that, shouldn't we also do it for Wrapping? (and Saturating, if we get to that at all)

@acrrd
Copy link
Contributor Author

acrrd commented Sep 18, 2024

I'm torn on AsPrimitive -- right now that is only implement for types where you could literally write x as T in non-generic settings. It's not sealed though, and the docs do mention how newtypes should behave too.

It was requested in #335, they have a use case for it. Let me know if you want to keep it.

So if we add that, shouldn't we also do it for Wrapping? (and Saturating, if we get to that at all)

We can't. The trait requires Add,Sub, etc., which are not implemented for NonZero*. This is related to #274.

@cuviper
Copy link
Member

cuviper commented Sep 18, 2024

I'm torn on AsPrimitive -- [...]

It was requested in #335, they have a use case for it. Let me know if you want to keep it.

I'm going to respond there, because I think that use is also problematic, even aside from NonZero. Let's drop it here for now.

So if we add that, shouldn't we also do it for Wrapping? (and Saturating, if we get to that at all)

We can't. The trait requires Add,Sub, etc., which are not implemented for NonZero*. This is related to #274.

I wasn't talking about any other traits, only impl<T, U> AsPrimitive<T> for core::num::Wrapping<U>. But that deserves a separate PR, if we even decide that's worthwhile.

@acrrd
Copy link
Contributor Author

acrrd commented Sep 18, 2024

I'm going to respond there, because I think that use is also problematic, even aside from NonZero. Let's drop it here for now.

done

I wasn't talking about any other traits, only impl<T, U> AsPrimitive<T> for core::num::Wrapping<U>. But that deserves a separate PR, if we even decide that's worthwhile.

I see it now, sorry

@marshrayms
Copy link

marshrayms commented Sep 26, 2024

To me, a big part of the value of add-on crates like the rust-num collection is that they can help make up where the base language falls short.

I'm torn on AsPrimitive -- right now that is only implement for types where you could literally write x as T in non-generic settings. It's not sealed though, and the docs do mention how newtypes should behave too.

For example, the concept you mentioned of "types where you could literally write x as T" is not expressible using any combination of traits from the standard library.

So if we add that, shouldn't we also do it for Wrapping? (and Saturating, if we get to that at all)

I think the difference is that NonZero* is a newtype which expresses an inherent property of the value itself. They are useful as type system constraints.

Whereas Wrapping and Saturating express nothing about the set of values they represent, they are simply used to select different sets of common functions via operator overloading. Purely syntactic convenience.

@cuviper
Copy link
Member

cuviper commented Sep 26, 2024

To me, a big part of the value of add-on crates like the rust-num collection is that they can help make up where the base language falls short.

I'm torn on AsPrimitive -- right now that is only implement for types where you could literally write x as T in non-generic settings. It's not sealed though, and the docs do mention how newtypes should behave too.

For example, the concept you mentioned of "types where you could literally write x as T" is not expressible using any combination of traits from the standard library.

This seems consistent to me -- AsPrimitive is currently filling that gap in the base language by making it expressible as a trait, though you only get .as_() instead of a real as in that generic code. We can choose to extend the domain of that trait as well, and maybe we should, but I think we should do that separately.

Anyway, let me give this PR an actual review...

@acrrd
Copy link
Contributor Author

acrrd commented Jan 20, 2025

@cuviper Do you think this is ok after the changes?

@cuviper
Copy link
Member

cuviper commented Jun 17, 2025

Thanks! And sorry for the slow review.

@cuviper cuviper added this pull request to the merge queue Jun 17, 2025
Merged via the queue into rust-num:master with commit e8c1926 Jun 17, 2025
4 checks passed
@acrrd
Copy link
Contributor Author

acrrd commented Aug 1, 2025

@cuviper do you plan to do a release soon with this? Is there any blocker I could help with?

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

Successfully merging this pull request may close these issues.

5 participants