Tracking Issue for inherent unchecked integer methods

[clarfonthey]

This is a tracking issue for the unchecked_* methods on integers.

unchecked_math (stable as of #122520)

impl uN {
    pub const unsafe fn unchecked_add(self, rhs: uN) -> uN;
    pub const unsafe fn unchecked_sub(self, rhs: uN) -> uN;
    pub const unsafe fn unchecked_mul(self, rhs: uN) -> uN;
}
impl iN {
    pub const unsafe fn unchecked_add(self, rhs: iN) -> iN;
    pub const unsafe fn unchecked_sub(self, rhs: iN) -> iN;
    pub const unsafe fn unchecked_mul(self, rhs: iN) -> iN;
}

Steps / History

• Rework the std::iter::Step trait #69659 added unchecked_{add,sub,mul}
• Make unchecked_{add,sub,mul} inherent methods unstably const #85096
• Clean up unchecked_math, separate out unchecked_shifts #115626
• Stabilize unchecked_{add,sub,mul} #122520
• Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571

unchecked_neg

impl iN {
    pub const unsafe fn unchecked_neg(self) -> iN;
}

Steps / History

• Clean up unchecked_math, separate out unchecked_shifts #115626 added unchecked_neg
• Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571

unchecked_shifts

impl uN {
    pub const unsafe fn unchecked_shl(self, rhs: u32) -> uN;
    pub const unsafe fn unchecked_shr(self, rhs: u32) -> uN;
}
impl iN {
    pub const unsafe fn unchecked_shl(self, rhs: u32) -> iN;
    pub const unsafe fn unchecked_shr(self, rhs: u32) -> iN;
}

Steps / History

• Add inherent unchecked_shl, unchecked_shr to integers #85703
• unchecked_{shl|shr} should use u32 as the RHS #103456
• Clean up unchecked_math, separate out unchecked_shifts #115626
• Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571

unchecked_* (aggregate)

impl uN {
    pub const unsafe fn unchecked_add(self, rhs: uN) -> uN;
    pub const unsafe fn unchecked_sub(self, rhs: uN) -> uN;
    pub const unsafe fn unchecked_mul(self, rhs: uN) -> uN;
    pub const unsafe fn unchecked_shl(self, rhs: u32) -> uN;
    pub const unsafe fn unchecked_shr(self, rhs: u32) -> uN;
}
impl iN {
    pub const unsafe fn unchecked_add(self, rhs: iN) -> iN;
    pub const unsafe fn unchecked_sub(self, rhs: iN) -> iN;
    pub const unsafe fn unchecked_mul(self, rhs: iN) -> iN;
    pub const unsafe fn unchecked_shl(self, rhs: u32) -> iN;
    pub const unsafe fn unchecked_shr(self, rhs: u32) -> iN;
    pub const unsafe fn unchecked_neg(self) -> iN;
}

Steps / History

• Rework the std::iter::Step trait #69659 added unchecked_{add,sub,mul}
• Make unchecked_{add,sub,mul} inherent methods unstably const #85096
• Add inherent unchecked_shl, unchecked_shr to integers #85703
• unchecked_{shl|shr} should use u32 as the RHS #103456
• Clean up unchecked_math, separate out unchecked_shifts #115626 added unchecked_neg
• Stabilize unchecked_{add,sub,mul} #122520
• Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571
• Final commenting period (FCP)
• Stabilization PR

Unresolved Questions

• Should the other unchecked intrinsics like exact_div also get inherent versions?
• From Add inherent unchecked_shl, unchecked_shr to integers #85703, consider if distinguishing the different kinds of ub could be useful (UB from overflow like MIN/-1 or MAX+1, LLVM's n[us]w; UB from input range like x/0 or x << -1; UB from lossy like 2/4 or 3 >> 1, LLVM's exact)

Resolved unresolved questions:

• Is unchecked_* the best naming for these? We stabilised unchecked_{add,sub,mul} already, so, yes.

[jordens]

Thanks for this! We are seeing some noticeably faster code in on several embedded DSP applications in cortex-m7 with unchecked_sh[lr]. The additional masking has always bothered me as unnecessary in our case.

[jordens]

One question on consistency and API choice though:

#85703 adds unchecked_sh[rl](self, rhs: Self) while all the other take a u32 as rhs: {overflowing,wrapping,checked}_sh[rl](self, rhs: u32). Why is that?

[cmpute]

I vote for adding exact_div into the library! It could be heavily used in integer factorization and other applications. I'm also developing a checked version of exact division in my crate. I think unchecked_exact_div or unchecked_div_exact will be good names. (I personally prefer div_exact over exact_div as it follows the pattern of div_euclid)

Besides, I'm also thinking about a shorter name for unchecked_, something like raw_add, raw_shl may be easier to type?

[clarfonthey]

I don't think that "raw" is a good naming scheme, since in general, you should not be using these primitives directly. In general, they should be hidden behind other methods that are safe to call. So, I would say that the "unchecked" naming is, at best, not descriptive enough, and that "raw" would be further from the goal of being descriptive.

[scottmcm]

@jordens Agreed the different RHS type is incorrect. I've sent PR #103456.

[jdahlstrom]

I was looking for an unchecked_div the other day, as I was trying to get rid of a superfluous zero test, and was surprised that one doesn't exist even under this feature. Any particular reason it's not included (as in, why is it an unresolved question whether there should be one?) Sure, one can use NonZero* as a divisor even on stable to get a safe unchecked div, but still.

[scottmcm]

I think that if there's a reason, it's that there two kinds of unchecked-ness in signed unchecked_div, and it'd be very easy to trigger the INT_MIN / -1 UB when you only remembered to prove that the denominator is non-zero.

I definitely prefer an unsafe type invariant to an unsafe operation where possible, so I think I like the Div<NonZeroTTT> better. After all, with #100428 I think it's x / NonZero::new_unchecked(y), which is very competitive with x.unchecked_div(y), especially if the producer of y can just change to reflect the non-zeroness of its result.

But it might fine to add unchecked_div under this gate and leave it a stabilization question to figure it out if it's worth keeping. We'll see what libs-api has to say.

(Notably, the "the addition overflows" kind of UB is the only kind of UB for unchecked_add, and it's also something that can't be feasibly pre-checked and thus put into a type invariant, the way "it's not zero" can be.)

[ojeda]

For the signed cases, I think it is good to have it for consistency with the other operations and to avoid having to play with unreachable_unchecked() (and hope it gets optimized away) or intrinsics (unstable). Or is there another way to do it in stable?

For the unsigned cases, I think it would still be good to offer it for consistency with the signed case and with the other operations, even if one can use NonZero* (perhaps documenting that as an alternative).

[RalfJung]

Are there any objections to stabilizing at least unchecked_{add,sub,mul}? For those there shouldn't be any surprises about what their safety requirements are.

[clarfonthey]

A unary unchecked_neg could probably also be added to that list.

[scottmcm]

@RalfJung That seems entirely reasonable to me. a.unchecked_add(b) is semantically a.checked_add(b).unwrap_or_else(hint::unreachable_unchecked), so I don't think there's any lang issues with it -- it's not a newly expressible concept, not even in const -- so I think a libs-api FCP and it'd be good to go.

Nominating it on behalf of Ralf to get libs-api's opinion. Would you want the FCP the whole thing, or FCP a PR that just does a subset? Or is there still skepticism about exposing these?

[m-ou-se]

We discussed this in a recent libs-api meeting. We mostly felt positive about stabilizing this, but it'd be helpful to have a clear overview of the exact APIs this tracking issue is tracking.

[RalfJung]

Great; does someone want to prepare a stabilization PR? I think that can cover everything that is currently under the unchecked_math feature gate; an unchecked_div function does not seem to exist (it only exists as an intrinsic, and we also have the exact_div intrinsic, but those will obviously not get stabilized).

A unary unchecked_neg could probably also be added to that list.

Such a function doesn't exist yet, so it would have to be added (in a separate PR) before it can be stabilized. This requires an ACP since it's a new public function. None of this is insurmountable but I think if someone wants to push for this they can do this in parallel to the effort of stabilizing the existing functions in the unchecked_math feature gate; there's no reason they have to all be stabilized at once.

[saethlin]

I started typing out a long reply, but I'll wait for a new issue. For now, please check #120848, I've already thought of quite a few follow-up tasks on these checks and wouldn't mind adding to the list.

[clarfonthey]

As an aside to specifically mention the status of adding unsafe checks to these methods, #117494 was stalled because of compiler errors I couldn't resolve, which lead to me focusing on #117571 to attempt to provide more info to said errors so I could debug them.

I haven't done any work on these PRs recently, but I'll try to rectify that this week, and if anyone else is interested in taking a look to maybe help out, I would appreciate it. I'm very unfamiliar with compiler internals, so, it's been a bit of a learning curve for me.

I'll also take a look about editing the docs while I'm at it.

[clarfonthey]

More up-to-date update: I decided to get around the compiler lint errors for now, and while it did require a bit of excursion for the shift operations in particular, #117494 should be ready to merge and will panic in debug mode when the unsafe assertions are ignored. That should hopefully help substantially with the concerns that people are using these methods incorrectly, since users without any additional setup to disable these assertions (who should not be doing that unless they know what they're doing) will run into errors in their tests when using these incorrectly.

I also don't think that we should care about users who are running into issues but not testing their code, because… well, yeah, that's how you verify your code is working!

[the8472]

Unchecked variants of the {signed_integer}::checked_{add,sub}_unsigned functions and vice-versa could be useful if those properties can be conveyed to LLVM.

See #122420 and #122461

[scottmcm]

@the8472 If we get https://discourse.llvm.org/t/rfc-add-nowrap-flags-to-trunc/77453?u=scottmcm then I think it'd be possible to communicate it -- we could do sext + zext + add nuw nsw + trunc nsw -- but I suspect that it would end up getting optimized back down to a normal wrapping add.

Might help for smaller types, though, if it encourages LLVM to use a larger IV where the flags could work.

---

Though for usize+isize, there's actually a way: ptr::invalid+wrapping_offset+bare_addr.

(Dunno if that's anywhere close to being a good idea, though.)

[scottmcm]

FYI: I put up a PR (like Ralf mentioned in #85122 (comment)) to propose stabilizing just unchecked_{add,sub,mul}: #122520

(Along with some extra documentation text in them.)

EDIT: ...and it's in FCP! Thanks, Amanieu.

[clarfonthey]

Cleaned up this issue a bit, separating out the various feature flags better and including more PRs that were omitted from the list. #121571 should hopefully be merged soon to ensure that all of these have unsafe preconditions checked in debug mode.

For now, we can keep this issue as an aggregate of all the unchecked methods, but if someone feels it'd be more useful separated into separate issues per feature flag, feel free to do so.

[clarfonthey]

Worth an extra update: the unsafe preconditions PR is finally merged! Won't ping everyone who helped get this over the line here since there were quite a few, but this means that in the next nightly all unchecked arithmetic using the methods (not the intrinsics) should be checked in debug mode.