Do not use SETLENGTH

.github/workflows/check-non-api.yaml

@@ -26,7 +26,7 @@ jobs:
 
           REGEX=$(Rscript tmp.R)
 
-          NON_API=$(grep -R -E "${REGEX}" ./savvy-ffi/src/)
+          NON_API=$(grep -R -E "${REGEX}" ./savvy-ffi/src/ || true)
 
           echo "Check result:"
           echo
@@ -36,4 +36,6 @@ jobs:
           if [ -n "${NON_API}" ]; then
             echo 'Found what they call "non-API"!'
             exit 1
+          else
+            echo "OK!"
           fi

CHANGELOG.md

@@ -3,6 +3,10 @@
 <!-- next-header -->
 ## [Unreleased] (ReleaseDate)
 
+### Minor improvements
+
+* Now savvy no longer uses `SETLENGTH`, which is a so-called "non-API" thing.
+
 ## [v0.6.0] (2024-04-20)
 
 ### Breaking changes

savvy-ffi/src/lib.rs

@@ -69,7 +69,6 @@ extern "C" {
 // Allocation and attributes
 extern "C" {
     pub fn Rf_xlength(arg1: SEXP) -> R_xlen_t;
-    pub fn SETLENGTH(x: SEXP, v: R_xlen_t);
     pub fn Rf_allocVector(arg1: SEXPTYPE, arg2: R_xlen_t) -> SEXP;
     pub fn Rf_install(arg1: *const ::std::os::raw::c_char) -> SEXP;
     pub fn Rf_getAttrib(arg1: SEXP, arg2: SEXP) -> SEXP;

src/sexp/complex.rs

@@ -5,7 +5,7 @@ use savvy_ffi::CPLXSXP;
 use savvy_ffi::{COMPLEX, SEXP};
 
 use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, utils::assert_len, Sexp};
-use crate::protect;
+use crate::protect::{self, local_protect};
 use crate::NotAvailableValue; // for na()
 
 /// An external SEXP of a complex vector.
@@ -198,27 +198,34 @@ impl OwnedComplexSexp {
                 // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
                 // truncated to the actual length at last.
 
-                let mut out = unsafe { Self::new_without_init(upper)? };
+                let inner = crate::alloc_vector(CPLXSXP, upper as _)?;
+                local_protect(inner);
+                let raw = unsafe { COMPLEX(inner) };
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
                     // The upper bound of size_hint() is just for optimization
-                    // and what we should not trust. So, we should't use
-                    // `set_elt_unchecked()` here.
-                    out.set_elt(i, v)?;
+                    // and what we should not trust.
+                    assert_len(upper, i)?;
+                    unsafe { *(raw.add(i)) = v };
 
                     last_index = i;
                 }
 
                 let new_len = last_index + 1;
-                if new_len != upper {
-                    unsafe {
-                        savvy_ffi::SETLENGTH(out.inner, new_len as _);
-                    }
-                    out.len = new_len;
+                if new_len == upper {
+                    // If the length is the same as expected, use it as it is.
+                    Self::new_from_raw_sexp(inner, upper)
+                } else {
+                    // If the length is shorter than expected, re-allocate a new
+                    // SEXP and copy the values into it.
+                    let mut out = unsafe { Self::new_without_init(new_len)? };
+                    // `raw` is longer than new_len, but the elements over new_len are ignored
+                    let raw_slice = unsafe { std::slice::from_raw_parts(raw, new_len) };
+                    out.as_mut_slice().copy_from_slice(raw_slice);
+
+                    Ok(out)
                 }
-
-                Ok(out)
             }
             (_, None) => {
                 // When the length is not known at all, collect() it first.

src/sexp/integer.rs

@@ -4,7 +4,7 @@ use savvy_ffi::{INTEGER, INTSXP, SEXP};
 
 use super::utils::assert_len;
 use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp};
-use crate::protect;
+use crate::protect::{self, local_protect};
 use crate::NotAvailableValue; // for na()
 
 /// An external SEXP of an integer vector.
@@ -284,27 +284,35 @@ impl OwnedIntegerSexp {
                 // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
                 // truncated to the actual length at last.
 
-                let mut out = unsafe { Self::new_without_init(upper)? };
+                let inner = crate::alloc_vector(INTSXP, upper as _)?;
+                local_protect(inner);
+                let raw = unsafe { INTEGER(inner) };
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
                     // The upper bound of size_hint() is just for optimization
-                    // and what we should not trust. So, we should't use
-                    // `set_elt_unchecked()` here.
-                    out.set_elt(i, v)?;
+                    // and what we should not trust.
+                    assert_len(upper, i)?;
+                    unsafe { *(raw.add(i)) = v };
 
                     last_index = i;
                 }
 
                 let new_len = last_index + 1;
-                if new_len != upper {
-                    unsafe {
-                        savvy_ffi::SETLENGTH(out.inner, new_len as _);
-                    }
-                    out.len = new_len;
+                if new_len == upper {
+                    // If the length is the same as expected, use it as it is.
+                    Self::new_from_raw_sexp(inner, upper)
+                } else {
+                    // If the length is shorter than expected, re-allocate a new
+                    // SEXP and copy the values into it.
+                    let out = unsafe { Self::new_without_init(new_len)? };
+                    let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) };
+                    // `raw` is longer than new_len, but the elements over new_len are ignored
+                    let src = unsafe { std::slice::from_raw_parts(raw, new_len) };
+                    dst.copy_from_slice(src);
+
+                    Ok(out)
                 }
