Repository: extendr/extendr
Pull request #291 (2021-10-03 23:02:28)

Add AddAssign, SubAssign, MulAssign, DivAssign support for Rint and Rfloat
This PR addresses Issue #285. As this is my first PR in this community, I wanted to check my approach with the maintainers before making the full PR. At present, this branch adds support for the AddAssign, SubAssign, MulAssign, and DivAssign operators for 
- `Rint op= Rint`
- `&mut Rint op= Rint`
- `Rint op= i32`
- `&mut Rint op= i32`
- `Option<i32> op= Rint`.

I wasn't 100% whether you all wanted that last one or not. I'd appreciate feedback or constructive criticism to let me know if I'm on the right track before implementing the same set of operations for `Rfloat`. Thanks!

clauswilke: Thanks for your contribution! I'll let @Ilia-Kosenkov comment on the technical aspects of this PR. I'll just say that I'd always want to see more comments, in particular in macro code, but also in general.
Ilia-Kosenkov: I am running CI here. I want to see if `cargo fmt` complains because of multiple statements per line.
Ilia-Kosenkov: https://github.com/extendr/extendr/pull/291/checks?check_run_id=3794895283#step:8:174
Yep, as I suspected. Please do run `cargo fmt` after you finish your code modifications.

Otherwise, LGTM.
Ilia-Kosenkov: Also, I would like to ask to implement a similar set of features for `Rfloat` as well. Either here or in a separate PR. 
With the macros you have defined it should be easy.
ericwburden: As requested, code has been run through rustfmt, and the Assignment operators have been implemented for `Rfloat` as well, including: 

- `Rfloat op= Rfloat`
- `&mut Rfloat op= Rfloat`
- `Rfloat op= f64`
- `&mut Rfloat op= f64`
- `Option<f64> op= Rfloat`

If this PR meets your expectations, I'd appreciate a 'hacktoberfest-accepted' label, if you don't mind. (Those T-shirts are snazzy :grin: )

ericwburden: TIL that normal comments in macros don't carry over to the expanded macros, so that's a win for me. :grin:
clauswilke: I think this sets a new standard in documenting macros! Poorly documented macros are one of my pet peeves. They are so difficult to figure out. I've spent hours staring at macro code just to figure out the purpose of specific macros in other peoples' code. (Not talking about the extendr project here.)
Ilia-Kosenkov: The new macros comments are failing the `cargo fmt` check now. Who would've thought?
clauswilke: That's pretty annoying. Surely it must be possible to comment macros. Do we know what exactly the problem is?
Ilia-Kosenkov: It has something to do with the empty commented lines. `diff` shows that `//` is removed then added, `--word-diff` shows nothing. Maybe some trailing spaces?

<details>
<summary>diff</summary>

```
diff --git a/extendr-api/src/scalar/macros.rs b/extendr-api/src/scalar/macros.rs
index 8d9e62de..f21b8aa3 100644
--- a/extendr-api/src/scalar/macros.rs
+++ b/extendr-api/src/scalar/macros.rs
@@ -131,12 +131,12 @@ macro_rules! gen_binopassign {
         // - impl $opname:snake<$type_prim> for $type {}
         // - impl $opname:snake<$type_prim> for &mut $type {}
         // - impl $opname:snake<$type> for Option<$type_prim> {}
-        // 
+        //
         // Note: $opname:snake snake cases the Trait name, i.e. AddAssign -> add_assign
 
         // Example call to this macro. The expansion examples below are all
         // derived from this example macro call.
-        // 
+        //
         // gen_binopassign!(
         //     Rint,                                    <= The Type the Trait is implemented for
         //     i32,                                     <= The generic for the Trait
@@ -144,7 +144,7 @@ macro_rules! gen_binopassign {
         //     |lhs: i32, rhs| lhs.checked_add(rhs),    <= Closure, provides the math logic
         //     "Doc Comment"                            <= Documentation comment for Traits
         // );
-        //  
+        //
 
         // This impl block expands to:
         //

```
</details>
ericwburden: Ah, sorry. Forgot `rustfmt` post-comments.

