Conversion from primitives not working with integer values

[tcsc]

NB: I'm entirely prepared to be told I'm doing it wrong.

I have created a unit system with a single dimension to represent pixels in an image like so:

pub mod pixel_space {
    make_units! {
        PIX;
        ONE: Unitless;

        base {
            PX: Pixel, "px", Length;
        }

        derived {
            PX2: Pixel2 = (Pixel * Pixel), Area;
        }

        constants {

        }

        fmt = true;
    }
    pub use self::f64consts::*;
}

In a unit test I then attempt to create a Pixel<isize> - as we'll always have an integral number of pixels - like so:

#[cfg(test)]
mod test {
    use super::pixel_space::{Pixel, PX};

    #[test]
    fn create_integer_value() {
        let x : Pixel<isize> = 42 * PX;
        assert_eq!(x.value_unsafe, 42);
    }
}

Which promptly fails to compile with:

$ cargo test
   Compiling firkin v0.1.0 (file:///home/tcc/projects/firkin)
error[E0277]: the trait bound `{integer}: std::ops::Mul<units::pixel_space::PIX<f64, dim::<unnamed>::TArr<dim::<unnamed>::PInt<dim::<unnamed>::UInt<dim::<unnamed>::UTerm, dim::<unnamed>::B1>>, dim::<unnamed>::ATerm>>>` is not satisfied
  --> src/units.rs:29:32
   |
29 |         let x : Pixel<isize> = 42 * PX;
   |                                ^^^^^^^ the trait `std::ops::Mul<units::pixel_space::PIX<f64, dim::<unnamed>::TArr<dim::<unnamed>::PInt<dim::<unnamed>::UInt<dim::<unnamed>::UTerm, dim::<unnamed>::B1>>, dim::<unnamed>::ATerm>>>` is not implemented for `{integer}`
   |
   = help: the following implementations were found:
             <dim::<unnamed>::TArr<V, A> as std::ops::Mul<Rhs>>
             <bool as std::ops::Mul<dim::si::SI<V, U>>>
             <bool as std::ops::Mul<dim::ucum::UCUM<V, U>>>
             <bool as std::ops::Mul<dim::mks::MKS<V, U>>>
           and 298 others

error: aborting due to previous error

error: Could not compile `firkin`.

Note that doing the same thing with a floating-point value works as expected:

#[cfg(test)]
mod test {
    use super::pixel_space::{Pixel, PX};

    #[test]
    fn create_real_value() {
        let x : Pixel<f64> = 42.0 * PX;
        assert_eq!(x.value_unsafe, 42.0);
    }
}

Am I using it wrong, or is this an issue?

[paholg]

The problem is that PX is a constant with type Pixel<f64>. The macro defining a unit system only creates constants over f32 and f64.

You have a couple options. One is to not use the constants, and set x equal to Pixel::new(42).

Another is to define them yourself, which would look like

const PX: Pixel<isize> = Pixel { value_unsafe: 1, _marker: PhantomData };

Finally, you can submit an issue or pull request to add integer constants to the macro.

It shouldn't be too much work if you want to do it. I think it's just a matter of adding i32consts, i64consts, ... similarly to the float versions in make_units.rs, and then adding some integer tests to make sure they work, and updating the make_units doc to include them.

I'm pretty busy right now, but if you'd like me to do it, go ahead and submit an issue to have them added and I'll hopefully get to it in the next few days.

[paholg]

A note: I know it's just a tiny test, so feel free to ignore this.

I would encourage you to avoid uning value_unsafe where possible. For example, your assert could be changed to

assert_eq!((x/PX).value(), 42.0);

or

assert_eq!(*(x/PX), 42.0);

Which makes the units of x clear, and will fail to compile if you're wrong about them.

[tcsc]

Thanks for the prompt reply, and thanks for the notes re: value_unsafe. The test was just to show a complete example rather than real code, but it's good to know the right way.

[tcsc]

I've had a quick peek at adding the i32/i64 constants, and I think it's beyond me.

Actually getting the macros to generate the basic conversions was pretty straightforward, and I managed to build a cut down SI prefix list which seemed to work.

The complication comes when you start having to consider all the user-defined constants in a system. Once you start generating integer constants you start truncating these user-supplied values and suddenly they don't make sense any more.

I'm not sure what the correct approach is at them moment. Casting the values to i32/i64 is easy enough - it just feels like it would be surprising behavour for someone using the library.

[paholg]

I'll take a look at it when I have time. I had a thought ... It might work to define constants with a unit struct for the value type, such that you can multiply them by anything to give units. I think it would require specialization to work properly, but I'll have to play with it to be sure. It also would not work for user defined constants, just the base and derived ones.

[tcsc]

I'm imagining something like this (grossly simplified for clarity):

use std::ops::Mul;

#[derive(Debug)]
struct Value<T> {
    v: T,
}

struct PX;

impl Mul<PX> for i32 {
    type Output = Value<i32>;
    fn mul(self, _: PX) -> Self::Output {
        Value::<i32> { v: self }
    }
}

impl Mul<PX> for f64 {
    type Output = Value<f64>;
    fn mul(self, _: PX) -> Self::Output {
        Value::<f64> { v: self }
    }
}

fn main() {
    let x = 42 * PX;
    let y = 42.3 * PX;
    println!("X = {:?}, y = {:?}", x, y);
}

Is that the sort of idea?

[paholg]

My idea was something like

struct Constant;

and then in the make_units macro

impl<T, U> Mul<T> for $system<Constant, U> {
    type Output = $system<T, U>;
    fn mul(self, rhs: T) -> Self::Output {
        $system::new(rhs)
    }
}

and also

impl<U> Mul<$system<Constant, U>> for $t {
    type Output = $system<$t, U>;
    fn mul(self, _: $system<Constant, U>) -> Self::Output {
        $system::new($t)
    }
}

where we have such a block for $t as each primitive.

Then if you have,

const PX: Pixel<Constant> = Pixel { value_unsafe: Constant, _marker: PhantomData };

you can do let x: Pixel<f64> = 3.0 * PX; and let y: Pixel<i32> = 3 * PX;

The problem comes when you try to do anything more involved than constants...

Wait, might be able to just do

impl<T> Mul<T> for Constant {
    type Output = T;
    fn mul(self, rhs: T) -> T {
        rhs
    }
}

and then

impl Mul<Constant> for $t {
    type Output = $t;
    fn mul(self, _: Constant) -> $t {
        self
    }
}

for each primitive as $t....

That might give as much functionality as we already have. I'll have to play with it to see.

[paholg]

Sorry it's taken a while, but I've finally had a few minutes to sit down and look at this.

I've added modules i8consts, i16consts, i32consts, i64consts, and isize_consts (and the same for unsigned ints) to the make_units! macro. Right now, they don't include anything from the constants block, just base and derived. I think this is the best solution, along with adding an optional int_consts block to the macro for defining more constants which I'll get to at some point.

These changes exist in the github repo, but have not been published on crates.io yet. I would like to do more testing and add documentation before doing that, as well as adding that int_consts block, but I really don't have much time right now, so it may be a little while.

Anyway, if you depend on the github version and switch pub use self::f64consts::*; to pub use self::isize_consts::*; in your make_units! invocation, it should behave as you want it to. Please let me know if it doesn't.

[paholg]

As of version 0.7.0, integer constant modules are generated by make_units! with all base and derived units. Also, an admittedly non-ideal way of constructing more constants is mentioned in the docs:

https://docs.rs/dimensioned/0.7.0/dimensioned/macro.make_units.html

Please feel free to re-open this if that does not suit your needs.