-
-                Ok(out)
             }
             (_, None) => {
                 // When the length is not known at all, collect() it first.

src/sexp/logical.rs

@@ -1,7 +1,7 @@
 use savvy_ffi::{R_NaInt, LGLSXP, LOGICAL, SET_LOGICAL_ELT, SEXP};
 
-use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp};
-use crate::protect;
+use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, utils::assert_len, Sexp};
+use crate::protect::{self, local_protect};
 
 /// An external SEXP of a logical vector.
 pub struct LogicalSexp(pub SEXP);
@@ -258,27 +258,35 @@ impl OwnedLogicalSexp {
                 // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
                 // truncated to the actual length at last.
 
-                let mut out = unsafe { Self::new_without_init(upper)? };
+                let inner = crate::alloc_vector(LGLSXP, upper as _)?;
+                local_protect(inner);
+                let raw = unsafe { LOGICAL(inner) };
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
                     // The upper bound of size_hint() is just for optimization
-                    // and what we should not trust. So, we should't use
-                    // `set_elt_unchecked()` here.
-                    out.set_elt(i, v)?;
+                    // and what we should not trust.
+                    assert_len(upper, i)?;
+                    unsafe { *(raw.add(i)) = v as _ };
 
                     last_index = i;
                 }
 
                 let new_len = last_index + 1;
-                if new_len != upper {
-                    unsafe {
-                        savvy_ffi::SETLENGTH(out.inner, new_len as _);
-                    }
-                    out.len = new_len;
+                if new_len == upper {
+                    // If the length is the same as expected, use it as it is.
+                    Self::new_from_raw_sexp(inner, upper)
+                } else {
+                    // If the length is shorter than expected, re-allocate a new
+                    // SEXP and copy the values into it.
+                    let out = unsafe { Self::new_without_init(new_len)? };
+                    let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) };
+                    // `raw` is longer than new_len, but the elements over new_len are ignored
+                    let src = unsafe { std::slice::from_raw_parts(raw, new_len) };
+                    dst.copy_from_slice(src);
+
+                    Ok(out)
                 }
-
-                Ok(out)
             }
             (_, None) => {
                 // When the length is not known at all, collect() it first.

src/sexp/real.rs

@@ -4,7 +4,7 @@ use savvy_ffi::{REAL, REALSXP, SEXP};
 
 use super::utils::assert_len;
 use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp};
