Implement nonzero arithmetics for NonZero types.

[iago-lito]

Hello'all, this is my first PR to this repo.

Non-zero natural numbers are stable by addition/multiplication/exponentiation, so it makes sense to make this arithmetic possible with NonZeroU*.

The major pitfall is that overflowing underlying u* types possibly lead to underlying 0 values, which break the major invariant of NonZeroU*. To accommodate it, only checked_ and saturating_ operations are implemented.

Other variants allowing wrapped results like wrapping_ or overflowing_ are ruled out de facto.

impl Add<u*> for NonZeroU* { .. } was considered, as it panics on overflow which enforces the invariant, but it does not so in release mode. I considered forcing NonZeroU*::add to panic in release mode by deferring the check to u*::checked_add, but this is less explicit for the user than directly using NonZeroU*::checked_add.
Following @Lokathor's advice on zulip, I have dropped the idea.

@poliorcetics on Discord also suggested implementing _sub operations, but I'd postpone this to another PR if there is a need for it. My opinion is that it could be useful in some cases, but that it makes less sense because non-null natural numbers are not stable by subtraction even in theory, while the overflowing problem is just about technical implementation.

One thing I don't like is that the type of the other arg differs in every implementation: _add methods accept any raw positive integer, _mul methods only accept non-zero values otherwise the invariant is also broken, and _pow only seems to accept u32 for a reason I ignore but that seems consistent throughout std. Maybe there is a better way to harmonize this?

This is it, Iope I haven't forgotten anything and I'll be happy to read your feedback.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[poliorcetics]

Thank you for the PR ! It's very well done, I left some comments, mostly about doc improvements, the code looks good to me.

I'm would instinctively think checked_add would receive a NonZero* type, but that's only my feeling. Using a generic over Into<$Int> is possible here, but that's probably overkill for an addition.

[scottmcm]

Using a generic over Into<$Int> is possible here, but that's probably overkill for an addition.

It'll also allow a whole bunch more things than is probably wanted, like NonZeroU64::checked_add accepting u8 as an argument, and thus .checked_add(1) being in inference failure.

It could potentially use a new pub-trait-in-private-module to accept just NonZeroU64 and u64, but IIRC libs tends to prefer not doing that.

[iago-lito]

I had initially thought about checked_add(self, other: NonZeroU*) indeed, but it turns out that (with say, let twelve = NonZeroU32::new(12).unwrap();)

• calling twelve.checked_add(0) is perfectly fine
• the user would prefer calling twelve.checked_add(twelve) for sure, but calling twelve.checked_add(twelve.get()) is not too much to ask imo.

This said, I agree that it seems weird at first, especially because these three signatures are inconsistent:

NonZeroU8::checked_add(self, other: u8)
NonZeroU8::checked_mul(self, other: NonZeroU8)
NonZeroU8::checked_pow(self, other: u32)

But I think it's easy enough to explain why it is the case.

[kennytm]

r? @dtolnay

[m-ou-se]

Looks like a good addition to try out. Can you open a tracking issue? Thanks!

(You can use the same feature gate for all of them, maybe nonzero_checked_ops or something like that.)

[iago-lito]

(You can use the same feature gate for all of them, maybe nonzero_checked_ops or something like that.)

@m-ou-se Okay. Since you also offer to implement mul and pow for signed nonzeroes, I'll get a little more ambitious then and implement other "ops" like neg, maybe next_power_of_two and other things I find in the std doc for usize.

I have no particular inclusion criterion yet (any idea?), but I'll exclude every operation for which it is possible to get a null result without overflowing the type. This rules out div, rem, sub, etc.

I'll make sure all tests pass and the new "ops" undergo revision again before I open the tracking issue.

[scottmcm]

maybe next_power_of_two

This one's a bit unfortunate, overflowing for about half the values. That's why it's implemented with one_less_than_next_power_of_two, which cannot overflow (but is private, which lets it get away with the gigantic name). I've often wished to find a good name for the core "set all the bits below the highest bit" operation that doesn't need all the checked/overflowing/etc versions.

But I guess that's a different PR's problem. I suppose checked_next_power_of_two does work fine for all valid object sizes anyway.

[scottmcm]

I'll get a little more ambitious then and implement other "ops" like neg

An important logistical note, @iago-lito: you'll probably want to put methods (like saturating_mul) in one PR and traits (like Neg) in a different PR. That's because methods can be marked unstable, and can thus go in with just an r+, but trait implementations are instantly-stable, so need a full FCP.

So impl Neg for NonZeroI32 does seems reasonable to me, but is probably worth putting in a separate PR so it has a nice small FCP.

[iago-lito]

@scottmcm Okay, I think I understand the need for a separate PR.

I have scanned methods of usize and isize and since I'm willing to add a few more methods to nonzero types, here is my attempt to summarize all that up:

