Tracking Issue for exact_div

[newpavlov]

Feature gate: #![feature(exact_div)]

This is a tracking issue for exact division methods (i.e. for division without remainder) on primitive integer types.

Public API

/// Checked integer division without remainder. Computes `self / rhs`,
/// returning `None` if `rhs == 0` or if division remainder is not zero.
pub const fn checked_exact_div(self, rhs: Self) -> Option<Self> { ... }

/// Integer division without remainder. Computes `self / rhs`,
/// panics if `rhs == 0` or if division remainder is not zero.
pub const fn exact_div(self, rhs: Self) -> Self { ... }

/// Integer division without remainder.
/// Unchecked version of `exact_div`.
pub unsafe const fn unchecked_exact_div(self, rhs: Self) -> Self { ... }

Steps / History

• ACP: Add ((un)checked_)exact_div methods for integer types libs-team#337
• Implementation: TODO
• Final comment period (FCP)
• Stabilization PR

Unresolved Questions

• Concerns over usefulness of the panicking exact_div: Add ((un)checked_)exact_div methods for integer types libs-team#337 (comment)

[hanna-kruppe]

How do these handle overflow (iN::MIN / -1)?

[newpavlov]

They return None/panic/cause UB respectively, see here.

[hanna-kruppe]

For the safe version this makes sense but for unchecked_exact_div it’s not obvious to me that UB is the right choice. Having three distinct preconditions to check (no remainder, no division by zero, no overflow) is not great because it’s easy to forget one, especially the overflow edge case. Even for unsigned types, the precedent in stable Rust is handling division by zero via impl Div<NonZeroT> for T instead of an unsafe method that’s UB on zero RHS. This may be useful in this case as well. And overflow in signed division could panic even if the “no remainder” portion is unchecked under threat of UB.

[newpavlov]

The unsafe variant would be a simple wrapper around the exact_div intrinsic.

[hanna-kruppe]

Just because the perma-unstable intrinsic works this way doesn’t mean it’s also the best option for a stable user-facing API. We also have an unchecked_div intrinsic but as noted in my last comment, it’s not exposed as-is.

[newpavlov]

Any other behavior would be surprising. You could argue that we don't need unchecked_exact_div methods similarly to how we don't have unchecked_div, but there was an explicit interest in it during previous discussion. Personally, I think we should add unchecked_div methods instead.

[hanna-kruppe]

I’m not here to argue about whether the functionality is needed, just point out that there’s other ways to expose it that may be better (easier to use correctly), at least for unsigned integers. Similarly , there’s no need to add unsafe unchecked_div methods for unsigned integers because the safe Div implementations already achieves the same effect (those impls use the unchecked_div intrinsic internally, and callers that need to unsafely assert that the RHS is non-zero can combine it with NonZero::new_unchecked), and unchecked_div for signed integers could also take a NonZero RHS and only require callers to check the “no overflow” condition.

[abgros]

For the safe version this makes sense but for unchecked_exact_div it’s not obvious to me that UB is the right choice. Having three distinct preconditions to check (no remainder, no division by zero, no overflow) is not great because it’s easy to forget one, especially the overflow edge case.

No, there is only one unsafe precondition: a.exact_div_unchecked(b) is only allowed if there is a number c such that b * c == a (without overflow). The three cases are just a consequence of that.

An "unchecked" method panicking on some inputs would be quite surprising.

[hanna-kruppe]

This “single” precondition leaves a third of the work to the “(without overflow)” and isn’t even correct: a = b = 0 is UB but c = 0 satisfies b * c = a.

In any case: even if there is some clever way to subsume all three conditions in a single one, that’s of little use to unsafe code authors if the generality makes it easy to overlook edge cases or difficult to connect to the concrete facts available at the call site.

[abgros]

This “single” precondition leaves a third of the work to the “(without overflow)” and isn’t even correct: a = b = 0 is UB but c = 0 satisfies b * c = a.

You're right, it should be a unique number c.

[Amanieu]

This came up in the @rust-lang/libs-api meeting while discussing rust-lang/libs-team#570.

We think that the unchecked version is valuable because it exposes the lowest-level primitive which others can build around. Only exposing higher-level wrappers can end up being harder for users to work with.

However we did feel that there was little value to the panicking version, especially since it can be emulated by calling .unwrap() on the result of the checked version (which additionally has the advantage of making it clear this is a potential panic point). As such we would like the panicking version removed and the checked_ prefix to be removed on the checked version.

Regarding naming, in the meeting there was a preference for exact_div and exact_div_unchecked, the reasoning being that unchecked_exact_div can be ambiguous as to whether "unchecked" refers just to "exact" or the entire "exact_div" operation.

[BurntSushi]

I don't think I'm on board with the checked variant dropping the checked_ prefix. From my comment on the ACP:

As of today, every single method on integer types (as far as I can see) that returns an Option starts with a checked_ prefix. I'm not one for a foolish consistency, but I think it makes sense here.

We should just keep that prefix given how strong the convention already is.

The other benefit of using the checked_ prefix is that it leaves room for adding the panicking variant in the future, should that become better motivated.

Could the libs-api members who want to drop the checked_ prefix say more about that here?

[traviscross]

It was mentioned in the meeting that we'd get pushback on this point. And looking at the docs now, with fresh eyes, I certainly see what you mean about the weight of precedent here.

The problem we were wrestling with, for rust-lang/libs-team#570, was what to do about those shl / shr methods where the names don't follow the convention about e.g. overflowing_, in that it should apply to the result, but with shifting methods it applies instead to the operand.

Similarly, checked_shl refers to checking the operand. With exact_shl, though, we need to check the result¹, and given the existing precedent, checked_exact_shl felt maybe a bit ambiguous about the scope of the checked_ there. One could imagine, of course, checked_unbounded_exact_shl (or would it be unbounded_checked_exact_shl?), so maybe it'd be most consistent to have checked_checked_exact_shl, but of course this is a bit of a mouthful.

So the context from which exact_shl returning Option emerged was in trying to find a way to plot something of a new and revised (and hopefully more minimal) course for naming these kind of operations. Maybe or maybe not that's a bridge too far -- I can see it either way -- but that's how we landed there. And then, of course, we wanted to be consistent with exact_div for whatever we do with exact_sh[lr].

Footnotes

1. That is, the infinite precision result. Or alternatively, I suppose, one could still frame this as checking (the combined effect of the) operands. In any case, it's a different and unrelated check. ↩

[newpavlov]

@Amanieu
If the libs team has made the final decision on this, feel free to edit the OP accordingly (personally, I disagree, but alas). I will leave creation of an implementation PR to the libs team as well, since I do not want to do it with potentially ongoing bikeshedding.

[Kixunil]

If the intention is to make the check more convenient shouldn't the checked version return a Result with the error variant reporting what exactly failed? (div by zero, overflow, non-zero remainder)

If so, there's an interesting case that it might be useful to report the non-zero remainder as NonZero<{integer}>