danlehmann / arbitrary-int

A modern and lightweight implementation of arbitrary integers for Rust
MIT License
32 stars 13 forks source link

Implement overflowing / wrapping operations #1

Closed veniamin-ilmer closed 1 year ago

veniamin-ilmer commented 2 years ago

First, thank you for your crate. I have been using it extensively to build an emulator with lots of varying bits.

I frequently take in integer as a generic type variable, and implements for example core::ops::AddAssign, and simply run += regardless if it is a u8, or a u4.

I just realized, however that arbitrary-int does not have overflowing / wrapping operations. Within my emulations, integers tend to wrap around, not panic when they overflow.

To get this working, I would have to start keeping track of how many bits is u4 or whatever. Currently this is abstracted away from me. It would be great if I could treat arbitrary ints the same way as I treat regular ints.

danlehmann commented 2 years ago

Thanks for your feedback!

The current version only does asserts when compiled with debugging enabled, like for example here: https://github.com/danlehmann/arbitrary-int/blob/main/src/lib.rs#L233

If you compile in release mode, the asserts should go away.

My understanding is that in practice Rust doesn't check for overflow on e.g. a u8, but that's not guaranteed (see https://doc.rust-lang.org/std/num/struct.Wrapping.html). Functions like wrapping_add and Wrapping<> make it explicit that wrapping behavior is not desired. Unfortunately, you can't add support for Wrapping<> for your own type - it only works with built-in types.

These seems to be one of those times where a custom type can't be as nice as a built-in one.

Some possible workarounds I can see at this point:

I don't really see a good option here (which is also why I didn't do this in the first place).

I'll try out some more things to see if there's something reasonable that can be done.

danlehmann commented 2 years ago

Another option: Provide a type like ArbitraryWrapping<> which works like the regular Wrapping type. Not great to have two types, but it would be more in line with the standard library.

Any thoughts?

veniamin-ilmer commented 2 years ago

I was thinking, perhaps implement traits in the num_traits crate?

Wrapping add would be implemented via https://docs.rs/num-traits/latest/num_traits/ops/wrapping/trait.WrappingAdd.html

Your max and min values would be implemented via https://docs.rs/num-traits/latest/num_traits/bounds/trait.Bounded.html

danlehmann commented 2 years ago

Hey,

I did an initial implementation for wrapping add and sub, but it's not yet on crates.io. You can try it out like this:

arbitrary-int = { git = "https://github.com/danlehmann/arbitrary-int.git", features=["num-traits"] } num-traits = { version="0.2.15", default-features = false }

Please let me know if this is useful for you!

veniamin-ilmer commented 2 years ago

Sorry, I have been busy at work, I will try this out a bit later and let you know. Thank you!

On Fri, Oct 14, 2022 at 11:10 PM Daniel Lehmann @.***> wrote:

Hey,

I did an initial implementation for wrapping add and sub, but it's not yet on crates.io. You can try it out like this:

arbitrary-int = { git = "https://github.com/danlehmann/arbitrary-int.git", features=["num-traits"] } num-traits = { version="0.2.15", default-features = false }

Please let me know if this is useful for you!

— Reply to this email directly, view it on GitHub https://github.com/danlehmann/arbitrary-int/issues/1#issuecomment-1279643784, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJP34BKEN7D6UODFE7FW5EDWDIOB7ANCNFSM6AAAAAARANUWE4 . You are receiving this because you were assigned.Message ID: @.***>

veniamin-ilmer commented 2 years ago

I tried out your new code. It almost works. There is just this one bit I am confused about.

I have this code:

use core::{default, convert};
use num_traits::ops::wrapping;

#[derive(default::Default)]
pub struct Counter<T> {
  count: T,
}

impl<T: default::Default + Copy + convert::From<u8> + wrapping::WrappingAdd> Counter<T> {
  #[inline]
  pub fn new() -> Self {
    Default::default()
  }
  #[inline]
  pub fn increment(&mut self) -> T {
    self.count = self.count.wrapping_add(&T::from(1u8));
    self.count
  }
}

Then later:

let mut counter = Counter<arbitrary_int::u4>;
counter.increment();

When I run this, I get this error:

error[E0080]: evaluation of `arbitrary_int::CompileTimeAssert::<8, 4>::SMALLER_OR_EQUAL` failed
  --> C:\Users\ilmer\.cargo\git\checkouts\arbitrary-int-70d1674038a444e5\6cd7951\src\lib.rs:39:9
   |
