Repository: extendr/extendr (rust)

Issue:
Can AltIntegerImpl::{elt, get_region} return Rint and Rfloat instead of Rust i32 and f64?

Pull request:
Let ALTREPs return Rint and Rfloat
Close #312 

Patch:
diff --git a/extendr-api/src/serializer.rs b/extendr-api/src/serializer.rs
index 4cdbccc52..c4781060a 100644
--- a/extendr-api/src/serializer.rs
+++ b/extendr-api/src/serializer.rs
@@ -1,10 +1,10 @@
 //! See https://serde.rs/impl-serializer.html
 
 use crate::error::{Error, Result};
-use crate::{call, eval_string, List, Rany, Robj};
 use crate::{
     Doubles, Environment, Function, Integers, Language, Pairlist, Promise, Raw, Rstr, Symbol,
 };
+use crate::{List, Rany, Robj};
 use serde::{ser, Serialize};
 
 impl ser::Error for Error {
@@ -654,7 +654,7 @@ impl ser::Serialize for Robj {
             Rany::Special(value) => value.serialize(serializer),
             Rany::Builtin(value) => value.serialize(serializer),
             Rany::Rstr(value) => value.serialize(serializer),
-            Rany::Logical(value) => {
+            Rany::Logical(_value) => {
                 todo!("implement Logicals")
             }
             Rany::Integer(value) => value.serialize(serializer),
diff --git a/extendr-api/src/wrapper/altrep.rs b/extendr-api/src/wrapper/altrep.rs
index 062ec7085..8ad63ed58 100644
--- a/extendr-api/src/wrapper/altrep.rs
+++ b/extendr-api/src/wrapper/altrep.rs
@@ -1,21 +1,23 @@
+use prelude::{Rfloat, Rint};
+
 use super::*;
 
 macro_rules! make_from_iterator {
-    ($fn_name : ident, $make_class : ident, $impl : ident, $type : ty) => {
+    ($fn_name : ident, $make_class : ident, $impl : ident, $r_type : ident, $prim_type : ty) => {
         pub fn $fn_name<Iter>(iter: Iter) -> Altrep
         where
             Iter: ExactSizeIterator + std::fmt::Debug + Clone + 'static + std::any::Any,
-            Iter::Item: Into<$type>,
+            Iter::Item: Into<$prim_type>,
         {
             impl<Iter: ExactSizeIterator + std::fmt::Debug + Clone> $impl for Iter
             where
-                Iter::Item: Into<$type>,
+                Iter::Item: Into<$prim_type>,
             {
-                fn elt(&self, index: usize) -> $type {
-                    self.clone().nth(index).unwrap().into()
+                fn elt(&self, index: usize) -> $r_type {
+                    $r_type(self.clone().nth(index).unwrap().into())
                 }
 
-                fn get_region(&self, index: usize, data: &mut [$type]) -> usize {
+                fn get_region(&self, index: usize, data: &mut [$r_type]) -> usize {
                     let len = self.len();
                     if index > len {
                         0
@@ -24,7 +26,7 @@ macro_rules! make_from_iterator {
                         let num_elems = data.len().min(len - index);
                         let dest = &mut data[0..num_elems];
                         for d in dest.iter_mut() {
-                            *d = iter.next().unwrap().into();
+                            *d = $r_type(iter.next().unwrap().into());
                         }
                         num_elems
                     }
@@ -188,9 +190,9 @@ pub trait AltIntegerImpl: AltrepImpl {
         for i in 0..len {
             let val = self.elt(i);
             if !val.is_na() {
-                tot += val as i64;
-                min = min.min(val);
-                max = max.max(val);
+                tot += val.0 as i64;
+                min = min.min(val.0);
+                max = max.max(val.0);
                 nas += 1;
             }
         }
@@ -198,10 +200,10 @@ pub trait AltIntegerImpl: AltrepImpl {
     }
 
     /// Get a single element from this vector.
-    fn elt(&self, _index: usize) -> i32;
+    fn elt(&self, _index: usize) -> Rint;
 
     /// Get a multiple elements from this vector.
-    fn get_region(&self, index: usize, data: &mut [i32]) -> usize {
+    fn get_region(&self, index: usize, data: &mut [Rint]) -> usize {
         let len = self.length();
         if index > len {
             0
@@ -269,9 +271,9 @@ pub trait AltRealImpl: AltrepImpl {
         for i in 0..len {
             let val = self.elt(i);
             if !val.is_na() {
-                tot += val;
-                min = min.min(val);
-                max = max.max(val);
+                tot += val.0;
+                min = min.min(val.0);
+                max = max.max(val.0);
                 nas += 1;
             }
         }
@@ -279,10 +281,10 @@ pub trait AltRealImpl: AltrepImpl {
     }
 
     /// Get a single element from this vector.
-    fn elt(&self, _index: usize) -> f64;
+    fn elt(&self, _index: usize) -> Rfloat;
 
     /// Get a multiple elements from this vector.
-    fn get_region(&self, index: usize, data: &mut [f64]) -> usize {
+    fn get_region(&self, index: usize, data: &mut [Rfloat]) -> usize {
         let len = self.length();
         if index > len {
             0
@@ -699,7 +701,7 @@ impl Altrep {
                 x: SEXP,
                 i: R_xlen_t,
             ) -> c_int {
-                Altrep::get_state::<StateType>(x).elt(i as usize) as c_int
+                Altrep::get_state::<StateType>(x).elt(i as usize).0 as c_int
             }
 
             unsafe extern "C" fn altinteger_Get_region<StateType: AltIntegerImpl + 'static>(
@@ -708,7 +710,7 @@ impl Altrep {
                 n: R_xlen_t,
                 buf: *mut c_int,
             ) -> R_xlen_t {
-                let slice = std::slice::from_raw_parts_mut(buf, n as usize);
+                let slice = std::slice::from_raw_parts_mut(buf as *mut Rint, n as usize);
                 Altrep::get_state::<StateType>(x).get_region(i as usize, slice) as R_xlen_t
             }
 
@@ -777,7 +779,7 @@ impl Altrep {
                 x: SEXP,
                 i: R_xlen_t,
             ) -> f64 {
-                Altrep::get_state::<StateType>(x).elt(i as usize) as f64
+                Altrep::get_state::<StateType>(x).elt(i as usize).0
             }
 
             unsafe extern "C" fn altreal_Get_region<StateType: AltRealImpl + 'static>(
@@ -786,7 +788,7 @@ impl Altrep {
                 n: R_xlen_t,
                 buf: *mut f64,
             ) -> R_xlen_t {
-                let slice = std::slice::from_raw_parts_mut(buf, n as usize);
+                let slice = std::slice::from_raw_parts_mut(buf as *mut Rfloat, n as usize);
                 Altrep::get_state::<StateType>(x).get_region(i as usize, slice) as R_xlen_t
             }
 
@@ -1025,12 +1027,14 @@ impl Altrep {
         make_altinteger_from_iterator,
         make_altinteger_class,
         AltIntegerImpl,
+        Rint,
         i32
     );
     make_from_iterator!(
         make_altreal_from_iterator,
         make_altreal_class,
         AltRealImpl,
+        Rfloat,
         f64
     );
 }
diff --git a/extendr-api/src/wrapper/matrix.rs b/extendr-api/src/wrapper/matrix.rs
index 77aa87dd8..bf6f63321 100644
--- a/extendr-api/src/wrapper/matrix.rs
+++ b/extendr-api/src/wrapper/matrix.rs
@@ -105,8 +105,8 @@ where
     Robj: AsTypedSlice<'a, T>,
 {
     /// Make a new column type.
-    pub fn new_column<F: FnMut(usize) -> T>(nrows: usize, mut f: F) -> Self {
-        let robj = (0..nrows).map(|r| f(r)).collect_robj();
+    pub fn new_column<F: FnMut(usize) -> T>(nrows: usize, f: F) -> Self {
+        let robj = (0..nrows).map(f).collect_robj();
         let dim = [nrows];
         let mut robj = robj.set_attrib(wrapper::symbol::dim_symbol(), dim).unwrap();
         let slice = robj.as_typed_slice_mut().unwrap();
diff --git a/extendr-macros/src/wrappers.rs b/extendr-macros/src/wrappers.rs
index e06ea9c2a..3d355e2ed 100644
--- a/extendr-macros/src/wrappers.rs
+++ b/extendr-macros/src/wrappers.rs
@@ -61,10 +61,7 @@ pub fn make_function_wrappers(
         .map(|input| translate_formal(input, self_ty))
         .collect();
 
-    let convert_args: Vec<syn::Stmt> = inputs
-        .iter()
-        .map(|input| translate_to_robj(input))
-        .collect();
+    let convert_args: Vec<syn::Stmt> = inputs.iter().map(translate_to_robj).collect();
 
     let actual_args: Punctuated<Expr, Token![,]> = inputs
         .iter()


Test patch:
diff --git a/extendr-api/tests/altrep_tests.rs b/extendr-api/tests/altrep_tests.rs
index c0552a5d4..fb5285575 100644
--- a/extendr-api/tests/altrep_tests.rs
+++ b/extendr-api/tests/altrep_tests.rs
@@ -8,6 +8,7 @@ fn test_altinteger() {
             start: i32,
             len: i32,
             step: i32,
+            missing_index: usize,  // For testing NA
         }
 
         impl AltrepImpl for MyCompactIntRange {
@@ -17,19 +18,33 @@ fn test_altinteger() {
         }
 
         impl AltIntegerImpl for MyCompactIntRange {
-            fn elt(&self, index: usize) -> i32 {
-                self.start + self.step * index as i32
+            fn elt(&self, index: usize) -> Rint {
+                if index == self.missing_index {
+                    Rint::na()
+                } else {
+                    Rint(self.start + self.step * index as i32)
+                }
             }
         }
 
-        let mystate = MyCompactIntRange { start: 0, len: 10, step: 1 };
+        // no missing values in the range
+        let mystate = MyCompactIntRange { start: 0, len: 10, step: 1, missing_index: usize::MAX };
 
         let class = Altrep::make_altinteger_class::<MyCompactIntRange>("cir", "mypkg");
-        let obj = Altrep::from_state_and_class(mystate, class, false);
+        let obj = Altrep::from_state_and_class(mystate, class.clone(), false);
 
         assert_eq!(obj.len(), 10);
         // assert_eq!(obj.sum(true), r!(45.0));
         assert_eq!(obj.as_integer_slice().unwrap(), [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]);
+
+        // index 5 is missing
+        let mystate_w_missing = MyCompactIntRange { start: 0, len: 10, step: 1, missing_index: 5 };
+
+        let obj_w_missing = Altrep::from_state_and_class(mystate_w_missing, class, false);
+        let robj_w_missing = Robj::from(obj_w_missing.clone());
+        let integers_w_missing: Integers = robj_w_missing.try_into()?;
+        assert_eq!(integers_w_missing.elt(9), Rint::from(9));
+        assert!(integers_w_missing.elt(5).is_na());
     }
 }
 
@@ -42,6 +57,7 @@ fn test_altreal() {
             start: f64,
             len: usize,
             step: f64,
+            missing_index: usize,  // For testing NA
         }
 
         impl AltrepImpl for MyCompactRealRange {
@@ -51,19 +67,37 @@ fn test_altreal() {
         }
 
         impl AltRealImpl for MyCompactRealRange {
-            fn elt(&self, index: usize) -> f64 {
-                self.start + self.step * index as f64
+            fn elt(&self, index: usize) -> Rfloat {
+                if index == self.missing_index {
+                    Rfloat::na()
+                } else {
+                    Rfloat(self.start + self.step * index as f64)
+                }
             }
         }
 
-        let mystate = MyCompactRealRange { start: 0.0, len: 10, step: 1.0 };
+        // no missing values in the range
+        let mystate = MyCompactRealRange { start: 0.0, len: 10, step: 1.0, missing_index: usize::MAX };
 
         let class = Altrep::make_altreal_class::<MyCompactRealRange>("crr", "mypkg");
-        let obj = Altrep::from_state_and_class(mystate, class, false);
+        let obj = Altrep::from_state_and_class(mystate, class.clone(), false);
 
         assert_eq!(obj.len(), 10);
         // assert_eq!(obj.sum(true), r!(45.0));
         assert_eq!(obj.as_real_slice().unwrap(), [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0]);
+
+        // index 5 is missing
+        let mystate_w_missing = MyCompactRealRange { start: 0.0, len: 10, step: 1.0, missing_index: 5 };
+
+        let obj_w_missing = Altrep::from_state_and_class(mystate_w_missing, class, false);
+        let robj_w_missing = Robj::from(obj_w_missing.clone());
+        let doubles_w_missing: Doubles = robj_w_missing.try_into()?;
+        assert_eq!(doubles_w_missing.elt(9), Rfloat::from(9.0));
+
+        // TODO: Win32 currently handles NA improperly. Re-enable this when the problem is fixed.
+        if cfg!(not(target_arch = "x86")) {
+            assert!(doubles_w_missing.elt(5).is_na());
+        }
     }
 }