Return Option instead of panicking inside new

[Lakelezz]

Hello!

I was looking at the crate and read that new will panic if the value cannot be represented by the type.
Understandably, uX's types require the new-function to be constructed as they are not integer primitives thus u31 cannot utilise the same construction as e.g. u32.

Nonetheless, when we look at e.g. NonZeroU32's new (and the other NonZero-types) - which is a custom integer too - it returns Option instead of panicking when the conditions are not met. This might an interesting guideline to follow.

Hence my question: Might changing uX's new-functions to return Option be of interest?
Mirroring new_unchecked could be feasible too but the std does not run assertions in them.

If wanted, I would be willing to perform this change : )

Thanks for your time!

[kjetilkjeka]

Hi,

Thanks for pointing this out @Lakelezz

I fully agree that doing like NonZeroU32 and returning an Option<_> on construction is better. If the current behavior is required it would be as simple as unwrapping the value. The drawback is that this would be a breaking change, this might be acceptable as 1.0 is not reached.

My original plan was to use TryFrom for this type of construction. It would return a Result instead of an Option but I don't think that matters much. My idea was that all construction would be done as uX::try_from(24u32).unwrap() and new would be deprecated in 0.1 and removed in 1.0. I think as long as it is documentet that the intended way to construct these integers is with TryFrom, having new() -> Option<Self> as well would be redundant.

Even though the stabilization of TryFrom is taking much longer than I had expected. I don't think there is enough gain in changing new when it requires breaking compatbility. Let me know if there are some aspects I've overlooked when making this decission.

It would be acceptable to have new_unchecked behind a nightly only unstable feature gate to allow for const construction. The point of this would be that users of the library required such feature. @phil-opp requested this at some point, I'm uncertain what the current status is.

[phil-opp]

TryFrom is on the verge to stabilization. It is already included in the current beta, which will become Rust 1.43 on the 11th April.

[kloenk]

TryFrom got implemented in this crate, so that it is usable, just not discoverable on the first look into the documentation. For me I think having new return an option like NonZero is nicer to read. but as said would create an api change.
I would also argue that having a new_unchecked (marked as unsafe) is reasonable if the check is done beforehand by the caller.

[kloenk]

I would argue having an assert in new, also makes it harder for non_std, as a panic handler can be tripped. Therefor a panic handler has to be written

[chrysn]

I'm not sure there is trouble with panicking on no_std -- it's almost impossible to not have any panickable situations in a nontrivial program, but the panic handler can be a plain abort.

Either way, with #60 on the way and TryInto available, what's left for this issue? Would it suffice to point to TryInto as a fallible alternative in the documentation of ::new()?

[kloenk]

I still would say having new return an option makes it nicer to write, as TryInto can get a bit annoying with type annotations. But that is mainly a personal preference.

[liarokapisv]

I also agree that using Option on new is a better default in general.