method/trait	NonZeroU*	NonZeroI*
Add	🗙	🗙	possibly wraps to zero in release mode
checked_add	✔	🗙	NonZeroI*: possible natural zero result
overflowing_add	🗙	🗙	possibly wraps to zero
saturating_add	✔	🗙	NonZeroI*: possible natural zero result
unchecked_add	✔	🗙	signed case: there are two preconditions to watch: overflow + cancellation. Make it explicit by not implementing the method.
Mul	🗙	🗙	possibly wraps to zero in release mode
checked_mul	✔	✔	checked_mul(self, other: NonZero*)
overflowing_mul	🗙	🗙	possibly wraps to zero
saturating_mul	✔	✔	saturating_mul(self, other: NonZero*)
unchecked_mul	✔	✔	unsafe fn unchecked_mul(self, other: NonZero*) -> NonZero* (only overflow precondition must be met)
wrapping_mul	🗙	🗙	possibly wraps to zero
Pow	🗙	🗙	possibly wraps to zero in release mode
checked_pow	✔	✔	checked_pow(self, other: u32)
overflowing_pow	🗙	🗙	wraps to zero
saturating_pow	✔	✔	saturating_pow(self, other: u32)
Sub	🗙	🗙	possible natural zero result
saturating_sub	🗙	🗙	possible natural zero result
unchecked_sub	🗙	🗙	there are two preconditions to watch: overflow + cancellation. Make it explicit by not implementing the method.
wrapping_sub	🗙	🗙	possibly wraps to zero
Div	🗙	🗙	possible natural zero result
*_div	🗙	🗙	possible natural zero result
Rem	🗙	🗙	possible natural zero result
*_rem	🗙	🗙	possible natural zero result
next_power_of_two	🗙	-	possibly wraps to zero
checked_next_power_of_two	✔	-	only unsigned types qualify
next_power_of_two	🗙	-	possibly wraps to zero
is_power_of_two	🗙	-	does not save a check
Neg	-	TODO	in a separate PR
signum	-	🗙	does not save a check
abs	-	✔	cannot wrap to zero
checked_abs	-	✔	only signed types qualify
overflowing_abs	-	✔	cannot wrap to zero
unsigned_abs	-	✔	returns a value of the corresponding NonZeroU* type
saturating_abs	-	✔	cannot return zero
wrapping_abs	-	✔	cannot wrap to zero
is_positive	-	🗙	does not save a check
is_negative	-	🗙	does not save a check

I have tried to only omit byte-level operations.

All these (if accepted) would be gathered behind the feature gate nonzero_arithmetics (except for Neg).

To clear things up, the advantage of implementing these methods is to save a check and replace the unsafe and verbose, say

unsafe { NonZeroU8::unchecked_new(nz.get().saturating_add(nz.get())) }

by

nz.saturating_add(nz)

However, note that signum, is_positive, is_negative, is_power_of_two and all the unchecked_* differ from the other methods because they do not save a check or the need for unsafe code, they just save a couple of verbose .get(). In this respect, maybe they deserve that a special decision be taken for them. (In other terms: is it worth it to implement them if only for saving a few .get()?)

[EDIT]: Following @scottmcm comment, withdraw the above methods from the list for this reason, except for unchecked_(add|mul) because there is no additional precondition to meet.

[scottmcm]

Good survey!

because they do not save a check or the need for unsafe code

I think this is an important consideration. If the .get() version optimizes to the same thing and doesn't require unsafe code to create the output type, then I think it's totally fine for people to just do that.

And I agree that if one is already using unsafe code to call unchecked_sub or whatever, then the extra unsafe code to call new_unchecked is no big deal. It might even be better for the two preconditions to be two different method calls -- it's more likely that the second one will be remembered that way. (I could easily see myself replacing a u32 with a NonZeroU32 and forgetting to double-check that unchecked_sub cannot return 0.)

But maybe it would be ok to have unchecked_foo in the same cases where there's checked_foo? Because then there isn't the additional precondition -- proving no overflow implies in those cases that the result won't be zero, so it might still be convenient for the result to have the more-constrained type.

[iago-lito]

@scottmcm Thank you for feedback :) We seem to agree with the reasoning, so I have withdrawn methods which do not save a check or the need for unsafe code from the list.

[When] there isn't the additional precondition[,] proving no overflow implies in those cases that the result won't be zero, so it might still be convenient for the result to have the more-constrained type.

Agreed. For this reason, NonZeroU*::unchecked_add and NonZero[UI]*::unchecked_mul resisted the cleanup: they only require that overflow be taken care of, and not natural cancellation. So I'm still planning on implementing them.

If it sounds okay to you, I'll jump into implementation soon :)

[iago-lito]

(github-related question: I have updated the PR title so it better matches the extended implementation plan, but the corresponding branch is still poorly named, is that a problem or not at all?)

[m-ou-se]

(Don't worry about the branch name. There's actually quite a few contributors who put jokes or puns in their branch names, or just something random. It's fine.)

[lqd]

@bors r=m-ou-se

[bors]

📌 Commit 7afdaf2 has been approved by m-ou-se

[bors]

⌛ Testing commit 7afdaf2 with merge 18a20b8311dd63a78420c7065eeb2f6baec2b9ae...

[rust-log-analyzer]

The job dist-x86_64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

      Memory: 14 GB
      Boot ROM Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VMYYxLlBWIQ0

hw.ncpu: 3
hw.byteorder: 1234
hw.memsize: 15032385536

[bors]

💔 Test failed - checks-actions

[lqd]

@bors retry

spurious network error:

failed to download from `https://crates.io/api/v1/crates/minifier/0.0.41/download`

[bors]

⌛ Testing commit 7afdaf2 with merge 705f1a35911507e5a5bdef7746d19eb02aee2f8b...

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

💔 Test failed - checks-actions

[jyn514]

2021-06-12T15:09:48.6473902Z Can't find any online and idle self-hosted runner in current repository that matches the required labels: 'macos-latest'

@bors retry

[bors]

⌛ Testing commit 7afdaf2 with merge da7ada5...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing da7ada5 to master...