Add get_attribute() and friends to RObject

Remove footgun of `r_names()` which didn't protect

Author: DavisVaughan

crates/ark/src/srcref.rs

@@ -139,7 +139,7 @@ fn generate_source(
     }
 
     // Ignore functions that already have sources
-    if let Some(_) = old.attr("srcref") {
+    if let Some(_) = old.get_attribute("srcref") {
         return Ok(None);
     }
 

crates/harp/src/attrib.rs

@@ -20,7 +20,7 @@ fn zap_srcref_fn(x: SEXP) -> RObject {
     unsafe {
         let x = RObject::view(x).shallow_duplicate();
 
-        x.set_attr("srcref", r_null());
+        x.set_attribute("srcref", r_null());
         libr::SET_BODY(x.sexp, zap_srcref(libr::R_ClosureExpr(x.sexp)).sexp);
 
         x
@@ -64,7 +64,7 @@ fn zap_srcref_expr(x: SEXP) -> RObject {
 
 fn zap_srcref_attrib(x: SEXP) {
     let x = RObject::view(x);
-    x.set_attr("srcfile", r_null());
-    x.set_attr("srcref", r_null());
-    x.set_attr("wholeSrcref", r_null());
+    x.set_attribute("srcfile", r_null());
+    x.set_attribute("srcref", r_null());
+    x.set_attribute("wholeSrcref", r_null());
 }

crates/harp/src/column_names.rs

@@ -5,6 +5,7 @@ use crate::exec::RFunctionExt;
 use crate::utils::*;
 use crate::vector::Vector;
 use crate::CharacterVector;
+use crate::RObject;
 
 /// Column names
 ///
@@ -17,21 +18,22 @@ pub struct ColumnNames {
 
 impl ColumnNames {
     pub fn new(names: SEXP) -> Self {
-        unsafe {
-            let names = if r_typeof(names) == STRSXP {
-                Some(CharacterVector::new_unchecked(names))
-            } else {
-                None
-            };
-            Self { names }
-        }
+        let names = if r_typeof(names) == STRSXP {
+            Some(unsafe { CharacterVector::new_unchecked(names) })
+        } else {
+            None
+        };
+        Self { names }
     }
 
     pub fn from_data_frame(x: SEXP) -> crate::Result<Self> {
         if !r_is_data_frame(x) {
             return Err(crate::anyhow!("`x` must be a data frame."));
         }
-        Ok(Self::new(r_names(x)))
+        let Some(column_names) = RObject::view(x).get_attribute_names() else {
+            return Err(crate::anyhow!("`x` must have column names."));
+        };
+        Ok(Self::new(column_names.sexp))
     }
 
     pub fn from_matrix(x: SEXP) -> crate::Result<Self> {

crates/harp/src/data_frame.rs

@@ -124,7 +124,11 @@ impl DataFrame {
         // their own custom methods (like for the variables pane). That keeps our hot path
         // simpler, as we unconditionally materialize ALTREP vectors, while still providing a
         // way to opt out.
-        let row_names = RObject::new(harp::r_row_names(x));
+        let row_names = RObject::view(x).get_attribute_row_names();
+
+        let Some(row_names) = row_names else {
+            return Err(crate::anyhow!("`x` must have row names"));
+        };
 
         // The row names object is typically an integer vector (possibly ALTREP compact
         // intrange that knows its length) or character vector, and we just take the length of
@@ -143,6 +147,7 @@ mod tests {
     use crate::r_alloc_integer;
     use crate::r_chr_poke;
     use crate::r_list_poke;
+    use crate::r_null;
     use crate::vector::Vector;
     use crate::DataFrame;
     use crate::List;
@@ -167,7 +172,7 @@ mod tests {
     fn test_data_frame_no_names() {
         crate::r_task(|| {
             let df = harp::parse_eval_base("data.frame(x = 1:2, y = 3:4)").unwrap();
-            df.set_attr("names", RObject::null().sexp);
+            df.set_attribute("names", r_null());
             let df = DataFrame::new(df.sexp);
 
             assert_match!(df, harp::Result::Err(err) => {
@@ -180,7 +185,7 @@ mod tests {
     fn test_data_frame_bad_names() {
         crate::r_task(|| {
             let df = harp::parse_eval_base("data.frame(x = 1:2, y = 3:4)").unwrap();
-            let nms = df.attr("names").unwrap();
+            let nms = df.get_attribute("names").unwrap();
             unsafe {
                 r_chr_poke(nms.sexp, 0, libr::R_NaString);
             }

crates/harp/src/object.rs

@@ -19,7 +19,6 @@ use libr::*;
 use crate::error::Error;
 use crate::exec::RFunction;
 use crate::exec::RFunctionExt;
-use crate::protect::RProtect;
 use crate::r_inherits;
 use crate::r_symbol;
 use crate::size::r_size;
@@ -31,7 +30,6 @@ use crate::utils::r_is_altrep;
 use crate::utils::r_is_null;
 use crate::utils::r_is_object;
 use crate::utils::r_is_s4;
-use crate::utils::r_names;
 use crate::utils::r_str_to_owned_utf8;
 use crate::utils::r_typeof;
 
@@ -460,39 +458,64 @@ impl RObject {
     /// Gets a vector containing names for the object's values (from the `names`
     /// attribute). Returns `None` if the object's value(s) don't have names.
     pub fn names(&self) -> Option<Vec<Option<String>>> {
-        let names = r_names(self.sexp);
-        let names = RObject::from(names);
-        match names.kind() {
-            STRSXP => Vec::<Option<String>>::try_from(names).ok(),
-            _ => None,
+        match self.get_attribute_names() {
+            Some(names) => match names.kind() {
+                STRSXP => Vec::<Option<String>>::try_from(names).ok(),
+                _ => None,
+            },
+            None => None,
         }
     }
 
-    /// Gets a named attribute from the object. Returns `None` if the attribute
-    /// doesn't exist.
-    pub fn attr(&self, name: &str) -> Option<RObject> {
-        let val = unsafe { Rf_getAttrib(self.sexp, r_symbol!(name)) };
-        if r_is_null(val) {
-            None
-        } else {
-            Some(RObject::new(val))
-        }
-    }
-
-    pub fn set_attr(&self, name: &str, value: SEXP) {
+    pub fn set_attribute(&self, name: &str, value: SEXP) {
         unsafe {
             Rf_protect(value);
             Rf_setAttrib(self.sexp, r_symbol!(name), value);
             Rf_unprotect(1);
         }
     }
 
+    /// Gets a named attribute from the object. Returns `None` if the attribute
+    /// was `NULL`.
+    pub fn get_attribute(&self, name: &str) -> Option<RObject> {
+        self.get_attribute_from_symbol(unsafe { r_symbol!(name) })
+    }
+
+    /// Gets the [R_NamesSymbol] attribute from the object. Returns `None` if there are no
+    /// names.
+    pub fn get_attribute_names(&self) -> Option<RObject> {
+        self.get_attribute_from_symbol(unsafe { R_NamesSymbol })
+    }
+
+    /// Gets the [R_RowNamesSymbol] attribute from the object. Returns `None` if there are
+    /// no row names.
+    ///
+    /// # Notes
+    ///
+    /// Note that [Rf_getAttrib()] will turn compact row names of the form `c(NA, -5)`
+    /// into ALTREP compact intrange objects. If you really need to avoid this, use
+    /// `.row_names_info(x, 0L)` instead, which goes through `getAttrib0()`, but note that
+    /// R core frowns on this.
+    /// https://github.com/wch/r-source/blob/e11e04d1f9966551991569b43da2ba6ab2251f30/src/main/attrib.c#L177-L187
+    pub fn get_attribute_row_names(&self) -> Option<RObject> {
+        self.get_attribute_from_symbol(unsafe { R_RowNamesSymbol })
+    }
+
+    fn get_attribute_from_symbol(&self, symbol: SEXP) -> Option<RObject> {
+        let out = unsafe { Rf_getAttrib(self.sexp, symbol) };
+        if r_is_null(out) {
+            None
+        } else {
+            Some(RObject::new(out))
+        }
+    }
+
     pub fn inherits(&self, class: &str) -> bool {
         return r_inherits(self.sexp, class);
     }
 
     pub fn class(&self) -> harp::Result<Option<Vec<String>>> {
-        let Some(class) = self.attr("class") else {
+        let Some(class) = self.get_attribute("class") else {
             return Ok(None);
         };
 
@@ -1050,28 +1073,26 @@ impl TryFrom<&Vec<i32>> for RObject {
 impl TryFrom<RObject> for HashMap<String, String> {
     type Error = crate::error::Error;
     fn try_from(value: RObject) -> Result<Self, Self::Error> {
-        unsafe {
-            r_assert_type(*value, &[STRSXP, VECSXP])?;
-
-            let mut protect = RProtect::new();
-            let names = protect.add(r_names(*value));
-            r_assert_type(names, &[STRSXP])?;
+        r_assert_type(value.sexp, &[STRSXP, VECSXP])?;
 
-            let value = protect.add(Rf_coerceVector(*value, STRSXP));
+        let Some(names) = value.get_attribute_names() else {
+            return Err(Error::UnexpectedType(NILSXP, vec![STRSXP]));
+        };
 
-            let n = Rf_xlength(names);
-            let mut map = HashMap::<String, String>::with_capacity(n as usize);
+        let value = RObject::new(unsafe { Rf_coerceVector(value.sexp, STRSXP) });
 
-            for i in (0..Rf_xlength(names)).rev() {
-                // Translate the name and value into Rust strings.
-                let lhs = r_chr_get_owned_utf8(names, i)?;
-                let rhs = r_chr_get_owned_utf8(value, i)?;
+        let n = names.length();
+        let mut map = HashMap::<String, String>::with_capacity(n as usize);
 
-                map.insert(lhs, rhs);
-            }
+        for i in (0..n).rev() {
+            // Translate the name and value into Rust strings.
+            let lhs = r_chr_get_owned_utf8(names.sexp, i)?;
+            let rhs = r_chr_get_owned_utf8(value.sexp, i)?;
 
-            Ok(map)
+            map.insert(lhs, rhs);
         }
+
+        Ok(map)
     }
 }
 
@@ -1080,28 +1101,26 @@ impl TryFrom<RObject> for HashMap<String, String> {
 impl TryFrom<RObject> for HashMap<String, i32> {
     type Error = crate::error::Error;
     fn try_from(value: RObject) -> Result<Self, Self::Error> {
-        unsafe {
-            r_assert_type(*value, &[INTSXP, VECSXP])?;
+        r_assert_type(*value, &[INTSXP, VECSXP])?;
 
-            let mut protect = RProtect::new();
-            let names = protect.add(r_names(*value));
-            r_assert_type(names, &[STRSXP])?;
+        let Some(names) = value.get_attribute_names() else {
+            return Err(Error::UnexpectedType(NILSXP, vec![STRSXP]));
+        };
 
-            let value = protect.add(Rf_coerceVector(*value, INTSXP));
+        let value = RObject::new(unsafe { Rf_coerceVector(value.sexp, INTSXP) });
 
-            let n = Rf_xlength(names);
-            let mut map = HashMap::<String, i32>::with_capacity(n as usize);
+        let n = names.length();
+        let mut map = HashMap::<String, i32>::with_capacity(n as usize);
 
-            for i in (0..Rf_xlength(names)).rev() {
-                // Translate the name and value into Rust strings.
-                let name = r_chr_get_owned_utf8(names, i)?;
-                let val = r_int_get(value, i);
+        for i in (0..n).rev() {
+            // Translate the name and value into Rust strings.
+            let name = r_chr_get_owned_utf8(names.sexp, i)?;
+            let val = r_int_get(value.sexp, i);
 
-                map.insert(name, val);
-            }
-
-            Ok(map)
+            map.insert(name, val);
         }
+
+        Ok(map)
     }
 }
 
@@ -1110,23 +1129,23 @@ impl TryFrom<RObject> for HashMap<String, i32> {
 impl TryFrom<RObject> for HashMap<String, RObject> {
     type Error = crate::error::Error;
     fn try_from(value: RObject) -> Result<Self, Self::Error> {
-        unsafe {
-            let mut protect = RProtect::new();
-            let names = protect.add(r_names(*value));
-            r_assert_type(names, &[STRSXP])?;
-
-            let n = Rf_xlength(names);
-            let mut map = HashMap::<String, RObject>::with_capacity(n as usize);
-
-            // iterate in the reverse order to keep the first occurence of a name
-            for i in (0..n).rev() {
-                let name = r_chr_get_owned_utf8(names, i)?;
-                let value: RObject = RObject::new(VECTOR_ELT(*value, i));
-                map.insert(name, value);
-            }
+        r_assert_type(*value, &[VECSXP])?;
 
-            Ok(map)
+        let Some(names) = value.get_attribute_names() else {
+            return Err(Error::UnexpectedType(NILSXP, vec![STRSXP]));
+        };
+
+        let n = names.length();
+        let mut map = HashMap::<String, RObject>::with_capacity(n as usize);
+
+        // iterate in the reverse order to keep the first occurence of a name
+        for i in (0..n).rev() {
+            let name = r_chr_get_owned_utf8(names.sexp, i)?;
+            let value = RObject::new(list_get(value.sexp, i));
+            map.insert(name, value);
         }
+
+        Ok(map)
     }
 }
 

crates/harp/src/parser/srcref.rs

@@ -40,7 +40,7 @@ pub struct SrcFile {
 // attributes.
 impl RObject {
     pub fn srcrefs(&self) -> anyhow::Result<Vec<SrcRef>> {
-        let srcref = unwrap!(self.attr("srcref"), None => {
+        let srcref = unwrap!(self.get_attribute("srcref"), None => {
             return Err(anyhow!("Can't find `srcref` attribute"));
         });
 

crates/harp/src/utils.rs

@@ -393,28 +393,6 @@ pub unsafe fn r_envir_remove(symbol: &str, envir: SEXP) {
     R_removeVarFromFrame(r_symbol!(symbol), envir);
 }
 
-/// Get the row names attribute of an object
-///
-/// Raw access to the [R_RowNamesSymbol] attribute. Will return [R_NilValue] when no names
-/// are present.
-///
-/// Note that [Rf_getAttrib()] will turn compact row names of the form `c(NA, -5)` into
-/// ALTREP compact intrange objects. If you really need to avoid this, use
-/// `.row_names_info(x, 0L)` instead, which goes through `getAttrib0()`, but note that R
-/// core frowns on this.
-/// https://github.com/wch/r-source/blob/e11e04d1f9966551991569b43da2ba6ab2251f30/src/main/attrib.c#L177-L187
-pub fn r_row_names(x: SEXP) -> SEXP {
-    unsafe { Rf_getAttrib(x, R_RowNamesSymbol) }
-}
-
-/// Get the names attribute of an object
-///
-/// Raw access to the [R_NamesSymbol] attribute. Will return [R_NilValue] when no names
-/// are present.
-pub fn r_names(x: SEXP) -> SEXP {
-    unsafe { Rf_getAttrib(x, R_NamesSymbol) }
-}
-
 /// Get names of a vector
 ///
 /// `r_names2()` always returns a character vector, even when the object does
@@ -427,7 +405,7 @@ pub fn r_names2(x: SEXP) -> SEXP {
 
     let size = r_length(x);
 
-    let names = r_names(x);
+    let names = unsafe { Rf_getAttrib(x, R_NamesSymbol) };
     unsafe { protect.add(names) };
 
     if r_is_null(names) {