Patch:
diff --git a/extendr-api/src/scalar/macros.rs b/extendr-api/src/scalar/macros.rs
index cfc856d3b..f21b8aa3f 100644
--- a/extendr-api/src/scalar/macros.rs
+++ b/extendr-api/src/scalar/macros.rs
@@ -121,6 +121,208 @@ macro_rules! gen_binop {
     };
 }
 
+/// Generates binary operate-assign operators for scalar types.
+macro_rules! gen_binopassign {
+    ($type : tt, $type_prim : tt, $opname : ident, $expr: expr, $docstring: expr) => {
+        // Implements the following trait definitions:
+        //
+        // - impl $opname:snake<$type> for $type {}
+        // - impl $opname:snake<$type> for &mut $type {}
+        // - impl $opname:snake<$type_prim> for $type {}
+        // - impl $opname:snake<$type_prim> for &mut $type {}
+        // - impl $opname:snake<$type> for Option<$type_prim> {}
+        //
+        // Note: $opname:snake snake cases the Trait name, i.e. AddAssign -> add_assign
+
+        // Example call to this macro. The expansion examples below are all
+        // derived from this example macro call.
+        //
+        // gen_binopassign!(
+        //     Rint,                                    <= The Type the Trait is implemented for
+        //     i32,                                     <= The generic for the Trait
+        //     AddAssign,                               <= The Trait to implement
+        //     |lhs: i32, rhs| lhs.checked_add(rhs),    <= Closure, provides the math logic
+        //     "Doc Comment"                            <= Documentation comment for Traits
+        // );
+        //
+
+        // This impl block expands to:
+        //
+        // impl AddAssign<Rint> for Rint {
+        //      /// Doc Comment
+        //      fn add_assign(&mut self, other: Rint) {
+        //          match (self.clone().into(), other.into()) {
+        //              (Some(lhs), Some(rhs)) => {
+        //                  let f = |lhs: i32, rhs| lhs.checked_add(rhs);
+        //                  match f(lhs, rhs) {
+        //                      Some(res) => *self = Rint::from(res),
+        //                      None => *self = Rint:na(),
+        //                  }
+        //              }
+        //              _ => *self = Rint::na(),
+        //          }
+        //      }
+        // }
+        impl $opname<$type> for $type {
+            paste::paste! {
+                #[doc = $docstring]
+                fn [< $opname:snake >](&mut self, other: $type) {
+                    // `.clone()` is needed to convert &mut Rint -> Rint -> Option<$type_prim>
+                    match (self.clone().into(), other.into()) {
+                        (Some(lhs), Some(rhs)) => {
+                            let f = $expr;
+                            match f(lhs, rhs) {
+                                Some(res) => *self = $type::from(res),
+                                None => *self = $type::na(),
+                            }
+                        },
+                        _ => *self = $type::na(),
+                    }
+                }
+            }
+        }
+
+        // This impl block expands to:
+        //
+        // impl AddAssign<Rint> for &mut Rint {
+        //      /// Doc Comment
+        //      fn add_assign(&mut self, other: Rint) {
+        //          match (self.clone().into(), other.into()) {
+        //              (Some(lhs), Some(rhs)) => {
+        //                  let f = |lhs: i32, rhs| lhs.checked_add(rhs);
+        //                  match f(lhs, rhs) {
+        //                      Some(res) => **self = Rint::from(res),
+        //                      None => **self = Rint:na(),
+        //                  }
+        //              }
+        //              _ => **self = Rint::na(),
+        //          }
+        //      }
+        // }
+        impl $opname<$type> for &mut $type {
+            paste::paste! {
+                #[doc = $docstring]
+                fn [< $opname:snake >](&mut self, other: $type) {
+                    // `.clone()` is needed to convert &mut &mut Rint -> Rint -> Option<$type_prim>
+                    match (self.clone().into(), other.into()) {
+                        (Some(lhs), Some(rhs)) => {
+                            let f = $expr;
+                            match f(lhs, rhs) {
+                                Some(res) => **self = $type::from(res),
+                                None => **self = $type::na(),
+                            }
+                        },
+                        _ => **self = $type::na(),
+                    }
+                }
+            }
+        }
+
+        // This impl block expands to:
+        //
+        // impl AddAssign<i32> for Rint {
+        //      /// Doc Comment
+        //      fn add_assign(&mut self, other: i32) {
+        //          match self.clone().int() {
+        //              Some(lhs) => {
+        //                  let f = |lhs: i32, rhs| lhs.checked_add(rhs);
+        //                  match f(lhs, rhs) {
+        //                      Some(res) => *self = Rint::from(res),
+        //                      None => *self = Rint:na(),
+        //                  }
+        //              }
+        //              _ => *self = Rint::na(),
+        //              }
+        //          }
+        //      }
+        // }
+        impl $opname<$type_prim> for $type {
+            paste::paste! {
+                #[doc = $docstring]
+                fn [< $opname:snake >](&mut self, other: $type_prim) {
+                    // `.clone()` is needed to convert &mut Rint -> Rint -> Option<$type_prim>
+                    match self.clone().into() {
+                        Some(lhs) => {
+                            let f = $expr;
+                            match f(lhs, other) {
+                                Some(res) => *self = $type::from(res),
+                                None => *self = $type::na(),
+                            }
+                        }
+                        None => *self = $type::na(),
+                    }
+                }
+            }
+        }
+
+        // This impl block expands to:
+        //
+        // impl AddAssign<i32> for &mut Rint {
+        //      /// Doc Comment
+        //      fn add_assign(&mut self, other: i32) {
+        //          match self.clone().int() {
+        //              Some(lhs) => {
+        //                  let f = |lhs: i32, rhs| lhs.checked_add(rhs);
+        //                  match f(lhs, rhs) {
+        //                      Some(res) => **self = Rint::from(res),
+        //                      None => **self = Rint:na(),
+        //                  }
+        //              }
+        //              _ => **self = Rint::na(),
+        //              }
+        //          }
+        //      }
+        // }
+        impl $opname<$type_prim> for &mut $type {
+            paste::paste! {
+                #[doc = $docstring]
+                fn [< $opname:snake >](&mut self, other: $type_prim) {
+                    // `.clone()` is needed to convert &mut &mut Rint -> Rint -> Option<$type_prim>
+                    match self.clone().into() {
+                        Some(lhs) => {
+                            let f = $expr;
+                            match f(lhs, other) {
+                                Some(res) => **self = $type::from(res),
+                                None => **self = $type::na(),
+                            }
+                        }
+                        None => **self = $type::na(),
+                    }
+                }
+            }
+        }
+
+        // This impl block expands to:
+        //
+        //  impl AddAssign<Rint> for Option<i32> {
+        //      /// Doc Comment
+        //      fn add_assign(&mut self, other: Rint) {
+        //          match (*self, other.into()) {
+        //              (Some(lhs), Some(rhs)) => {
+        //                  let f = |lhs: i32, rhs| lhs.checked_add(rhs);
+        //                  *self = f(lhs, rhs);
+        //              },
+        //              _ => *self = None,
+        //          }
+        //      }
+        //  }
+        impl $opname<$type> for Option<$type_prim> {
+            paste::paste! {
+                #[doc = $docstring]
+                fn [< $opname:snake >](&mut self, other: $type) {
+                    match (*self, other.into()) {
+                        (Some(lhs), Some(rhs)) => {
+                            let f = $expr;
+                            *self = f(lhs, rhs);
+                        },
+                        _ => *self = None,
+                    }
+                }
+            }
+        }
+    };
+}
+
 /// Generates conversions from primitive to scalar type.
 macro_rules! gen_from_primitive {
     ($type : tt, $type_prim : tt) => {
@@ -290,6 +492,7 @@ macro_rules! gen_sum_iter {
 }
 
 pub(in crate::scalar) use gen_binop;
+pub(in crate::scalar) use gen_binopassign;
 pub(in crate::scalar) use gen_from_primitive;
 pub(in crate::scalar) use gen_from_scalar;
 pub(in crate::scalar) use gen_impl;
diff --git a/extendr-api/src/scalar/rfloat.rs b/extendr-api/src/scalar/rfloat.rs
index 4660799ea..83e8ddad6 100644
--- a/extendr-api/src/scalar/rfloat.rs
+++ b/extendr-api/src/scalar/rfloat.rs
@@ -2,6 +2,7 @@ use crate::scalar::macros::*;
 use crate::*;
 use std::convert::TryFrom;
 use std::ops::{Add, Div, Mul, Neg, Sub};
+use std::ops::{AddAssign, DivAssign, MulAssign, SubAssign};
 
 /// Rfloat is a wrapper for f64 in the context of an R's integer vector.
 ///
@@ -70,6 +71,34 @@ gen_binop!(
     |lhs: f64, rhs: f64| Some(lhs / rhs),
     "Divide two Rfloat values or an option of f64."
 );
+gen_binopassign!(
+    Rfloat,
+    f64,
+    AddAssign,
+    |lhs: f64, rhs: f64| Some(lhs + rhs),
+    "Add two Rfloat values or an option of f64, modifying the left-hand side in place. Overflows to NA."
+);
+gen_binopassign!(
+    Rfloat,
+    f64,
+    SubAssign,
+    |lhs: f64, rhs: f64| Some(lhs - rhs),
+    "Subtract two Rfloat values or an option of f64, modifying the left-hand side in place. Overflows to NA."
+);
+gen_binopassign!(
+    Rfloat,
+    f64,
+    MulAssign,
+    |lhs: f64, rhs: f64| Some(lhs * rhs),
+    "Multiply two Rfloat values or an option of f64, modifying the left-hand side in place. Overflows to NA."
+);
+gen_binopassign!(
+    Rfloat,
+    f64,
+    DivAssign,
+    |lhs: f64, rhs: f64| Some(lhs / rhs),
+    "Divide two Rfloat values or an option of f64, modifying the left-hand side in place. Overflows to NA."
+);
 
 // Generate unary ops for -, !
 gen_unop!(Rfloat, Neg, |lhs: f64| Some(-lhs), "Negate a Rfloat value.");
diff --git a/extendr-api/src/scalar/rint.rs b/extendr-api/src/scalar/rint.rs
index f51619bd4..ab7fa0ecd 100644
--- a/extendr-api/src/scalar/rint.rs
+++ b/extendr-api/src/scalar/rint.rs
@@ -2,6 +2,7 @@ use crate::scalar::macros::*;
 use crate::*;
 use std::convert::TryFrom;
 use std::ops::{Add, Div, Mul, Neg, Not, Sub};
+use std::ops::{AddAssign, DivAssign, MulAssign, SubAssign};
 
 /// Rint is a wrapper for i32 in the context of an R's integer vector.
 ///
@@ -50,6 +51,34 @@ gen_binop!(
     |lhs: i32, rhs| lhs.checked_div(rhs),
     "Divide two Rint values or an option of i32, overflows to NA."
 );
+gen_binopassign!(
+    Rint,
+    i32,
+    AddAssign,
+    |lhs: i32, rhs| lhs.checked_add(rhs),
+    "Add two Rint values or an option of i32, modifying the left-hand side in place. Overflows to NA."
+);
+gen_binopassign!(
+    Rint,
+    i32,
+    SubAssign,
+    |lhs: i32, rhs| lhs.checked_sub(rhs),
+    "Subtract two Rint values or an option of i32, modifying the left-hand side in place. Overflows to NA."
+);
+gen_binopassign!(
+    Rint,
+    i32,
+    MulAssign,
+    |lhs: i32, rhs| lhs.checked_mul(rhs),
+    "Multiply two Rint values or an option of i32, modifying the left-hand side in place. Overflows to NA."
+);
+gen_binopassign!(
+    Rint,
+    i32,
+    DivAssign,
+    |lhs: i32, rhs| lhs.checked_div(rhs),
+    "Divide two Rint values or an option of i32, modifying the left-hand side in place. Overflows to NA."
+);
 
 // Generate unary ops for -, !
 gen_unop!(


Test patch:
diff --git a/extendr-api/tests/scalar_tests.rs b/extendr-api/tests/scalar_tests.rs
index 381e6edc1..5893bbd0b 100644
--- a/extendr-api/tests/scalar_tests.rs
+++ b/extendr-api/tests/scalar_tests.rs
@@ -49,6 +49,111 @@ fn test_rint() {
     assert!((a + b).is_na());
 }
 
+#[test]
+fn test_rint_opassign() {
+    // LHS Rint, RHS Rint
+    let mut a = Rint::from(20);
+    a += Rint::from(10);
+    assert_eq!(a, Rint::from(30));
+    a -= Rint::from(20);
+    assert_eq!(a, Rint::from(10));
+    a *= Rint::from(20);
+    assert_eq!(a, Rint::from(200));
+    a /= Rint::from(100);
+    assert_eq!(a, Rint::from(2));
+
+    // LHS &mut Rint, RHS Rint
+    let mut a = Rint::from(20);
+    let mut b = &mut a;
+    b += Rint::from(10);
+    assert_eq!(b, &Rint::from(30));
+    b -= Rint::from(20);
+    assert_eq!(b, &Rint::from(10));
+    b *= Rint::from(20);
+    assert_eq!(b, &Rint::from(200));
+    b /= Rint::from(100);
+    assert_eq!(b, &Rint::from(2));
+
+    // LHS Rint, RHS i32
+    let mut a = Rint::from(20);
+    a += 10;
+    assert_eq!(a, Rint::from(30));
+    a -= 20;
+    assert_eq!(a, Rint::from(10));
+    a *= 20;
+    assert_eq!(a, Rint::from(200));
+    a /= 100;
+    assert_eq!(a, Rint::from(2));
+
+    // LHS &mut Rint, RHS i32
+    let mut a = Rint::from(20);
+    let mut b = &mut a;
+    b += 10;
+    assert_eq!(b, &Rint::from(30));
+    b -= 20;
+    assert_eq!(b, &Rint::from(10));
+    b *= 20;
+    assert_eq!(b, &Rint::from(200));
+    b /= 100;
+    assert_eq!(b, &Rint::from(2));
+
+    // LHS Option<i32>, RHS Rint
+    let mut a = Some(20);
+    a += Rint::from(10);
+    assert_eq!(a, Some(30));
+    a -= Rint::from(20);
+    assert_eq!(a, Some(10));
+    a *= Rint::from(20);
+    assert_eq!(a, Some(200));
+    a /= Rint::from(100);
+    assert_eq!(a, Some(2));
+
+    // LHS NA
+    let mut a = Rint::na();
+    a += Rint::from(10);
+    assert!(a.is_na());
+    a -= Rint::from(20);
+    assert!(a.is_na());
+    a *= Rint::from(20);
+    assert!(a.is_na());
+    a /= Rint::from(100);
+    assert!(a.is_na());
+
+    // RHS NA
+    let mut a = Rint::from(20);
+    a += Rint::na();
+    assert!(a.is_na());
+    let mut a = Rint::from(20);
+    a -= Rint::na();
+    assert!(a.is_na());
+    let mut a = Rint::from(20);
+    a *= Rint::na();
+    assert!(a.is_na());
+    let mut a = Rint::from(20);
+    a /= Rint::na();
+    assert!(a.is_na());
+
+    // Overflow | LHS Rint, RHS Rint
+    let mut a = Rint::from(i32::MAX - 1);
+    a += Rint::from(10);
+    assert!(a.is_na());
+    let mut a = Rint::from(i32::MAX - 1);
+    a *= Rint::from(10);
+    assert!(a.is_na());
+
+    let mut a = Rint::from(1);
+    a /= Rint::from(0);
+    assert!(a.is_na());
+    let mut a = Rint::from(-1);
+    a /= Rint::na();
+    assert!(a.is_na());
+
+    // Underflow | LHS Rint, RHS Rint
+    let mut a = Rint::from(i32::MIN + 1);
+    a += Rint::from(-10);
+    assert!(a.is_na());
+}
+
 #[test]
 fn test_rfloat() {
     let a = Rfloat::from(20.);
@@ -124,3 +229,129 @@ fn test_rfloat() {
     assert!((Rfloat::from(f64::INFINITY) + 1.).is_infinite());
     assert!((42. - Rfloat::from(f64::INFINITY)).is_sign_negative());
 }
+
+#[test]
+fn test_rfloat_opassign() {
+    // LHS Rfloat, RHS Rfloat
+    let mut a = Rfloat::from(20.);
+    a += Rfloat::from(10.);
+    assert_eq!(a, Rfloat::from(30.));
+    a -= Rfloat::from(20.);
+    assert_eq!(a, Rfloat::from(10.));
+    a *= Rfloat::from(20.);
+    assert_eq!(a, Rfloat::from(200.));
+    a /= Rfloat::from(100.);
+    assert_eq!(a, Rfloat::from(2.));
+
+    // LHS &mut Rfloat, RHS Rfloat
+    let mut a = Rfloat::from(20.);
+    let mut b = &mut a;
+    b += Rfloat::from(10.);
+    assert_eq!(b, &Rfloat::from(30.));
+    b -= Rfloat::from(20.);
+    assert_eq!(b, &Rfloat::from(10.));
+    b *= Rfloat::from(20.);
+    assert_eq!(b, &Rfloat::from(200.));
+    b /= Rfloat::from(100.);
+    assert_eq!(b, &Rfloat::from(2.));
+
+    // LHS Rfloat, RHS f64
+    let mut a = Rfloat::from(20.);
+    a += 10.;
+    assert_eq!(a, Rfloat::from(30.));
+    a -= 20.;
+    assert_eq!(a, Rfloat::from(10.));
+    a *= 20.;
+    assert_eq!(a, Rfloat::from(200.));
+    a /= 100.;
+    assert_eq!(a, Rfloat::from(2.));
+
+    // LHS &mut Rfloat, RHS f64
+    let mut a = Rfloat::from(20.);
+    let mut b = &mut a;
+    b += 10.;
+    assert_eq!(b, &Rfloat::from(30.));
+    b -= 20.;
+    assert_eq!(b, &Rfloat::from(10.));
+    b *= 20.;
+    assert_eq!(b, &Rfloat::from(200.));
+    b /= 100.;
+    assert_eq!(b, &Rfloat::from(2.));
+
+    // LHS Option<f64>, RHS Rfloat
+    let mut a = Some(20.);
+    a += Rfloat::from(10.);
+    assert_eq!(a, Some(30.));
+    a -= Rfloat::from(20.);
+    assert_eq!(a, Some(10.));
+    a *= Rfloat::from(20.);
+    assert_eq!(a, Some(200.));
+    a /= Rfloat::from(100.);
+    assert_eq!(a, Some(2.));
+
+    // LHS NA
+    let mut a = Rfloat::na();
+    a += Rfloat::from(10.);
+    assert!(a.is_na());
+    a -= Rfloat::from(20.);
+    assert!(a.is_na());
+    a *= Rfloat::from(20.);
+    assert!(a.is_na());
+    a /= Rfloat::from(100.);
+    assert!(a.is_na());
+
+    // RHS NA
+    let mut a = Rfloat::from(20.);
+    a += Rfloat::na();
+    assert!(a.is_na());
+    let mut a = Rfloat::from(20.);
+    a -= Rfloat::na();
+    assert!(a.is_na());
+    let mut a = Rfloat::from(20.);
+    a *= Rfloat::na();
+    assert!(a.is_na());
+    let mut a = Rfloat::from(20.);
+    a /= Rfloat::na();
+    assert!(a.is_na());
+
+    // Inf is a single value, so can be tested for equality
+    let mut a = Rfloat::from(f64::INFINITY);
+    let mut b = Rfloat::from(42.);
+    a += b;
+    assert_eq!(a, f64::INFINITY);
+    a -= b;
+    assert_eq!(a, f64::INFINITY);
+    a *= b;
+    assert_eq!(a, f64::INFINITY);
+    a /= b;
+    assert_eq!(a, f64::INFINITY);
+    b -= a;
+    assert_eq!(b, f64::NEG_INFINITY);
+
+    let mut a = Rfloat::from(f64::NEG_INFINITY);
+    let mut b = Rfloat::from(42.);
+    a += b;
+    assert_eq!(a, f64::NEG_INFINITY);
+    a -= b;
+    assert_eq!(a, f64::NEG_INFINITY);
+    a *= b;
+    assert_eq!(a, f64::NEG_INFINITY);
+    a /= b;
+    assert_eq!(a, f64::NEG_INFINITY);
+    b -= a;
+    assert_eq!(b, f64::INFINITY);
+
+    // Operations with NaN produce NaN
+    let mut a = Rfloat::from(f64::NAN);
+    let mut b = Rfloat::from(42.);
+    a += b;
+    assert!(a.is_nan());
+    a -= b;
+    assert!(a.is_nan());
+    a *= b;
+    assert!(a.is_nan());
+    a /= b;
+    assert!(a.is_nan());
+    b -= a;
+    assert!(b.is_nan());
+}