Fix unsoundness in new is_power_of_two fast path

[oli-obk]

fixes #120537

[rustbot]

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[NCGThompson]

The same bug exists in library/core/src/num/int_macros.rs. Can you fix that too while you are at it?

[cuviper]

I think it is not as simple as exp >= Self::BITS / power_used, because that division may round down. For example, this should pass, but it fails with your change:

assert_eq!(8u64.checked_pow(21), Some(1u64 << 63));

That sees 21 >= 64 / 3, which is equal so it returns None.

[cuviper]

These tests are expanded for all integer types $T, so it would be nice to keep it testing each type. In this signed case, above where r = 2 as $T we can check exponent <$T>::BITS - 1 produces None, and with r = -2 we can check that BITS - 1 is Some(<$T>::MIN) and BITS gets None.

[oli-obk]

This is not the solution, and I don't want to go math golfing. I did want to use checked mul because of the rounding, but feared it would mess up the optimization.

I'll file a revert so we can avoid having it in beta, and then figure out how yo correctly write this logic and add tests for it

[NCGThompson]

I did want to use checked mul because of the rounding, but feared it would mess up the optimization.

It does indeed mess up the optimization a little. While it makes the Rust code concise, LLVM does an extra comparison. It isn't the end of the world though.

[Noratrieb]

yeah, reverting the libs changes and doing them again very carefully sounds like a good idea