39 |         assert!(A <= B);
   |         ^^^^^^^^^^^^^^^ the evaluated program panicked at 'assertion failed: A <= B', C:\Users\ilmer\.cargo\git\checkouts\arbitrary-int-70d1674038a444e5\6cd7951\src\lib.rs:39:9
   |
   = note: this error originates in the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

note: the above error was encountered while instantiating `fn <arbitrary_int::UInt<u8, 4> as std::convert::From<u8>>::from`
  --> D:\dev\chips\src\counter.rs:49:43
   |
49 |     self.count = self.count.wrapping_add(&T::from(1u8));
   |                                           ^^^^^^^^^^^^

How should I write "add 1" in a generic way here that works for both arbitrary_int and a native int?

Update

I tried using this code instead:

self.count = self.count.wrapping_add(&1.into());

And got this error:

error[E0080]: evaluation of `arbitrary_int::CompileTimeAssert::<8, 4>::SMALLER_OR_EQUAL` failed
  --> C:\Users\ilmer\.cargo\git\checkouts\arbitrary-int-70d1674038a444e5\6cd7951\src\lib.rs:39:9
   |
39 |         assert!(A <= B);
   |         ^^^^^^^^^^^^^^^ the evaluated program panicked at 'assertion failed: A <= B', C:\Users\ilmer\.cargo\git\checkouts\arbitrary-int-70d1674038a444e5\6cd7951\src\lib.rs:39:9
   |
   = note: this error originates in the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

note: the above error was encountered while instantiating `fn <arbitrary_int::UInt<u8, 4> as std::convert::From<u8>>::from`

I don't really understand, why is it having trouble with this assert?

2nd Update

Reading through your code, I think the problem is that you are comparing the number of bits between the 1u8 and u4, rather than the value 1. So it is impossible for me to do a from(1u8) for anything less than u8.

I would suggest you change this code to look at the value instead of the number of bits.

Otherwise, it is not possible for me to call T::new(1u8) as there is no trait with the new method.

Perhaps move over the bit based check into new and the value based check into from?

danlehmann commented 2 years ago

Welcome to the limits of Rust's type checking!

from() is defined to never fail, so unfortunately we can only use it for example in a case like u15::from(u8). If there's a possibility of failure, there's try_from().

So ideally, u15 would implement try_from(u16) and from(u8). Unfortunately, that can't be done with Rust at present. A type combination can only ever implement either From or TryFrom, but not both. And Rust doesn't allow making a trait implementation dependent on the value of a const generic.

Once Rust allows limiting trait implementations based on const generics I can add TryFrom.

Some options:

danlehmann commented 2 years ago

I also tried using from(u1), which almost works:

    fn increment<T: WrappingAdd + From<u1>>(foo: T) -> T {
        foo.wrapping_add(&u1::new(1).into())
    }

    // Works:
    assert_eq!(increment(0u8), 1);
    assert_eq!(increment(u15::new(0)), u15::new(1));

    // Fails:
    assert_eq!(increment(u7::new(0)), u7::new(1));

Unfortunately, I can't make u7 implement From using const generics, as you can't implement From for itself.

I also tried implementing From(bool) as a hack, which works....but unfortunately num-traits doesn't implement From(bool), so that also won't solve your usecase.

All of this would be at least easier if Rust allowed me to specify where restrictions on const generics. Something like where NUM1 < NUM2 would allow me to easily implement all From and TryFrom properly.

veniamin-ilmer commented 2 years ago

Unfortunately I cannot use foo.wrapping_add(&u1::new(1).into()) because I want to reserve the ability for T to be a native int. You can't do u8.wrapping_add on a u1...

danlehmann commented 2 years ago

Alright I figured something out! Try the newest version from my Github. The following code will work:

    fn increment<T: WrappingAdd + Number>(foo: T) -> T
    where
        <<T as Number>::UnderlyingType as TryFrom<u32>>::Error: Debug,
    {
        foo.wrapping_add(&T::new(1.into()))
            .wrapping_add(&T::new(512u32.try_into().unwrap()))
    }

The long "where ... Debug" line is only needed if you want to use u16/u32. For u8 (the first example) you can drop that. Also, the compiler will tell you when to write it, so that's nice.

veniamin-ilmer commented 2 years ago

I confirm, this works for me! Thank you!

danlehmann commented 2 years ago

Great. I'd like for this to bake a bit more before I put it onto crates.io. Do you mind pulling your dependency from my github for the time being?

Also please let me know if you find any more issues. I use this crate quite a bit in my projects, but I never had a usecase to perform calculations on these types.