-use crate::protect;
+use crate::protect::{self, local_protect};
 use crate::NotAvailableValue; // for na()
 
 /// An external SEXP of a real vector.
@@ -287,27 +287,35 @@ impl OwnedRealSexp {
                 // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
                 // truncated to the actual length at last.
 
-                let mut out = unsafe { Self::new_without_init(upper)? };
+                let inner = crate::alloc_vector(REALSXP, upper as _)?;
+                local_protect(inner);
+                let raw = unsafe { REAL(inner) };
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
                     // The upper bound of size_hint() is just for optimization
-                    // and what we should not trust. So, we should't use
-                    // `set_elt_unchecked()` here.
-                    out.set_elt(i, v)?;
+                    // and what we should not trust.
+                    assert_len(upper, i)?;
+                    unsafe { *(raw.add(i)) = v };
 
                     last_index = i;
                 }
 
                 let new_len = last_index + 1;
-                if new_len != upper {
-                    unsafe {
-                        savvy_ffi::SETLENGTH(out.inner, new_len as _);
-                    }
-                    out.len = new_len;
+                if new_len == upper {
+                    // If the length is the same as expected, use it as it is.
+                    Self::new_from_raw_sexp(inner, upper)
+                } else {
+                    // If the length is shorter than expected, re-allocate a new
+                    // SEXP and copy the values into it.
+                    let out = unsafe { Self::new_without_init(new_len)? };
+                    let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) };
+                    // `raw` is longer than new_len, but the elements over new_len are ignored
+                    let src = unsafe { std::slice::from_raw_parts(raw, new_len) };
+                    dst.copy_from_slice(src);
+
+                    Ok(out)
                 }
-
-                Ok(out)
             }
             (_, None) => {
                 // When the length is not known at all, collect() it first.

src/sexp/string.rs

@@ -187,28 +187,32 @@ impl OwnedStringSexp {
                 // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
                 // truncated to the actual length at last.
 
-                // string doesn't have new_without_init() method
-                let mut out = Self::new(upper)?;
+                let inner = crate::alloc_vector(STRSXP, upper as _)?;
+                local_protect(inner);
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
                     // The upper bound of size_hint() is just for optimization
-                    // and what we should not trust. So, we should't use
-                    // `set_elt_unchecked()` here.
-                    out.set_elt(i, v.as_ref())?;
+                    // and what we should not trust.
+                    assert_len(upper, i)?;
+                    unsafe { SET_STRING_ELT(inner, i as _, str_to_charsxp(v.as_ref())?) };
 
                     last_index = i;
                 }
 
                 let new_len = last_index + 1;
-                if new_len != upper {
-                    unsafe {
-                        savvy_ffi::SETLENGTH(out.inner, new_len as _);
+                if new_len == upper {
+                    // If the length is the same as expected, use it as it is.
+                    Self::new_from_raw_sexp(inner, upper)
+                } else {
+                    // If the length is shorter than expected, re-allocate a new
+                    // SEXP and copy the values into it.
+                    let mut out = Self::new(new_len)?;
+                    for i in 0..new_len {
+                        unsafe { out.set_elt_unchecked(i, STRING_ELT(inner, i as _)) };
                     }
-                    out.len = new_len;
+                    Ok(out)
                 }
-
-                Ok(out)
             }
             (_, None) => {
                 // When the length is not known at all, collect() it first.

xtask/src/main.rs

@@ -84,7 +84,6 @@ fn show() -> Result<(), DynError> {
         .allowlist_function("R_IsNA")
         // Allocation and attributes
         .allowlist_function("Rf_xlength")
-        .allowlist_function("SETLENGTH")
         .allowlist_function("Rf_allocVector")
         .allowlist_function("Rf_install")
         .allowlist_function("Rf_getAttrib")