search

isaacholt100 / bnum

Arbitrary, fixed size numeric types that extend the functionality of primitive numeric types in Rust.
https://crates.io/crates/bnum
Other
67 stars 8 forks source link

Overflow behaviour #41

Open chipshort opened 4 weeks ago

chipshort commented 4 weeks ago

This crate currently panics in debug mode and wraps in release mode, which is by default the same behaviour as Rust's primitives. However, it is not affected by overflow-checks = true.

Some context: We are using this crate as a dependency for the math types in cosmwasm-std and care a lot about avoiding overflows (which is also why we recommend all our users to enable overflow-checks). We recently mistakenly used the non-checked version of some operations, leading to potential overflows, even though we had tests in place to guard against that (but those didn't fail because they run in debug mode by default). If overflow-checks worked for this crate, that would have made this much less severe.

Long story short: I am wondering if you see some way to make the overflow behaviour less error-prone. Would it make sense to default to panicing operations? Or maybe it is more in the spirit of this crate to detect the overflow-checks flag? I found this somewhat hacky method of detecting the flag. Would love to hear your thoughts on this.

isaacholt100 commented 3 weeks ago

Hi @chipshort, thanks for opening the issue. I am sorry to hear you encountered this unexpected behaviour and hope you were able to recover quickly from it. Thank you for alerting me to this, I had not realised there was an option to enable overflow checks in release mode as well as in debug.

You're absolutely right that detecting the overflow-checks flag would be in the spirit of this crate, I try to keep the functionality in line with the primitive integer types wherever possible. It's a shame there isn't an easier way of detecting the flag, I don't know if this is a foolproof method though and as there currently isn't a build.rs file, I'd like to avoid creating one if possible. What do you think of the idea of having an extra crate feature, something like overflow-checks which could whether overflow checks were used in release mode or not?

chipshort commented 3 weeks ago

I totally get the point about the build.rs file method. There is a cfg(overflow_checks) on nightly, but it doesn't look very close to stabilization for me. For our usecase, an extra crate feature would work well. However, one thing to note is that cargo features are supposed to be additive in nature, which this one wouldn't be. This can lead to the following situation: Two crates in a user's dependency tree use this crate, one with and one without the feature. The two versions get unified by cargo, meaning both crates get a version with the feature enabled. This would lead to the opposite problem we have right now: People expecting no overflow checks in release, but getting them. It's arguably better than the current situation, but not ideal either.

isaacholt100 commented 3 weeks ago

That's a very good point about the non-additivity, a similar issue was raised with the first version of this crate, so thanks for the reminder about this. I probably shouldn't add this as a feature then unfortunately. Do you think it would speed progress on this language feature if I asked when the PR is expected to be merged on this issue and mentioned that it is needed for this use case?

Do you think you could use the checked versions of the operations you mentioned for now? I appreciate this isn't ideal, and I want to get this issue fixed quickly but preferably there would be some "clean" solution to it.

chipshort commented 3 weeks ago

I guess it might help a little. The feature itself is already implemented and merged in nightly. What is apparently missing is documentation and then making it available in stable rust. I'm not too familiar with the whole process, but maybe I can look into this when I have some time.

Yes, we are currently using the checked math, so this is not a huge problem for us anymore, but it would be nice to have this as a second safeguard if we (or some other bnum user) would mess this up, like we did.

isaacholt100 commented 2 weeks ago

Yes, it would definitely be good to have this behaviour in line with the standard Rust behaviour. I guess the problem with adding this to the crate on nightly only would be that it could be confusing if there was this difference between the nightly and stable versions. If it's ok, I'll leave this for now until the feature becomes stabilised, and then I'll happily add it in.

isaacholt100 commented 1 week ago

Looks like there are now unstable strict_... methods for arithmetic operations on integers (https://doc.rust-lang.org/stable/core/primitive.i32.html#method.strict_sub) which panic on overflow, regardless of overflow checks. I'll try to get around to implementing these in bnum at some point, I suppose this would partly fix your issue? (Obviously it's still not ideal)