Repository: extendr/extendr
Pull request #124 (2021-01-13 19:57:09)

Unify iterators
This PR fills some gaps in the set of iterators to cover environments
and to make all string iterators use the `is_na()` methods rather than `Option<&str>`.

The environment method `as_env()` now returns the parent and an iterator over elements
of  the environment.

The `PartialEq` operator only compares pointers for environments, so `enva == envb` will
just compare SEXP pointers,

The integer and real iterators return actual i32 and f64 values instead of references which
will make it easier to write stats functions.




clauswilke: There's a stack overflow error on Windows:
https://github.com/extendr/extendr/pull/124/checks?check_run_id=1697541518#step:8:92

The error on macOS seems to be related to the R setup, not your code.
andy-thomason: The windows error is in the `rprintln` tests which use pipes. I'm not sure how well this is supported on windows.

It may be worth putting a `[cfg]` guard around the test.

andy-thomason: We do not increase the stack size for windows, it seems:

```
            if cfg!(not(target_os = "windows")) {
                R_CStackLimit = usize::max_value();
            }
```

Is there a reason for this. Maybe a more conservative value would work, such as 32MB?
yutannihilation: Here's a discussion about the stack size and the error:

https://github.com/extendr/extendr/issues/129#issuecomment-761196797

Patch:
diff --git a/.gitignore b/.gitignore
index bffe514f3..71eca2c56 100644
--- a/.gitignore
+++ b/.gitignore
@@ -22,3 +22,9 @@
 # CLion support files
 **/.idea
 .vscode/tasks.json
+
+# Rstudio temps.
+.RData
+.Rhistory
+extendr.Rproj
+tests/extendrtests_0.1.0.tar.gz
diff --git a/extendr-api/src/lib.rs b/extendr-api/src/lib.rs
index 47ae3ee32..f052e82dc 100644
--- a/extendr-api/src/lib.rs
+++ b/extendr-api/src/lib.rs
@@ -209,7 +209,7 @@ pub use lang::*;
 pub use logical::*;
 pub use rmacros::*;
 pub use robj::*;
-pub use thread_safety::{handle_panic, single_threaded, throw_r_error, catch_r_error};
+pub use thread_safety::{handle_panic, single_threaded, throw_r_error, catch_r_error, this_thread_id};
 pub use wrapper::*;
 pub use matrix::*;
 
diff --git a/extendr-api/src/matrix.rs b/extendr-api/src/matrix.rs
index f66ac5926..541ac0cdf 100644
--- a/extendr-api/src/matrix.rs
+++ b/extendr-api/src/matrix.rs
@@ -216,7 +216,7 @@ impl Robj {
         if self.is_array() {
             if let Some(data) = self.as_typed_slice() {
                 if let Some(dim) = self.dim() {
-                    let dim: Vec<_> = dim.cloned().collect();
+                    let dim: Vec<_> = dim.collect();
                     let dim = [dim[0] as usize, dim[1] as usize, dim[2] as usize];
                     return Some(RArray { data, dim });
                 }
diff --git a/extendr-api/src/robj/iter.rs b/extendr-api/src/robj/iter.rs
index 46b575974..c046a763a 100644
--- a/extendr-api/src/robj/iter.rs
+++ b/extendr-api/src/robj/iter.rs
@@ -1,14 +1,24 @@
 use crate::*;
-use std::marker::PhantomData;
 
 // Iterator over the objects in a VECSXP, EXPRSXP or WEAKREFSXP.
 #[derive(Clone)]
 pub struct ListIter {
-    vector: SEXP,
+    vector: Robj,
     i: usize,
     len: usize,
 }
 
+impl ListIter {
+    // A new, empty list iterator.
+    pub fn new() -> Self {
+        ListIter {
+            vector: ().into(),
+            i: 0,
+            len: 0,
+        }
+    }
+}
+
 impl Iterator for ListIter {
     type Item = Robj;
 
@@ -22,7 +32,7 @@ impl Iterator for ListIter {
         if i >= self.len {
             return None;
         } else {
-            Some(unsafe { new_owned(VECTOR_ELT(self.vector, i as isize)) })
+            Some(unsafe { new_owned(VECTOR_ELT(self.vector.get(), i as isize)) })
         }
     }
 
@@ -32,27 +42,17 @@ impl Iterator for ListIter {
     }
 }
 
-impl std::fmt::Debug for ListIter {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "[")?;
-        for s in self.clone() {
-            write!(f, "{:?}", s)?;
-        }
-        write!(f, "]")
-    }
-}
-
 /// Iterator over name-value pairs in lists.
 pub type NamedListIter = std::iter::Zip<StrIter, ListIter>;
 
 /// Iterator over primitives in integer objects.
-pub type IntegerIter<'a> = std::slice::Iter<'a, i32>;
+pub type IntegerIter<'a> = std::iter::Cloned<std::slice::Iter<'a, i32>>;
 
 /// Iterator over primitives in real objects.
-pub type RealIter<'a> = std::slice::Iter<'a, f64>;
+pub type RealIter<'a> = std::iter::Cloned<std::slice::Iter<'a, f64>>;
 
 /// Iterator over primitives in logical objects.
-pub type LogicalIter<'a> = std::slice::Iter<'a, Bool>;
+pub type LogicalIter<'a> = std::iter::Cloned<std::slice::Iter<'a, Bool>>;
 
 /// Iterator over the objects in a vector or string.
 ///
@@ -75,6 +75,7 @@ pub type LogicalIter<'a> = std::slice::Iter<'a, Bool>;
 /// ```
 #[derive(Clone)]
 pub struct PairlistIter {
+    root_obj: Robj,
     list_elem: SEXP,
 }
 
@@ -83,6 +84,7 @@ impl PairlistIter {
     pub fn new() -> Self {
         unsafe {
             Self {
+                root_obj: ().into(),
                 list_elem: R_NilValue,
             }
         }
@@ -105,16 +107,6 @@ impl Iterator for PairlistIter {
     }
 }
 
-impl std::fmt::Debug for PairlistIter {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "[")?;
-        for s in self.clone() {
-            write!(f, "{:?}", s)?;
-        }
-        write!(f, "]")
-    }
-}
-
 #[derive(Clone)]
 /// Iterator over pairlist tag names.
 /// ```
