Skip to content

ACP: try_exact_div method on NonZero<{integer}> #587

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

Open
Kixunil opened this issue May 13, 2025 · 6 comments
Open

ACP: try_exact_div method on NonZero<{integer}> #587

Kixunil opened this issue May 13, 2025 · 6 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@Kixunil
Copy link

Kixunil commented May 13, 2025

Proposal

Problem statement

Given x: NonZero<{unsigned_integer}> and y: NonZero<{unsigned_integer}> if x % y is zero then x / y is guaranteed to not be zero.
In some computations one needs to compute modulo and then divide if it's zero, so it'd be helpful to preserve this property. Today, the optimizer doesn't understand this property so unsafe might be needed to optimize it rather than relying on unwrap. It'd be helpful to share the burden of reviewing the correctness among all crates that need to do this.

Motivating examples or use cases

In decimal formatting, to display the number after decimal point it's useful to normalize it by removing the trailing zeros. This can be done by dividing by 10 in a loop while the remainder is 0. While NonZero doesn't need to be used at all in the code, it's still helpful to express this property because the number after decimal point being zero is special and needs to be handled differently (not adding the dot). And it's also nice to express that the non-zero number stayed non-zero.

Solution sketch

impl<T: Div<Output=T> + Mod<Output=T>> NonZero<T> {
    /// Divides the number if it's exactly divisible.
    ///
    /// In that case the result is guaranteed to be non-zero. In case of error, the remainder is returned.
    fn try_exact_div(self, rhs: Self) -> Result<Self, Self> {
        match NonZero::new(self % rhs) {
            None => unsafe { Ok(NonZero::new_unchecked(self / rhs)) },
            Some(remainder) => Err(remainder),
        }
    }
}

Alternatives

  • Ignore this - it may be legitimately too niche. (But the function is also very simple, is it worth it?)
  • Teach the optimizer about this pattern instead, so people could use unwrap without perf hit.

Links and related work

https://internals.rust-lang.org/t/div-mod-for-nonzero-t-is-it-worth-acp

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@Kixunil Kixunil added T-libs-api api-change-proposal A proposal to add or alter unstable APIs in the standard libraries labels May 13, 2025
@programmerjake
Copy link
Member

i would call it checked_exact_div, see: rust-lang/rust#139911

@programmerjake
Copy link
Member

also, you need to check for signed overflow when doing i32::MIN / -1

@Kixunil
Copy link
Author

Kixunil commented May 13, 2025

Yeah, that could make sense. Signed is problematic since the remainder is always zero and then the method doesn't make sense.
So it should be only for unsigned integers.

@programmerjake
Copy link
Member

Signed is problematic since the remainder is always zero

it's still plenty useful for signed integers, just that checked_exact_div would return None in cases of overflow too. so i32::MIN.checked_exact_div(-1) == None, but there are non-zero remainders in signed division: -23i32.checked_exact_div(-10) == None and -30i32.checked_exact_div(-10) == Some(3)

@Kixunil
Copy link
Author

Kixunil commented May 14, 2025

The proposed API specifically returns Result so for Signed it'd have to be Result<NonZero, Option<NonZero>` or some kind of error type.

@Amanieu
Copy link
Member

Amanieu commented May 20, 2025

I think this is way too niche for the standard library, even though it could in theory avoid a NonZero::unchecked_new. In particular the signature doesn't match the ones for the proposed exact-div methods proposed in #337, especially the Err return type where nothing in the method name indicates that this is the remainder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants