Repository: extendr/extendr
Pull request #287 (2021-09-16 20:40:58)

Reduce robj to a single pointer
This PR make Robj (and most wrappers like Rstr) the same layout as a SEXP.

This will enable read-only slices of Robs for lists and slices of Rstr for strings.

There is a small extra overhead as system objects have to be reference counted
by `ownership::protect()` but the ability to reference list may counteract this.

We may want to check system objects in `ownership`, especially `RNilValue`.

yutannihilation: I'll take a look in a few days, though I'm not confident I can review well...

A quick comment. I think `extendr-api/src/graphics/mod.rs` will conflict with #279. In what order do you think we should merge?
clauswilke: Same here. I think it would be great if @Ilia-Kosenkov could review.
andy-thomason: This is in two parts (with two commits).

The first changes the enum to a struct.

The second renames `new_owned` and `new_sys` to `Robj::from_sexp`.

The next PR will include `to_typed_slice` for `List` and `Strings`.
Ilia-Kosenkov: Will review in the next couple of hours. 
andy-thomason: I have left `new_owned` in for the while as it is used by #279 and the function wrappers.

Without the `s/new_owned/Robj::from_sexp/` change, there are relatively few differences.

By protecting more objects, we may have helped with the S4 tests. It is difficult to say. I have also
folded `Rf_AllocateVector` into a `single_threaded` block by using the `Robj::allocVector` call.

yutannihilation: @andy-thomason 
Sorry for reviewing late, I think now I get the point of this pull request. Looks good.

One thing, what I wanted to ask you by this comment was if you accidentally commit `extendr-api/src/graphics/mod.rs` here, which should exist after #279 gets merged. Is it intentionally included here? If so, it's fine.

> A quick comment. I think `extendr-api/src/graphics/mod.rs` will conflict with #279. In what order do you think we should merge?


andy-thomason: It should not now contain changes to `graphics/mod.rs` but required ninja-like rebase skills.

Patch:
diff --git a/extendr-api/src/functions.rs b/extendr-api/src/functions.rs
index e1d8733cf..449d979cc 100644
--- a/extendr-api/src/functions.rs
+++ b/extendr-api/src/functions.rs
@@ -83,7 +83,7 @@ pub fn find_namespace<K: Into<Robj>>(key: K) -> Result<Environment> {
 /// }
 /// ```
 pub fn current_env() -> Environment {
-    unsafe { new_owned(R_GetCurrentEnv()).try_into().unwrap() }
+    unsafe { Robj::from_sexp(R_GetCurrentEnv()).try_into().unwrap() }
 }
 
 /// The "global" environment
@@ -96,12 +96,12 @@ pub fn current_env() -> Environment {
 /// }
 /// ```
 pub fn global_env() -> Environment {
-    unsafe { new_sys(R_GlobalEnv).try_into().unwrap() }
+    unsafe { Robj::from_sexp(R_GlobalEnv).try_into().unwrap() }
 }
 
 /// An empty environment at the root of the environment tree
 pub fn empty_env() -> Environment {
-    unsafe { new_sys(R_EmptyEnv).try_into().unwrap() }
+    unsafe { Robj::from_sexp(R_EmptyEnv).try_into().unwrap() }
 }
 
 /// Create a new environment
@@ -118,7 +118,7 @@ pub fn empty_env() -> Environment {
 pub fn new_env(parent: Environment, hash: bool, capacity: i32) -> Environment {
     unsafe {
         let env = R_NewEnv(parent.robj.get(), hash as i32, capacity);
-        new_sys(env).try_into().unwrap()
+        Robj::from_sexp(env).try_into().unwrap()
     }
 }
 
@@ -141,7 +141,7 @@ pub fn new_env(parent: Environment, hash: bool, capacity: i32) -> Environment {
 /// }
 /// ```
 pub fn base_env() -> Environment {
-    unsafe { new_sys(R_BaseEnv).try_into().unwrap() }
+    unsafe { Robj::from_sexp(R_BaseEnv).try_into().unwrap() }
 }
 
 /// The namespace for base.
@@ -153,7 +153,7 @@ pub fn base_env() -> Environment {
 /// }
 /// ```
 pub fn base_namespace() -> Environment {
-    unsafe { new_sys(R_BaseNamespace).try_into().unwrap() }
+    unsafe { Robj::from_sexp(R_BaseNamespace).try_into().unwrap() }
 }
 
 /// For registered namespaces.
@@ -165,37 +165,37 @@ pub fn base_namespace() -> Environment {
 /// }
 /// ```
 pub fn namespace_registry() -> Environment {
-    unsafe { new_sys(R_NamespaceRegistry).try_into().unwrap() }
+    unsafe { Robj::from_sexp(R_NamespaceRegistry).try_into().unwrap() }
 }
 
 /// Current srcref, for debuggers
 pub fn srcref() -> Robj {
-    unsafe { new_sys(R_Srcref) }
+    unsafe { Robj::from_sexp(R_Srcref) }
 }
 
 /// The nil object
 pub fn nil_value() -> Robj {
-    unsafe { new_sys(R_NilValue) }
+    unsafe { Robj::from_sexp(R_NilValue) }
 }
 
 /* fix version issues.
 /// ".Generic"
-pub fn dot_Generic() -> Robj { unsafe { new_sys(R_dot_Generic) }}
+pub fn dot_Generic() -> Robj { unsafe { Robj::from_sexp(R_dot_Generic) }}
 */
 
 /// NA_STRING as a CHARSXP
 pub fn na_string() -> Robj {
-    unsafe { new_sys(R_NaString) }
+    unsafe { Robj::from_sexp(R_NaString) }
 }
 
 /// "" as a CHARSXP
 pub fn blank_string() -> Robj {
-    unsafe { new_sys(R_BlankString) }
+    unsafe { Robj::from_sexp(R_BlankString) }
 }
 
 /// "" as a STRSXP
 pub fn blank_scalar_string() -> Robj {
-    unsafe { new_sys(R_BlankScalarString) }
+    unsafe { Robj::from_sexp(R_BlankScalarString) }
 }
 
 /// Special "NA" string that represents null strings.
@@ -226,7 +226,7 @@ pub fn parse(code: &str) -> Result<Robj> {
         let mut status = 0_u32;
         let status_ptr = &mut status as *mut u32;
         let codeobj: Robj = code.into();
-        let parsed = new_owned(R_ParseVector(codeobj.get(), -1, status_ptr, R_NilValue));
+        let parsed = Robj::from_sexp(R_ParseVector(codeobj.get(), -1, status_ptr, R_NilValue));
         match status {
             1 => Ok(parsed),
             _ => Err(Error::ParseError(code.into())),
diff --git a/extendr-api/src/lang_macros.rs b/extendr-api/src/lang_macros.rs
index 30595d107..3c69da6c7 100644
--- a/extendr-api/src/lang_macros.rs
+++ b/extendr-api/src/lang_macros.rs
@@ -4,7 +4,7 @@
 use libR_sys::*;
 //use crate::robj::*;
 use crate::robj::Robj;
-use crate::{new_owned, single_threaded};
+use crate::single_threaded;
 
 /// Convert a list of tokens to an array of tuples.
 #[doc(hidden)]
@@ -69,7 +69,7 @@ pub unsafe fn make_lang(sym: &str) -> Robj {
     name.push(0);
     let sexp =
         single_threaded(|| Rf_lang1(Rf_install(name.as_ptr() as *const std::os::raw::c_char)));
-    new_owned(sexp)
+    Robj::from_sexp(sexp)
 }
 
 /// Convert a list of tokens to an array of tuples.
diff --git a/extendr-api/src/lib.rs b/extendr-api/src/lib.rs
index 6d49346fe..fcb431918 100644
--- a/extendr-api/src/lib.rs
+++ b/extendr-api/src/lib.rs
@@ -245,6 +245,8 @@ pub use std::convert::{TryFrom, TryInto};
 pub use std::ops::Deref;
 pub use std::ops::DerefMut;
 
+pub use robj::Robj;
+
 //////////////////////////////////////////////////
 // Note these pub use statements are deprectaed
 //
@@ -725,18 +727,18 @@ mod tests {
                 wrap__robjtype(Robj::from(1).get());
 
                 // General integer types.
-                assert_eq!(new_owned(wrap__return_u8()), Robj::from(123_u8));
-                assert_eq!(new_owned(wrap__return_u16()), Robj::from(123));
-                assert_eq!(new_owned(wrap__return_u32()), Robj::from(123.));
-                assert_eq!(new_owned(wrap__return_u64()), Robj::from(123.));
-                assert_eq!(new_owned(wrap__return_i8()), Robj::from(123));
-                assert_eq!(new_owned(wrap__return_i16()), Robj::from(123));
-                assert_eq!(new_owned(wrap__return_i32()), Robj::from(123));
-                assert_eq!(new_owned(wrap__return_i64()), Robj::from(123.));
+                assert_eq!(Robj::from_sexp(wrap__return_u8()), Robj::from(123_u8));
+                assert_eq!(Robj::from_sexp(wrap__return_u16()), Robj::from(123));
+                assert_eq!(Robj::from_sexp(wrap__return_u32()), Robj::from(123.));
+                assert_eq!(Robj::from_sexp(wrap__return_u64()), Robj::from(123.));
+                assert_eq!(Robj::from_sexp(wrap__return_i8()), Robj::from(123));
+                assert_eq!(Robj::from_sexp(wrap__return_i16()), Robj::from(123));
+                assert_eq!(Robj::from_sexp(wrap__return_i32()), Robj::from(123));
+                assert_eq!(Robj::from_sexp(wrap__return_i64()), Robj::from(123.));
 
                 // Floating point types.
-                assert_eq!(new_owned(wrap__return_f32()), Robj::from(123.));
-                assert_eq!(new_owned(wrap__return_f64()), Robj::from(123.));
+                assert_eq!(Robj::from_sexp(wrap__return_f32()), Robj::from(123.));
+                assert_eq!(Robj::from_sexp(wrap__return_f64()), Robj::from(123.));
             }
         }
     }
@@ -761,56 +763,56 @@ mod tests {
                 // pub fn f64_slice(x: &[f64]) -> &[f64] { x }
 
                 let robj = r!([1., 2., 3.]);
-                assert_eq!(new_owned(wrap__f64_slice(robj.get())), robj);
+                assert_eq!(Robj::from_sexp(wrap__f64_slice(robj.get())), robj);
 
                 // #[extendr]
                 // pub fn i32_slice(x: &[i32]) -> &[i32] { x }
 
                 let robj = r!([1, 2, 3]);
-                assert_eq!(new_owned(wrap__i32_slice(robj.get())), robj);
+                assert_eq!(Robj::from_sexp(wrap__i32_slice(robj.get())), robj);
 
                 // #[extendr]
                 // pub fn bool_slice(x: &[Bool]) -> &[Bool] { x }
 
                 let robj = r!([TRUE, FALSE, TRUE]);
-                assert_eq!(new_owned(wrap__bool_slice(robj.get())), robj);
+                assert_eq!(Robj::from_sexp(wrap__bool_slice(robj.get())), robj);
 
                 // #[extendr]
                 // pub fn f64_iter(x: Real) -> Real { x }
 
                 let robj = r!([1., 2., 3.]);
-                assert_eq!(new_owned(wrap__f64_iter(robj.get())), robj);
+                assert_eq!(Robj::from_sexp(wrap__f64_iter(robj.get())), robj);
 
                 // #[extendr]
                 // pub fn i32_iter(x: Int) -> Int { x }
 
                 let robj = r!([1, 2, 3]);
-                assert_eq!(new_owned(wrap__i32_iter(robj.get())), robj);
+                assert_eq!(Robj::from_sexp(wrap__i32_iter(robj.get())), robj);
 
                 // #[extendr]
                 // pub fn bool_iter(x: Logical) -> Logical { x }
 
                 let robj = r!([TRUE, FALSE, TRUE]);
-                assert_eq!(new_owned(wrap__bool_iter(robj.get())), robj);
+                assert_eq!(Robj::from_sexp(wrap__bool_iter(robj.get())), robj);
 
                 // #[extendr]
                 // pub fn symbol(x: Symbol) -> Symbol { x }
 
                 let robj = sym!(fred);
-                assert_eq!(new_owned(wrap__symbol(robj.get())), robj);
+                assert_eq!(Robj::from_sexp(wrap__symbol(robj.get())), robj);
 
                 // #[extendr]
                 // pub fn matrix(x: Matrix<&[f64]>) -> Matrix<&[f64]> { x }
 
                 let m = RMatrix::new_matrix(1, 2, |r, c| if r == c {1.0} else {0.});
                 let robj = r!(m);
-                assert_eq!(new_owned(wrap__matrix(robj.get())), robj);
+                assert_eq!(Robj::from_sexp(wrap__matrix(robj.get())), robj);
 
                 // #[extendr]
                 // pub fn hash_map(x: HashMap<&str, Robj>) -> HashMap<&str, Robj> { x }
                 let robj = r!(List::from_values(&[1, 2]));
                 robj.set_attrib(names_symbol(), r!(["a", "b"]))?;
-                let res = new_owned(wrap__hash_map(robj.get()));
+                let res = Robj::from_sexp(wrap__hash_map(robj.get()));
                 assert_eq!(res.len(), 2);
             }
         }
@@ -859,7 +861,7 @@ mod tests {
             assert_eq!(metadata.impls[0].methods.len(), 3);
 
             // R interface
-            let robj = unsafe { new_owned(wrap__get_my_module_metadata()) };
+            let robj = Robj::from_sexp(wrap__get_my_module_metadata());
             let functions = robj.dollar("functions").unwrap();
             let impls = robj.dollar("impls").unwrap();
             assert_eq!(functions.len(), 3);
diff --git a/extendr-api/src/prelude.rs b/extendr-api/src/prelude.rs
index 974eede01..68a5ae613 100644
--- a/extendr-api/src/prelude.rs
+++ b/extendr-api/src/prelude.rs
@@ -4,8 +4,8 @@
 //! using deprecated features.
 
 pub use super::{
-    new_owned, print_r_error, print_r_output, FromRobj, IsNA, RType, FALSE, NA_INTEGER, NA_LOGICAL,
-    NA_REAL, NA_STRING, NULL, TRUE,
+    print_r_error, print_r_output, FromRobj, IsNA, RType, FALSE, NA_INTEGER, NA_LOGICAL, NA_REAL,
+    NA_STRING, NULL, TRUE,
 };
 
 pub use super::error::{Error, Result};
diff --git a/extendr-api/src/robj/from_robj.rs b/extendr-api/src/robj/from_robj.rs
index 882e90e44..3bdec87ed 100644
--- a/extendr-api/src/robj/from_robj.rs
+++ b/extendr-api/src/robj/from_robj.rs
@@ -159,7 +159,7 @@ impl_iter_from_robj!(Logical, as_logical_iter, "Not a logical vector.");
 /// Pass-through Robj conversion, essentially a clone.
 impl<'a> FromRobj<'a> for Robj {
     fn from_robj(robj: &'a Robj) -> std::result::Result<Self, &'static str> {
-        Ok(unsafe { new_owned(robj.get()) })
+        Ok(unsafe { Robj::from_sexp(robj.get()) })
     }
 }
 
diff --git a/extendr-api/src/robj/into_robj.rs b/extendr-api/src/robj/into_robj.rs
index f7cbf4267..6baa7d65b 100644
--- a/extendr-api/src/robj/into_robj.rs
+++ b/extendr-api/src/robj/into_robj.rs
@@ -16,7 +16,7 @@ pub(crate) fn str_to_character(s: &str) -> SEXP {
 impl From<()> for Robj {
     fn from(_: ()) -> Self {
         // Note: we do not need to protect this.
-        unsafe { Robj::Sys(R_NilValue) }
+        unsafe { Robj::from_sexp(R_NilValue) }
     }
 }
 
@@ -49,7 +49,7 @@ impl From<&Robj> for Robj {
     // Note: we should probably have a much better reference
     // mechanism as double-free or underprotection is a distinct possibility.
     fn from(val: &Robj) -> Self {
-        unsafe { new_owned(val.get()) }
+        unsafe { Robj::from_sexp(val.get()) }
     }
 }
 
@@ -348,8 +348,8 @@ where
         // Length of the vector is known in advance.
         let sexptype = I::Item::sexptype();
         if sexptype != 0 {
-            let sexp = Rf_allocVector(sexptype, len as R_xlen_t);
-            ownership::protect(sexp);
+            let res = Robj::alloc_vector(sexptype, len);
+            let sexp = res.get();
             match sexptype {
                 REALSXP => {
                     let ptr = REAL(sexp);
@@ -384,7 +384,7 @@ where
                     panic!("unexpected SEXPTYPE in collect_robj");
                 }
             }
-            Robj::Owned(sexp)
+            res
         } else {
             Robj::from(())
         }
diff --git a/extendr-api/src/robj/mod.rs b/extendr-api/src/robj/mod.rs
index 6e80c684a..9b8a5c631 100644
--- a/extendr-api/src/robj/mod.rs
+++ b/extendr-api/src/robj/mod.rs
@@ -117,24 +117,13 @@ pub use rinternals::*;
 /// panics and segfaults. We will take great trouble to ensure that this
 /// is true.
 ///
-pub enum Robj {
-    // This object owns the SEXP and must free it.
-    #[doc(hidden)]
-    Owned(SEXP),
-
-    //  This object references a SEXP owned by libR.
-    #[doc(hidden)]
-    Sys(SEXP),
+pub struct Robj {
+    inner: SEXP,
 }
 
 impl Clone for Robj {
     fn clone(&self) -> Self {
-        unsafe {
-            match *self {
-                Robj::Owned(sexp) => new_owned(sexp),
-                Robj::Sys(sexp) => new_sys(sexp),
-            }
-        }
+        unsafe { Robj::from_sexp(self.get()) }
     }
 }
 
@@ -145,14 +134,18 @@ impl Default for Robj {
 }
 
 impl Robj {
+    pub fn from_sexp(sexp: SEXP) -> Self {
+        single_threaded(|| {
+            unsafe { ownership::protect(sexp) };
+            Robj { inner: sexp }
+        })
+    }
+
     /// Get a copy of the underlying SEXP.
     /// Note: this is unsafe.
     #[doc(hidden)]
     pub unsafe fn get(&self) -> SEXP {
-        match self {
-            Robj::Owned(sexp) => *sexp,
-            Robj::Sys(sexp) => *sexp,
-        }
+        self.inner
     }
 
     #[doc(hidden)]
@@ -630,7 +623,7 @@ impl Robj {
             if error != 0 {
                 Err(Error::EvalError(self.clone()))
             } else {
-                Ok(new_owned(res))
+                Ok(Robj::from_sexp(res))
             }
         })
     }
@@ -735,7 +728,7 @@ impl Robj {
         if self.sexptype() == CHARSXP {
             None
         } else {
-            let res = unsafe { new_owned(Rf_getAttrib(self.get(), name.get())) };
+            let res = unsafe { Robj::from_sexp(Rf_getAttrib(self.get(), name.get())) };
             if res.is_null() {
                 None
             } else {
@@ -925,13 +918,10 @@ impl Robj {
 
 #[doc(hidden)]
 pub unsafe fn new_owned(sexp: SEXP) -> Robj {
-    single_threaded(|| ownership::protect(sexp));
-    Robj::Owned(sexp)
-}
-
-#[doc(hidden)]
-pub unsafe fn new_sys(sexp: SEXP) -> Robj {
-    Robj::Sys(sexp)
+    single_threaded(|| {
+        ownership::protect(sexp);
+        Robj { inner: sexp }
+    })
 }
 
 /// Compare equality with integer slices.
@@ -1125,10 +1115,7 @@ pub(crate) unsafe fn to_str<'a>(ptr: *const u8) -> &'a str {
 impl Drop for Robj {
     fn drop(&mut self) {
         unsafe {
-            match self {
-                Robj::Owned(sexp) => ownership::unprotect(*sexp),
-                Robj::Sys(_) => (),
-            }
+            ownership::unprotect(self.inner);
         }
     }
 }
diff --git a/extendr-api/src/robj/operators.rs b/extendr-api/src/robj/operators.rs
index 5d0660249..568637460 100644
--- a/extendr-api/src/robj/operators.rs
+++ b/extendr-api/src/robj/operators.rs
@@ -102,7 +102,7 @@ impl Robj {
     pub fn call(&self, args: Pairlist) -> Result<Robj> {
         if self.is_function() {
             unsafe {
-                let call = new_owned(Rf_lcons(self.get(), args.get()));
+                let call = Robj::from_sexp(Rf_lcons(self.get(), args.get()));
                 call.eval()
             }
         } else {
diff --git a/extendr-api/src/robj/rinternals.rs b/extendr-api/src/robj/rinternals.rs
index 572b5b55a..2cb72e92d 100644
--- a/extendr-api/src/robj/rinternals.rs
+++ b/extendr-api/src/robj/rinternals.rs
@@ -67,48 +67,50 @@ impl Robj {
 
     /// Get the source ref.
     pub fn get_current_srcref(val: i32) -> Robj {
-        unsafe { new_owned(R_GetCurrentSrcref(val as raw::c_int)) }
+        unsafe { Robj::from_sexp(R_GetCurrentSrcref(val as raw::c_int)) }
     }
 
     /// Get the source filename.
     pub fn get_src_filename(&self) -> Robj {
-        unsafe { new_owned(R_GetSrcFilename(self.get())) }
+        unsafe { Robj::from_sexp(R_GetSrcFilename(self.get())) }
     }
 
     /// Convert to a string vector.
     pub fn as_character_vector(&self) -> Robj {
-        unsafe { new_owned(Rf_asChar(self.get())) }
+        unsafe { Robj::from_sexp(Rf_asChar(self.get())) }
     }
 
     /// Convert to vectors of many kinds.
     pub fn coerce_vector(&self, sexptype: u32) -> Robj {
-        single_threaded(|| unsafe { new_owned(Rf_coerceVector(self.get(), sexptype as SEXPTYPE)) })
+        single_threaded(|| unsafe {
+            Robj::from_sexp(Rf_coerceVector(self.get(), sexptype as SEXPTYPE))
+        })
     }
 
     /// Convert a pairlist (LISTSXP) to a vector list (VECSXP).
     pub fn pair_to_vector_list(&self) -> Robj {
-        single_threaded(|| unsafe { new_owned(Rf_PairToVectorList(self.get())) })
+        single_threaded(|| unsafe { Robj::from_sexp(Rf_PairToVectorList(self.get())) })
     }
 
     /// Convert a vector list (VECSXP) to a pair list (LISTSXP)
     pub fn vector_to_pair_list(&self) -> Robj {
-        single_threaded(|| unsafe { new_owned(Rf_VectorToPairList(self.get())) })
+        single_threaded(|| unsafe { Robj::from_sexp(Rf_VectorToPairList(self.get())) })
     }
 
     /// Convert a factor to a string vector.
     pub fn as_character_factor(&self) -> Robj {
-        single_threaded(|| unsafe { new_owned(Rf_asCharacterFactor(self.get())) })
+        single_threaded(|| unsafe { Robj::from_sexp(Rf_asCharacterFactor(self.get())) })
     }
 
     /// Allocate a matrix object.
     pub fn alloc_matrix(sexptype: SEXPTYPE, rows: i32, cols: i32) -> Robj {
-        single_threaded(|| unsafe { new_owned(Rf_allocMatrix(sexptype, rows, cols)) })
+        single_threaded(|| unsafe { Robj::from_sexp(Rf_allocMatrix(sexptype, rows, cols)) })
     }
 
     /// Do a deep copy of this object.
     /// Note that clone() only adds a reference.
     pub fn duplicate(&self) -> Self {
-        single_threaded(|| unsafe { new_owned(Rf_duplicate(self.get())) })
+        single_threaded(|| unsafe { Robj::from_sexp(Rf_duplicate(self.get())) })
     }
 
     /// Find a function in an environment ignoring other variables.
@@ -149,7 +151,7 @@ impl Robj {
         // }
         unsafe {
             if let Ok(var) = catch_r_error(|| Rf_findFun(key.get(), self.get())) {
-                Ok(new_owned(var))
+                Ok(Robj::from_sexp(var))
             } else {
                 Err(Error::NotFound(key.into()))
             }
@@ -198,7 +200,7 @@ impl Robj {
         unsafe {
             if let Ok(var) = catch_r_error(|| Rf_findVar(key.get(), self.get())) {
                 if var != R_UnboundValue {
-                    Ok(new_owned(var))
+                    Ok(Robj::from_sexp(var))
                 } else {
                     Err(Error::NotFound(key.into()))
                 }
@@ -238,7 +240,7 @@ impl Robj {
     /// Internal function used to implement `#[extendr]` impl
     #[doc(hidden)]
     pub unsafe fn make_external_ptr<T>(p: *mut T, tag: Robj, prot: Robj) -> Self {
-        new_owned(single_threaded(|| {
+        Robj::from_sexp(single_threaded(|| {
             R_MakeExternalPtr(p as *mut ::std::os::raw::c_void, tag.get(), prot.get())
         }))
     }
@@ -252,13 +254,13 @@ impl Robj {
     /// Internal function used to implement `#[extendr]` impl
     #[doc(hidden)]
     pub unsafe fn external_ptr_tag(&self) -> Self {
-        new_owned(R_ExternalPtrTag(self.get()))
+        Robj::from_sexp(R_ExternalPtrTag(self.get()))
     }
 
     /// Internal function used to implement `#[extendr]` impl
     #[doc(hidden)]
     pub unsafe fn external_ptr_protected(&self) -> Self {
-        new_owned(R_ExternalPtrProtected(self.get()))
+        Robj::from_sexp(R_ExternalPtrProtected(self.get()))
     }
 
     #[doc(hidden)]
@@ -272,7 +274,7 @@ impl Robj {
         unsafe {
             if self.is_vector() {
                 Ok(single_threaded(|| {
-                    new_owned(Rf_xlengthgets(self.get(), new_len as R_xlen_t))
+                    Robj::from_sexp(Rf_xlengthgets(self.get(), new_len as R_xlen_t))
                 }))
             } else {
                 Err(Error::ExpectedVector(self.clone()))
@@ -282,7 +284,7 @@ impl Robj {
 
     /// Allocated an owned object of a certain type.
     pub fn alloc_vector(sexptype: u32, len: usize) -> Robj {
-        single_threaded(|| unsafe { new_owned(Rf_allocVector(sexptype, len as R_xlen_t)) })
+        single_threaded(|| unsafe { Robj::from_sexp(Rf_allocVector(sexptype, len as R_xlen_t)) })
     }
 
     /// Return true if two arrays have identical dims.
@@ -421,7 +423,7 @@ impl Robj {
     }
 
     pub fn package_env_name(&self) -> Robj {
-        unsafe { new_owned(R_PackageEnvName(self.get())) }
+        unsafe { Robj::from_sexp(R_PackageEnvName(self.get())) }
     }
 
     pub fn is_namespace_env(&self) -> bool {
@@ -429,7 +431,7 @@ impl Robj {
     }
 
     pub fn namespace_env_spec(&self) -> Robj {
-        unsafe { new_owned(R_NamespaceEnvSpec(self.get())) }
+        unsafe { Robj::from_sexp(R_NamespaceEnvSpec(self.get())) }
     }
 
     /// Returns `true` if this is an ALTREP object.
diff --git a/extendr-api/src/wrapper/altrep.rs b/extendr-api/src/wrapper/altrep.rs
index 974cc4c02..93e2f3eb9 100644
--- a/extendr-api/src/wrapper/altrep.rs
+++ b/extendr-api/src/wrapper/altrep.rs
@@ -88,7 +88,7 @@ pub trait AltrepImpl: Clone + std::fmt::Debug {
     /// Duplicate this object. Called by Rf_duplicate.
     /// Currently this manifests the array but preserves the original object.
     fn duplicate(x: SEXP, _deep: bool) -> Robj {
-        unsafe { new_owned(manifest(x)) }
+        Robj::from_sexp(manifest(x))
     }
 
     /// Coerce this object into some other type, if possible.
@@ -459,8 +459,8 @@ impl Altrep {
     pub fn data(&self) -> (Robj, Robj) {
         unsafe {
             (
-                new_owned(R_altrep_data1(self.robj.get())),
-                new_owned(R_altrep_data1(self.robj.get())),
+                Robj::from_sexp(R_altrep_data1(self.robj.get())),
+                Robj::from_sexp(R_altrep_data1(self.robj.get())),
             )
         }
     }
@@ -476,7 +476,7 @@ impl Altrep {
 
     /// Safely implement ALTREP_CLASS.
     pub fn class(&self) -> Robj {
-        unsafe { new_owned(ALTREP_CLASS(self.robj.get())) }
+        unsafe { Robj::from_sexp(ALTREP_CLASS(self.robj.get())) }
     }
 
     pub fn from_state_and_class<StateType: 'static>(
@@ -507,7 +507,7 @@ impl Altrep {
             }
 
             Altrep {
-                robj: new_owned(sexp),
+                robj: Robj::from_sexp(sexp),
             }
         })
     }
@@ -547,9 +547,9 @@ impl Altrep {
             levs: c_int,
         ) -> SEXP {
             <StateType>::unserialize_ex(
-                new_owned(class),
-                new_owned(state),
-                new_owned(attr),
+                Robj::from_sexp(class),
+                Robj::from_sexp(state),
+                Robj::from_sexp(attr),
                 objf as i32,
                 levs as i32,
             )
@@ -560,7 +560,7 @@ impl Altrep {
             class: SEXP,
             state: SEXP,
         ) -> SEXP {
-            <StateType>::unserialize(new_owned(class), new_owned(state)).get()
+            <StateType>::unserialize(Robj::from_sexp(class), Robj::from_sexp(state)).get()
         }
 
         unsafe extern "C" fn altrep_Serialized_state<StateType: AltrepImpl + 'static>(
@@ -626,7 +626,12 @@ impl Altrep {
             indx: SEXP,
             call: SEXP,
         ) -> SEXP {
-            <StateType>::extract_subset(new_owned(x), new_owned(indx), new_owned(call)).get()
+            <StateType>::extract_subset(
+                Robj::from_sexp(x),
+                Robj::from_sexp(indx),
+                Robj::from_sexp(call),
+            )
+            .get()
         }
 
         unsafe {
@@ -674,7 +679,7 @@ impl Altrep {
             );
             R_set_altvec_Extract_subset_method(class_ptr, Some(altvec_Extract_subset::<StateType>));
 
-            new_owned(class_ptr.ptr)
+            Robj::from_sexp(class_ptr.ptr)
         }
     }
 
@@ -989,7 +994,7 @@ impl Altrep {
                 i: R_xlen_t,
                 v: SEXP,
             ) {
-                Altrep::get_state_mut::<StateType>(x).set_elt(i as usize, new_owned(v))
+                Altrep::get_state_mut::<StateType>(x).set_elt(i as usize, Robj::from_sexp(v))
             }
 
             unsafe extern "C" fn altstring_Is_sorted<StateType: AltStringImpl + 'static>(
diff --git a/extendr-api/src/wrapper/char.rs b/extendr-api/src/wrapper/char.rs
index 5c9fc7570..2bc01de1b 100644
--- a/extendr-api/src/wrapper/char.rs
+++ b/extendr-api/src/wrapper/char.rs
@@ -20,10 +20,8 @@ pub struct Rstr {
 impl Rstr {
     /// Make a character object from a string.
     pub fn from_string(val: &str) -> Self {
-        unsafe {
-            Rstr {
-                robj: new_owned(str_to_character(val)),
-            }
+        Rstr {
+            robj: Robj::from_sexp(str_to_character(val)),
         }
     }
 
diff --git a/extendr-api/src/wrapper/environment.rs b/extendr-api/src/wrapper/environment.rs
index 7deebaf71..9ad98ebc1 100644
--- a/extendr-api/src/wrapper/environment.rs
+++ b/extendr-api/src/wrapper/environment.rs
@@ -75,7 +75,7 @@ impl Environment {
     pub fn parent(&self) -> Option<Environment> {
         unsafe {
             let sexp = self.robj.get();
-            let robj = new_owned(ENCLOS(sexp));
+            let robj = Robj::from_sexp(ENCLOS(sexp));
             robj.try_into().ok()
         }
     }
@@ -109,8 +109,8 @@ impl Environment {
     /// Iterate over an environment.
     pub fn iter(&self) -> EnvIter {
         unsafe {
-            let hashtab = new_owned(HASHTAB(self.get()));
-            let frame = new_owned(FRAME(self.get()));
+            let hashtab = Robj::from_sexp(HASHTAB(self.get()));
+            let frame = Robj::from_sexp(FRAME(self.get()));
             if hashtab.is_null() && frame.is_pairlist() {
                 EnvIter {
                     hash_table: ListIter::new(),
@@ -170,7 +170,13 @@ impl Environment {
     pub fn local<K: Into<Robj>>(&self, key: K) -> Result<Robj> {
         let key = key.into();
         if key.is_symbol() {
-            unsafe { Ok(new_owned(Rf_findVarInFrame3(self.get(), key.get(), 1))) }
+            unsafe {
+                Ok(Robj::from_sexp(Rf_findVarInFrame3(
+                    self.get(),
+                    key.get(),
+                    1,
+                )))
+            }
         } else {
             Err(Error::NotFound(key))
         }
diff --git a/extendr-api/src/wrapper/function.rs b/extendr-api/src/wrapper/function.rs
index 8cd843be2..a08f39ed9 100644
--- a/extendr-api/src/wrapper/function.rs
+++ b/extendr-api/src/wrapper/function.rs
@@ -43,7 +43,7 @@ impl Function {
     pub fn from_parts(formals: Pairlist, body: Language, env: Environment) -> Result<Self> {
         unsafe {
             let sexp = Rf_allocSExp(CLOSXP);
-            let robj = new_owned(sexp);
+            let robj = Robj::from_sexp(sexp);
             SET_FORMALS(sexp, formals.get());
             SET_BODY(sexp, body.get());
             SET_CLOENV(sexp, env.get());
@@ -61,7 +61,7 @@ impl Function {
     /// ```
     pub fn call(&self, args: Pairlist) -> Result<Robj> {
         unsafe {
-            let call = new_owned(Rf_lcons(self.get(), args.get()));
+            let call = Robj::from_sexp(Rf_lcons(self.get(), args.get()));
             call.eval()
         }
     }
@@ -71,7 +71,7 @@ impl Function {
         unsafe {
             if self.rtype() == RType::Function {
                 let sexp = self.robj.get();
-                Some(new_owned(FORMALS(sexp)).try_into().unwrap())
+                Some(Robj::from_sexp(FORMALS(sexp)).try_into().unwrap())
             } else {
                 None
             }
@@ -83,7 +83,7 @@ impl Function {
         unsafe {
             if self.rtype() == RType::Function {
                 let sexp = self.robj.get();
-                Some(new_owned(BODY(sexp)))
+                Some(Robj::from_sexp(BODY(sexp)))
             } else {
                 None
             }
@@ -96,7 +96,7 @@ impl Function {
             if self.rtype() == RType::Function {
                 let sexp = self.robj.get();
                 Some(
-                    new_owned(CLOENV(sexp))
+                    Robj::from_sexp(CLOENV(sexp))
                         .try_into()
                         .expect("Should be an environment"),
                 )
diff --git a/extendr-api/src/wrapper/lang.rs b/extendr-api/src/wrapper/lang.rs
index 5d8513e08..d0b083668 100644
--- a/extendr-api/src/wrapper/lang.rs
+++ b/extendr-api/src/wrapper/lang.rs
@@ -31,7 +31,7 @@ impl Language {
                 res = Rf_protect(Rf_lcons(val, res));
                 num_protected += 2;
             }
-            let robj = new_owned(res);
+            let robj = Robj::from_sexp(res);
             Rf_unprotect(num_protected);
             Language { robj }
         })
diff --git a/extendr-api/src/wrapper/list.rs b/extendr-api/src/wrapper/list.rs
index 5bce9e408..3c12cf5e3 100644
--- a/extendr-api/src/wrapper/list.rs
+++ b/extendr-api/src/wrapper/list.rs
@@ -169,7 +169,7 @@ impl Iterator for ListIter {
         if i >= self.len {
             None
         } else {
-            Some(unsafe { new_owned(VECTOR_ELT(self.robj.get(), i as isize)) })
+            Some(unsafe { Robj::from_sexp(VECTOR_ELT(self.robj.get(), i as isize)) })
         }
     }
 
diff --git a/extendr-api/src/wrapper/mod.rs b/extendr-api/src/wrapper/mod.rs
index 7792f5f19..6b0e58ce7 100644
--- a/extendr-api/src/wrapper/mod.rs
+++ b/extendr-api/src/wrapper/mod.rs
@@ -61,12 +61,12 @@ where
 {
     single_threaded(|| unsafe {
         let values = values.into_iter();
-        let sexp = Rf_allocVector(sexptype, values.len() as R_xlen_t);
-        ownership::protect(sexp);
+        let res = Robj::alloc_vector(sexptype, values.len());
+        let sexp = res.get();
         for (i, val) in values.enumerate() {
             SET_VECTOR_ELT(sexp, i as R_xlen_t, val.into().get());
         }
-        Robj::Owned(sexp)
+        res
     })
 }
 
diff --git a/extendr-api/src/wrapper/pairlist.rs b/extendr-api/src/wrapper/pairlist.rs
index 57e32e64f..009487052 100644
--- a/extendr-api/src/wrapper/pairlist.rs
+++ b/extendr-api/src/wrapper/pairlist.rs
@@ -47,7 +47,7 @@ impl Pairlist {
                 }
             }
             let res = Pairlist {
-                robj: new_owned(res),
+                robj: Robj::from_sexp(res),
             };
             Rf_unprotect(num_protects as i32);
             res
@@ -135,7 +135,7 @@ impl Iterator for PairlistIter {
                 None
             } else {
                 let tag = TAG(sexp);
-                let value = new_owned(CAR(sexp));
+                let value = Robj::from_sexp(CAR(sexp));
                 self.list_elem = CDR(sexp);
                 if TYPEOF(tag) == SYMSXP as i32 {
                     let printname = PRINTNAME(tag);
diff --git a/extendr-api/src/wrapper/primitive.rs b/extendr-api/src/wrapper/primitive.rs
index 6009d4c71..30fad3651 100644
--- a/extendr-api/src/wrapper/primitive.rs
+++ b/extendr-api/src/wrapper/primitive.rs
@@ -31,7 +31,7 @@ impl Primitive {
         single_threaded(|| unsafe {
             // Primitives have a special "SYMVALUE" entry in their symbol.
             let sym = Symbol::from_string(val);
-            let symvalue = new_owned(SYMVALUE(sym.get()));
+            let symvalue = Robj::from_sexp(SYMVALUE(sym.get()));
             if symvalue.is_primitive() {
                 Ok(Primitive { robj: symvalue })
             } else {
diff --git a/extendr-api/src/wrapper/promise.rs b/extendr-api/src/wrapper/promise.rs
index 094d10966..1da6eeaf7 100644
--- a/extendr-api/src/wrapper/promise.rs
+++ b/extendr-api/src/wrapper/promise.rs
@@ -20,7 +20,7 @@ impl Promise {
     pub fn from_parts(code: Robj, environment: Environment) -> Result<Self> {
         unsafe {
             let sexp = Rf_allocSExp(PROMSXP);
-            let robj = new_owned(sexp);
+            let robj = Robj::from_sexp(sexp);
             SET_PRCODE(sexp, code.get());
             SET_PRENV(sexp, environment.robj.get());
             SET_PRVALUE(sexp, R_UnboundValue);
@@ -32,7 +32,7 @@ impl Promise {
     pub fn code(&self) -> Robj {
         unsafe {
             let sexp = self.robj.get();
-            new_owned(PRCODE(sexp))
+            Robj::from_sexp(PRCODE(sexp))
         }
     }
 
@@ -40,7 +40,7 @@ impl Promise {
     pub fn environment(&self) -> Environment {
         unsafe {
             let sexp = self.robj.get();
-            new_owned(PRENV(sexp)).try_into().unwrap()
+            Robj::from_sexp(PRENV(sexp)).try_into().unwrap()
         }
     }
 
@@ -48,7 +48,7 @@ impl Promise {
     pub fn value(&self) -> Robj {
         unsafe {
             let sexp = self.robj.get();
-            new_owned(PRVALUE(sexp))
+            Robj::from_sexp(PRVALUE(sexp))
         }
     }
 
diff --git a/extendr-api/src/wrapper/raw.rs b/extendr-api/src/wrapper/raw.rs
index 17a9bcd68..b4f919d5a 100644
--- a/extendr-api/src/wrapper/raw.rs
+++ b/extendr-api/src/wrapper/raw.rs
@@ -28,7 +28,7 @@ impl Raw {
     pub fn from_bytes(bytes: &[u8]) -> Self {
         single_threaded(|| unsafe {
             let sexp = Rf_allocVector(RAWSXP, bytes.len() as R_xlen_t);
-            let robj = new_owned(sexp);
+            let robj = Robj::from_sexp(sexp);
             let ptr = RAW(sexp);
             for (i, &v) in bytes.iter().enumerate() {
                 *ptr.add(i) = v;
diff --git a/extendr-api/src/wrapper/s4.rs b/extendr-api/src/wrapper/s4.rs
index 363cee3c6..0b37122fc 100644
--- a/extendr-api/src/wrapper/s4.rs
+++ b/extendr-api/src/wrapper/s4.rs
@@ -61,7 +61,7 @@ impl S4 {
         let name = Robj::from(name);
         unsafe {
             if R_has_slot(self.get(), name.get()) != 0 {
-                Some(new_owned(R_do_slot(self.get(), name.get())))
+                Some(Robj::from_sexp(R_do_slot(self.get(), name.get())))
             } else {
                 None
             }
diff --git a/extendr-api/src/wrapper/symbol.rs b/extendr-api/src/wrapper/symbol.rs
index 45736d441..81cbb5a1f 100644
--- a/extendr-api/src/wrapper/symbol.rs
+++ b/extendr-api/src/wrapper/symbol.rs
@@ -28,7 +28,7 @@ impl Symbol {
     pub fn from_string<S: AsRef<str>>(val: S) -> Self {
         let val = val.as_ref();
         Symbol {
-            robj: unsafe { new_owned(make_symbol(val)) },
+            robj: Robj::from_sexp(make_symbol(val)),
         }
     }
 
@@ -38,7 +38,7 @@ impl Symbol {
             assert!(TYPEOF(sexp) == SYMSXP as i32);
         }
         Symbol {
-            robj: unsafe { new_sys(sexp) },
+            robj: Robj::from_sexp(sexp),
         }
     }
 


Test patch:
diff --git a/extendr-api/tests/extendr_macro.rs b/extendr-api/tests/extendr_macro.rs
index 5ad4deaec..dab7092f5 100644
--- a/extendr-api/tests/extendr_macro.rs
+++ b/extendr-api/tests/extendr_macro.rs
@@ -62,59 +62,59 @@ fn tests_with_successful_outcomes() {
     unsafe {
         test! {
             // Matching integer.
-            assert_eq!(new_owned(wrap__test_i32(r!(1).get())), r!(1));
+            assert_eq!(Robj::from_sexp(wrap__test_i32(r!(1).get())), r!(1));
 
             // i32 takes any numeric.
-            assert_eq!(new_owned(wrap__test_i32(r!(1.0).get())), r!(1));
+            assert_eq!(Robj::from_sexp(wrap__test_i32(r!(1.0).get())), r!(1));
 
 
             // Matching integer.
-            assert_eq!(new_owned(wrap__test_option_i32(r!(1).get())), r!(1));
+            assert_eq!(Robj::from_sexp(wrap__test_option_i32(r!(1).get())), r!(1));
 
             // Option<i32> takes any numeric.
-            assert_eq!(new_owned(wrap__test_option_i32(r!(1.0).get())), r!(1));
+            assert_eq!(Robj::from_sexp(wrap__test_option_i32(r!(1.0).get())), r!(1));
 
             // NA input.
-            assert_eq!(new_owned(wrap__test_option_i32(r!(NA_REAL).get())), r!(-1));
+            assert_eq!(Robj::from_sexp(wrap__test_option_i32(r!(NA_REAL).get())), r!(-1));
 
             // NA input.
-            assert_eq!(new_owned(wrap__test_option_i32(r!(NA_INTEGER).get())), r!(-1));
+            assert_eq!(Robj::from_sexp(wrap__test_option_i32(r!(NA_INTEGER).get())), r!(-1));
 
 
             // Matching integer.
-            assert_eq!(new_owned(wrap__test_option_i16(r!(1).get())), r!(1));
+            assert_eq!(Robj::from_sexp(wrap__test_option_i16(r!(1).get())), r!(1));
 
             // Option<i16> takes any numeric.
-            assert_eq!(new_owned(wrap__test_option_i16(r!(1.0).get())), r!(1));
+            assert_eq!(Robj::from_sexp(wrap__test_option_i16(r!(1.0).get())), r!(1));
 
             // NA input.
-            assert_eq!(new_owned(wrap__test_option_i16(r!(NA_REAL).get())), r!(-1));
+            assert_eq!(Robj::from_sexp(wrap__test_option_i16(r!(NA_REAL).get())), r!(-1));
 
             // NA input.
-            assert_eq!(new_owned(wrap__test_option_i16(r!(NA_INTEGER).get())), r!(-1));
+            assert_eq!(Robj::from_sexp(wrap__test_option_i16(r!(NA_INTEGER).get())), r!(-1));
 
 
             // Matching integer.
-            assert_eq!(new_owned(wrap__test_option_f64(r!(1).get())), r!(1.0));
+            assert_eq!(Robj::from_sexp(wrap__test_option_f64(r!(1).get())), r!(1.0));
 
             // Option<f64> takes any numeric.
-            assert_eq!(new_owned(wrap__test_option_f64(r!(1.0).get())), r!(1.0));
+            assert_eq!(Robj::from_sexp(wrap__test_option_f64(r!(1.0).get())), r!(1.0));
 
             // NA input.
-            assert_eq!(new_owned(wrap__test_option_f64(r!(NA_REAL).get())), r!(-1.0));
+            assert_eq!(Robj::from_sexp(wrap__test_option_f64(r!(NA_REAL).get())), r!(-1.0));
 
             // NA input.
-            assert_eq!(new_owned(wrap__test_option_f64(r!(NA_INTEGER).get())), r!(-1.0));
+            assert_eq!(Robj::from_sexp(wrap__test_option_f64(r!(NA_INTEGER).get())), r!(-1.0));
 
             // Rint.
-            assert_eq!(new_owned(wrap__test_rint(r!(1).get())), r!(1));
-            assert_eq!(new_owned(wrap__test_rint(r!(1.0).get())), r!(1));
-            assert_eq!(new_owned(wrap__test_rint(r!(NA_INTEGER).get())), r!(NA_INTEGER));
+            assert_eq!(Robj::from_sexp(wrap__test_rint(r!(1).get())), r!(1));
+            assert_eq!(Robj::from_sexp(wrap__test_rint(r!(1.0).get())), r!(1));
+            assert_eq!(Robj::from_sexp(wrap__test_rint(r!(NA_INTEGER).get())), r!(NA_INTEGER));
 
             // Integers
-            assert_eq!(new_owned(wrap__test_integers(r!([1, 2]).get())), r!([1, 2]));
-            assert_eq!(new_owned(wrap__test_integers2(r!([1, 2]).get())), r!([2, 3]));
-            assert_eq!(new_owned(wrap__test_integers3(r!(0..4).get())), r!(6));
+            assert_eq!(Robj::from_sexp(wrap__test_integers(r!([1, 2]).get())), r!([1, 2]));
+            assert_eq!(Robj::from_sexp(wrap__test_integers2(r!([1, 2]).get())), r!([2, 3]));
+            assert_eq!(Robj::from_sexp(wrap__test_integers3(r!(0..4).get())), r!(6));
         }
     }
 }