@@ -124,28 +116,27 @@ impl std::fmt::Debug for PairlistIter {
 /// let mut robj = R!(pairlist(a = 1, b = 2, 3)).unwrap();
 /// // let mut robj = pairlist!(a = 1, b = 2, 3);
 /// let tags : Vec<_> = robj.as_pairlist_tag_iter().unwrap().collect();
-/// assert_eq!(tags, vec![Some("a"), Some("b"), None]);
+/// assert_eq!(tags, vec!["a", "b", na_str()]);
 /// ```
-pub struct PairlistTagIter<'a> {
+pub struct PairlistTagIter {
+    root_obj: Robj,
     list_elem: SEXP,
-    phantom: PhantomData<&'a ()>,
 }
 
-impl<'a> PairlistTagIter<'a> {
+impl PairlistTagIter {
     /// Make an empty list iterator.
     pub fn new() -> Self {
         unsafe {
             Self {
+                root_obj: ().into(),
                 list_elem: R_NilValue,
-                phantom: PhantomData,
             }
         }
     }
 }
 
-// 'a is the lifetime of the pairlist.
-impl<'a> Iterator for PairlistTagIter<'a> {
-    type Item = Option<&'a str>;
+impl Iterator for PairlistTagIter {
+    type Item = &'static str;
 
     fn next(&mut self) -> Option<Self::Item> {
         unsafe {
@@ -154,26 +145,16 @@ impl<'a> Iterator for PairlistTagIter<'a> {
                 None
             } else {
                 self.list_elem = CDR(sexp);
-                if let Some(symbol) = new_borrowed::<'a>(TAG(sexp)).as_symbol() {
-                    Some(Some(std::mem::transmute(symbol.0)))
+                if let Some(symbol) = new_borrowed(TAG(sexp)).as_symbol() {
+                    Some(std::mem::transmute(symbol.0))
                 } else {
-                    Some(None)
+                    Some(na_str())
                 }
             }
         }
     }
 }
 
-impl<'a> std::fmt::Debug for PairlistTagIter<'a> {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "[")?;
-        for s in self.clone() {
-            write!(f, "{:?}", s)?;
-        }
-        write!(f, "]")
-    }
-}
-
 /// Iterator over strings or string factors.
 ///
 /// ```
@@ -191,7 +172,7 @@ impl<'a> std::fmt::Debug for PairlistTagIter<'a> {
 /// ```
 #[derive(Clone)]
 pub struct StrIter {
-    vector: SEXP,
+    vector: Robj,
     i: usize,
     len: usize,
     levels: SEXP,
@@ -202,7 +183,7 @@ impl StrIter {
     pub fn new() -> Self {
         unsafe {
             Self {
-                vector: R_NilValue,
+                vector: ().into(),
                 i: 0,
                 len: 0,
                 levels: R_NilValue,
@@ -242,12 +223,13 @@ impl Iterator for StrIter {
         unsafe {
             let i = self.i;
             self.i += 1;
+            let vector = self.vector.get();
             if i >= self.len {
                 return None;
-            } else if TYPEOF(self.vector) as u32 == STRSXP {
-                Some(str_from_strsxp(self.vector, i as isize))
-            } else if TYPEOF(self.vector) as u32 == INTSXP && TYPEOF(self.levels) as u32 == STRSXP {
-                let j = *(INTEGER(self.vector).offset(i as isize));
+            } else if TYPEOF(vector) as u32 == STRSXP {
+                Some(str_from_strsxp(vector, i as isize))
+            } else if TYPEOF(vector) as u32 == INTSXP && TYPEOF(self.levels) as u32 == STRSXP {
+                let j = *(INTEGER(vector).offset(i as isize));
                 Some(str_from_strsxp(self.levels, j as isize - 1))
             } else {
                 return None;
@@ -261,18 +243,98 @@ impl Iterator for StrIter {
     }
 }
 
-impl std::fmt::Debug for StrIter {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "[")?;
-        let mut comma = "";
-        for s in self.clone() {
-            write!(f, "{}{:?}", comma, s)?;
-            comma = ", ";
+macro_rules! impl_iter_debug {
+    ($name: ty) => {
+        impl std::fmt::Debug for $name {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                write!(f, "[")?;
+                let mut comma = "";
+                for s in self.clone() {
+                    write!(f, "{}{:?}", comma, s)?;
+                    comma = ", ";
+                }
+                write!(f, "]")
+            }
         }
-        write!(f, "]")
     }
 }
 
+/// Iterator over the names and values of an environment
+///
+/// ```
+/// use extendr_api::*;
+/// test! {
+///     let names_and_values = (0..100).map(|i| (format!("n{}", i), i));
+///     let env = Env{parent: global_env(), names_and_values};
+///     let robj = r!(env);
+///     let names_and_values = robj.as_env_iter().unwrap().collect::<Vec<_>>();
+///     assert_eq!(names_and_values.len(), 100);
+///
+///     let small_env = new_env_with_capacity(1);
+///     small_env.set_local(sym!(x), 1);
+///     let names_and_values = small_env.as_env_iter().unwrap().collect::<Vec<_>>();
+///     assert_eq!(names_and_values, vec![("x", r!(1))]);
+///
+///     let large_env = new_env_with_capacity(1000);
+///     large_env.set_local(sym!(x), 1);
+///     let names_and_values = large_env.as_env_iter().unwrap().collect::<Vec<_>>();
+///     assert_eq!(names_and_values, vec![("x", r!(1))]);
+/// }
+///
+/// ```
+#[derive(Clone)]
+pub struct EnvIter {
+    hash_table: ListIter,
+    pairlist: PairlistIter,
+    pairlisttags: PairlistTagIter,
+}
+
+impl Iterator for EnvIter {
+    type Item = (&'static str, Robj);
+
+    fn next(&mut self) -> Option<Self::Item> {
+        loop {
+            // Environments are a hash table (list) or pair lists (pairlist)
+            // Get the first available value from the pair list.
+            loop {
+                match (self.pairlisttags.next(), self.pairlist.next()) {
+                    (Some(key), Some(value)) => {
+                        // if the key and value are valid, return a pair.
+                        if !key.is_na() && !value.is_unbound_value() {
+                            println!("value: {:?}", (&key, &value));
+                            return Some((key, value));
+                        }
+                    }
+                    // if the key and value are invalid, move on to the hash table.
+                    _ => break
+                }
+                // continue pair list loop.
+            }
+
+            // Get the first pairlist from the hash table.
+            loop {
+                if let Some(obj) = self.hash_table.next() {
+                    if !obj.is_null() && obj.is_pairlist() {
+                        self.pairlisttags = obj.as_pairlist_tag_iter().unwrap();
+                        self.pairlist = obj.as_pairlist_iter().unwrap();
+                        break;
+                    }
+                    // continue hash table loop.
+                } else {
+                    // The hash table is empty, end of iteration.
+                    return None;
+                }
+            }
+        }
+    }
+}
+
+impl_iter_debug!(ListIter);
+impl_iter_debug!(PairlistIter);
+impl_iter_debug!(PairlistTagIter);
+impl_iter_debug!(StrIter);
+impl_iter_debug!(EnvIter);
+
 impl Robj {
     /// Get an iterator over a pairlist objects.
     /// ```
@@ -287,6 +349,7 @@ impl Robj {
         match self.sexptype() {
             LISTSXP | LANGSXP | DOTSXP => unsafe {
                 Some(PairlistIter {
+                    root_obj: self.into(),
                     list_elem: self.get(),
                 })
             },
@@ -302,14 +365,14 @@ impl Robj {
     /// let mut robj = R!(pairlist(a = 1, b = 2, 3)).unwrap();
     /// // let mut robj = pairlist!(a = 1, b = 2, 3);
     /// let tags : Vec<_> = robj.as_pairlist_tag_iter().unwrap().collect();
-    /// assert_eq!(tags, vec![Some("a"), Some("b"), None]);
+    /// assert_eq!(tags, vec!["a", "b", na_str()]);
     /// ```
-    pub fn as_pairlist_tag_iter<'a>(&self) -> Option<PairlistTagIter<'a>> {
+    pub fn as_pairlist_tag_iter(&self) -> Option<PairlistTagIter> {
         match self.sexptype() {
             LISTSXP | LANGSXP | DOTSXP => unsafe {
                 Some(PairlistTagIter {
+                    root_obj: self.into(),
                     list_elem: self.get(),
-                    phantom: PhantomData,
                 })
             },
             _ => None,
@@ -327,9 +390,9 @@ impl Robj {
     /// ```
     pub fn as_list_iter(&self) -> Option<ListIter> {
         match self.sexptype() {
-            VECSXP | EXPRSXP | WEAKREFSXP => unsafe {
+            VECSXP | EXPRSXP | WEAKREFSXP => {
                 Some(ListIter {
-                    vector: self.get(),
+                    vector: self.into(),
                     i: 0,
                     len: self.len(),
                 })
@@ -370,20 +433,18 @@ impl Robj {
         let len = self.len();
         match self.sexptype() {
             STRSXP => unsafe {
-                let vector = self.get();
                 Some(StrIter {
-                    vector,
+                    vector: self.into(),
                     i,
                     len,
                     levels: R_NilValue,
                 })
             },
             INTSXP => unsafe {
-                let vector = self.get();
                 if let Some(levels) = self.get_attrib(levels_symbol()) {
                     if self.is_factor() && levels.sexptype() == STRSXP {
                         Some(StrIter {
-                            vector,
+                            vector: self.into(),
                             i,
                             len,
                             levels: levels.get(),
@@ -398,4 +459,32 @@ impl Robj {
             _ => None,
         }
     }
+
+    /// Iterate over an environment.
+    pub fn as_env_iter(&self) -> Option<EnvIter> {
+        if self.is_environment() {
+            unsafe {
+                let hashtab = new_owned(HASHTAB(self.get()));
+                let frame = new_owned(FRAME(self.get()));
+                if hashtab.is_null() && frame.is_pairlist() {
+                    Some(EnvIter {
+                        hash_table: ListIter::new(),
+                        pairlisttags: frame.as_pairlist_tag_iter().unwrap(),
+                        pairlist: frame.as_pairlist_iter().unwrap(),
+                    })
+        
+                } else if hashtab.is_list() {
+                    Some(EnvIter {
+                        hash_table: hashtab.as_list_iter().unwrap(),
+                        pairlist: PairlistIter::new(),
+                        pairlisttags: PairlistTagIter::new(),
+                    })
+                } else {
+                    None
+                }
+            }
+        } else {
+            None
+        }
+    }
 }
diff --git a/extendr-api/src/robj/mod.rs b/extendr-api/src/robj/mod.rs
index c213db618..c25e36743 100644
--- a/extendr-api/src/robj/mod.rs
+++ b/extendr-api/src/robj/mod.rs
@@ -79,7 +79,7 @@ pub use rinternals::*;
 ///
 /// let a : Robj = c!(1, 2, 3, 4, 5);
 /// let iter = a.as_integer_iter().unwrap();
-/// let robj = iter.filter(|&&x| x < 3).collect_robj();
+/// let robj = iter.filter(|&x| x < 3).collect_robj();
 /// assert_eq!(robj, c!(1, 2));
 /// ```
 ///
@@ -319,7 +319,7 @@ impl Robj {
         Self: 'a,
     {
         if let Some(slice) = self.as_integer_slice() {
-            Some(slice.iter())
+            Some(slice.iter().cloned())
         } else {
             None
         }
@@ -380,7 +380,7 @@ impl Robj {
     /// let robj = r!([TRUE, FALSE, NA_LOGICAL]);
     /// let (mut nt, mut nf, mut nna) = (0, 0, 0);
     /// for val in robj.as_logical_iter().unwrap() {
-    ///   match *val {
+    ///   match val {
     ///     TRUE => nt += 1,
     ///     FALSE => nf += 1,
     ///     NA_LOGICAL => nna += 1,
@@ -391,7 +391,7 @@ impl Robj {
     /// ```
     pub fn as_logical_iter(&self) -> Option<LogicalIter> {
         if let Some(slice) = self.as_logical_slice() {
-            Some(slice.iter())
+            Some(slice.iter().cloned())
         } else {
             None
         }
@@ -433,7 +433,7 @@ impl Robj {
     /// ```
     pub fn as_real_iter(&self) -> Option<RealIter> {
         if let Some(slice) = self.as_real_slice() {
-            Some(slice.iter())
+            Some(slice.iter().cloned())
         } else {
             None
         }
@@ -894,7 +894,7 @@ impl Robj {
     ///    extendr_engine::start_r();
     ///
     ///    let array = R!(array(data = c(1, 2, 3, 4), dim = c(2, 2), dimnames = list(c("x", "y"), c("a","b")))).unwrap();
-    ///    let dim : Vec<_> = array.dim().unwrap().cloned().collect();
+    ///    let dim : Vec<_> = array.dim().unwrap().collect();
     ///    assert_eq!(dim, vec![2, 2]);
     /// ```
     pub fn dim<'a>(&self) -> Option<IntegerIter<'a>>
@@ -1001,35 +1001,12 @@ impl Robj {
     ///
     ///    let names_and_values : std::collections::HashMap<_, _> = (0..4).map(|i| (format!("n{}", i), r!(i))).collect();
     ///    let env = r!(Env{parent: global_env(), names_and_values});
-    ///    assert_eq!(env.ls().unwrap(), vec!["n0".to_string(), "n1".to_string(), "n2".to_string(), "n3".to_string()]);
+    ///    assert_eq!(env.ls().unwrap(), vec!["n0", "n1", "n2", "n3"]);
     /// ```
-    pub fn ls(&self) -> Option<Vec<String>> {
-        if self.is_environment() {
-            let mut names = Vec::new();
-            unsafe {
-                let hashtab = new_owned(HASHTAB(self.get()));
-                let frame = new_owned(FRAME(self.get()));
-                if let Some(as_list_iter) = hashtab.as_list_iter() {
-                    for frame in as_list_iter {
-                        if let Some(tag_iter) = frame.as_pairlist_tag_iter() {
-                            for tag in tag_iter {
-                                if tag.is_some() {
-                                    names.push(tag.unwrap().to_string());
-                                }
-                            }
-                        }
-                    }
-                } else if let Some(tag_iter) = frame.as_pairlist_tag_iter() {
-                    for tag in tag_iter {
-                        if tag.is_some() {
-                            names.push(tag.unwrap().to_string());
-                        }
-                    }
-                }
-            }
-            return Some(names);
-        }
-        None
+    pub fn ls(&self) -> Option<Vec<&str>> {
+        self.as_env_iter().map(
+            |iter| iter.map(|(k, _)| k).collect::<Vec<_>>()
+        )
     }
 }
 
@@ -1091,7 +1068,7 @@ impl PartialEq<Robj> for Robj {
                         .unwrap()
                         .eq(rhs.as_pairlist_iter().unwrap()),
                     CLOSXP => false,
-                    ENVSXP => self.as_environment() == rhs.as_environment(),
+                    ENVSXP => false, // objects must match.
                     PROMSXP => false,
                     SPECIALSXP => false,
                     BUILTINSXP => false,
diff --git a/extendr-api/src/thread_safety.rs b/extendr-api/src/thread_safety.rs
index ecfce2758..79e7973ba 100644
--- a/extendr-api/src/thread_safety.rs
+++ b/extendr-api/src/thread_safety.rs
@@ -1,29 +1,18 @@
 //! Provide limited protection for multithreaded access to the R API.
 
 use crate::*;
-
-use std::cell::RefCell;
 use std::sync::atomic::{AtomicU32, Ordering};
 
 static OWNER_THREAD: AtomicU32 = AtomicU32::new(0);
 static NEXT_THREAD_ID: AtomicU32 = AtomicU32::new(1);
 
 thread_local! {
-    static THREAD_ID: RefCell<u32> = RefCell::new(0);
+    static THREAD_ID: u32 = NEXT_THREAD_ID.fetch_add(1, Ordering::SeqCst);
 }
 
 // Get an integer 1.. for each thread that calls this.
-fn this_thread_id() -> u32 {
-    THREAD_ID.with(|f| {
-        let mut id = *f.borrow();
-        if id == 0 {
-            // Initialise with next value.
-            let mut ptr = f.borrow_mut();
-            id = NEXT_THREAD_ID.fetch_add(1, Ordering::SeqCst);
-            *ptr = id;
-        }
-        id
-    })
+pub fn this_thread_id() -> u32 {
+    THREAD_ID.with(|&v| v)
 }
 
 /// Run a function single threaded.
@@ -57,13 +46,7 @@ where
     let id = this_thread_id();
     let old_id = OWNER_THREAD.load(Ordering::Acquire);
 
-    if old_id == id {
-        // println!("{:?} id={} RECURSIVE", std::thread::current().name(), id);
-        // recursive call, don't re-lock.
-        f()
-    } else {
-        // println!("{:?} id={}", std::thread::current().name(), id);
-
+    if old_id != id {
         // wait for OWNER_THREAD to become 0 and put us as the owner.
         while OWNER_THREAD
             .compare_exchange(0, id, Ordering::Acquire, Ordering::Relaxed)
@@ -71,17 +54,16 @@ where
         {
             std::thread::sleep(std::time::Duration::from_millis(1));
         }
+    }
 
-        // println!("{:?} id={} lock", std::thread::current().name(), id);
-
-        let res = f();
-
-        // println!("{:?} id={} unlock", std::thread::current().name(), id);
+    let res = f();
 
-        // release the lock.
+    if old_id != id {
+        // release the lock and signal waiting threads.
         OWNER_THREAD.store(0, Ordering::Release);
-        res
     }
+
+    res
 }
 
 /// This function is used by the wrapper logic to catch
@@ -106,7 +88,11 @@ where
 
 pub fn throw_r_error<S: AsRef<str>>(s: S) {
     let s = s.as_ref();
-    unsafe { libR_sys::Rf_error(std::ffi::CString::new(s).unwrap().as_ptr()) };
+    unsafe {
+        let cstring = std::ffi::CString::new(s).unwrap();
+        libR_sys::Rf_error(cstring.as_ptr());
+        unreachable!("Rf_error does not return");
+    };
 }
 
 /// Wrap an R function such as Rf_findFunction and convert errors and panics into results.
diff --git a/extendr-api/src/wrapper.rs b/extendr-api/src/wrapper.rs
index fd2aca94e..133798a0b 100644
--- a/extendr-api/src/wrapper.rs
+++ b/extendr-api/src/wrapper.rs
@@ -5,7 +5,6 @@ use crate::*;
 #[doc(hidden)]
 use libR_sys::*;
 #[doc(hidden)]
-use std::collections::HashMap;
 
 /// Wrapper for creating symbols.
 ///
@@ -56,7 +55,6 @@ pub struct Raw<'a>(pub &'a [u8]);
 /// let call_to_xyz = r!(Lang(&[r!(Symbol("xyz")), r!(1), r!(2)]));
 /// assert_eq!(call_to_xyz.is_language(), true);
 /// assert_eq!(call_to_xyz.len(), 3);
-/// assert_eq!(call_to_xyz.as_lang(), Some(Lang(vec![r!(Symbol("xyz")), r!(1), r!(2)])));
 /// ```
 ///
 /// Note: You can use the [lang!] macro for this.
@@ -66,12 +64,11 @@ pub struct Lang<T>(pub T);
 /// Wrapper for creating pair list (LISTSXP) objects.
 /// ```
 /// use extendr_api::*;
-/// extendr_engine::start_r();
-/// let hashmap : std::collections::HashMap<_, _> = (0..100)
-///     .map(|i| (Some(format!("n{}", i)), r!(i))).collect();
-/// let pairlist = Pairlist{names_and_values: hashmap};
-/// let expr = r!(pairlist);
-/// assert_eq!(expr.len(), 100);
+/// test! {
+///     let names_and_values = (0..100).map(|i| (format!("n{}", i), i));
+///     let expr = r!(Pairlist{names_and_values});
+///     assert_eq!(expr.len(), 100);
+/// }
 /// ```
 #[derive(Debug, PartialEq, Clone)]
 pub struct Pairlist<NV> {
@@ -84,7 +81,7 @@ pub struct Pairlist<NV> {
 /// extendr_engine::start_r();
 /// let list = r!(List(&[r!(0), r!(1), r!(2)]));
 /// assert_eq!(list.is_list(), true);
-/// assert_eq!(list.as_list(), Some(List(vec![r!(0), r!(1), r!(2)])));
+/// assert_eq!(list.len(), 3);
 /// assert_eq!(format!("{:?}", list), r#"r!(List([r!(0), r!(1), r!(2)]))"#);
 /// ```
 ///
@@ -117,7 +114,7 @@ pub struct Env<P, NV> {
 /// let func = expr.as_func().unwrap();
 ///
 /// let expected_formals = Pairlist {
-///     names_and_values: vec![(Some("a"), r!(1.0)), (Some("b"), missing_arg())] };
+///     names_and_values: vec![("a", r!(1.0)), ("b", missing_arg())] };
 /// let expected_body = lang!(
 ///     "{", lang!("<-", sym!(c), lang!("+", sym!(a), sym!(b))));
 /// assert_eq!(func.formals.as_pairlist().unwrap(), expected_formals);
@@ -258,17 +255,17 @@ impl<'a, P, NV> From<Env<P, NV>> for Robj
 where
     P: Into<Robj>,
     NV: IntoIterator + 'a,
-    NV::Item: Into<(String, Robj)>,
+    NV::Item: SymPair,
 {
     /// Convert a wrapper to an R environment object.
     /// ```
     /// use extendr_api::*;
-    /// extendr_engine::start_r();
-    /// let hashmap : std::collections::HashMap<_, _> = (0..100)
-    ///     .map(|i| (format!("n{}", i), r!(i))).collect();
-    /// let env = Env{parent: global_env(), names_and_values: hashmap};
-    /// let expr = r!(env);
-    /// assert_eq!(expr.len(), 100);
+    /// test! {
+    ///     let names_and_values = (0..100).map(|i| (format!("n{}", i), i));
+    ///     let env = Env{parent: global_env(), names_and_values};
+    ///     let expr = r!(env);
+    ///     assert_eq!(expr.len(), 100);
+    /// }
     /// ```
     fn from(val: Env<P, NV>) -> Self {
         single_threaded(|| {
@@ -276,8 +273,8 @@ where
             let dict_len = 29;
             let res = call!("new.env", TRUE, parent.into(), dict_len).unwrap();
             for nv in names_and_values {
-                let (n, v) = nv.into();
-                unsafe { Rf_defineVar(r!(Symbol(n.as_str())).get(), v.get(), res.get()) }
+                let (n, v) = nv.sym_pair();
+                unsafe { Rf_defineVar(n.get(), v.get(), res.get()) }
             }
             res
         })
@@ -287,17 +284,16 @@ where
 impl<'a, NV> From<Pairlist<NV>> for Robj
 where
     NV: IntoIterator + 'a,
-    NV::Item: Into<(Option<String>, Robj)>,
+    NV::Item: SymPair,
 {
     /// Convert a wrapper to a LISTSXP object.
     /// ```
     /// use extendr_api::*;
-    /// extendr_engine::start_r();
-    /// let hashmap : std::collections::HashMap<_, _> = (0..100)
-    ///     .map(|i| (Some(format!("n{}", i)), r!(i))).collect();
-    /// let pairlist = Pairlist{names_and_values: hashmap};
-    /// let expr = r!(pairlist);
-    /// assert_eq!(expr.len(), 100);
+    /// test! {
+    ///     let names_and_values = (0..100).map(|i| (format!("n{}", i), i));
+    ///     let expr = r!(Pairlist{names_and_values});
+    ///     assert_eq!(expr.len(), 100);
+    /// }
     /// ```
     fn from(val: Pairlist<NV>) -> Self {
         single_threaded(|| unsafe {
@@ -306,13 +302,12 @@ where
             let mut res = R_NilValue;
             let names_and_values: Vec<_> = names_and_values.into_iter().collect();
             for nv in names_and_values.into_iter().rev() {
-                let (name, val) = nv.into();
+                let (name, val) = nv.sym_pair();
                 let val = Rf_protect(val.get());
                 res = Rf_protect(Rf_cons(val, res));
                 num_protects += 2;
-                if let Some(name) = name {
-                    let name = r!(Symbol(name.as_str())).get();
-                    SET_TAG(res, name);
+                if !name.is_na() {
+                    SET_TAG(res, name.get());
                 }
             }
             let res = new_owned(res);
@@ -370,9 +365,8 @@ impl Robj {
                         to_str(R_CHAR(printname) as *const u8)
                     ))
                 } else {
-                    Some(Symbol(
-                        "bad symbol"
-                    ))
+                    // This should never occur.
+                    None
                 }
             }
         } else {
@@ -419,17 +413,11 @@ impl Robj {
     /// let call_to_xyz = r!(Lang(&[r!(Symbol("xyz")), r!(1), r!(2)]));
     /// assert_eq!(call_to_xyz.is_language(), true);
     /// assert_eq!(call_to_xyz.len(), 3);
-    /// assert_eq!(call_to_xyz.as_lang(), Some(Lang(vec![r!(Symbol("xyz")), r!(1), r!(2)])));
     /// assert_eq!(format!("{:?}", call_to_xyz), r#"r!(Lang([sym!(xyz), r!(1), r!(2)]))"#);
     /// ```
-    pub fn as_lang(&self) -> Option<Lang<Vec<Robj>>> {
+    pub fn as_lang(&self) -> Option<Lang<PairlistIter>> {
         if self.sexptype() == LANGSXP {
-            let res: Vec<_> = self
-                .as_pairlist_iter()
-                .unwrap()
-                .map(|robj| robj.to_owned())
-                .collect();
-            Some(Lang(res))
+            Some(Lang(self.as_pairlist_iter().unwrap()))
         } else {
             None
         }
@@ -439,14 +427,12 @@ impl Robj {
     /// ```
     /// use extendr_api::*;
     /// extendr_engine::start_r();
-    /// let names_and_values = vec![(Some("a".to_string()), r!(1)), (Some("b".to_string()), r!(2)), (None, r!(3))];
-    /// let pairlist1 = Pairlist{ names_and_values };
-    /// let names_and_values = vec![(Some("a"), r!(1)), (Some("b"), r!(2)), (None, r!(3))];
-    /// let pairlist2 = Pairlist{ names_and_values };
-    /// let robj = r!(pairlist1);
-    /// assert_eq!(robj.as_pairlist().unwrap(), pairlist2);
+    /// let names_and_values = vec![("a", r!(1)), ("b", r!(2)), (na_str(), r!(3))];
+    /// let pairlist = Pairlist{ names_and_values };
+    /// let robj = r!(pairlist.clone());
+    /// assert_eq!(robj.as_pairlist().unwrap(), pairlist);
     /// ```
-    pub fn as_pairlist(&self) -> Option<Pairlist<Vec<(Option<&str>, Robj)>>> {
+    pub fn as_pairlist(&self) -> Option<Pairlist<Vec<(&str, Robj)>>> {
         if self.sexptype() == LISTSXP {
             let names = self.as_pairlist_tag_iter().unwrap();
             let values = self.as_pairlist_iter().unwrap();
@@ -463,20 +449,10 @@ impl Robj {
     /// extendr_engine::start_r();
     /// let list = r!(List(&[r!(0), r!(1), r!(2)]));
     /// assert_eq!(list.is_list(), true);
-    /// assert_eq!(list.as_list(), Some(List(vec![r!(0), r!(1), r!(2)])));
     /// assert_eq!(format!("{:?}", list), r#"r!(List([r!(0), r!(1), r!(2)]))"#);
     /// ```
-    pub fn as_list(&self) -> Option<List<Vec<Robj>>> {
-        if self.sexptype() == VECSXP {
-            let res: Vec<_> = self
-                .as_list_iter()
-                .unwrap()
-                .map(|robj| robj.to_owned())
-                .collect();
-            Some(List(res))
-        } else {
-            None
-        }
+    pub fn as_list(&self) -> Option<List<ListIter>> {
+        self.as_list_iter().map(|l| List(l))
     }
 
     /// Convert an expression object (EXPRSXP) to a Expr wrapper.
@@ -504,51 +480,23 @@ impl Robj {
     /// Convert an environment object (ENVSXP) to a Env wrapper.
     /// ```
     /// use extendr_api::*;
-    /// extendr_engine::start_r();
-    /// let names_and_values : std::collections::HashMap<_, _> = (0..100).map(|i| (format!("n{}", i), r!(i))).collect();
-    /// let env = Env{parent: global_env(), names_and_values};
-    /// let expr = r!(env.clone());
-    /// assert_eq!(expr.len(), 100);
-    /// let env2 = expr.as_environment().unwrap();
-    /// assert_eq!(env2.names_and_values.len(), 100);
+    /// test! {
+    ///     let names_and_values = (0..100).map(|i| (format!("n{}", i), r!(i)));
+    ///     let env = Env{parent: global_env(), names_and_values};
+    ///     let expr = r!(env.clone());
+    ///     assert_eq!(expr.len(), 100);
+    ///     let env2 = expr.as_environment().unwrap();
+    ///     assert_eq!(env2.names_and_values.count(), 100);
+    /// }
     /// ```
-    pub fn as_environment(&self) -> Option<Env<Robj, HashMap<&str, Robj>>> {
-        if self.sexptype() == ENVSXP {
-            unsafe {
-                let parent = new_owned(ENCLOS(self.get()));
-                let hashtab = new_owned(HASHTAB(self.get()));
-                let frame = new_owned(FRAME(self.get()));
-                let mut names_and_values = HashMap::new();
-                if let Some(as_list_iter) = hashtab.as_list_iter() {
-                    for frame in as_list_iter {
-                        if let (Some(obj_iter), Some(tag_iter)) =
-                            (frame.as_pairlist_iter(), frame.as_pairlist_tag_iter())
-                        {
-                            for (obj, tag) in obj_iter.zip(tag_iter) {
-                                if !obj.is_unbound_value() && tag.is_some() {
-                                    names_and_values.insert(tag.unwrap(), obj);
-                                }
-                            }
-                        }
-                    }
-                } else if let (Some(obj_iter), Some(tag_iter)) =
-                    (frame.as_pairlist_iter(), frame.as_pairlist_tag_iter())
-                {
-                    for (obj, tag) in obj_iter.zip(tag_iter) {
-                        if !obj.is_unbound_value() && tag.is_some() {
-                            names_and_values.insert(tag.unwrap(), obj);
-                        }
-                    }
-                }
-                Some(Env {
-                    parent,
-                    names_and_values,
-                })
-            }
+    pub fn as_environment(&self) -> Option<Env<Robj, EnvIter>> {
+        if self.is_environment() {
+            Some(Env{ parent: self.parent().unwrap(), names_and_values: self.as_env_iter().unwrap()})
         } else {
             None
         }
     }
+
     /// Convert a function object (CLOSXP) to a Func wrapper.
     /// ```
     /// use extendr_api::*;
@@ -607,3 +555,18 @@ impl Robj {
         }
     }
 }
+
+pub trait SymPair {
+    fn sym_pair(self) -> (Robj, Robj);
+}
+
+impl<S, R> SymPair for (S, R)
+where
+    S : AsRef<str>,
+    R : Into<Robj>,
+{
+    fn sym_pair(self) -> (Robj, Robj) {
+        (r!(Symbol(self.0.as_ref())), self.1.into())
+    }
+}
+


Test patch:
diff --git a/extendr-api/src/robj/tests.rs b/extendr-api/src/robj/tests.rs
index 052d9826b..322829869 100644
--- a/extendr-api/src/robj/tests.rs
+++ b/extendr-api/src/robj/tests.rs
@@ -245,17 +245,17 @@ fn input_iterator_test() {
     let src: &[i32] = &[1, 2, 3];
     let robj = Robj::from(src);
     let iter = <IntegerIter>::from_robj(&robj).unwrap();
-    assert_eq!(iter.cloned().collect::<Vec<_>>(), src);
+    assert_eq!(iter.collect::<Vec<_>>(), src);
 
     let src: &[f64] = &[1., 2., 3.];
     let robj = Robj::from(src);
     let iter = <RealIter>::from_robj(&robj).unwrap();
-    assert_eq!(iter.cloned().collect::<Vec<_>>(), src);
+    assert_eq!(iter.collect::<Vec<_>>(), src);
 
     /*
     let src: &[Bool] = &[TRUE, FALSE, TRUE];
     let robj = Robj::from(src);
     let iter = <LogicalIter>::from_robj(&robj).unwrap();
-    assert_eq!(iter.cloned().collect::<Vec<_>>(), src);
+    assert_eq!(iter.collect::<Vec<_>>(), src);
     */
 }