Update int_roundings methods from feedback

[jhpratt]

This updates #![feature(int_roundings)] (#88581) from feedback. All methods now take NonZeroX. The documentation makes clear that they panic in debug mode and wrap in release mode.

r? @joshtriplett

@rustbot label +T-libs +T-libs-api +S-waiting-on-review

[joshtriplett]

I'm kinda hesitant about the NonZero switchover.

Looking at my own uses of div_ceil and next_multiple_of, most of them use compile-time constants, but a few don't.

Leaving aside the non-zero numeric literals work (which I am impressed by), I'm wondering if we really want to move this far from the original num-integer trait methods. The original methods took the same type on both sides of the operation.

And panic-on-zero seems normal for a divide operation.

In constant cases, I'd expect the panic to get compiled out anyway.

Normally I'm all for "convince the type system an error can never happen". But this feels like we're adding a tiny fraction of dependent types, and a substantial amount of complexity to go along with it.

Among other things, to make this fully usable, it'd need more impls (including impls on NonZero types that return NonZero types where appropriate), and it'd still require a lot of try_into().context(...)? calls and converting half my types to NonZero variants, changes that I'd expect to ripple through half the code. Part of me is tempted to do it anyway, but part of me feels like I'd rather just have the divide-by-zero panics and not have even more type conversion complexity than the code already has. (I already need to convert extensively between usize, u64, and various other types.)

I'd like to propose that we keep the change to overflow handling, but switch back to the non-NonZero types.

cc @scottmcm; @jhpratt suggested that you had opinions in this area.

[scottmcm]

@joshtriplett My thoughts being mentioned are probably those in #88581 (comment)

I actually agree that it would be completely reasonable for next_multiple_of in release mode to panic for division by zero, and wrap for the addition in things like u8::MAX.next_multiple_of(2).

The problem for me comes in the checked_ one. We already see people confused by checked_shl, thinking that u32::MAX.checked_shl(1) would return None because of the overflow, but no, it's not checking that part. I thus worry that checked_next_multiple_of is checking too many things, making it confusing. Or if one day we have saturating_next_multiple_of -- which seems like it'd also be quite reasonable -- what does it do for rhs == 0? Saturating the "next" part I understand, but I have no idea how to saturate the / 0 part. So they could both panic for zero, but that would probably be bad for the https://rust-lang.github.io/rust-clippy/master/index.html#integer_arithmetic folks who are expecting that the checked_ method isn't going to panic.

Taking nonzero types as the RHS for next_multiple_of immediately solves all those questions, which makes it worth it to me -- especially since the ergonomic imperfection is an existing one that we all (AFAIK) agree we want to improve in general anyway, and is no worse than ones that already exist like "I have a u32 I want to use to index my array". (If you're fine with a panic for division-by-zero, .try_into().unwrap() is always available.)

All that, however, I think applies only to the next_multiple_of methods. I would be perfectly fine with div_floor and div_ceil to continue to take rhs: Self. Any checking these is mostly about the zero, with the only exceptions being the rare ones like / -1 that already exist for normal division. I also think the consistency with Div::div and the existing checked_div is far more important for those two than for the no-existing-precedent-in-std next_multiple_of.

including impls on NonZero types that return NonZero types where appropriate

I'll note that we have a bunch of these where appropriate -- for example, checked/saturating add/mul/pow are there for the unsigned ones, BitOr for all of them, abs on the signed ones, etc. They just don't have the "wrap in release" versions, since nearly everything (except BitOr) can wrap to zero in at least one situation.

---

EDIT: To make a concrete proposal, I suggest that next_multiple_of keeps the NonZero types as in this PR, but that div_* switch back to the non-NonZero types. (And we keep the overflow handing from this PR, as in your proposal.)

[jhpratt]

I likely won't be a user of next_multiple_of, so I don't particularly care the type it accepts as a parameter. I would like to see these get stabilized, however! If agreement on next_multiple_of/checked_next_multiple_of can't be reached, perhaps it may be worthwhile to only update div_ceil and div_floor, permitting those two to be stabilized?

[joshtriplett]

@scottmcm I disagree; I think using NonZero types in next_multiple_of will substantially decrease usability, and I don't think it's worth that ergonomic hit.

[joshtriplett]

We talked about this very briefly in today's @rust-lang/libs-api meeting. It seemed to me that a few people (not sure about overall team consensus) agreed that we should just use the same integer types here, rather than trying to use NonZero types, and just panic on zero as we already do for division.

[jhpratt]

Rebased and pushed the amended commit. It now alters documentation and removes #[rustc_inherit_overflow_checks] in a few places. Additionally, it removes a duplicate parameter to a macro in tests.

[joshtriplett]

@jhpratt I posted a comment on the phrasing of the documentation about overflow checks. (People can enable overflow checks in release mode, or disable them in debug mode.)

Otherwise, this LGTM; r=me with the comment updated.

[jhpratt]

I'm not a reviewer, so I don't think I can actually do this. Can't hurt to try, though.

@bors r=@joshtriplett rollup

[bors]

@jhpratt: 🔑 Insufficient privileges: Not in reviewers

[bors]

@jhpratt: 🔑 Insufficient privileges: not in try users

[joshtriplett]

@bors r+ rollup

[bors]

📌 Commit dde590d has been approved by joshtriplett