From 4272da5bbf18b640f1787990679d0aaa02424154 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 11 Mar 2026 11:27:47 +0100 Subject: [PATCH 1/6] Harp functions are not unsafe --- crates/ark/src/debug.rs | 2 + crates/ark/src/sys/unix/console.rs | 1 + crates/ark/src/variables/variable.rs | 1 + crates/harp/src/exec.rs | 2 +- crates/harp/src/utils.rs | 112 ++++++++++++++------------- 5 files changed, 65 insertions(+), 53 deletions(-) diff --git a/crates/ark/src/debug.rs b/crates/ark/src/debug.rs index 5e457d3da..6b95ea64a 100644 --- a/crates/ark/src/debug.rs +++ b/crates/ark/src/debug.rs @@ -32,6 +32,7 @@ static _ARK_DISPLAY_VALUE: unsafe extern "C" fn(x: libr::SEXP) -> *const ffi::c_ // Implementations for entry points in `debug.c`. #[cfg_attr(not(test), no_mangle)] +#[allow(clippy::not_unsafe_ptr_arg_deref)] pub extern "C-unwind" fn ark_print_rs(x: libr::SEXP) -> *const ffi::c_char { capture_console_output(|| { unsafe { libr::Rf_PrintValue(x) }; @@ -81,6 +82,7 @@ pub extern "C-unwind" fn ark_trace_back_rs() -> *const ffi::c_char { } #[cfg_attr(not(test), no_mangle)] +#[allow(clippy::not_unsafe_ptr_arg_deref)] pub extern "C-unwind" fn ark_display_value_rs(x: libr::SEXP) -> *const ffi::c_char { let value = unsafe { let kind = tidy_kind(r_typeof(x)); diff --git a/crates/ark/src/sys/unix/console.rs b/crates/ark/src/sys/unix/console.rs index dd8ff2045..59223c335 100644 --- a/crates/ark/src/sys/unix/console.rs +++ b/crates/ark/src/sys/unix/console.rs @@ -155,6 +155,7 @@ pub extern "C-unwind" fn r_cleanup_for_tests(_save_act: i32, _status: i32, _run_ } /// On Unix, we assume that the buffer to write to the console is /// already in UTF-8 +#[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn console_to_utf8(x: *const c_char) -> anyhow::Result { let x = unsafe { CStr::from_ptr(x) }; diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 8305fb4df..7589aee43 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -409,6 +409,7 @@ impl WorkspaceVariableDisplayType { /// - value: The R object to create the display type and type info for. /// - include_length: Whether to include the length of the object in the /// display type. + #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn from(value: SEXP, include_length: bool) -> Self { match Self::try_from_method(value, include_length) { Err(err) => log::error!( diff --git a/crates/harp/src/exec.rs b/crates/harp/src/exec.rs index 02b6063d6..1fb82c735 100644 --- a/crates/harp/src/exec.rs +++ b/crates/harp/src/exec.rs @@ -102,7 +102,7 @@ pub fn try_eval(expr: SEXP, env: SEXP) -> crate::Result { if let Err(Error::TryCatchError { ref mut call, .. }) = res { if call.is_none() { - *call = Some(unsafe { r_stringify(expr, "\n")? }); + *call = Some(r_stringify(expr, "\n")?); } } diff --git a/crates/harp/src/utils.rs b/crates/harp/src/utils.rs index 114147209..b4ebdae7e 100644 --- a/crates/harp/src/utils.rs +++ b/crates/harp/src/utils.rs @@ -347,22 +347,22 @@ pub fn r_formals(object: SEXP) -> Result> { Ok(arguments) } -pub unsafe fn r_envir_name(envir: SEXP) -> Result { +pub fn r_envir_name(envir: SEXP) -> Result { r_assert_type(envir, &[ENVSXP])?; if r_env_is_pkg_env(envir) { let name = RObject::from(r_pkg_env_name(envir)); - return name.to::(); + return unsafe { name.to::() }; } if r_env_is_ns_env(envir) { let name = RObject::from(r_ns_env_name(envir)); - return name.to::(); + return unsafe { name.to::() }; } - let name = Rf_getAttrib(envir, r_symbol!("name")); + let name = unsafe { Rf_getAttrib(envir, r_symbol!("name")) }; if r_typeof(name) == STRSXP { - let name = RObject::view(name).to::()?; + let name = unsafe { RObject::view(name).to::()? }; return Ok(name); } @@ -381,12 +381,12 @@ pub fn r_envir_get(symbol: &str, envir: SEXP) -> Option { } } -pub unsafe fn r_envir_set(symbol: &str, value: SEXP, envir: SEXP) { - Rf_defineVar(r_symbol!(symbol), value, envir); +pub fn r_envir_set(symbol: &str, value: SEXP, envir: SEXP) { + unsafe { Rf_defineVar(r_symbol!(symbol), value, envir) }; } -pub unsafe fn r_envir_remove(symbol: &str, envir: SEXP) { - R_removeVarFromFrame(r_symbol!(symbol), envir); +pub fn r_envir_remove(symbol: &str, envir: SEXP) { + unsafe { R_removeVarFromFrame(r_symbol!(symbol), envir) }; } /// Get names of a vector @@ -445,10 +445,10 @@ pub fn r_names2(x: SEXP) -> SEXP { out } -pub unsafe fn r_stringify(object: SEXP, delimiter: &str) -> Result { +pub fn r_stringify(object: SEXP, delimiter: &str) -> Result { // handle SYMSXPs upfront if r_typeof(object) == SYMSXP { - return RObject::view(object).to::(); + return unsafe { RObject::view(object).to::() }; } // call format on the object @@ -457,20 +457,24 @@ pub unsafe fn r_stringify(object: SEXP, delimiter: &str) -> Result { .call()?; // paste into a single string - let object = RFunction::new("base", "paste") - .add(object) - .param("collapse", delimiter) - .call()? - .to::()?; + let object = unsafe { + RFunction::new("base", "paste") + .add(object) + .param("collapse", delimiter) + .call()? + .to::()? + }; Ok(object) } -pub unsafe fn r_inspect(object: SEXP) { - let mut protect = RProtect::new(); - let inspect = protect.add(Rf_lang2(r_symbol!("inspect"), object)); - let internal = protect.add(Rf_lang2(r_symbol!(".Internal"), inspect)); - Rf_eval(internal, R_BaseEnv); +pub fn r_inspect(object: SEXP) { + unsafe { + let mut protect = RProtect::new(); + let inspect = protect.add(Rf_lang2(r_symbol!("inspect"), object)); + let internal = protect.add(Rf_lang2(r_symbol!(".Internal"), inspect)); + Rf_eval(internal, R_BaseEnv); + } } pub fn r_is_promise(x: SEXP) -> bool { @@ -490,9 +494,9 @@ pub fn r_promise_expr(x: SEXP) -> SEXP { unsafe { R_PromiseExpr(x) } } -pub unsafe fn r_promise_force(x: SEXP) -> harp::Result { +pub fn r_promise_force(x: SEXP) -> harp::Result { // Expect that the promise protects its own result - harp::try_eval(x, R_EmptyEnv) + harp::try_eval(x, unsafe { R_EmptyEnv }) } pub fn r_promise_force_with_rollback(x: SEXP) -> harp::Result { @@ -506,30 +510,30 @@ pub fn r_promise_force_with_rollback(x: SEXP) -> harp::Result { } } -pub unsafe fn r_promise_is_lazy_load_binding(x: SEXP) -> bool { +pub fn r_promise_is_lazy_load_binding(x: SEXP) -> bool { // `rlang:::promise_expr("across", asNamespace("dplyr"))` // returns: // `lazyLoadDBfetch(c(105202L, 4670L), datafile, compressed, envhook)` // We can take advantage of this to identify promises in namespaces // that correspond to symbols we should evaluate when generating completions. - let expr = PRCODE(x); + let expr = unsafe { PRCODE(x) }; if r_typeof(expr) != LANGSXP { return false; } - if Rf_xlength(expr) == 0 { + if unsafe { Rf_xlength(expr) } == 0 { return false; } - let expr = CAR(expr); + let expr = unsafe { CAR(expr) }; if r_typeof(expr) != SYMSXP { return false; } - expr == r_symbol!("lazyLoadDBfetch") + expr == unsafe { r_symbol!("lazyLoadDBfetch") } } pub fn r_bytecode_expr(x: SEXP) -> SEXP { @@ -563,46 +567,50 @@ pub fn r_env_binding_is_active(env: SEXP, sym: SEXP) -> harp::Result { } } -pub unsafe fn r_env_is_pkg_env(env: SEXP) -> bool { - R_IsPackageEnv(env) == Rboolean_TRUE || env == R_BaseEnv +pub fn r_env_is_pkg_env(env: SEXP) -> bool { + unsafe { R_IsPackageEnv(env) == Rboolean_TRUE || env == R_BaseEnv } } -pub unsafe fn r_pkg_env_name(env: SEXP) -> SEXP { - if env == R_BaseEnv { - // `R_BaseEnv` is not handled by `R_PackageEnvName()`, but most of the time we want to - // treat it like a package namespace - return r_char!("base"); - } +pub fn r_pkg_env_name(env: SEXP) -> SEXP { + unsafe { + if env == R_BaseEnv { + // `R_BaseEnv` is not handled by `R_PackageEnvName()`, but most of the time we want to + // treat it like a package namespace + return r_char!("base"); + } - let name = R_PackageEnvName(env); + let name = R_PackageEnvName(env); - if name == libr::R_NilValue { - // Should be very unlikely, but `NULL` can be returned - return r_char!(""); - } + if name == libr::R_NilValue { + // Should be very unlikely, but `NULL` can be returned + return r_char!(""); + } - STRING_ELT(name, 0) + STRING_ELT(name, 0) + } } -pub unsafe fn r_env_is_ns_env(env: SEXP) -> bool { +pub fn r_env_is_ns_env(env: SEXP) -> bool { // Does handle `R_BaseNamespace` // https://github.com/wch/r-source/blob/1cb35ff692d3eb3ab546e0db4761102b5ea4ac89/src/main/envir.c#L3689 - R_IsNamespaceEnv(env) == Rboolean_TRUE + unsafe { R_IsNamespaceEnv(env) == Rboolean_TRUE } } -pub unsafe fn r_ns_env_name(env: SEXP) -> SEXP { +pub fn r_ns_env_name(env: SEXP) -> SEXP { // Does handle `R_BaseNamespace` // https://github.com/wch/r-source/blob/1cb35ff692d3eb3ab546e0db4761102b5ea4ac89/src/main/envir.c#L3720 - let mut protect = RProtect::new(); + unsafe { + let mut protect = RProtect::new(); - let spec = protect.add(R_NamespaceEnvSpec(env)); + let spec = protect.add(R_NamespaceEnvSpec(env)); - if spec == libr::R_NilValue { - // Should be very unlikely, but `NULL` can be returned - return r_char!(""); - } + if spec == libr::R_NilValue { + // Should be very unlikely, but `NULL` can be returned + return r_char!(""); + } - STRING_ELT(spec, 0) + STRING_ELT(spec, 0) + } } /// Returns `true` if `f` returns `true` for any node of the pairlist From 1a5b4681a3afd31c99d82ab5cbbb99574fca5bd9 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 11 Mar 2026 11:58:04 +0100 Subject: [PATCH 2/6] Allow derefencing `SEXP` --- crates/ark/src/debug.rs | 2 - crates/ark/src/variables/r_variables.rs | 2 +- crates/ark/src/variables/variable.rs | 1 - crates/harp/src/object.rs | 2 +- crates/harp/src/r.rs | 2 +- crates/harp/src/raii.rs | 1 + crates/libr/src/r.rs | 108 ++++++++++++------------ crates/libr/src/types.rs | 32 ++++++- 8 files changed, 89 insertions(+), 61 deletions(-) diff --git a/crates/ark/src/debug.rs b/crates/ark/src/debug.rs index 6b95ea64a..5e457d3da 100644 --- a/crates/ark/src/debug.rs +++ b/crates/ark/src/debug.rs @@ -32,7 +32,6 @@ static _ARK_DISPLAY_VALUE: unsafe extern "C" fn(x: libr::SEXP) -> *const ffi::c_ // Implementations for entry points in `debug.c`. #[cfg_attr(not(test), no_mangle)] -#[allow(clippy::not_unsafe_ptr_arg_deref)] pub extern "C-unwind" fn ark_print_rs(x: libr::SEXP) -> *const ffi::c_char { capture_console_output(|| { unsafe { libr::Rf_PrintValue(x) }; @@ -82,7 +81,6 @@ pub extern "C-unwind" fn ark_trace_back_rs() -> *const ffi::c_char { } #[cfg_attr(not(test), no_mangle)] -#[allow(clippy::not_unsafe_ptr_arg_deref)] pub extern "C-unwind" fn ark_display_value_rs(x: libr::SEXP) -> *const ffi::c_char { let value = unsafe { let kind = tidy_kind(r_typeof(x)); diff --git a/crates/ark/src/variables/r_variables.rs b/crates/ark/src/variables/r_variables.rs index bc79c515c..8ab598499 100644 --- a/crates/ark/src/variables/r_variables.rs +++ b/crates/ark/src/variables/r_variables.rs @@ -303,7 +303,7 @@ impl RVariables { let env = self.env.get().clone(); let result = RFunction::new("base", "rm") - .param("list", CharacterVector::create(variables).cast()) + .param("list", CharacterVector::create(variables)) .param("envir", env) .call(); diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 7589aee43..8305fb4df 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -409,7 +409,6 @@ impl WorkspaceVariableDisplayType { /// - value: The R object to create the display type and type info for. /// - include_length: Whether to include the length of the object in the /// display type. - #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn from(value: SEXP, include_length: bool) -> Self { match Self::try_from_method(value, include_length) { Err(err) => log::error!( diff --git a/crates/harp/src/object.rs b/crates/harp/src/object.rs index 496cac478..e5d45a511 100644 --- a/crates/harp/src/object.rs +++ b/crates/harp/src/object.rs @@ -370,7 +370,7 @@ impl RObject { /// Address in hexadecimal format pub fn address(&self) -> String { - format!("{:p}", self.sexp as *const _) + format!("{:p}", self.sexp) } /// String accessor; get a string value from a vector of strings. diff --git a/crates/harp/src/r.rs b/crates/harp/src/r.rs index 776df14fd..45dd5eedd 100644 --- a/crates/harp/src/r.rs +++ b/crates/harp/src/r.rs @@ -118,7 +118,7 @@ pub fn attrib_for_each(x: SEXP, mut f: F) { ) -> SEXP { let f = &mut *(data as *mut F); f(tag, val); - std::ptr::null_mut() + SEXP::null() } let data = &mut f as *mut F as *mut std::ffi::c_void; libr::R_mapAttrib(x, Some(trampoline::), data); diff --git a/crates/harp/src/raii.rs b/crates/harp/src/raii.rs index 148a0d944..7ca0bd85d 100644 --- a/crates/harp/src/raii.rs +++ b/crates/harp/src/raii.rs @@ -49,6 +49,7 @@ impl RLocal where T: Copy, { + #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn new(new_value: T, variable: *mut T) -> RLocal { unsafe { let old_value = libr::get(variable); diff --git a/crates/libr/src/r.rs b/crates/libr/src/r.rs index 81b6c0a16..0dd1a1836 100644 --- a/crates/libr/src/r.rs +++ b/crates/libr/src/r.rs @@ -473,219 +473,219 @@ constant_globals::generate! { pub static R_NaInt: std::ffi::c_int; #[doc = "The \"global\" environment"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_GlobalEnv: SEXP; #[doc = "An empty environment at the root of the\nenvironment tree"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_EmptyEnv: SEXP; #[doc = "The base environment; formerly R_NilValue"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_BaseEnv: SEXP; #[doc = "The (fake) namespace for base"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_BaseNamespace: SEXP; #[doc = "Registry for registered namespaces"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_NamespaceRegistry: SEXP; #[doc = "The nil object"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_NilValue: SEXP; #[doc = "Unbound marker"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_UnboundValue: SEXP; #[doc = "Missing argument marker"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_MissingArg: SEXP; #[doc = "To be found in BC interp. state\n(marker)"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_InBCInterpreter: SEXP; #[doc = "Use current expression (marker)"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_CurrentExpression: SEXP; #[doc = "Marker for restarted function calls"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_RestartToken: SEXP; #[doc = "\"as.character\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_AsCharacterSymbol: SEXP; #[doc = "\"@\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_AtsignSymbol: SEXP; #[doc = "\"base\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_BaseSymbol: SEXP; #[doc = "\"{\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_BraceSymbol: SEXP; #[doc = "\"\\[\\[\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_Bracket2Symbol: SEXP; #[doc = "\"\\[\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_BracketSymbol: SEXP; #[doc = "\"class\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_ClassSymbol: SEXP; #[doc = "\".Device\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_DeviceSymbol: SEXP; #[doc = "\"dimnames\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_DimNamesSymbol: SEXP; #[doc = "\"dim\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_DimSymbol: SEXP; #[doc = "\"$\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_DollarSymbol: SEXP; #[doc = "\"...\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_DotsSymbol: SEXP; #[doc = "\"::\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_DoubleColonSymbol: SEXP; #[doc = "\"drop\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_DropSymbol: SEXP; #[doc = "\"eval\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_EvalSymbol: SEXP; #[doc = "\"function\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_FunctionSymbol: SEXP; #[doc = "\".Last.value\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_LastvalueSymbol: SEXP; #[doc = "\"levels\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_LevelsSymbol: SEXP; #[doc = "\"mode\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_ModeSymbol: SEXP; #[doc = "\"na.rm\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_NaRmSymbol: SEXP; #[doc = "\"name\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_NameSymbol: SEXP; #[doc = "\"names\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_NamesSymbol: SEXP; #[doc = "\".__NAMESPACE__.\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_NamespaceEnvSymbol: SEXP; #[doc = "\"package\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_PackageSymbol: SEXP; #[doc = "\"previous\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_PreviousSymbol: SEXP; #[doc = "\"quote\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_QuoteSymbol: SEXP; #[doc = "\"row.names\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_RowNamesSymbol: SEXP; #[doc = "\".Random.seed\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_SeedsSymbol: SEXP; #[doc = "\"sort.list\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_SortListSymbol: SEXP; #[doc = "\"source\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_SourceSymbol: SEXP; #[doc = "\"spec\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_SpecSymbol: SEXP; #[doc = "\":::\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_TripleColonSymbol: SEXP; #[doc = "\"srcfile\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_SrcfileSymbol: SEXP; #[doc = "\"srcref\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_SrcrefSymbol: SEXP; #[doc = "\"tsp\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_TspSymbol: SEXP; #[doc = "\".defined\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_dot_defined: SEXP; #[doc = "\".Method\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_dot_Method: SEXP; #[doc = "\".packageName\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_dot_packageName: SEXP; #[doc = "\".target\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_dot_target: SEXP; #[doc = "\".Generic\""] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_dot_Generic: SEXP; #[doc = "NA_STRING as a CHARSXP"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_NaString: SEXP; #[doc = "\"\" as a CHARSXP"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_BlankString: SEXP; #[doc = "\"\" as a STRSXP"] - #[default = std::ptr::null_mut()] + #[default = SEXP::null()] pub static R_BlankScalarString: SEXP; #[default = 4503599627370496] diff --git a/crates/libr/src/types.rs b/crates/libr/src/types.rs index adb7375d3..1247966aa 100644 --- a/crates/libr/src/types.rs +++ b/crates/libr/src/types.rs @@ -50,7 +50,37 @@ pub const FUNSXP: u32 = 99; pub struct SEXPREC { _unused: [u8; 0], } -pub type SEXP = *mut SEXPREC; + +/// SEXP as a newtype rather than a type alias for `*mut SEXPREC`. This way +/// clippy doesn't see it as a raw pointer, avoiding +/// `clippy::not_unsafe_ptr_arg_deref` on every public function that takes a +/// SEXP. `#[repr(transparent)]` guarantees the same ABI as `*mut SEXPREC` +/// across FFI boundaries. +#[repr(transparent)] +#[derive(Copy, Clone, PartialEq, Eq, Hash)] +pub struct SEXP(pub *mut SEXPREC); + +impl SEXP { + pub const fn null() -> Self { + Self(std::ptr::null_mut()) + } + + pub fn is_null(self) -> bool { + self.0.is_null() + } +} + +impl std::fmt::Debug for SEXP { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "SEXP({:?})", self.0) + } +} + +impl std::fmt::Pointer for SEXP { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Pointer::fmt(&self.0, f) + } +} #[doc = "R 4.3 redefined `Rcomplex` to a union for compatibility with Fortran.\n But the old definition is compatible both the union version\n and the struct version.\n See: https://github.com/extendr/extendr/issues/524"] #[repr(C)] From a6714bc72ffe549bc216ca73968667df2d7ecd2a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 11 Mar 2026 17:43:28 +0100 Subject: [PATCH 3/6] More removal of `unsafe` tags --- crates/ark/src/connections/r_connection.rs | 168 ++++++------- crates/ark/src/console/console_repl.rs | 137 +++++------ crates/ark/src/dap/dap_variables.rs | 4 +- crates/ark/src/data_explorer/format.rs | 18 +- .../ark/src/data_explorer/r_data_explorer.rs | 148 ++++++------ crates/ark/src/data_explorer/summary_stats.rs | 2 +- crates/ark/src/debug.rs | 6 +- crates/ark/src/help/r_help.rs | 2 +- crates/ark/src/help_proxy.rs | 2 +- .../lsp/completions/sources/composite/call.rs | 2 +- .../lsp/completions/sources/unique/comment.rs | 4 +- .../completions/sources/unique/namespace.rs | 4 +- crates/ark/src/lsp/signature_help.rs | 20 +- crates/ark/src/modules.rs | 8 +- crates/ark/src/plots/graphics_device.rs | 4 +- crates/ark/src/repos.rs | 2 +- crates/ark/src/srcref.rs | 82 +++---- crates/ark/src/startup.rs | 2 +- crates/ark/src/ui/ui_comm.rs | 6 +- crates/ark/src/variables/variable.rs | 115 ++++----- crates/ark/tests/connections.rs | 2 +- crates/ark/tests/help.rs | 4 +- crates/ark/tests/variables.rs | 8 +- crates/harp/src/attrib.rs | 48 ++-- crates/harp/src/column_names.rs | 2 +- crates/harp/src/data_frame.rs | 2 +- crates/harp/src/environment.rs | 41 ++-- crates/harp/src/exec.rs | 124 +++++----- crates/harp/src/external_ptr.rs | 16 +- crates/harp/src/fixtures.rs | 4 +- crates/harp/src/format.rs | 2 +- crates/harp/src/json.rs | 20 +- crates/harp/src/modules.rs | 14 +- crates/harp/src/object.rs | 228 ++++++++---------- crates/harp/src/options.rs | 2 +- crates/harp/src/parse.rs | 30 +-- crates/harp/src/parser/srcref.rs | 6 +- crates/harp/src/protect.rs | 9 +- crates/harp/src/r.rs | 74 +++--- crates/harp/src/raii.rs | 18 +- crates/harp/src/routines.rs | 4 +- crates/harp/src/session.rs | 2 +- crates/harp/src/size.rs | 32 +-- crates/harp/src/symbol.rs | 4 +- crates/harp/src/syntax.rs | 2 +- crates/harp/src/utils.rs | 112 ++++----- crates/harp/src/vec_format.rs | 2 +- crates/harp/src/vector.rs | 30 ++- crates/harp/src/vector/character_vector.rs | 20 +- crates/harp/src/vector/complex_vector.rs | 6 +- crates/harp/src/vector/factor.rs | 4 +- crates/harp/src/vector/formatted_vector.rs | 30 +-- crates/harp/src/vector/integer_vector.rs | 4 +- crates/harp/src/vector/list.rs | 8 +- crates/harp/src/vector/logical_vector.rs | 4 +- crates/harp/src/vector/names.rs | 24 +- crates/harp/src/vector/numeric_vector.rs | 6 +- crates/harp/src/vector/raw_vector.rs | 4 +- crates/harp/src/weak_ref.rs | 12 +- crates/libr/src/constant_globals.rs | 4 +- crates/libr/src/functions.rs | 8 +- crates/libr/src/functions_variadic.rs | 8 +- crates/libr/src/lib.rs | 8 +- crates/libr/src/mutable_globals.rs | 4 +- 64 files changed, 790 insertions(+), 942 deletions(-) diff --git a/crates/ark/src/connections/r_connection.rs b/crates/ark/src/connections/r_connection.rs index 7f6d6311a..8cf14ec8a 100644 --- a/crates/ark/src/connections/r_connection.rs +++ b/crates/ark/src/connections/r_connection.rs @@ -104,73 +104,69 @@ impl RConnection { match message { ConnectionsBackendRequest::ListObjects(ListObjectsParams { path }) => { let tables = r_task(|| -> Result<_, anyhow::Error> { - unsafe { - let mut call = RFunction::from(".ps.connection_list_objects"); - call.add(RObject::from(self.comm.comm_id.clone())); - for obj in path { - call.param(obj.kind.as_str(), obj.name); - } - // returns a data.frame with columns name and type - let tables = call.call()?; - - let names = RFunction::from("[[") - .add(tables.clone()) - .add(RObject::from("name")) - .call()?; - - let types = RFunction::from("[[") - .add(tables) - .add(RObject::from("type")) - .call()?; - - let resulting = RObject::to::>(names)? - .iter() - .zip(RObject::to::>(types)?.iter()) - .map(|(name, kind)| ObjectSchema { - name: name.clone(), - kind: kind.clone(), - has_children: None, - }) - .collect::>(); - - Ok(resulting) + let mut call = RFunction::from(".ps.connection_list_objects"); + call.add(RObject::from(self.comm.comm_id.clone())); + for obj in path { + call.param(obj.kind.as_str(), obj.name); } + // returns a data.frame with columns name and type + let tables = call.call()?; + + let names = RFunction::from("[[") + .add(tables.clone()) + .add(RObject::from("name")) + .call()?; + + let types = RFunction::from("[[") + .add(tables) + .add(RObject::from("type")) + .call()?; + + let resulting = RObject::to::>(names)? + .iter() + .zip(RObject::to::>(types)?.iter()) + .map(|(name, kind)| ObjectSchema { + name: name.clone(), + kind: kind.clone(), + has_children: None, + }) + .collect::>(); + + Ok(resulting) })?; Ok(ConnectionsBackendReply::ListObjectsReply(tables)) }, ConnectionsBackendRequest::ListFields(ListFieldsParams { path }) => { let fields = r_task(|| -> Result<_, anyhow::Error> { - unsafe { - let mut call = RFunction::from(".ps.connection_list_fields"); - call.add(RObject::from(self.comm.comm_id.clone())); - for obj in path { - call.param(obj.kind.as_str(), obj.name); - } - let fields = call.call()?; - - // for now we only need the name column - let names = RFunction::from("[[") - .add(fields.clone()) - .add(RObject::from("name")) - .call()?; - - let dtypes = RFunction::from("[[") - .add(fields) - .add(RObject::from("type")) - .call()?; - - let resulting = RObject::to::>(names)? - .iter() - .zip(RObject::to::>(dtypes)?.iter()) - .map(|(name, dtype)| FieldSchema { - name: name.clone(), - dtype: dtype.clone(), - }) - .collect::>(); - - Ok(resulting) + let mut call = RFunction::from(".ps.connection_list_fields"); + call.add(RObject::from(self.comm.comm_id.clone())); + for obj in path { + call.param(obj.kind.as_str(), obj.name); } + let fields = call.call()?; + + // for now we only need the name column + let names = RFunction::from("[[") + .add(fields.clone()) + .add(RObject::from("name")) + .call()?; + + let dtypes = RFunction::from("[[") + .add(fields) + .add(RObject::from("type")) + .call()?; + + let resulting = RObject::to::>(names)? + .iter() + .zip(RObject::to::>(dtypes)?.iter()) + .map(|(name, dtype)| FieldSchema { + name: name.clone(), + dtype: dtype.clone(), + }) + .collect::>(); + + Ok(resulting) })?; Ok(ConnectionsBackendReply::ListFieldsReply(fields)) @@ -191,21 +187,19 @@ impl RConnection { ConnectionsBackendRequest::GetIcon(GetIconParams { path }) => { // Calls back into R to get the icon. let icon_path = r_task(|| -> Result<_, anyhow::Error> { - unsafe { - let mut call = RFunction::from(".ps.connection_icon"); - call.add(RObject::from(self.comm.comm_id.clone())); - for obj in path { - call.param(obj.kind.as_str(), obj.name); - } - - let icon = call.call()?; - - if r_is_null(*icon) { - // we'd rather use the option type but couldn't find a way to autogenerate RPC optionals - Ok("".to_string()) - } else { - Ok(RObject::to::(icon)?) - } + let mut call = RFunction::from(".ps.connection_icon"); + call.add(RObject::from(self.comm.comm_id.clone())); + for obj in path { + call.param(obj.kind.as_str(), obj.name); + } + + let icon = call.call()?; + + if r_is_null(*icon) { + // we'd rather use the option type but couldn't find a way to autogenerate RPC optionals + Ok("".to_string()) + } else { + Ok(RObject::to::(icon)?) } })?; Ok(ConnectionsBackendReply::GetIconReply(icon_path)) @@ -213,16 +207,14 @@ impl RConnection { ConnectionsBackendRequest::ContainsData(ContainsDataParams { path }) => { // Calls back into R to check if the object contains data. let contains_data = r_task(|| -> Result<_, anyhow::Error> { - unsafe { - let mut contains_data_call: RFunction = - RFunction::from(".ps.connection_contains_data"); - contains_data_call.add(RObject::from(self.comm.comm_id.clone())); - for obj in path { - contains_data_call.param(obj.kind.as_str(), obj.name); - } - let contains_data = contains_data_call.call()?; - Ok(RObject::to::(contains_data)?) + let mut contains_data_call: RFunction = + RFunction::from(".ps.connection_contains_data"); + contains_data_call.add(RObject::from(self.comm.comm_id.clone())); + for obj in path { + contains_data_call.param(obj.kind.as_str(), obj.name); } + let contains_data = contains_data_call.call()?; + Ok(RObject::to::(contains_data)?) })?; Ok(ConnectionsBackendReply::ContainsDataReply(contains_data)) }, @@ -255,12 +247,10 @@ impl RConnection { fn disconnect(&self) -> std::result::Result { // Execute database side disconnect method. r_task(|| -> Result { - unsafe { - let mut call = RFunction::from(".ps.connection_close"); - call.add(RObject::from(self.comm.comm_id.clone())); - let closed = call.call()?; - Ok(RObject::to::(closed)?) - } + let mut call = RFunction::from(".ps.connection_close"); + call.add(RObject::from(self.comm.comm_id.clone())); + let closed = call.call()?; + Ok(RObject::to::(closed)?) }) } diff --git a/crates/ark/src/console/console_repl.rs b/crates/ark/src/console/console_repl.rs index f8a21dc92..2ff4fce1a 100644 --- a/crates/ark/src/console/console_repl.rs +++ b/crates/ark/src/console/console_repl.rs @@ -316,7 +316,7 @@ impl Drop for ConsoleOutputCapture { // Restore previous capture state console.captured_output = self.previous_output.take(); - unsafe { r_poke_option(r_symbol!("warn"), self.previous_warn.sexp) }; + r_poke_option(r_symbol!("warn"), self.previous_warn.sexp); } } @@ -428,50 +428,48 @@ impl Console { libraries.initialize_post_setup_r(); - unsafe { - // Register embedded routines - r_register_routines(); - - // Initialize harp (after routine registration) - harp::initialize(); + // Register embedded routines + r_register_routines(); - // Optionally run a frontend specified R startup script (after harp init) - if let Some(file) = &startup_file { - harp::source(file) - .context(format!("Failed to source startup file '{file}' due to")) - .log_err(); - } + // Initialize harp (after routine registration) + harp::initialize(); - // Initialize support functions (after routine registration, after - // r_task initialization). Intentionally panic if module loading - // fails. Modules are critical for ark to function. - match modules::initialize() { - Err(err) => { - panic!("Failed to load R modules: {err:?}"); - }, - Ok(namespace) => { - console.positron_ns = Some(namespace); - }, - } + // Optionally run a frontend specified R startup script (after harp init) + if let Some(file) = &startup_file { + harp::source(file) + .context(format!("Failed to source startup file '{file}' due to")) + .log_err(); + } - // Populate srcrefs for namespaces already loaded in the session. - // Namespaces of future loaded packages will be populated on load. - // (after r_task initialization) - if do_resource_namespaces() { - if let Err(err) = resource_loaded_namespaces() { - log::error!("Can't populate srcrefs for loaded packages: {err:?}"); - } - } + // Initialize support functions (after routine registration, after + // r_task initialization). Intentionally panic if module loading + // fails. Modules are critical for ark to function. + match modules::initialize() { + Err(err) => { + panic!("Failed to load R modules: {err:?}"); + }, + Ok(namespace) => { + console.positron_ns = Some(namespace); + }, + } - // Set default repositories - if let Err(err) = apply_default_repos(default_repos) { - log::error!("Error setting default repositories: {err:?}"); + // Populate srcrefs for namespaces already loaded in the session. + // Namespaces of future loaded packages will be populated on load. + // (after r_task initialization) + if do_resource_namespaces() { + if let Err(err) = resource_loaded_namespaces() { + log::error!("Can't populate srcrefs for loaded packages: {err:?}"); } + } - // Initialise Ark's last value - libr::SETCDR(r_symbol!(".ark_last_value"), harp::r_null()); + // Set default repositories + if let Err(err) = apply_default_repos(default_repos) { + log::error!("Error setting default repositories: {err:?}"); } + // Initialise Ark's last value + libr::SETCDR(r_symbol!(".ark_last_value"), harp::r_null()); + // Now that R has started (emitting any startup messages that we capture in the // banner), and now that we have set up all hooks and handlers, officially finish // the R initialization process. @@ -711,8 +709,7 @@ impl Console { let previous_output = self.captured_output.replace(String::new()); // Force immediate warning output so it gets captured instead of deferred - let previous_warn = - RObject::new(unsafe { r_poke_option(r_symbol!("warn"), Rf_ScalarInteger(1)) }); + let previous_warn = RObject::new(r_poke_option(r_symbol!("warn"), Rf_ScalarInteger(1))); ConsoleOutputCapture { previous_output, @@ -2241,14 +2238,14 @@ impl Console { // either (i.e. if `UserBreak` is set), but it will reset `UserBreak` // so we need to ensure we handle interrupts right before calling // this. - unsafe { R_ProcessEvents() }; + R_ProcessEvents(); crate::sys::console::run_activity_handlers(); // Run pending finalizers. We need to do this eagerly as otherwise finalizers // might end up being executed on the LSP thread. // https://github.com/rstudio/positron/issues/431 - unsafe { R_RunPendingFinalizers() }; + R_RunPendingFinalizers(); // Check for Positron render requests. // @@ -2338,13 +2335,11 @@ impl Console { /// Converts a data frame to HTML fn to_html(frame: SEXP) -> Result { - unsafe { - let result = RFunction::from(".ps.format.toHtml") - .add(frame) - .call()? - .to::()?; - Ok(result) - } + let result = RFunction::from(".ps.format.toHtml") + .add(frame) + .call()? + .to::()?; + Ok(result) } // Inputs generated by `ReadConsole` for the LSP @@ -2385,7 +2380,7 @@ struct EvalBodyData { /// Simply evaluates the expression in the given frame. unsafe extern "C-unwind" fn eval_body_callback(data: *mut c_void) -> libr::SEXP { let data = unsafe { &*(data as *const EvalBodyData) }; - unsafe { libr::Rf_eval(data.expr, data.frame) } + libr::Rf_eval(data.expr, data.frame) } /// Error handler callback for `R_withCallingErrorHandler` in `Console::eval`. @@ -2399,11 +2394,9 @@ unsafe extern "C-unwind" fn eval_error_callback(err: libr::SEXP, _data: *mut c_v // Call the R-side global error handler which sets the stopped reason, // calls `browser()`, saves the backtrace, and invokes the `abort` or // `browser` restart. - unsafe { - let call = libr::Rf_lang2(r_symbol!(".ps.errors.globalErrorHandler"), err); - libr::Rf_protect(call); - libr::Rf_eval(call, ARK_ENVS.positron_ns); - } + let call = libr::Rf_lang2(r_symbol!(".ps.errors.globalErrorHandler"), err); + libr::Rf_protect(call); + libr::Rf_eval(call, ARK_ENVS.positron_ns); unreachable!("globalErrorHandler longjumps via invokeRestart") } @@ -2599,9 +2592,7 @@ fn r_read_console_impl( ConsoleResult::Interrupt => { log::trace!("Interrupting `ReadConsole()`"); - unsafe { - Rf_onintr(); - } + Rf_onintr(); // This normally does not return log::error!("`Rf_onintr()` did not longjump"); @@ -2614,7 +2605,7 @@ fn r_read_console_impl( // possibility of `CString` conversion failure since the error // message comes from the frontend and might be corrupted. console.r_error_buffer = Some(new_cstring(message)); - unsafe { Rf_error(console.r_error_buffer.as_ref().unwrap().as_ptr()) } + Rf_error(console.r_error_buffer.as_ref().unwrap().as_ptr()) }, } } @@ -2741,24 +2732,22 @@ fn is_auto_printing() -> bool { return false; } - unsafe { - let car = libr::CAR(call.sexp); + let car = libr::CAR(call.sexp); - let Ok(print_fun) = harp::try_eval(r_symbol!("print"), R_ENVS.base) else { - return false; - }; - if car == print_fun.sexp { - return true; - } - - let Ok(methods_ns) = r_ns_env("methods") else { - return false; - }; - let Ok(show_fun) = harp::try_eval(r_symbol!("show"), methods_ns.into()) else { - return false; - }; - car == show_fun.sexp + let Ok(print_fun) = harp::try_eval(r_symbol!("print"), R_ENVS.base) else { + return false; + }; + if car == print_fun.sexp { + return true; } + + let Ok(methods_ns) = r_ns_env("methods") else { + return false; + }; + let Ok(show_fun) = harp::try_eval(r_symbol!("show"), methods_ns.into()) else { + return false; + }; + car == show_fun.sexp } #[harp::register] diff --git a/crates/ark/src/dap/dap_variables.rs b/crates/ark/src/dap/dap_variables.rs index d5d1049d8..31cff61ea 100644 --- a/crates/ark/src/dap/dap_variables.rs +++ b/crates/ark/src/dap/dap_variables.rs @@ -117,7 +117,7 @@ fn env_binding_variable(name: String, x: SEXP) -> Option { return None; } - let symbol = unsafe { r_symbol!(name) }; + let symbol = r_symbol!(name); match r_env_binding_is_active(x, symbol) { Ok(false) => { @@ -435,7 +435,7 @@ mod tests { #[test] fn test_env_binding_variable_base() { - r_task(|| unsafe { + r_task(|| { let env = RFunction::new("base", "new.env") .param("parent", R_ENVS.base) .call() diff --git a/crates/ark/src/data_explorer/format.rs b/crates/ark/src/data_explorer/format.rs index 680c1ad25..4dd497618 100644 --- a/crates/ark/src/data_explorer/format.rs +++ b/crates/ark/src/data_explorer/format.rs @@ -89,17 +89,11 @@ fn format_values(x: SEXP, format_options: &FormatOptions) -> anyhow::Result Ok(format_dbl( - unsafe { NumericVector::new_unchecked(x) }, - format_options, - )), - INTSXP => Ok(format_int( - unsafe { IntegerVector::new_unchecked(x) }, - format_options, - )), - STRSXP => Ok(format_chr(unsafe { CharacterVector::new_unchecked(x) })), - LGLSXP => Ok(format_lgl(unsafe { LogicalVector::new_unchecked(x) })), - CPLXSXP => Ok(format_cpl(unsafe { ComplexVector::new_unchecked(x) })), + REALSXP => Ok(format_dbl(NumericVector::new_unchecked(x), format_options)), + INTSXP => Ok(format_int(IntegerVector::new_unchecked(x), format_options)), + STRSXP => Ok(format_chr(CharacterVector::new_unchecked(x))), + LGLSXP => Ok(format_lgl(LogicalVector::new_unchecked(x))), + CPLXSXP => Ok(format_cpl(ComplexVector::new_unchecked(x))), VECSXP => Ok(format_list(x)), _ => Err(anyhow::anyhow!("Unsupported column type")), } @@ -147,7 +141,7 @@ fn format_object(x: SEXP) -> Vec { return formatted.collect(); }); - unsafe { LogicalVector::new_unchecked(is_na.sexp) } + LogicalVector::new_unchecked(is_na.sexp) .iter() .zip(formatted) .map(|(is_na, v)| { diff --git a/crates/ark/src/data_explorer/r_data_explorer.rs b/crates/ark/src/data_explorer/r_data_explorer.rs index 372fdf59d..7c9ee2ce1 100644 --- a/crates/ark/src/data_explorer/r_data_explorer.rs +++ b/crates/ark/src/data_explorer/r_data_explorer.rs @@ -203,7 +203,7 @@ impl RDataExplorer { let binding = self.binding.as_ref().unwrap(); let env = binding.env.sexp; - let new = unsafe { + let new = { let sym = r_symbol!(binding.name); Rf_findVarInFrame(env, sym) }; @@ -447,88 +447,86 @@ impl CommHandler for RDataExplorer { impl RDataExplorer { pub(crate) fn get_shape(table: RObject) -> anyhow::Result { - unsafe { - let table = table.clone(); + let table = table.clone(); - let Some(kind) = table_kind(table.sexp) else { - return Err(anyhow!("Unsupported type for the data viewer")); - }; - - // `DataFrame::n_row()` will materialize duckplyr compact row names, but we - // are ok with that for the data explorer and don't provide a hook to opt out. - let (n_row, n_col, column_names) = match kind { - TableKind::Dataframe => ( - harp::DataFrame::n_row(table.sexp)?, - harp::DataFrame::n_col(table.sexp)?, - ColumnNames::from_data_frame(table.sexp)?, - ), - TableKind::Matrix => { - let (n_row, n_col) = harp::Matrix::dim(table.sexp)?; - (n_row, n_col, ColumnNames::from_matrix(table.sexp)?) - }, - }; + let Some(kind) = table_kind(table.sexp) else { + return Err(anyhow!("Unsupported type for the data viewer")); + }; - let mut column_schemas = Vec::::new(); - for i in 0..(n_col as isize) { - let column_name = match column_names.get_unchecked(i) { - Some(name) => name, - None => String::from(""), - }; + // `DataFrame::n_row()` will materialize duckplyr compact row names, but we + // are ok with that for the data explorer and don't provide a hook to opt out. + let (n_row, n_col, column_names) = match kind { + TableKind::Dataframe => ( + harp::DataFrame::n_row(table.sexp)?, + harp::DataFrame::n_col(table.sexp)?, + ColumnNames::from_data_frame(table.sexp)?, + ), + TableKind::Matrix => { + let (n_row, n_col) = harp::Matrix::dim(table.sexp)?; + (n_row, n_col, ColumnNames::from_matrix(table.sexp)?) + }, + }; - // TODO: handling for nested data frame columns + let mut column_schemas = Vec::::new(); + for i in 0..(n_col as isize) { + let column_name = match column_names.get_unchecked(i) { + Some(name) => name, + None => String::from(""), + }; - let col = match kind { - harp::TableKind::Dataframe => VECTOR_ELT(table.sexp, i), - harp::TableKind::Matrix => table.sexp, - }; + // TODO: handling for nested data frame columns - let type_name = WorkspaceVariableDisplayType::from(col, false).display_type; - let type_display = display_type(col); - - // Get the label attribute if present (for data frames only) - let column_label = match kind { - harp::TableKind::Dataframe => { - let col_obj = harp::RObject::view(col); - col_obj.get_attribute("label").and_then(|label_obj| { - // CharacterVector::new() already checks if it's a STRSXP - CharacterVector::new(label_obj.sexp) - .ok() - .filter(|cv| cv.len() > 0) // Only proceed if non-empty - .and_then(|cv| cv.get_unchecked(0)) - .and_then(|label| { - // Filter out empty strings - treat them as no label - if label.trim().is_empty() { - None - } else { - Some(label.to_string()) - } - }) - }) - }, - _ => None, - }; + let col = match kind { + harp::TableKind::Dataframe => VECTOR_ELT(table.sexp, i), + harp::TableKind::Matrix => table.sexp, + }; - column_schemas.push(ColumnSchema { - column_name, - column_label, - column_index: i as i64, - type_name, - type_display, - description: None, - children: None, - precision: None, - scale: None, - timezone: None, - type_size: None, - }); - } + let type_name = WorkspaceVariableDisplayType::from(col, false).display_type; + let type_display = display_type(col); + + // Get the label attribute if present (for data frames only) + let column_label = match kind { + harp::TableKind::Dataframe => { + let col_obj = harp::RObject::view(col); + col_obj.get_attribute("label").and_then(|label_obj| { + // CharacterVector::new() already checks if it's a STRSXP + CharacterVector::new(label_obj.sexp) + .ok() + .filter(|cv| cv.len() > 0) // Only proceed if non-empty + .and_then(|cv| cv.get_unchecked(0)) + .and_then(|label| { + // Filter out empty strings - treat them as no label + if label.trim().is_empty() { + None + } else { + Some(label.to_string()) + } + }) + }) + }, + _ => None, + }; - Ok(DataObjectShape { - columns: column_schemas, - kind, - num_rows: n_row, - }) + column_schemas.push(ColumnSchema { + column_name, + column_label, + column_index: i as i64, + type_name, + type_display, + description: None, + children: None, + precision: None, + scale: None, + timezone: None, + type_size: None, + }); } + + Ok(DataObjectShape { + columns: column_schemas, + kind, + num_rows: n_row, + }) } fn launch_get_column_profiles_handler( diff --git a/crates/ark/src/data_explorer/summary_stats.rs b/crates/ark/src/data_explorer/summary_stats.rs index 022a70526..733ec7f80 100644 --- a/crates/ark/src/data_explorer/summary_stats.rs +++ b/crates/ark/src/data_explorer/summary_stats.rs @@ -69,7 +69,7 @@ fn summary_stats_number( ) -> anyhow::Result { let r_stats = call_summary_fn("summary_stats_number", column)?; - let names = unsafe { CharacterVector::new_unchecked(r_names2(r_stats.sexp)) }; + let names = CharacterVector::new_unchecked(r_names2(r_stats.sexp)); let values = format_string(r_stats.sexp, format_options); let r_stats: HashMap = names diff --git a/crates/ark/src/debug.rs b/crates/ark/src/debug.rs index 5e457d3da..1359de547 100644 --- a/crates/ark/src/debug.rs +++ b/crates/ark/src/debug.rs @@ -34,7 +34,7 @@ static _ARK_DISPLAY_VALUE: unsafe extern "C" fn(x: libr::SEXP) -> *const ffi::c_ #[cfg_attr(not(test), no_mangle)] pub extern "C-unwind" fn ark_print_rs(x: libr::SEXP) -> *const ffi::c_char { capture_console_output(|| { - unsafe { libr::Rf_PrintValue(x) }; + libr::Rf_PrintValue(x); }) } @@ -57,7 +57,7 @@ pub extern "C-unwind" fn ark_inspect_rs(x: libr::SEXP) -> *const ffi::c_char { // messing with namedness and refcounts: // https://github.com/r-lib/lobstr/issues/77 let out = RFunction::new("lobstr", "sxp").add(x).call().unwrap(); - unsafe { libr::Rf_PrintValue(out.sexp) }; + libr::Rf_PrintValue(out.sexp); }) } @@ -82,7 +82,7 @@ pub extern "C-unwind" fn ark_trace_back_rs() -> *const ffi::c_char { #[cfg_attr(not(test), no_mangle)] pub extern "C-unwind" fn ark_display_value_rs(x: libr::SEXP) -> *const ffi::c_char { - let value = unsafe { + let value = { let kind = tidy_kind(r_typeof(x)); let tag = format!("<{kind}>"); diff --git a/crates/ark/src/help/r_help.rs b/crates/ark/src/help/r_help.rs index 421ded92c..dadb7f0ab 100644 --- a/crates/ark/src/help/r_help.rs +++ b/crates/ark/src/help/r_help.rs @@ -235,7 +235,7 @@ impl RHelp { let topic = HelpTopic::parse(topic); let found = match topic { - HelpTopic::Simple(symbol) => r_task(|| unsafe { + HelpTopic::Simple(symbol) => r_task(|| { // Try evaluating the help handler first and then fall back to // the default help topic display function. diff --git a/crates/ark/src/help_proxy.rs b/crates/ark/src/help_proxy.rs index de084d03e..78953f7eb 100644 --- a/crates/ark/src/help_proxy.rs +++ b/crates/ark/src/help_proxy.rs @@ -240,7 +240,7 @@ async fn preview_rd(params: web::Query) -> HttpResponse { return HttpResponse::BadGateway().finish(); } - let content = r_task(|| unsafe { + let content = r_task(|| { RFunction::from(".ps.Rd2HTML") .param("rd_file", file) .call() diff --git a/crates/ark/src/lsp/completions/sources/composite/call.rs b/crates/ark/src/lsp/completions/sources/composite/call.rs index 16288dcf8..6c16e3765 100644 --- a/crates/ark/src/lsp/completions/sources/composite/call.rs +++ b/crates/ark/src/lsp/completions/sources/composite/call.rs @@ -211,7 +211,7 @@ fn completions_from_session_arguments( return Ok(Some(completions)); } - let strings = unsafe { + let strings = { RFunction::from(".ps.completions.formalNames") .add(r_callable) .add(object) diff --git a/crates/ark/src/lsp/completions/sources/unique/comment.rs b/crates/ark/src/lsp/completions/sources/unique/comment.rs index 979e0fdbf..596f1aec2 100644 --- a/crates/ark/src/lsp/completions/sources/unique/comment.rs +++ b/crates/ark/src/lsp/completions/sources/unique/comment.rs @@ -64,7 +64,7 @@ fn completions_from_comment( // TODO: cache these? // TODO: use an indexer to build the tag list? - let tags = unsafe { + let tags = { RFunction::new("base", "system.file") .param("package", "roxygen2") .add("roxygen2-tags.yml") @@ -172,7 +172,7 @@ fn test_roxygen_comment() { use crate::lsp::document::Document; use crate::r_task; - r_task(|| unsafe { + r_task(|| { let installed = RFunction::new("", ".ps.is_installed") .add("roxygen2") .add("7.2.1.9000") diff --git a/crates/ark/src/lsp/completions/sources/unique/namespace.rs b/crates/ark/src/lsp/completions/sources/unique/namespace.rs index 5117b76cd..0ce8ef7a0 100644 --- a/crates/ark/src/lsp/completions/sources/unique/namespace.rs +++ b/crates/ark/src/lsp/completions/sources/unique/namespace.rs @@ -94,7 +94,7 @@ fn completions_from_namespace( list_namespace_symbols(*namespace) }; - let strings = unsafe { symbols.to::>()? }; + let strings = symbols.to::>()?; for string in strings.iter() { let item = unsafe { @@ -219,7 +219,7 @@ fn completions_from_namespace_lazydata( } fn list_namespace_symbols(namespace: SEXP) -> RObject { - unsafe { RObject::new(R_lsInternal(namespace, 1)) } + RObject::new(R_lsInternal(namespace, 1)) } fn list_namespace_exports(namespace: SEXP) -> RObject { diff --git a/crates/ark/src/lsp/signature_help.rs b/crates/ark/src/lsp/signature_help.rs index d8742c367..d00318347 100644 --- a/crates/ark/src/lsp/signature_help.rs +++ b/crates/ark/src/lsp/signature_help.rs @@ -611,11 +611,11 @@ fn <- function( #[test] fn test_argument_label_symbol() { crate::r_task(|| { - let x = unsafe { r_symbol!("name") }; + let x = r_symbol!("name"); let label = argument_label(String::from("x"), x); assert_eq!(label, String::from("x = name")); - let x = unsafe { r_symbol!("hi there") }; + let x = r_symbol!("hi there"); let label = argument_label(String::from("x"), x); assert_eq!(label, String::from("x = `hi there`")); }) @@ -624,12 +624,10 @@ fn <- function( #[test] fn test_argument_label_call() { crate::r_task(|| { - let x = unsafe { - RCall::new(r_symbol!("source")) - .add(r_symbol!("exprs")) - .param("local", r_symbol!("is_local")) - .build() - }; + let x = RCall::new(r_symbol!("source")) + .add(r_symbol!("exprs")) + .param("local", r_symbol!("is_local")) + .build(); let label = argument_label(String::from("x"), x.sexp); assert_eq!(label, String::from("x = source(exprs, local = is_local)")); }) @@ -687,8 +685,8 @@ fn <- function( assert_eq!(label, String::from("x = c(1+2.5i, Inf-Infi, NA+2.5i)")); let x = RObject::from(r_alloc_character(3)); - r_chr_poke(x.sexp, 0, unsafe { r_char!("hi") }); - r_chr_poke(x.sexp, 1, unsafe { r_char!("there") }); + r_chr_poke(x.sexp, 0, r_char!("hi")); + r_chr_poke(x.sexp, 1, r_char!("there")); r_chr_poke(x.sexp, 2, r_str_na()); let label = argument_label(String::from("x"), x.sexp); assert_eq!(label, String::from("x = c(\"hi\", \"there\", NA)")); @@ -736,7 +734,7 @@ fn <- function( assert_eq!(label, String::from("x = 1.5+2.5i")); let x = RObject::from(r_alloc_character(1)); - r_chr_poke(x.sexp, 0, unsafe { r_char!("hi") }); + r_chr_poke(x.sexp, 0, r_char!("hi")); let label = argument_label(String::from("x"), x.sexp); assert_eq!(label, String::from("x = \"hi\"")); }) diff --git a/crates/ark/src/modules.rs b/crates/ark/src/modules.rs index 1631bf024..7e42c33ba 100644 --- a/crates/ark/src/modules.rs +++ b/crates/ark/src/modules.rs @@ -152,7 +152,7 @@ pub fn initialize() -> anyhow::Result { // Do this separately with a bare eval because `errors_initialize()` should // be called without any condition handlers on the stack let init = RFunction::from("initialize_errors"); - unsafe { libr::Rf_eval(init.call.build().sexp, namespace.sexp) }; + libr::Rf_eval(init.call.build().sexp, namespace.sexp); // Lock all module environments now that loading is complete. In debug // builds we skip the lock so modules can be hot-reloaded by the watcher. @@ -303,10 +303,8 @@ mod debug { } fn r_poke_option_ark_testing() { - unsafe { - let value = Rf_ScalarLogical(1); - r_poke_option(r_symbol!("ark.testing"), value); - } + let value = Rf_ScalarLogical(1); + r_poke_option(r_symbol!("ark.testing"), value); } #[harp::register] diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index ab16da17d..a8d1a22e4 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -457,7 +457,7 @@ impl DeviceContext { match result { Ok(kind) => { // Safety: We just called an R function that returns a string - unsafe { kind.to::() }.unwrap_or_else(|err| { + kind.to::().unwrap_or_else(|err| { log::warn!("Failed to convert plot kind to string: {err:?}"); "plot".to_string() }) @@ -942,7 +942,7 @@ impl DeviceContext { fn render_plot(&self, id: &PlotId, settings: &PlotRenderSettings) -> anyhow::Result { log::trace!("Rendering plot"); - let image_path = r_task(|| unsafe { + let image_path = r_task(|| { RFunction::from(".ps.graphics.render_plot_from_recording") .param("id", id) .param("width", RObject::try_from(settings.size.width)?) diff --git a/crates/ark/src/repos.rs b/crates/ark/src/repos.rs index 2a353c2f5..14bc2661a 100644 --- a/crates/ark/src/repos.rs +++ b/crates/ark/src/repos.rs @@ -359,7 +359,7 @@ fn get_ppm_binary_package_repo(repo_url: Option) -> String { #[harp::register] pub extern "C-unwind" fn ps_get_ppm_binary_url(url: SEXP) -> anyhow::Result { - let url_string = unsafe { + let url_string = { RObject::view(url) .to::() .context("`url` must be a string")? diff --git a/crates/ark/src/srcref.rs b/crates/ark/src/srcref.rs index c07e015cd..9781a6f2c 100644 --- a/crates/ark/src/srcref.rs +++ b/crates/ark/src/srcref.rs @@ -161,49 +161,47 @@ fn generate_source( // Inject source references in functions. This is slightly unsafe but we // couldn't think of a dire failure mode. - unsafe { - // First replace the body which contains expressions tagged with srcrefs - // such as calls to `{`. Compiled functions are a little more tricky. - let body = harp::fn_body(old.sexp); - - if r_typeof(body) == BCODESXP { - // This is a compiled function. We could recompile the fresh - // function we just created but the compiler is very slow. Instead, - // update the expression stored in the bytecode. This expression is - // used by `eval()` when stepping with the debugger. - - // Get the constant pool: BCODE_CONSTS = CDR - let consts = CDR(body); - - // The original body expression is stored as first element - // of the constant pool - if r_length(consts) > 0 { - // Inject new body instrumented with source references - SET_VECTOR_ELT(consts, 0, R_ClosureExpr(new)); - } - - harp::attrib_poke( - old.sexp, - r_symbol!("srcref"), - harp::attrib_get(new, r_symbol!("srcref")), - ); - } else { - let new_body = harp::fn_body(new); - let out = RObject::new(harp::new_function( - harp::fn_formals(old.sexp), - new_body, - harp::fn_env(old.sexp), - )); - - harp::attrib_poke_from(out.sexp, old.sexp); - harp::attrib_poke( - out.sexp, - r_symbol!("srcref"), - harp::attrib_get(new, r_symbol!("srcref")), - ); - - harp::env_bind_force(ns_env, binding.name.sexp, out.sexp); + // First replace the body which contains expressions tagged with srcrefs + // such as calls to `{`. Compiled functions are a little more tricky. + let body = harp::fn_body(old.sexp); + + if r_typeof(body) == BCODESXP { + // This is a compiled function. We could recompile the fresh + // function we just created but the compiler is very slow. Instead, + // update the expression stored in the bytecode. This expression is + // used by `eval()` when stepping with the debugger. + + // Get the constant pool: BCODE_CONSTS = CDR + let consts = CDR(body); + + // The original body expression is stored as first element + // of the constant pool + if r_length(consts) > 0 { + // Inject new body instrumented with source references + SET_VECTOR_ELT(consts, 0, R_ClosureExpr(new)); } + + harp::attrib_poke( + old.sexp, + r_symbol!("srcref"), + harp::attrib_get(new, r_symbol!("srcref")), + ); + } else { + let new_body = harp::fn_body(new); + let out = RObject::new(harp::new_function( + harp::fn_formals(old.sexp), + new_body, + harp::fn_env(old.sexp), + )); + + harp::attrib_poke_from(out.sexp, old.sexp); + harp::attrib_poke( + out.sexp, + r_symbol!("srcref"), + harp::attrib_get(new, r_symbol!("srcref")), + ); + + harp::env_bind_force(ns_env, binding.name.sexp, out.sexp); } let text: Vec = RObject::view(text).try_into()?; diff --git a/crates/ark/src/startup.rs b/crates/ark/src/startup.rs index c102a3e92..d3a72913f 100644 --- a/crates/ark/src/startup.rs +++ b/crates/ark/src/startup.rs @@ -73,7 +73,7 @@ fn source_r_profile(path: &PathBuf) { // call it when there are handlers on the stack). That is a common place to // register global calling handlers, including in Gabor's prompt package. // Source in the global env to mimic R. - let result = unsafe { + let result = { let call = RFunction::new("base", "sys.source") .param("file", path) .param("envir", R_ENVS.global) diff --git a/crates/ark/src/ui/ui_comm.rs b/crates/ark/src/ui/ui_comm.rs index a2d81635d..37dc4fd6d 100644 --- a/crates/ark/src/ui/ui_comm.rs +++ b/crates/ark/src/ui/ui_comm.rs @@ -180,7 +180,7 @@ impl UiComm { let method = format!(".ps.rpc.{}", request.method); // Use the `exists` function to see if the method exists - let exists = r_task(|| unsafe { + let exists = r_task(|| { let exists = RFunction::from("exists") .param("x", method.clone()) .call()?; @@ -347,7 +347,7 @@ mod tests { let ui_comm_tx = UiComm::start(comm_socket.clone(), stdin_request_tx, graphics_device_tx); // Get the current console width - let old_width = r_task(|| unsafe { + let old_width = r_task(|| { let width = RFunction::from("getOption") .param("x", "width") .call() @@ -387,7 +387,7 @@ mod tests { } // Get the new console width - let new_width = r_task(|| unsafe { + let new_width = r_task(|| { let width = RFunction::from("getOption") .param("x", "width") .call() diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 8305fb4df..80bc50579 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -214,17 +214,15 @@ impl WorkspaceVariableDisplayValue { } fn from_closure(value: SEXP) -> Self { - unsafe { - let args = RFunction::from("args").add(value).call().unwrap(); - let formatted = RFunction::from("format").add(args.sexp).call().unwrap(); - let formatted = CharacterVector::new_unchecked(formatted); - let out = formatted - .iter() - .take(formatted.len() - 1) - .map(|o| o.unwrap()) - .join(""); - Self::new(out, false) - } + let args = RFunction::from("args").add(value).call().unwrap(); + let formatted = RFunction::from("format").add(args.sexp).call().unwrap(); + let formatted = CharacterVector::new_unchecked(formatted); + let out = formatted + .iter() + .take(formatted.len() - 1) + .map(|o| o.unwrap()) + .join(""); + Self::new(out, false) } fn from_env(value: SEXP) -> Self { @@ -453,7 +451,7 @@ impl WorkspaceVariableDisplayType { match rtype { EXPRSXP => { let default = match include_length { - true => format!("expression [{}]", unsafe { Rf_xlength(value) }), + true => format!("expression [{}]", Rf_xlength(value)), false => String::from("expression"), }; Self::from_class(value, default) @@ -477,7 +475,7 @@ impl WorkspaceVariableDisplayType { false => Self::simple(String::from("pairlist")), }, - VECSXP => unsafe { + VECSXP => { if r_is_data_frame(value) { let classes = r_classes(value).unwrap(); let dfclass = classes.get_unchecked(0).unwrap(); @@ -562,25 +560,21 @@ fn has_children(value: SEXP) -> bool { } if RObject::view(value).is_s4() { - unsafe { - let names = RFunction::new("methods", ".slotNames") - .add(value) - .call() - .unwrap(); - let names = CharacterVector::new_unchecked(names); - names.len() > 0 - } + let names = RFunction::new("methods", ".slotNames") + .add(value) + .call() + .unwrap(); + let names = CharacterVector::new_unchecked(names); + names.len() > 0 } else { match r_typeof(value) { - VECSXP | EXPRSXP => unsafe { Rf_xlength(value) != 0 }, + VECSXP | EXPRSXP => Rf_xlength(value) != 0, LISTSXP => true, ENVSXP => { !Environment::new_filtered(RObject::view(value), EnvironmentFilter::ExcludeHidden) .is_empty() }, - LGLSXP | RAWSXP | STRSXP | INTSXP | REALSXP | CPLXSXP => unsafe { - Rf_xlength(value) > 1 - }, + LGLSXP | RAWSXP | STRSXP | INTSXP | REALSXP | CPLXSXP => Rf_xlength(value) > 1, _ => false, } } @@ -701,21 +695,19 @@ impl PositronVariable { fn from_promise(display_name: String, promise: SEXP) -> Self { let display_value = local! { - unsafe { - let code = PRCODE(promise); - match r_typeof(code) { - SYMSXP => { - Ok(RSymbol::new_unchecked(code).to_string()) - }, - LANGSXP => { - let fun = RSymbol::new(CAR(code))?; - if fun == "lazyLoadDBfetch" { - return Ok(String::from("(unevaluated)")) - } - harp::call::expr_deparse_collapse(code) - }, - _ => Err(Error::UnexpectedType(r_typeof(code), vec!(SYMSXP, LANGSXP))) - } + let code = PRCODE(promise); + match r_typeof(code) { + SYMSXP => { + Ok(RSymbol::new_unchecked(code).to_string()) + }, + LANGSXP => { + let fun = RSymbol::new(CAR(code))?; + if fun == "lazyLoadDBfetch" { + return Ok(String::from("(unevaluated)")) + } + harp::call::expr_deparse_collapse(code) + }, + _ => Err(Error::UnexpectedType(r_typeof(code), vec!(SYMSXP, LANGSXP))) } }; @@ -791,10 +783,10 @@ impl PositronVariable { // Otherwise treat as vector let rtype = r_typeof(x); match rtype { - LGLSXP | RAWSXP | INTSXP | REALSXP | CPLXSXP | STRSXP | LISTSXP => unsafe { + LGLSXP | RAWSXP | INTSXP | REALSXP | CPLXSXP | STRSXP | LISTSXP => { Rf_xlength(x) as usize }, - VECSXP => unsafe { + VECSXP => { // TODO: Support vctrs types like record vectors if r_inherits(x, "POSIXlt") && r_typeof(x) == VECSXP && r_length(x) > 0 { Rf_xlength(VECTOR_ELT(x, 0)) as usize @@ -1048,8 +1040,8 @@ impl PositronVariable { object: RObject, access_key: &String, ) -> harp::Result { - let symbol = unsafe { r_symbol!(access_key) }; - let mut x = unsafe { Rf_findVarInFrame(object.sexp, symbol) }; + let symbol = r_symbol!(access_key); + let mut x = Rf_findVarInFrame(object.sexp, symbol); if r_typeof(x) == PROMSXP { // if we are here, it means the promise is either evaluated @@ -1060,9 +1052,9 @@ impl PositronVariable { // Actual promises, i.e. unevaluated promises can't be // expanded in the variables pane so we would not get here. - let value = unsafe { PRVALUE(x) }; + let value = PRVALUE(x); if r_is_unbound(value) { - x = unsafe { PRCODE(x) }; + x = PRCODE(x); } else { x = value; } @@ -1121,9 +1113,8 @@ impl PositronVariable { // For S4 objects, we acess child nodes using R_do_slot. if object.is_s4() { - let name = unsafe { r_symbol!(access_key) }; - let child: RObject = - harp::try_catch(|| unsafe { R_do_slot(object.sexp, name) }.into())?; + let name = r_symbol!(access_key); + let child: RObject = harp::try_catch(|| R_do_slot(object.sexp, name).into())?; return Ok(EnvironmentVariableNode::Concrete { object: child }); } @@ -1148,10 +1139,10 @@ impl PositronVariable { let mut pairlist = object.sexp; let index = parse_index(access_key)?; for _i in 0..index { - pairlist = unsafe { CDR(pairlist) }; + pairlist = CDR(pairlist); } Ok(EnvironmentVariableNode::Concrete { - object: RObject::view(unsafe { CAR(pairlist) }), + object: RObject::view(CAR(pairlist)), }) }, LGLSXP | RAWSXP | STRSXP | INTSXP | REALSXP | CPLXSXP => { @@ -1516,17 +1507,15 @@ impl PositronVariable { fn inspect_s4(value: SEXP) -> Result, harp::error::Error> { let mut out: Vec = vec![]; - unsafe { - let slot_names = RFunction::new("methods", ".slotNames").add(value).call()?; - - let slot_names = CharacterVector::new_unchecked(slot_names.sexp); - let mut iter = slot_names.iter(); - while let Some(Some(display_name)) = iter.next() { - let slot_symbol = r_symbol!(display_name); - let slot: RObject = harp::try_catch(|| R_do_slot(value, slot_symbol).into())?; - let access_key = display_name.clone(); - out.push(PositronVariable::from(access_key, display_name, slot.sexp).var()); - } + let slot_names = RFunction::new("methods", ".slotNames").add(value).call()?; + + let slot_names = CharacterVector::new_unchecked(slot_names.sexp); + let mut iter = slot_names.iter(); + while let Some(Some(display_name)) = iter.next() { + let slot_symbol = r_symbol!(display_name); + let slot: RObject = harp::try_catch(|| R_do_slot(value, slot_symbol).into())?; + let access_key = display_name.clone(); + out.push(PositronVariable::from(access_key, display_name, slot.sexp).var()); } Ok(out) @@ -1570,7 +1559,7 @@ impl PositronVariable { // 1. Provide the name and the index for the `get_child_at` method. // 2. (Not necessary) Given an access key, we can detect if we want to apply a custom get_child_method. let list = List::new(value.sexp)?; - let n = unsafe { list.len() }; + let n = list.len(); let names = match value.names() { None => vec![None; n], diff --git a/crates/ark/tests/connections.rs b/crates/ark/tests/connections.rs index 48bbddef4..672700aef 100644 --- a/crates/ark/tests/connections.rs +++ b/crates/ark/tests/connections.rs @@ -29,7 +29,7 @@ fn open_dummy_connection() -> (socket::comm::CommSocket, Receiver) // Create a dummy iopub channel to receive responses. let (iopub_tx, iopub_rx) = bounded::(10); - let comm_id = r_task(|| unsafe { + let comm_id = r_task(|| { let mut dummy_connection = RFunction::new("", ".ps.register_dummy_connection"); let comm_id = dummy_connection.call_in(ARK_ENVS.positron_ns)?; RObject::to::(comm_id) diff --git a/crates/ark/tests/help.rs b/crates/ark/tests/help.rs index 78a4ec16c..4905f70c5 100644 --- a/crates/ark/tests/help.rs +++ b/crates/ark/tests/help.rs @@ -107,7 +107,7 @@ fn test_help_comm() { r_help.test_topic("utils:::find", "help-test-id-3"); // Figure out which port the R help server is running on (or would run on) - let r_help_port = r_task(|| unsafe { + let r_help_port = r_task(|| { RFunction::new_internal("tools", "httpdPort") .call()? .to::() @@ -151,7 +151,7 @@ fn test_custom_help_handlers() { r_help.test_topic("obj$hello", "help-test-id-4"); assert_eq!( - r_task(|| unsafe { harp::parse_eval_global("called").unwrap().to::() }).unwrap(), + r_task(|| { harp::parse_eval_global("called").unwrap().to::() }).unwrap(), true, ); } diff --git a/crates/ark/tests/variables.rs b/crates/ark/tests/variables.rs index ec7587359..ee1eacb27 100644 --- a/crates/ark/tests/variables.rs +++ b/crates/ark/tests/variables.rs @@ -97,7 +97,7 @@ fn test_variables_list() { // Now create a variable in the R environment and ensure we get a list of // variables with the new variable in it. - r_task(|| unsafe { + r_task(|| { let test_env = test_env.get().clone(); let sym = r_symbol!("everything"); Rf_defineVar(sym, Rf_ScalarInteger(42), *test_env); @@ -171,7 +171,7 @@ fn test_variables_list() { } // Create another variable - r_task(|| unsafe { + r_task(|| { let test_env = test_env.get().clone(); r_envir_set("nothing", Rf_ScalarInteger(43), *test_env); r_envir_remove("everything", *test_env); @@ -253,14 +253,14 @@ fn test_variables_list() { } // test the env is now empty - r_task(|| unsafe { + r_task(|| { let test_env = test_env.get().clone(); let contents = RObject::new(R_lsInternal(*test_env, Rboolean_TRUE)); assert_eq!(Rf_xlength(*contents), 0); }); // Create some more variables - r_task(|| unsafe { + r_task(|| { let test_env = test_env.get().clone(); let sym = r_symbol!("a"); diff --git a/crates/harp/src/attrib.rs b/crates/harp/src/attrib.rs index 1958112e9..d33679e7c 100644 --- a/crates/harp/src/attrib.rs +++ b/crates/harp/src/attrib.rs @@ -23,42 +23,38 @@ pub fn zap_srcref(x: SEXP) -> RObject { } fn zap_srcref_fn(x: SEXP) -> RObject { - unsafe { - let formals = fn_formals(x); - let body = fn_body(x); - let env = fn_env(x); + let formals = fn_formals(x); + let body = fn_body(x); + let env = fn_env(x); - let new_body = zap_srcref(body); - let out = RObject::new(new_function(formals, new_body.sexp, env)); + let new_body = zap_srcref(body); + let out = RObject::new(new_function(formals, new_body.sexp, env)); - // Copy attributes from the original, but zap `srcref` - attrib_poke_from(out.sexp, x); - attrib_poke(out.sexp, r_symbol!("srcref"), r_null()); + // Copy attributes from the original, but zap `srcref` + attrib_poke_from(out.sexp, x); + attrib_poke(out.sexp, r_symbol!("srcref"), r_null()); - out - } + out } fn zap_srcref_call(x: SEXP) -> RObject { - unsafe { - let x = RObject::view(x).shallow_duplicate(); - - zap_srcref_attrib(x.sexp); + let x = RObject::view(x).shallow_duplicate(); - if libr::CAR(x.sexp) == r_symbol!("function") { - // Remove `call[[4]]` where the parser stores srcref information - // for calls to `function` - libr::SETCDR(libr::CDDR(x.sexp), r_null()); - } + zap_srcref_attrib(x.sexp); - let mut node = x.sexp; - while node != r_null() { - libr::SETCAR(node, zap_srcref(libr::CAR(node)).sexp); - node = libr::CDR(node); - } + if libr::CAR(x.sexp) == r_symbol!("function") { + // Remove `call[[4]]` where the parser stores srcref information + // for calls to `function` + libr::SETCDR(libr::CDDR(x.sexp), r_null()); + } - x + let mut node = x.sexp; + while node != r_null() { + libr::SETCAR(node, zap_srcref(libr::CAR(node)).sexp); + node = libr::CDR(node); } + + x } fn zap_srcref_expr(x: SEXP) -> RObject { diff --git a/crates/harp/src/column_names.rs b/crates/harp/src/column_names.rs index 120711b92..0ddce5481 100644 --- a/crates/harp/src/column_names.rs +++ b/crates/harp/src/column_names.rs @@ -20,7 +20,7 @@ pub struct ColumnNames { impl ColumnNames { pub fn new(names: SEXP) -> Self { let names = if r_typeof(names) == STRSXP { - Some(unsafe { CharacterVector::new_unchecked(names) }) + Some(CharacterVector::new_unchecked(names)) } else { None }; diff --git a/crates/harp/src/data_frame.rs b/crates/harp/src/data_frame.rs index 600246956..434fe4c43 100644 --- a/crates/harp/src/data_frame.rs +++ b/crates/harp/src/data_frame.rs @@ -44,7 +44,7 @@ impl DataFrame { for obj in list.iter() { let obj = RObject::view(obj); - if unsafe { libr::Rf_isVector(obj.sexp) == 0 } { + if libr::Rf_isVector(obj.sexp) == 0 { return Err(harp::anyhow!("Data frame column must be a vector")); } diff --git a/crates/harp/src/environment.rs b/crates/harp/src/environment.rs index bb6da1202..4cef4665d 100644 --- a/crates/harp/src/environment.rs +++ b/crates/harp/src/environment.rs @@ -53,7 +53,7 @@ impl Environment { /// environment pub fn new_empty() -> Self { // Passing `size = 0` causes default size to be picked up - let env = unsafe { libr::R_NewEnv(R_ENVS.empty, 1, 0) }; + let env = libr::R_NewEnv(R_ENVS.empty, 1, 0); Self::new(RObject::new(env)) } @@ -85,9 +85,7 @@ impl Environment { } pub fn bind(&self, name: RSymbol, value: &RObject) { - unsafe { - Rf_defineVar(name.sexp, value.sexp, self.inner.sexp); - } + Rf_defineVar(name.sexp, value.sexp, self.inner.sexp); } pub fn force_bind(&self, name: RSymbol, value: &RObject) { @@ -108,7 +106,7 @@ impl Environment { } pub fn exists(&self, name: impl Into) -> bool { - unsafe { libr::R_existsVarInFrame(self.inner.sexp, name.into().sexp) != 0 } + libr::R_existsVarInFrame(self.inner.sexp, name.into().sexp) != 0 } pub fn find(&self, name: impl Into) -> harp::Result { @@ -187,8 +185,7 @@ impl Environment { // `all = all_names`, `sorted = false` // We don't sort the elements when fetchhing from R, but sort them // later in Rust - let names = - RObject::from(unsafe { R_lsInternal3(self.inner.sexp, all_names, Rboolean_FALSE) }); + let names = RObject::from(R_lsInternal3(self.inner.sexp, all_names, Rboolean_FALSE)); let mut names = Vec::::try_from(names).unwrap_or(Vec::new()); names.sort(); @@ -196,29 +193,23 @@ impl Environment { } pub fn lock(&self, bindings: bool) { - unsafe { - libr::R_LockEnvironment(self.inner.sexp, bindings.into()); - } + libr::R_LockEnvironment(self.inner.sexp, bindings.into()); } pub fn lock_binding(&self, name: RSymbol) { - unsafe { - libr::R_LockBinding(name.sexp, self.inner.sexp); - } + libr::R_LockBinding(name.sexp, self.inner.sexp); } pub fn unlock_binding(&self, name: RSymbol) { - unsafe { - libr::R_unLockBinding(name.sexp, self.inner.sexp); - } + libr::R_unLockBinding(name.sexp, self.inner.sexp); } pub fn is_locked(&self) -> bool { - unsafe { libr::R_EnvironmentIsLocked(self.inner.sexp) != 0 } + libr::R_EnvironmentIsLocked(self.inner.sexp) != 0 } pub fn is_locked_binding(&self, name: RSymbol) -> bool { - unsafe { libr::R_BindingIsLocked(name.sexp, self.inner.sexp) != 0 } + libr::R_BindingIsLocked(name.sexp, self.inner.sexp) != 0 } pub fn is_active(&self, name: RSymbol) -> harp::Result { @@ -282,16 +273,14 @@ mod tests { .call() .unwrap(); - unsafe { - let sym = r_symbol!("a"); - Rf_defineVar(sym, Rf_ScalarInteger(42), test_env.sexp); + let sym = r_symbol!("a"); + Rf_defineVar(sym, Rf_ScalarInteger(42), test_env.sexp); - let sym = r_symbol!("b"); - Rf_defineVar(sym, Rf_ScalarInteger(43), test_env.sexp); + let sym = r_symbol!("b"); + Rf_defineVar(sym, Rf_ScalarInteger(43), test_env.sexp); - let sym = r_symbol!("c"); - Rf_defineVar(sym, Rf_ScalarInteger(44), test_env.sexp); - } + let sym = r_symbol!("c"); + Rf_defineVar(sym, Rf_ScalarInteger(44), test_env.sexp); Environment::new(test_env) } diff --git a/crates/harp/src/exec.rs b/crates/harp/src/exec.rs index 1fb82c735..8e38eab54 100644 --- a/crates/harp/src/exec.rs +++ b/crates/harp/src/exec.rs @@ -57,21 +57,19 @@ impl RFunction { } fn new_ext(package: &str, function: &str, internal: bool) -> Self { - unsafe { - let is_namespaced = !package.is_empty(); - - let fun = if is_namespaced { - let op = if internal { ":::" } else { "::" }; - Rf_lang3(r_symbol!(op), r_symbol!(package), r_symbol!(function)) - } else { - r_symbol!(function) - }; - let fun = RObject::new(fun); + let is_namespaced = !package.is_empty(); - RFunction { - call: RCall::new(fun), - is_namespaced, - } + let fun = if is_namespaced { + let op = if internal { ":::" } else { "::" }; + Rf_lang3(r_symbol!(op), r_symbol!(package), r_symbol!(function)) + } else { + r_symbol!(function) + }; + let fun = RObject::new(fun); + + RFunction { + call: RCall::new(fun), + is_namespaced, } } @@ -98,7 +96,7 @@ impl RFunction { /// /// Calls `Rf_eval()` inside `try_catch()`. pub fn try_eval(expr: SEXP, env: SEXP) -> crate::Result { - let mut res = try_catch(|| unsafe { Rf_eval(expr, env) }).map(RObject::from); + let mut res = try_catch(|| Rf_eval(expr, env)).map(RObject::from); if let Err(Error::TryCatchError { ref mut call, .. }) = res { if call.is_none() { @@ -175,7 +173,7 @@ pub type CallingErrorHandlerCallback = /// a local calling error handler on `R_HandlerStack`. Local handlers have /// priority over global handlers, which is useful when you need to intercept /// errors before other handlers (like R's debug handler) can catch them. -pub unsafe fn with_calling_error_handler( +pub fn with_calling_error_handler( body: CallingErrorHandlerBody, body_data: *mut c_void, handler: CallingErrorHandlerCallback, @@ -291,14 +289,12 @@ where }; // We've dropped our non-POD types and are ready to jump - unsafe { - libr::Rf_protect(call); - libr::Rf_eval(call, R_ENVS.base); - } + libr::Rf_protect(call); + libr::Rf_eval(call, R_ENVS.base); unreachable!(); } - let longjump = top_level_exec(|| unsafe { + let longjump = top_level_exec(|| { with_calling_error_handler(callback::, payload, handler::, payload); }); @@ -370,7 +366,7 @@ where *(data.res) = Some(Ok(closure())); } - unsafe { R_ToplevelExec(Some(callback::), payload) }; + R_ToplevelExec(Some(callback::), payload); match res { Some(res) => res, @@ -472,7 +468,7 @@ where } // Call into R; the callbacks will populate `res` and always return R_NilValue. - unsafe { + { R_ExecWithCleanup( Some(exec_callback::), payload, @@ -487,7 +483,7 @@ where pub fn r_peek_error_buffer() -> String { // SAFETY: Returns pointer to static memory buffer owned by R. - let buffer = unsafe { R_curErrorBuf() }; + let buffer = R_curErrorBuf(); // SAFETY: The aforementioned buffer is never null. let cstr = unsafe { CStr::from_ptr(buffer) }; @@ -540,9 +536,7 @@ where Ok(out) => { // First check for interrupts since we might just have spent some // time in a sandbox - unsafe { - R_CheckUserInterrupt(); - } + R_CheckUserInterrupt(); return out; }, Err(err) => format!("{err:}"), @@ -556,9 +550,7 @@ where // protection stack. We're relying on automatic unprotection after an // error, which requires `r_unwrap()` to be run within an R context // frame such as `.Call()` or `R_ExecWithCleanup()`. - unsafe { - Rf_protect(sexp_msg); - } + Rf_protect(sexp_msg); // Clear the Rust stack. We only need to drop `robj_msg` because `out` // was moved to `msg` and `msg` to `robj_msg` already. @@ -576,37 +568,35 @@ where /// close to the limit. The latter case is useful for stopping recursive /// algorithms from blowing the stack. pub fn r_check_stack(size: Option) -> Result<()> { - unsafe { - let out = top_level_exec(|| { - if let Some(size) = size { - R_CheckStack2(size); - } else { - R_CheckStack(); - } - }); - - match out { - Ok(_) => Ok(()), - Err(err) => { - // Reset error buffer because we detect stack overflows by - // inspecting this buffer, see `peek_execute_response()` - let _ = RFunction::new("base", "stop").call(); - - // Convert TopLevelExecError to StackUsageError - match err { - Error::TopLevelExecError { - message, - backtrace, - span_trace, - } => Err(Error::StackUsageError { - message, - backtrace, - span_trace, - }), - _ => unreachable!(), - } - }, + let out = top_level_exec(|| { + if let Some(size) = size { + R_CheckStack2(size); + } else { + R_CheckStack(); } + }); + + match out { + Ok(_) => Ok(()), + Err(err) => { + // Reset error buffer because we detect stack overflows by + // inspecting this buffer, see `peek_execute_response()` + let _ = RFunction::new("base", "stop").call(); + + // Convert TopLevelExecError to StackUsageError + match err { + Error::TopLevelExecError { + message, + backtrace, + span_trace, + } => Err(Error::StackUsageError { + message, + backtrace, + span_trace, + }), + _ => unreachable!(), + } + }, } } @@ -624,7 +614,7 @@ mod tests { #[test] fn test_basic_function() { - crate::r_task(|| unsafe { + crate::r_task(|| { // try adding some numbers let result = RFunction::new("", "+").add(2).add(2).call().unwrap(); @@ -649,7 +639,7 @@ mod tests { #[test] fn test_utf8_strings() { - crate::r_task(|| unsafe { + crate::r_task(|| { // try sending some UTF-8 strings to and from R let result = RFunction::new("base", "paste") .add("世界") @@ -669,7 +659,7 @@ mod tests { #[test] fn test_named_arguments() { - crate::r_task(|| unsafe { + crate::r_task(|| { let result = RFunction::new("stats", "rnorm") .add(1.0) .param("mean", 10) @@ -684,7 +674,7 @@ mod tests { #[test] fn test_try_catch_error() { - crate::r_task(|| unsafe { + crate::r_task(|| { // ok SEXP let ok: harp::Result = try_catch(|| Rf_ScalarInteger(42).into()); assert_match!(ok, Ok(value) => { @@ -774,7 +764,7 @@ mod tests { let result = exec_with_cleanup( || { // Create a simple R object and return it directly (T = RObject) - let obj = RObject::from(unsafe { Rf_ScalarInteger(42) }); + let obj = RObject::from(Rf_ScalarInteger(42)); obj }, || { @@ -782,7 +772,7 @@ mod tests { }, ); - assert_eq!(unsafe { Rf_asInteger(*result) }, 42); + assert_eq!(Rf_asInteger(*result), 42); assert!( *cleanup_called.lock().unwrap(), "Cleanup should have been called" @@ -796,7 +786,7 @@ mod tests { exec_with_cleanup( || -> RObject { let msg = CString::new("ouch").unwrap(); // This leaks - unsafe { Rf_error(msg.as_ptr()) }; + Rf_error(msg.as_ptr()); }, || { *cleanup_called_error_clone.lock().unwrap() = true; diff --git a/crates/harp/src/external_ptr.rs b/crates/harp/src/external_ptr.rs index 9fb3febe2..21aa85c81 100644 --- a/crates/harp/src/external_ptr.rs +++ b/crates/harp/src/external_ptr.rs @@ -14,12 +14,14 @@ pub struct ExternalPointer<'a, T: 'a> { } impl<'a, T> ExternalPointer<'a, T> { - pub unsafe fn new(object: &T) -> Self { - let pointer = RObject::from(R_MakeExternalPtr( - object as *const T as *const c_void as *mut c_void, - R_NilValue, - R_NilValue, - )); + pub fn new(object: &T) -> Self { + let pointer = unsafe { + RObject::from(R_MakeExternalPtr( + object as *const T as *const c_void as *mut c_void, + R_NilValue, + R_NilValue, + )) + }; Self { pointer, @@ -27,7 +29,7 @@ impl<'a, T> ExternalPointer<'a, T> { } } - pub unsafe fn reference(pointer: SEXP) -> &'static T { + pub fn reference(pointer: SEXP) -> &'static T { unsafe { &*(R_ExternalPtrAddr(pointer) as *const c_void as *const T) } } } diff --git a/crates/harp/src/fixtures.rs b/crates/harp/src/fixtures.rs index ab27f5173..0cf2294d4 100644 --- a/crates/harp/src/fixtures.rs +++ b/crates/harp/src/fixtures.rs @@ -72,9 +72,7 @@ pub fn r_test_init() { libraries.initialize_post_setup_r(); // Initialize harp globals - unsafe { - crate::routines::r_register_routines(); - } + crate::routines::r_register_routines(); // After routine registration crate::initialize(); }); diff --git a/crates/harp/src/format.rs b/crates/harp/src/format.rs index a0a2070d5..e10a539f1 100644 --- a/crates/harp/src/format.rs +++ b/crates/harp/src/format.rs @@ -123,7 +123,7 @@ mod tests { #[test] fn test_to_string_methods() { - crate::r_task(|| unsafe { + crate::r_task(|| { assert_eq!(lgl_to_string(1), String::from("TRUE")); assert_eq!(lgl_to_string(0), String::from("FALSE")); assert_eq!(lgl_to_string(r_lgl_na()), String::from("NA")); diff --git a/crates/harp/src/json.rs b/crates/harp/src/json.rs index d4c305f33..f9d624554 100644 --- a/crates/harp/src/json.rs +++ b/crates/harp/src/json.rs @@ -71,7 +71,7 @@ impl TryFrom for Value { // A single integer becomes a JSON number 1 => { - let value = unsafe { obj.to::()? }; + let value = obj.to::()?; Ok(Value::Number(value.into())) }, @@ -96,7 +96,7 @@ impl TryFrom for Value { // A single value becomes a JSON number 1 => { - let value = unsafe { obj.to::()? }; + let value = obj.to::()?; // There's no try/into implicit conversion from f64 to a // JSON number, but json! handles it. Ok(json!(value)) @@ -123,7 +123,7 @@ impl TryFrom for Value { // A single value becomes a JSON true/false value 1 => { - let value = unsafe { obj.to::()? }; + let value = obj.to::()?; Ok(Value::Bool(value)) }, @@ -159,7 +159,7 @@ impl TryFrom for Value { // With exactly one value, convert to a string 1 => { - let str = unsafe { obj.to::()? }; + let str = obj.to::()?; Ok(Value::String(str)) }, @@ -314,14 +314,12 @@ impl TryFrom> for RObject { // Consider: currently, this creates an unnamed list. It would be // better, presuming that the values are all the same type, to create an // atomic vector of that type. - unsafe { - let list = RObject::from(Rf_allocVector(VECSXP, vals.len() as isize)); - for (i, val) in vals.iter().enumerate() { - let val = RObject::try_from(val.clone())?; - SET_VECTOR_ELT(list.sexp, i as isize, val.sexp); - } - Ok(list) + let list = RObject::from(Rf_allocVector(VECSXP, vals.len() as isize)); + for (i, val) in vals.iter().enumerate() { + let val = RObject::try_from(val.clone())?; + SET_VECTOR_ELT(list.sexp, i as isize, val.sexp); } + Ok(list) } } diff --git a/crates/harp/src/modules.rs b/crates/harp/src/modules.rs index a270f5f79..f38a27266 100644 --- a/crates/harp/src/modules.rs +++ b/crates/harp/src/modules.rs @@ -28,7 +28,7 @@ where } pub fn init_modules() -> anyhow::Result<()> { - let namespace = unsafe { + let namespace = { let new_env_call = RCall::new(r_symbol!("new.env")).build(); Rf_eval(new_env_call.sexp, R_ENVS.base) }; @@ -41,13 +41,11 @@ pub fn init_modules() -> anyhow::Result<()> { // We don't have `safe_eval()` yet so source the init file manually with_asset::("init.R", |source| { let exprs = harp::parse_exprs(source)?; - unsafe { - let source_call = RCall::new(r_symbol!("source")) - .param("exprs", exprs) - .param("local", namespace) - .build(); - top_level_exec(|| Rf_eval(source_call.sexp, R_ENVS.base))?; - } + let source_call = RCall::new(r_symbol!("source")) + .param("exprs", exprs) + .param("local", namespace) + .build(); + top_level_exec(|| Rf_eval(source_call.sexp, R_ENVS.base))?; Ok(()) })?; diff --git a/crates/harp/src/object.rs b/crates/harp/src/object.rs index e5d45a511..bbbff9991 100644 --- a/crates/harp/src/object.rs +++ b/crates/harp/src/object.rs @@ -144,7 +144,7 @@ impl> RObjectExt for RObject { } pub fn r_length(x: SEXP) -> isize { - unsafe { Rf_xlength(x) } + Rf_xlength(x) } /// Raw access to the underlying `R_DimSymbol` attribute @@ -153,30 +153,30 @@ pub fn r_dim(x: SEXP) -> SEXP { } pub fn r_lgl_get(x: SEXP, i: isize) -> i32 { - unsafe { LOGICAL_ELT(x, i) } + LOGICAL_ELT(x, i) } pub fn r_int_get(x: SEXP, i: isize) -> i32 { - unsafe { INTEGER_ELT(x, i) } + INTEGER_ELT(x, i) } pub fn r_dbl_get(x: SEXP, i: isize) -> f64 { - unsafe { REAL_ELT(x, i) } + REAL_ELT(x, i) } pub fn r_cpl_get(x: SEXP, i: isize) -> Rcomplex { - unsafe { COMPLEX_ELT(x, i) } + COMPLEX_ELT(x, i) } pub fn r_chr_get(x: SEXP, i: isize) -> SEXP { - unsafe { STRING_ELT(x, i) } + STRING_ELT(x, i) } // TODO: Once we have a Rust list type, move this back to unsafe. // Should be unsafe because the type and bounds are not checked and // will result in a crash if used incorrectly. pub fn list_get(x: SEXP, i: isize) -> SEXP { - unsafe { VECTOR_ELT(x, i) } + VECTOR_ELT(x, i) } pub fn list_poke(x: SEXP, i: isize, value: SEXP) { - unsafe { SET_VECTOR_ELT(x, i, value) }; + SET_VECTOR_ELT(x, i, value); } pub fn r_lgl_na() -> i32 { @@ -207,71 +207,69 @@ pub fn r_dbl_negative_infinity() -> f64 { } pub fn r_dbl_is_na(x: f64) -> bool { - unsafe { R_IsNA(x) != 0 } + R_IsNA(x) != 0 } pub fn r_dbl_is_nan(x: f64) -> bool { - unsafe { R_IsNaN(x) != 0 } + R_IsNaN(x) != 0 } /// Returns `true` if `x` is not `NA`, `NaN`, `Inf`, or `-Inf` pub fn r_dbl_is_finite(x: f64) -> bool { - unsafe { R_finite(x) != 0 } + R_finite(x) != 0 } pub fn r_lgl_poke(x: SEXP, i: R_xlen_t, value: i32) { - unsafe { SET_LOGICAL_ELT(x, i, value) } + SET_LOGICAL_ELT(x, i, value) } pub fn r_int_poke(x: SEXP, i: R_xlen_t, value: i32) { - unsafe { SET_INTEGER_ELT(x, i, value) } + SET_INTEGER_ELT(x, i, value) } pub fn r_dbl_poke(x: SEXP, i: R_xlen_t, value: f64) { - unsafe { SET_REAL_ELT(x, i, value) } + SET_REAL_ELT(x, i, value) } pub fn r_cpl_poke(x: SEXP, i: R_xlen_t, value: Rcomplex) { - unsafe { SET_COMPLEX_ELT(x, i, value) } + SET_COMPLEX_ELT(x, i, value) } pub fn r_chr_poke(x: SEXP, i: R_xlen_t, value: SEXP) { - unsafe { SET_STRING_ELT(x, i, value) } + SET_STRING_ELT(x, i, value) } pub fn r_list_poke(x: SEXP, i: R_xlen_t, value: SEXP) { - unsafe { - SET_VECTOR_ELT(x, i, value); - } + SET_VECTOR_ELT(x, i, value); } pub fn r_lgl_begin(x: SEXP) -> *mut i32 { - unsafe { LOGICAL(x) } + LOGICAL(x) } pub fn r_int_begin(x: SEXP) -> *mut i32 { - unsafe { INTEGER(x) } + INTEGER(x) } pub fn r_dbl_begin(x: SEXP) -> *mut f64 { - unsafe { REAL(x) } + REAL(x) } -pub unsafe fn chr_cbegin(x: SEXP) -> *const SEXP { +pub fn chr_cbegin(x: SEXP) -> *const SEXP { libr::DATAPTR_RO(x) as *const SEXP } pub fn list_cbegin(x: SEXP) -> *const SEXP { - unsafe { libr::DATAPTR_RO(x) as *const SEXP } + libr::DATAPTR_RO(x) as *const SEXP } // TODO: Make these wrappers robust to allocation failures // https://github.com/posit-dev/positron/issues/2600 pub fn r_alloc_logical(size: R_xlen_t) -> SEXP { - unsafe { Rf_allocVector(LGLSXP, size) } + Rf_allocVector(LGLSXP, size) } pub fn r_alloc_integer(size: R_xlen_t) -> SEXP { - unsafe { Rf_allocVector(INTSXP, size) } + Rf_allocVector(INTSXP, size) } pub fn r_alloc_double(size: R_xlen_t) -> SEXP { - unsafe { Rf_allocVector(REALSXP, size) } + Rf_allocVector(REALSXP, size) } pub fn r_alloc_complex(size: R_xlen_t) -> SEXP { - unsafe { Rf_allocVector(CPLXSXP, size) } + Rf_allocVector(CPLXSXP, size) } pub fn r_alloc_character(size: R_xlen_t) -> SEXP { - unsafe { Rf_allocVector(STRSXP, size) } + Rf_allocVector(STRSXP, size) } pub fn alloc_list(size: usize) -> crate::Result { @@ -280,7 +278,7 @@ pub fn alloc_list(size: usize) -> crate::Result { fn alloc_vector(kind: libr::SEXPTYPE, size: usize) -> crate::Result { let size = as_r_ssize(size)?; - let res = crate::try_catch(|| unsafe { Rf_allocVector(kind, size) }); + let res = crate::try_catch(|| Rf_allocVector(kind, size)); match res { Ok(_) => res, @@ -299,18 +297,18 @@ pub fn as_r_ssize(size: usize) -> crate::Result { } pub fn r_node_car(x: SEXP) -> SEXP { - unsafe { CAR(x) } + CAR(x) } pub fn r_node_tag(x: SEXP) -> SEXP { - unsafe { TAG(x) } + TAG(x) } pub fn r_node_cdr(x: SEXP) -> SEXP { - unsafe { CDR(x) } + CDR(x) } pub fn is_identical(x: SEXP, y: SEXP) -> bool { // 16 corresponds to the default arguments of the R-level `identical()` - unsafe { libr::R_compute_identical(x, y, 16) != 0 } + libr::R_compute_identical(x, y, 16) != 0 } impl RObject { @@ -336,7 +334,7 @@ impl RObject { } // A helper function that makes '.try_into()' more ergonomic to use. - pub unsafe fn to>(self) -> Result { + pub fn to>(self) -> Result { TryInto::::try_into(self) } @@ -453,11 +451,9 @@ impl RObject { /// /// Returns an RObject representing the value at the given index. pub fn vector_elt(&self, idx: isize) -> crate::error::Result { - unsafe { - r_assert_type(self.sexp, &[VECSXP])?; - r_assert_capacity(self.sexp, idx as usize)?; - Ok(RObject::new(VECTOR_ELT(self.sexp, idx))) - } + r_assert_type(self.sexp, &[VECSXP])?; + r_assert_capacity(self.sexp, idx as usize)?; + Ok(RObject::new(VECTOR_ELT(self.sexp, idx))) } /// Gets a vector containing names for the object's values (from the `names` @@ -473,17 +469,15 @@ impl RObject { } pub fn set_attribute(&self, name: &str, value: SEXP) { - unsafe { - Rf_protect(value); - Rf_setAttrib(self.sexp, r_symbol!(name), value); - Rf_unprotect(1); - } + 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 { - self.get_attribute_from_symbol(unsafe { r_symbol!(name) }) + self.get_attribute_from_symbol(r_symbol!(name)) } /// Gets the [R_NamesSymbol] attribute from the object. Returns `None` if there are no @@ -507,7 +501,7 @@ impl RObject { } fn get_attribute_from_symbol(&self, symbol: SEXP) -> Option { - let out = unsafe { Rf_getAttrib(self.sexp, symbol) }; + let out = Rf_getAttrib(self.sexp, symbol); if r_is_null(out) { None } else { @@ -534,11 +528,11 @@ impl RObject { } pub fn duplicate(&self) -> RObject { - unsafe { RObject::new(libr::Rf_duplicate(self.sexp)) } + RObject::new(libr::Rf_duplicate(self.sexp)) } pub fn shallow_duplicate(&self) -> RObject { - unsafe { RObject::new(libr::Rf_shallow_duplicate(self.sexp)) } + RObject::new(libr::Rf_shallow_duplicate(self.sexp)) } } @@ -591,62 +585,52 @@ impl From<()> for RObject { impl From for RObject { fn from(value: bool) -> Self { - unsafe { - let value = Rf_ScalarLogical(value as c_int); - RObject::new(value) - } + let value = Rf_ScalarLogical(value as c_int); + RObject::new(value) } } impl From for RObject { fn from(value: i32) -> Self { - unsafe { - let value = Rf_ScalarInteger(value as c_int); - RObject::new(value) - } + let value = Rf_ScalarInteger(value as c_int); + RObject::new(value) } } impl TryFrom for RObject { type Error = crate::error::Error; fn try_from(value: i64) -> Result { - unsafe { - // Ensure the value is within the range of an i32. - if value < i32::MIN as i64 || value > i32::MAX as i64 { - return Err(Error::ValueOutOfRange { - value, - min: i32::MIN as i64, - max: i32::MAX as i64, - }); - } - let value = Rf_ScalarInteger(value as c_int); - Ok(RObject::new(value)) + // Ensure the value is within the range of an i32. + if value < i32::MIN as i64 || value > i32::MAX as i64 { + return Err(Error::ValueOutOfRange { + value, + min: i32::MIN as i64, + max: i32::MAX as i64, + }); } + let value = Rf_ScalarInteger(value as c_int); + Ok(RObject::new(value)) } } impl From for RObject { fn from(value: f64) -> Self { - unsafe { - let value = Rf_ScalarReal(value); - RObject::new(value) - } + let value = Rf_ScalarReal(value); + RObject::new(value) } } impl From<&str> for RObject { fn from(value: &str) -> Self { - unsafe { - let vector = Rf_protect(Rf_allocVector(STRSXP, 1)); - let element = Rf_mkCharLenCE( - value.as_ptr() as *mut c_char, - value.len() as i32, - cetype_t_CE_UTF8, - ); - SET_STRING_ELT(vector, 0, element); - Rf_unprotect(1); - RObject::new(vector) - } + let vector = Rf_protect(Rf_allocVector(STRSXP, 1)); + let element = Rf_mkCharLenCE( + value.as_ptr() as *mut c_char, + value.len() as i32, + cetype_t_CE_UTF8, + ); + SET_STRING_ELT(vector, 0, element); + Rf_unprotect(1); + RObject::new(vector) } } @@ -658,42 +642,36 @@ impl From for RObject { impl From> for RObject { fn from(values: Vec) -> Self { - unsafe { - let vector = RObject::from(Rf_allocVector(STRSXP, values.len() as isize)); - for idx in 0..values.len() { - let value_str = Rf_mkCharLenCE( - values[idx].as_ptr() as *mut c_char, - values[idx].len() as i32, - cetype_t_CE_UTF8, - ); - SET_STRING_ELT(vector.sexp, idx as isize, value_str); - } - vector + let vector = RObject::from(Rf_allocVector(STRSXP, values.len() as isize)); + for idx in 0..values.len() { + let value_str = Rf_mkCharLenCE( + values[idx].as_ptr() as *mut c_char, + values[idx].len() as i32, + cetype_t_CE_UTF8, + ); + SET_STRING_ELT(vector.sexp, idx as isize, value_str); } + vector } } impl From<&Vec> for RObject { fn from(values: &Vec) -> Self { - unsafe { - let vector = RObject::from(Rf_allocVector(INTSXP, values.len() as isize)); - for idx in 0..values.len() { - SET_INTEGER_ELT(vector.sexp, idx as isize, values[idx] as c_int); - } - vector + let vector = RObject::from(Rf_allocVector(INTSXP, values.len() as isize)); + for idx in 0..values.len() { + SET_INTEGER_ELT(vector.sexp, idx as isize, values[idx] as c_int); } + vector } } impl From<&Vec> for RObject { fn from(values: &Vec) -> Self { - unsafe { - let vector = RObject::from(Rf_allocVector(REALSXP, values.len() as isize)); - for idx in 0..values.len() { - SET_REAL_ELT(vector.sexp, idx as isize, values[idx] as c_double); - } - vector + let vector = RObject::from(Rf_allocVector(REALSXP, values.len() as isize)); + for idx in 0..values.len() { + SET_REAL_ELT(vector.sexp, idx as isize, values[idx] as c_double); } + vector } } @@ -999,16 +977,14 @@ impl TryFrom<&RObject> for Vec { impl TryFrom for Vec> { type Error = crate::error::Error; fn try_from(value: RObject) -> Result { - unsafe { - r_assert_type(*value, &[STRSXP, NILSXP])?; + r_assert_type(*value, &[STRSXP, NILSXP])?; - let n = Rf_xlength(*value); - let mut result: Vec> = Vec::with_capacity(n as usize); - for i in 0..n { - result.push(value.get_string(i)?); - } - Ok(result) + let n = Rf_xlength(*value); + let mut result: Vec> = Vec::with_capacity(n as usize); + for i in 0..n { + result.push(value.get_string(i)?); } + Ok(result) } } @@ -1030,20 +1006,18 @@ impl TryFrom<&RObject> for Vec { impl TryFrom> for RObject { type Error = crate::error::Error; fn try_from(value: Vec) -> Result { - unsafe { - let n = value.len(); - - // Create the list object. - let out_raw = Rf_allocVector(VECSXP, n as R_xlen_t); - let out = RObject::new(out_raw); + let n = value.len(); - // Copy the values into the list. - for i in 0..n { - r_list_poke(out.sexp, i as isize, value[i].sexp) - } + // Create the list object. + let out_raw = Rf_allocVector(VECSXP, n as R_xlen_t); + let out = RObject::new(out_raw); - Ok(out) + // Copy the values into the list. + for i in 0..n { + r_list_poke(out.sexp, i as isize, value[i].sexp) } + + Ok(out) } } @@ -1102,7 +1076,7 @@ impl TryFrom for HashMap { return Err(Error::UnexpectedType(NILSXP, vec![STRSXP])); }; - let value = RObject::new(unsafe { Rf_coerceVector(value.sexp, STRSXP) }); + let value = RObject::new(Rf_coerceVector(value.sexp, STRSXP)); let n = names.length(); let mut map = HashMap::::with_capacity(n as usize); @@ -1130,7 +1104,7 @@ impl TryFrom for HashMap { return Err(Error::UnexpectedType(NILSXP, vec![STRSXP])); }; - let value = RObject::new(unsafe { Rf_coerceVector(value.sexp, INTSXP) }); + let value = RObject::new(Rf_coerceVector(value.sexp, INTSXP)); let n = names.length(); let mut map = HashMap::::with_capacity(n as usize); diff --git a/crates/harp/src/options.rs b/crates/harp/src/options.rs index 4b203586a..d92427642 100644 --- a/crates/harp/src/options.rs +++ b/crates/harp/src/options.rs @@ -9,7 +9,7 @@ use crate::r_symbol; use crate::RObject; pub fn get_option(name: &str) -> RObject { - unsafe { libr::Rf_GetOption1(r_symbol!(name)).into() } + libr::Rf_GetOption1(r_symbol!(name)).into() } pub fn get_option_bool(name: &str) -> bool { diff --git a/crates/harp/src/parse.rs b/crates/harp/src/parse.rs index c9afa7a36..296eb1dfc 100644 --- a/crates/harp/src/parse.rs +++ b/crates/harp/src/parse.rs @@ -40,20 +40,18 @@ pub enum ParseInput<'a> { /// Returns a single expression pub fn parse_expr(code: &str) -> crate::Result { - unsafe { - let exprs = parse_exprs(code)?; - - let n = libr::Rf_xlength(*exprs); - if n != 1 { - return Err(crate::Error::ParseError { - code: code.to_string(), - message: String::from("Expected a single expression, got {n}"), - }); - } + let exprs = parse_exprs(code)?; - let expr = libr::VECTOR_ELT(*exprs, 0); - Ok(expr.into()) + let n = libr::Rf_xlength(*exprs); + if n != 1 { + return Err(crate::Error::ParseError { + code: code.to_string(), + message: String::from("Expected a single expression, got {n}"), + }); } + + let expr = libr::VECTOR_ELT(*exprs, 0); + Ok(expr.into()) } /// Returns an EXPRSXP vector @@ -145,11 +143,9 @@ pub fn parse_status<'a>(input: &ParseInput<'a>) -> crate::Result { } pub fn as_parse_text(text: &str) -> RObject { - unsafe { - let mut protect = RProtect::new(); - let input = r_string!(convert_line_endings(text, LineEnding::Posix), &mut protect); - input.into() - } + let mut protect = RProtect::new(); + let input = r_string!(convert_line_endings(text, LineEnding::Posix), &mut protect); + input.into() } fn parse_input_as_string<'a>(input: &ParseInput<'a>) -> crate::Result { diff --git a/crates/harp/src/parser/srcref.rs b/crates/harp/src/parser/srcref.rs index 7eac60084..94b5f9216 100644 --- a/crates/harp/src/parser/srcref.rs +++ b/crates/harp/src/parser/srcref.rs @@ -98,7 +98,7 @@ impl TryFrom for SrcRef { end: line_end, }; - let line_parsed = if unsafe { value.len() >= 8 } { + let line_parsed = if value.len() >= 8 { let line_parsed_start = adjust_start(value.get_value(6)?); let line_parsed_end = adjust_end(value.get_value(7)?); std::ops::Range { @@ -222,7 +222,7 @@ pub fn srcref_list_get(srcrefs: libr::SEXP, ind: isize) -> RObject { return RObject::null(); } - if unsafe { libr::TYPEOF(result) as u32 } != libr::INTSXP { + if libr::TYPEOF(result) as u32 != libr::INTSXP { return RObject::null(); } @@ -239,7 +239,7 @@ pub fn srcref_list_get(srcrefs: libr::SEXP, ind: isize) -> RObject { pub fn get_srcref_list(call: libr::SEXP) -> Option { let srcrefs = unsafe { libr::Rf_getAttrib(call, libr::R_SrcrefSymbol) }; - if unsafe { libr::TYPEOF(srcrefs) as u32 } == libr::VECSXP { + if libr::TYPEOF(srcrefs) as u32 == libr::VECSXP { return Some(RObject::new(srcrefs)); } diff --git a/crates/harp/src/protect.rs b/crates/harp/src/protect.rs index f55e34e73..8118fc84e 100644 --- a/crates/harp/src/protect.rs +++ b/crates/harp/src/protect.rs @@ -18,21 +18,18 @@ pub struct RProtect { } impl RProtect { - /// SAFETY: Assumes that the R lock is held. - pub unsafe fn new() -> Self { + pub fn new() -> Self { Self { count: 0 } } - /// SAFETY: Assumes that the R lock is held. - pub unsafe fn add(&mut self, object: SEXP) -> SEXP { + pub fn add(&mut self, object: SEXP) -> SEXP { self.count += 1; Rf_protect(object) } } impl Drop for RProtect { - /// SAFETY: Assumes that the R lock is held. fn drop(&mut self) { - unsafe { Rf_unprotect(self.count) } + Rf_unprotect(self.count) } } diff --git a/crates/harp/src/r.rs b/crates/harp/src/r.rs index 45dd5eedd..287a9a47a 100644 --- a/crates/harp/src/r.rs +++ b/crates/harp/src/r.rs @@ -3,59 +3,49 @@ use libr::SEXP; // --- Closure accessors --- pub fn fn_formals(x: SEXP) -> SEXP { - unsafe { libr::FORMALS(x) } + libr::FORMALS(x) } pub fn fn_body(x: SEXP) -> SEXP { - unsafe { - if libr::has::R_ClosureBody() { - libr::R_ClosureBody(x) - } else { - libr::BODY(x) - } + if libr::has::R_ClosureBody() { + libr::R_ClosureBody(x) + } else { + libr::BODY(x) } } pub fn fn_env(x: SEXP) -> SEXP { - unsafe { - if libr::has::R_ClosureEnv() { - libr::R_ClosureEnv(x) - } else { - libr::CLOENV(x) - } + if libr::has::R_ClosureEnv() { + libr::R_ClosureEnv(x) + } else { + libr::CLOENV(x) } } pub fn new_function(formals: SEXP, body: SEXP, env: SEXP) -> SEXP { - unsafe { - if libr::has::R_mkClosure() { - libr::R_mkClosure(formals, body, env) - } else { - let out = libr::Rf_allocSExp(libr::CLOSXP); - libr::SET_FORMALS(out, formals); - libr::SET_BODY(out, body); - libr::SET_CLOENV(out, env); - out - } + if libr::has::R_mkClosure() { + libr::R_mkClosure(formals, body, env) + } else { + let out = libr::Rf_allocSExp(libr::CLOSXP); + libr::SET_FORMALS(out, formals); + libr::SET_BODY(out, body); + libr::SET_CLOENV(out, env); + out } } // --- Environment bindings --- pub fn env_binding_is_locked(env: SEXP, sym: SEXP) -> bool { - unsafe { libr::R_BindingIsLocked(sym, env) != 0 } + libr::R_BindingIsLocked(sym, env) != 0 } pub fn env_binding_lock(env: SEXP, sym: SEXP) { - unsafe { - libr::R_LockBinding(sym, env); - } + libr::R_LockBinding(sym, env); } pub fn env_binding_unlock(env: SEXP, sym: SEXP) { - unsafe { - libr::R_unLockBinding(sym, env); - } + libr::R_unLockBinding(sym, env); } /// Binds a value in an environment, temporarily unlocking the binding if needed. @@ -64,9 +54,7 @@ pub fn env_bind_force(env: SEXP, sym: SEXP, value: SEXP) { if locked { env_binding_unlock(env, sym); } - unsafe { - libr::Rf_defineVar(sym, value, env); - } + libr::Rf_defineVar(sym, value, env); if locked { env_binding_lock(env, sym); } @@ -74,12 +62,10 @@ pub fn env_bind_force(env: SEXP, sym: SEXP, value: SEXP) { /// Returns the parent (enclosing) environment. pub fn env_parent(env: SEXP) -> SEXP { - unsafe { - if libr::has::R_ParentEnv() { - libr::R_ParentEnv(env) - } else { - libr::ENCLOS(env) - } + if libr::has::R_ParentEnv() { + libr::R_ParentEnv(env) + } else { + libr::ENCLOS(env) } } @@ -87,13 +73,11 @@ pub fn env_parent(env: SEXP) -> SEXP { /// Gets an attribute from `x`. pub fn attrib_get(x: SEXP, tag: SEXP) -> SEXP { - unsafe { libr::Rf_getAttrib(x, tag) } + libr::Rf_getAttrib(x, tag) } pub fn attrib_poke(x: SEXP, tag: SEXP, value: SEXP) { - unsafe { - libr::Rf_setAttrib(x, tag, value); - } + libr::Rf_setAttrib(x, tag, value); } /// Returns `true` if `x` has any attributes. @@ -137,7 +121,5 @@ pub fn attrib_for_each(x: SEXP, mut f: F) { /// Shallow-copies all attributes from `src` to `dst`. pub fn attrib_poke_from(dst: SEXP, src: SEXP) { - unsafe { - libr::SHALLOW_DUPLICATE_ATTRIB(dst, src); - } + libr::SHALLOW_DUPLICATE_ATTRIB(dst, src); } diff --git a/crates/harp/src/raii.rs b/crates/harp/src/raii.rs index 7ca0bd85d..f8409f72e 100644 --- a/crates/harp/src/raii.rs +++ b/crates/harp/src/raii.rs @@ -51,14 +51,12 @@ where { #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn new(new_value: T, variable: *mut T) -> RLocal { - unsafe { - let old_value = libr::get(variable); - libr::set(variable, new_value); + let old_value = libr::get(variable); + libr::set(variable, new_value); - Self { - old_value, - variable, - } + Self { + old_value, + variable, } } } @@ -68,15 +66,13 @@ where T: Copy, { fn drop(&mut self) { - unsafe { - libr::set(self.variable, self.old_value); - } + libr::set(self.variable, self.old_value); } } impl RLocalOption { pub fn new(option: &str, new_value: libr::SEXP) -> RLocalOption { - let option = crate::RSymbol::new_unchecked(unsafe { crate::r_symbol!(option) }); + let option = crate::RSymbol::new_unchecked(crate::r_symbol!(option)); let old_value = crate::r_poke_option(option.sexp, new_value); Self { diff --git a/crates/harp/src/routines.rs b/crates/harp/src/routines.rs index fb7b73c57..9e90a9951 100644 --- a/crates/harp/src/routines.rs +++ b/crates/harp/src/routines.rs @@ -26,14 +26,14 @@ static R_ROUTINES: Mutex> = Mutex::new(vec![]); // NOTE: This function is used via the #[harp::register] macro, // which ensures that routines are initialized and executed on // application startup. -pub unsafe fn add(def: R_CallMethodDef) { +pub fn add(def: R_CallMethodDef) { R_ROUTINES .lock() .unwrap() .push(CallMethodDef { inner: def }); } -pub unsafe fn r_register_routines() { +pub fn r_register_routines() { let info = R_getEmbeddingDllInfo(); if info.is_null() { error!("internal error: no embedding DllInfo available"); diff --git a/crates/harp/src/session.rs b/crates/harp/src/session.rs index 3ea727c42..a7fe9f1ce 100644 --- a/crates/harp/src/session.rs +++ b/crates/harp/src/session.rs @@ -112,7 +112,7 @@ pub fn r_env_is_browsed(env: SEXP) -> anyhow::Result { anyhow::bail!("`env` must be an environment"); } - let browsed = unsafe { RDEBUG(env) }; + let browsed = RDEBUG(env); Ok(browsed != 0) } diff --git a/crates/harp/src/size.rs b/crates/harp/src/size.rs index 559b011f1..1a0a7e741 100644 --- a/crates/harp/src/size.rs +++ b/crates/harp/src/size.rs @@ -87,20 +87,20 @@ fn obj_size_tree( // Use sizeof(SEXPREC) and sizeof(VECTOR_SEXPREC) computed in R. // CHARSXP are treated as vectors for this purpose - let mut size = if unsafe { Rf_isVector(x) == Rboolean_TRUE } || r_typeof(x) == CHARSXP { + let mut size = if Rf_isVector(x) == Rboolean_TRUE || r_typeof(x) == CHARSXP { sizeof_vector } else { sizeof_node }; if r_is_altrep(x) { - let klass = unsafe { libr::ALTREP_CLASS(x) }; + let klass = libr::ALTREP_CLASS(x); size += 3 * size_of::(); size += obj_size_tree(klass, base_env, sizeof_node, sizeof_vector, seen, depth + 1); size += obj_size_tree( - unsafe { libr::R_altrep_data1(x) }, + libr::R_altrep_data1(x), base_env, sizeof_node, sizeof_vector, @@ -108,7 +108,7 @@ fn obj_size_tree( depth + 1, ); size += obj_size_tree( - unsafe { libr::R_altrep_data2(x) }, + libr::R_altrep_data2(x), base_env, sizeof_node, sizeof_vector, @@ -183,7 +183,7 @@ fn obj_size_tree( size += sizeof_node } size += obj_size_tree( - unsafe { libr::TAG(cons) }, + libr::TAG(cons), base_env, sizeof_node, sizeof_vector, @@ -191,14 +191,14 @@ fn obj_size_tree( depth + 1, ); size += obj_size_tree( - unsafe { libr::CAR(cons) }, + libr::CAR(cons), base_env, sizeof_node, sizeof_vector, seen, depth + 1, ); - cons = unsafe { libr::CDR(cons) }; + cons = libr::CDR(cons); } // Handle non-nil CDRs size += obj_size_tree(cons, base_env, sizeof_node, sizeof_vector, seen, depth + 1); @@ -206,7 +206,7 @@ fn obj_size_tree( }, BCODESXP => { size += obj_size_tree( - unsafe { libr::TAG(x) }, + libr::TAG(x), base_env, sizeof_node, sizeof_vector, @@ -214,7 +214,7 @@ fn obj_size_tree( depth + 1, ); size += obj_size_tree( - unsafe { libr::CAR(x) }, + libr::CAR(x), base_env, sizeof_node, sizeof_vector, @@ -222,7 +222,7 @@ fn obj_size_tree( depth + 1, ); size += obj_size_tree( - unsafe { libr::CDR(x) }, + libr::CDR(x), base_env, sizeof_node, sizeof_vector, @@ -334,7 +334,7 @@ fn obj_size_tree( }, PROMSXP => { size += obj_size_tree( - unsafe { libr::PRVALUE(x) }, + libr::PRVALUE(x), base_env, sizeof_node, sizeof_vector, @@ -342,7 +342,7 @@ fn obj_size_tree( depth + 1, ); size += obj_size_tree( - unsafe { libr::PRCODE(x) }, + libr::PRCODE(x), base_env, sizeof_node, sizeof_vector, @@ -350,7 +350,7 @@ fn obj_size_tree( depth + 1, ); size += obj_size_tree( - unsafe { libr::PRENV(x) }, + libr::PRENV(x), base_env, sizeof_node, sizeof_vector, @@ -361,7 +361,7 @@ fn obj_size_tree( EXTPTRSXP => { size += size_of::<*mut c_void>(); // the actual pointer size += obj_size_tree( - unsafe { libr::R_ExternalPtrProtected(x) }, + libr::R_ExternalPtrProtected(x), base_env, sizeof_node, sizeof_vector, @@ -369,7 +369,7 @@ fn obj_size_tree( depth + 1, ); size += obj_size_tree( - unsafe { libr::R_ExternalPtrTag(x) }, + libr::R_ExternalPtrTag(x), base_env, sizeof_node, sizeof_vector, @@ -379,7 +379,7 @@ fn obj_size_tree( }, S4SXP => { size += obj_size_tree( - unsafe { libr::TAG(x) }, + libr::TAG(x), base_env, sizeof_node, sizeof_vector, diff --git a/crates/harp/src/symbol.rs b/crates/harp/src/symbol.rs index 8c5c83fa2..70a2f6b79 100644 --- a/crates/harp/src/symbol.rs +++ b/crates/harp/src/symbol.rs @@ -71,14 +71,14 @@ impl Deref for RSymbol { impl From for String { fn from(symbol: RSymbol) -> Self { - unsafe { r_str_to_owned_utf8_unchecked(PRINTNAME(*symbol)) } + r_str_to_owned_utf8_unchecked(PRINTNAME(*symbol)) } } impl From<&str> for RSymbol { fn from(value: &str) -> Self { RSymbol { - sexp: unsafe { r_symbol!(value) }, + sexp: r_symbol!(value), } } } diff --git a/crates/harp/src/syntax.rs b/crates/harp/src/syntax.rs index 784187067..dd5c21af1 100644 --- a/crates/harp/src/syntax.rs +++ b/crates/harp/src/syntax.rs @@ -48,7 +48,7 @@ fn is_reserved_word(name: &str) -> bool { pub extern "C-unwind" fn harp_is_valid_symbol(name: SEXP) -> anyhow::Result { let name = String::try_from(RObject::view(name))?; let result = is_valid_symbol(&name); - Ok(unsafe { libr::Rf_ScalarLogical(result as i32) }) + Ok(libr::Rf_ScalarLogical(result as i32)) } /// Returns `true` if `name` is a syntactic R identifier that doesn't need backtick-quoting. diff --git a/crates/harp/src/utils.rs b/crates/harp/src/utils.rs index b4ebdae7e..9d047fb32 100644 --- a/crates/harp/src/utils.rs +++ b/crates/harp/src/utils.rs @@ -81,7 +81,7 @@ pub fn r_assert_type(object: SEXP, expected: &[u32]) -> Result { } pub fn r_assert_capacity(object: SEXP, required: usize) -> Result { - let actual = unsafe { Rf_xlength(object) } as usize; + let actual = Rf_xlength(object) as usize; if actual < required { return Err(Error::UnexpectedLength(actual, required)); } @@ -90,7 +90,7 @@ pub fn r_assert_capacity(object: SEXP, required: usize) -> Result { } pub fn r_assert_length(object: SEXP, expected: usize) -> Result { - let actual = unsafe { Rf_xlength(object) as usize }; + let actual = Rf_xlength(object) as usize; if actual != expected { return Err(Error::UnexpectedLength(actual, expected)); } @@ -123,15 +123,15 @@ pub fn r_is_null(object: SEXP) -> bool { } pub fn r_is_altrep(object: SEXP) -> bool { - unsafe { libr::ALTREP(object) != 0 } + libr::ALTREP(object) != 0 } pub fn r_is_object(object: SEXP) -> bool { - unsafe { libr::Rf_isObject(object) != 0 } + libr::Rf_isObject(object) != 0 } pub fn r_is_s4(object: SEXP) -> bool { - unsafe { libr::Rf_isS4(object) != 0 } + libr::Rf_isS4(object) != 0 } pub fn r_is_unbound(object: SEXP) -> bool { @@ -190,7 +190,7 @@ pub fn r_classes(value: SEXP) -> Option { /// - `x` is the R vector to translate from. /// - `i` is the index in the vector of the string to translate. pub fn r_chr_get_owned_utf8(x: SEXP, i: isize) -> Result { - unsafe { r_str_to_owned_utf8(STRING_ELT(x, i)) } + r_str_to_owned_utf8(STRING_ELT(x, i)) } /// Translates an R string to a UTF-8 Rust string. @@ -285,10 +285,10 @@ pub fn r_vec_shape(value: SEXP) -> String { } pub fn r_altrep_class(object: SEXP) -> String { - let serialized_klass = unsafe { ATTRIB(ALTREP_CLASS(object)) }; + let serialized_klass = ATTRIB(ALTREP_CLASS(object)); - let klass = RSymbol::new_unchecked(unsafe { CAR(serialized_klass) }); - let pkg = RSymbol::new_unchecked(unsafe { CADR(serialized_klass) }); + let klass = RSymbol::new_unchecked(CAR(serialized_klass)); + let pkg = RSymbol::new_unchecked(CADR(serialized_klass)); format!("{}::{}", pkg, klass) } @@ -296,7 +296,7 @@ pub fn r_altrep_class(object: SEXP) -> String { pub fn r_typeof(object: SEXP) -> u32 { // SAFETY: The type of an R object is typically considered constant, // and TYPEOF merely queries the R type directly from the SEXPREC struct. - unsafe { TYPEOF(object) as u32 } + TYPEOF(object) as u32 } pub fn r_type2char>(kind: T) -> String { @@ -309,7 +309,7 @@ pub fn r_type2char>(kind: T) -> String { pub fn r_inherits(object: SEXP, class: &str) -> bool { let class = CString::new(class).unwrap(); - unsafe { libr::Rf_inherits(object, class.as_ptr()) != 0 } + libr::Rf_inherits(object, class.as_ptr()) != 0 } pub fn r_is_function(object: SEXP) -> bool { @@ -335,13 +335,11 @@ pub fn r_formals(object: SEXP) -> Result> { // iterate through the entries let mut arguments = Vec::new(); - unsafe { - while formals != r_null() { - let name = RObject::from(TAG(formals)).to::()?; - let value = CAR(formals); - arguments.push(RArgument::new(name.as_str(), RObject::new(value))); - formals = CDR(formals); - } + while formals != r_null() { + let name = RObject::from(TAG(formals)).to::()?; + let value = CAR(formals); + arguments.push(RArgument::new(name.as_str(), RObject::new(value))); + formals = CDR(formals); } Ok(arguments) @@ -352,17 +350,17 @@ pub fn r_envir_name(envir: SEXP) -> Result { if r_env_is_pkg_env(envir) { let name = RObject::from(r_pkg_env_name(envir)); - return unsafe { name.to::() }; + return name.to::(); } if r_env_is_ns_env(envir) { let name = RObject::from(r_ns_env_name(envir)); - return unsafe { name.to::() }; + return name.to::(); } - let name = unsafe { Rf_getAttrib(envir, r_symbol!("name")) }; + let name = Rf_getAttrib(envir, r_symbol!("name")); if r_typeof(name) == STRSXP { - let name = unsafe { RObject::view(name).to::()? }; + let name = RObject::view(name).to::()?; return Ok(name); } @@ -382,11 +380,11 @@ pub fn r_envir_get(symbol: &str, envir: SEXP) -> Option { } pub fn r_envir_set(symbol: &str, value: SEXP, envir: SEXP) { - unsafe { Rf_defineVar(r_symbol!(symbol), value, envir) }; + Rf_defineVar(r_symbol!(symbol), value, envir); } pub fn r_envir_remove(symbol: &str, envir: SEXP) { - unsafe { R_removeVarFromFrame(r_symbol!(symbol), envir) }; + R_removeVarFromFrame(r_symbol!(symbol), envir); } /// Get names of a vector @@ -397,12 +395,12 @@ pub fn r_envir_remove(symbol: &str, envir: SEXP) { /// /// Note that it does not use S3 dispatch for length or names. pub fn r_names2(x: SEXP) -> SEXP { - let mut protect = unsafe { RProtect::new() }; + let mut protect = RProtect::new(); let size = r_length(x); let names = unsafe { Rf_getAttrib(x, R_NamesSymbol) }; - unsafe { protect.add(names) }; + protect.add(names); if r_is_null(names) { // Comes initialized with `""`. @@ -430,7 +428,7 @@ pub fn r_names2(x: SEXP) -> SEXP { } let out = r_alloc_character(size); - unsafe { protect.add(out) }; + protect.add(out); for i in 0..size { let elt = r_chr_get(names, i); @@ -448,7 +446,7 @@ pub fn r_names2(x: SEXP) -> SEXP { pub fn r_stringify(object: SEXP, delimiter: &str) -> Result { // handle SYMSXPs upfront if r_typeof(object) == SYMSXP { - return unsafe { RObject::view(object).to::() }; + return RObject::view(object).to::(); } // call format on the object @@ -457,7 +455,7 @@ pub fn r_stringify(object: SEXP, delimiter: &str) -> Result { .call()?; // paste into a single string - let object = unsafe { + let object = { RFunction::new("base", "paste") .add(object) .param("collapse", delimiter) @@ -486,12 +484,12 @@ pub fn r_promise_is_forced(x: SEXP) -> bool { } pub fn r_promise_value(x: SEXP) -> SEXP { - unsafe { PRVALUE(x) } + PRVALUE(x) } pub fn r_promise_expr(x: SEXP) -> SEXP { // Like `PRCODE()`, but also handles bytecode expressions - unsafe { R_PromiseExpr(x) } + R_PromiseExpr(x) } pub fn r_promise_force(x: SEXP) -> harp::Result { @@ -503,11 +501,9 @@ pub fn r_promise_force_with_rollback(x: SEXP) -> harp::Result { // Like `r_promise_force()`, but if evaluation results in an error // then the original promise is untouched, i.e. `PRSEEN` isn't modified, // avoiding `"restarting interrupted promise evaluation"` warnings. - unsafe { - let out = harp::try_eval(PRCODE(x), PRENV(x))?; - SET_PRVALUE(x, out.sexp); - Ok(out) - } + let out = harp::try_eval(PRCODE(x), PRENV(x))?; + SET_PRVALUE(x, out.sexp); + Ok(out) } pub fn r_promise_is_lazy_load_binding(x: SEXP) -> bool { @@ -517,36 +513,36 @@ pub fn r_promise_is_lazy_load_binding(x: SEXP) -> bool { // We can take advantage of this to identify promises in namespaces // that correspond to symbols we should evaluate when generating completions. - let expr = unsafe { PRCODE(x) }; + let expr = PRCODE(x); if r_typeof(expr) != LANGSXP { return false; } - if unsafe { Rf_xlength(expr) } == 0 { + if Rf_xlength(expr) == 0 { return false; } - let expr = unsafe { CAR(expr) }; + let expr = CAR(expr); if r_typeof(expr) != SYMSXP { return false; } - expr == unsafe { r_symbol!("lazyLoadDBfetch") } + expr == r_symbol!("lazyLoadDBfetch") } pub fn r_bytecode_expr(x: SEXP) -> SEXP { - unsafe { R_BytecodeExpr(x) } + R_BytecodeExpr(x) } pub fn r_env_names(env: SEXP) -> SEXP { // `all = true`, `sorted = false` - unsafe { R_lsInternal3(env, Rboolean_TRUE, Rboolean_FALSE) } + R_lsInternal3(env, Rboolean_TRUE, Rboolean_FALSE) } pub fn r_env_has(env: SEXP, sym: SEXP) -> bool { - unsafe { libr::R_existsVarInFrame(env, sym) == libr::Rboolean_TRUE } + libr::R_existsVarInFrame(env, sym) == libr::Rboolean_TRUE } /// Check if a symbol is an active binding in an environment @@ -563,7 +559,7 @@ pub fn r_env_binding_is_active(env: SEXP, sym: SEXP) -> harp::Result { let name = RSymbol::new(sym)?.to_string(); Err(harp::Error::MissingBindingError { name }) } else { - Ok(unsafe { R_BindingIsActive(sym, env) == Rboolean_TRUE }) + Ok(R_BindingIsActive(sym, env) == Rboolean_TRUE) } } @@ -593,7 +589,7 @@ pub fn r_pkg_env_name(env: SEXP) -> SEXP { pub fn r_env_is_ns_env(env: SEXP) -> bool { // Does handle `R_BaseNamespace` // https://github.com/wch/r-source/blob/1cb35ff692d3eb3ab546e0db4761102b5ea4ac89/src/main/envir.c#L3689 - unsafe { R_IsNamespaceEnv(env) == Rboolean_TRUE } + R_IsNamespaceEnv(env) == Rboolean_TRUE } pub fn r_ns_env_name(env: SEXP) -> SEXP { @@ -651,15 +647,13 @@ pub fn r_normalize_path(x: RObject) -> anyhow::Result { if !r_is_string(x.sexp) { anyhow::bail!("Expected string for srcfile's filename"); } - unsafe { - let path = RFunction::new("base", "normalizePath") - .param("path", x) - .param("winslash", "/") - .param("mustWork", false) - .call()? - .to::()?; - Ok(path) - } + let path = RFunction::new("base", "normalizePath") + .param("path", x) + .param("winslash", "/") + .param("mustWork", false) + .call()? + .to::()?; + Ok(path) } pub fn save_rds(x: SEXP, path: &str) { @@ -707,16 +701,12 @@ pub fn push_rds(x: SEXP, path: &str, context: &str) { } pub fn r_print(x: impl Into) { - unsafe { - Rf_PrintValue(x.into()); - } + Rf_PrintValue(x.into()); } pub fn r_printf(x: &str) { let c_str = std::ffi::CString::new(x).unwrap(); - unsafe { - libr::Rprintf(c_str.as_ptr()); - } + libr::Rprintf(c_str.as_ptr()); } pub fn r_format_vec(x: SEXP) -> Result { @@ -781,7 +771,7 @@ mod tests { "; let x = harp::parse_eval0(code, env).unwrap(); - let x = unsafe { STRING_ELT(x.sexp, 0) }; + let x = STRING_ELT(x.sexp, 0); let x = r_str_to_owned_utf8_unchecked(x); assert_eq!(x, String::from(std::char::REPLACEMENT_CHARACTER)); diff --git a/crates/harp/src/vec_format.rs b/crates/harp/src/vec_format.rs index 4cdf0a1ea..b377b4d08 100644 --- a/crates/harp/src/vec_format.rs +++ b/crates/harp/src/vec_format.rs @@ -183,7 +183,7 @@ mod tests { #[test] fn test_vec_format_methods() { - crate::r_task(|| unsafe { + crate::r_task(|| { let x = RObject::from(r_alloc_integer(2)); r_int_poke(x.sexp, 0, 1); r_int_poke(x.sexp, 1, r_int_na()); diff --git a/crates/harp/src/vector.rs b/crates/harp/src/vector.rs index 8679f013f..0f5d92fab 100644 --- a/crates/harp/src/vector.rs +++ b/crates/harp/src/vector.rs @@ -55,7 +55,7 @@ pub trait Vector: Sized { type UnderlyingType; type CompareType; - unsafe fn new_unchecked(object: impl Into) -> Self; + fn new_unchecked(object: impl Into) -> Self; fn data(&self) -> SEXP; fn is_na(x: &Self::UnderlyingType) -> bool; fn get_unchecked_elt(&self, index: isize) -> Self::UnderlyingType; @@ -88,10 +88,10 @@ pub trait Vector: Sized { { let object = object.into(); r_assert_type(object, &[Self::SEXPTYPE])?; - unsafe { Ok(Self::new_unchecked(object)) } + Ok(Self::new_unchecked(object)) } - unsafe fn with_length(size: usize) -> Self + fn with_length(size: usize) -> Self where Self: Sized, { @@ -105,7 +105,7 @@ pub trait Vector: Sized { ::IntoIter: ExactSizeIterator, ::Item: AsRef; - unsafe fn len(&self) -> usize { + fn len(&self) -> usize { Rf_xlength(self.data()) as usize } @@ -119,7 +119,7 @@ pub trait Vector: Sized { } fn iter(&self) -> harp::vector::VectorIterator<'_, Self> { - let size = unsafe { self.len() as isize }; + let size = self.len() as isize; harp::vector::VectorIterator { data: self, index: 0, @@ -170,18 +170,16 @@ pub(crate) fn try_vec_from_r_vector( where VectorType: Vector, { - unsafe { - let mut result: Vec = Vec::with_capacity(value.len()); - - for val in value.iter() { - let Some(x) = val else { - return Err(harp::Error::MissingValueError); - }; - result.push(x); - } + let mut result: Vec = Vec::with_capacity(value.len()); - Ok(result) + for val in value.iter() { + let Some(x) = val else { + return Err(harp::Error::MissingValueError); + }; + result.push(x); } + + Ok(result) } pub(crate) fn try_r_vector_from_r_sexp(value: SEXP) -> harp::Result @@ -189,7 +187,7 @@ where VectorType: Vector, { if value == harp::RObject::null().sexp { - return Ok(unsafe { VectorType::with_length(0) }); + return Ok(VectorType::with_length(0)); } VectorType::new(value) diff --git a/crates/harp/src/vector/character_vector.rs b/crates/harp/src/vector/character_vector.rs index ef98cca13..2ec530a97 100644 --- a/crates/harp/src/vector/character_vector.rs +++ b/crates/harp/src/vector/character_vector.rs @@ -49,7 +49,7 @@ impl Vector for CharacterVector { self.object.sexp } - unsafe fn new_unchecked(object: impl Into) -> Self { + fn new_unchecked(object: impl Into) -> Self { Self { object: RObject::new(object.into()), } @@ -88,7 +88,7 @@ impl Vector for CharacterVector { } fn get_unchecked_elt(&self, index: isize) -> Self::UnderlyingType { - unsafe { STRING_ELT(self.data(), index as R_xlen_t) } + STRING_ELT(self.data(), index as R_xlen_t) } fn convert_value(x: &Self::UnderlyingType) -> Self::Type { @@ -112,17 +112,15 @@ impl TryFrom<&[SEXP]> for CharacterVector { type Error = harp::Error; fn try_from(value: &[SEXP]) -> harp::Result { - unsafe { - let vec = Self::with_length(value.len()); - let sexp = vec.object.sexp; - - for (i, elt) in value.iter().enumerate() { - r_assert_type(*elt, &[libr::CHARSXP])?; - r_chr_poke(sexp, i as R_xlen_t, *elt); - } + let vec = Self::with_length(value.len()); + let sexp = vec.object.sexp; - Ok(vec) + for (i, elt) in value.iter().enumerate() { + r_assert_type(*elt, &[libr::CHARSXP])?; + r_chr_poke(sexp, i as R_xlen_t, *elt); } + + Ok(vec) } } diff --git a/crates/harp/src/vector/complex_vector.rs b/crates/harp/src/vector/complex_vector.rs index d83f2ab6e..0f3d4df1a 100644 --- a/crates/harp/src/vector/complex_vector.rs +++ b/crates/harp/src/vector/complex_vector.rs @@ -42,7 +42,7 @@ impl Vector for ComplexVector { type UnderlyingType = Complex; type CompareType = Complex; - unsafe fn new_unchecked(object: impl Into) -> Self { + fn new_unchecked(object: impl Into) -> Self { Self { object: RObject::new(object.into()), } @@ -73,11 +73,11 @@ impl Vector for ComplexVector { } fn is_na(x: &Self::UnderlyingType) -> bool { - unsafe { R_IsNA(x.r) == 1 || R_IsNA(x.i) == 1 } + R_IsNA(x.r) == 1 || R_IsNA(x.i) == 1 } fn get_unchecked_elt(&self, index: isize) -> Self::UnderlyingType { - unsafe { Complex::new(COMPLEX_ELT(self.data(), index as R_xlen_t)) } + Complex::new(COMPLEX_ELT(self.data(), index as R_xlen_t)) } fn convert_value(x: &Self::UnderlyingType) -> Self::Type { diff --git a/crates/harp/src/vector/factor.rs b/crates/harp/src/vector/factor.rs index 615f24a2d..fd343aa19 100644 --- a/crates/harp/src/vector/factor.rs +++ b/crates/harp/src/vector/factor.rs @@ -33,7 +33,7 @@ impl Vector for Factor { type UnderlyingType = i32; type CompareType = i32; - unsafe fn new_unchecked(object: impl Into) -> Self { + fn new_unchecked(object: impl Into) -> Self { let object = RObject::new(object.into()); let levels = CharacterVector::new(Rf_getAttrib(*object, r_symbol!("levels"))).unwrap(); @@ -69,7 +69,7 @@ impl Vector for Factor { } fn get_unchecked_elt(&self, index: isize) -> Self::UnderlyingType { - unsafe { INTEGER_ELT(self.data(), index as R_xlen_t) } + INTEGER_ELT(self.data(), index as R_xlen_t) } fn convert_value(x: &Self::UnderlyingType) -> Self::Type { diff --git a/crates/harp/src/vector/formatted_vector.rs b/crates/harp/src/vector/formatted_vector.rs index 40b0e03e3..a5e4fbbff 100644 --- a/crates/harp/src/vector/formatted_vector.rs +++ b/crates/harp/src/vector/formatted_vector.rs @@ -120,14 +120,12 @@ enum AtomicVector { impl AtomicVector { fn new(vector: RObject) -> anyhow::Result { let vector = match r_typeof(vector.sexp) { - RAWSXP => AtomicVector::Raw(unsafe { RawVector::new_unchecked(vector.sexp) }), - LGLSXP => AtomicVector::Logical(unsafe { LogicalVector::new_unchecked(vector.sexp) }), - INTSXP => AtomicVector::Integer(unsafe { IntegerVector::new_unchecked(vector.sexp) }), - REALSXP => AtomicVector::Numeric(unsafe { NumericVector::new_unchecked(vector.sexp) }), - STRSXP => { - AtomicVector::Character(unsafe { CharacterVector::new_unchecked(vector.sexp) }) - }, - CPLXSXP => AtomicVector::Complex(unsafe { ComplexVector::new_unchecked(vector.sexp) }), + RAWSXP => AtomicVector::Raw(RawVector::new_unchecked(vector.sexp)), + LGLSXP => AtomicVector::Logical(LogicalVector::new_unchecked(vector.sexp)), + INTSXP => AtomicVector::Integer(IntegerVector::new_unchecked(vector.sexp)), + REALSXP => AtomicVector::Numeric(NumericVector::new_unchecked(vector.sexp)), + STRSXP => AtomicVector::Character(CharacterVector::new_unchecked(vector.sexp)), + CPLXSXP => AtomicVector::Complex(ComplexVector::new_unchecked(vector.sexp)), _ => { return Err(anyhow!("Unsupported type")); }, @@ -150,15 +148,13 @@ impl AtomicVector { } fn len(&self) -> usize { - unsafe { - match self { - AtomicVector::Raw(v) => v.len(), - AtomicVector::Logical(v) => v.len(), - AtomicVector::Integer(v) => v.len(), - AtomicVector::Numeric(v) => v.len(), - AtomicVector::Character(v) => v.len(), - AtomicVector::Complex(v) => v.len(), - } + match self { + AtomicVector::Raw(v) => v.len(), + AtomicVector::Logical(v) => v.len(), + AtomicVector::Integer(v) => v.len(), + AtomicVector::Numeric(v) => v.len(), + AtomicVector::Character(v) => v.len(), + AtomicVector::Complex(v) => v.len(), } } } diff --git a/crates/harp/src/vector/integer_vector.rs b/crates/harp/src/vector/integer_vector.rs index 851a5371b..1cb0231e5 100644 --- a/crates/harp/src/vector/integer_vector.rs +++ b/crates/harp/src/vector/integer_vector.rs @@ -29,7 +29,7 @@ impl Vector for IntegerVector { type UnderlyingType = i32; type CompareType = i32; - unsafe fn new_unchecked(object: impl Into) -> Self { + fn new_unchecked(object: impl Into) -> Self { Self { object: RObject::new(object.into()), } @@ -64,7 +64,7 @@ impl Vector for IntegerVector { } fn get_unchecked_elt(&self, index: isize) -> Self::UnderlyingType { - unsafe { INTEGER_ELT(self.data(), index as R_xlen_t) } + INTEGER_ELT(self.data(), index as R_xlen_t) } fn convert_value(x: &Self::UnderlyingType) -> Self::Type { diff --git a/crates/harp/src/vector/list.rs b/crates/harp/src/vector/list.rs index 9d171d40b..9a1ac34fe 100644 --- a/crates/harp/src/vector/list.rs +++ b/crates/harp/src/vector/list.rs @@ -29,7 +29,7 @@ pub struct ListIter { impl List { pub fn iter(&self) -> ListIter { - unsafe { ListIter::new_unchecked(self.obj.sexp) } + ListIter::new_unchecked(self.obj.sexp) } } @@ -40,7 +40,7 @@ impl super::Vector for List { type UnderlyingType = SEXP; type CompareType = RObject; - unsafe fn new_unchecked(object: impl Into) -> Self { + fn new_unchecked(object: impl Into) -> Self { let object: SEXP = object.into(); let ptr = crate::list_cbegin(object); @@ -109,11 +109,11 @@ impl ListIter { }, }; - Ok(unsafe { Self::new_unchecked(x) }) + Ok(Self::new_unchecked(x)) } /// SAFETY: Assumes `x` is VECSXP or EXPRSXP - pub unsafe fn new_unchecked(x: SEXP) -> Self { + pub fn new_unchecked(x: SEXP) -> Self { let ptr = list_cbegin(x); Self { diff --git a/crates/harp/src/vector/logical_vector.rs b/crates/harp/src/vector/logical_vector.rs index 593a33018..65649c9ae 100644 --- a/crates/harp/src/vector/logical_vector.rs +++ b/crates/harp/src/vector/logical_vector.rs @@ -29,7 +29,7 @@ impl Vector for LogicalVector { type UnderlyingType = i32; type CompareType = bool; - unsafe fn new_unchecked(object: impl Into) -> Self { + fn new_unchecked(object: impl Into) -> Self { Self { object: RObject::new(object.into()), } @@ -64,7 +64,7 @@ impl Vector for LogicalVector { } fn get_unchecked_elt(&self, index: isize) -> Self::UnderlyingType { - unsafe { LOGICAL_ELT(self.data(), index as R_xlen_t) } + LOGICAL_ELT(self.data(), index as R_xlen_t) } fn convert_value(x: &Self::UnderlyingType) -> Self::Type { diff --git a/crates/harp/src/vector/names.rs b/crates/harp/src/vector/names.rs index 7e0e8ee0c..e7c7cbc52 100644 --- a/crates/harp/src/vector/names.rs +++ b/crates/harp/src/vector/names.rs @@ -17,19 +17,17 @@ pub struct Names { impl Names { pub fn new(x: SEXP, default: impl Fn(isize) -> String + 'static) -> Self { - unsafe { - let names = RObject::view(x).get_attribute_names(); - let default = Box::new(default); - match names { - Some(names) => Self { - data: Some(CharacterVector::new_unchecked(names.sexp)), - default, - }, - None => Self { - data: None, - default, - }, - } + let names = RObject::view(x).get_attribute_names(); + let default = Box::new(default); + match names { + Some(names) => Self { + data: Some(CharacterVector::new_unchecked(names.sexp)), + default, + }, + None => Self { + data: None, + default, + }, } } diff --git a/crates/harp/src/vector/numeric_vector.rs b/crates/harp/src/vector/numeric_vector.rs index eac1570bb..ac1dbb85c 100644 --- a/crates/harp/src/vector/numeric_vector.rs +++ b/crates/harp/src/vector/numeric_vector.rs @@ -29,7 +29,7 @@ impl Vector for NumericVector { type UnderlyingType = f64; type CompareType = f64; - unsafe fn new_unchecked(object: impl Into) -> Self { + fn new_unchecked(object: impl Into) -> Self { Self { object: RObject::new(object.into()), } @@ -60,11 +60,11 @@ impl Vector for NumericVector { } fn is_na(x: &Self::UnderlyingType) -> bool { - unsafe { R_IsNA(*x) == 1 } + R_IsNA(*x) == 1 } fn get_unchecked_elt(&self, index: isize) -> Self::UnderlyingType { - unsafe { REAL_ELT(self.data(), index as R_xlen_t) } + REAL_ELT(self.data(), index as R_xlen_t) } fn convert_value(x: &Self::UnderlyingType) -> Self::Type { diff --git a/crates/harp/src/vector/raw_vector.rs b/crates/harp/src/vector/raw_vector.rs index 8e0a79ab3..3b5825e05 100644 --- a/crates/harp/src/vector/raw_vector.rs +++ b/crates/harp/src/vector/raw_vector.rs @@ -28,7 +28,7 @@ impl Vector for RawVector { type UnderlyingType = u8; type CompareType = u8; - unsafe fn new_unchecked(object: impl Into) -> Self { + fn new_unchecked(object: impl Into) -> Self { Self { object: RObject::new(object.into()), } @@ -63,7 +63,7 @@ impl Vector for RawVector { } fn get_unchecked_elt(&self, index: isize) -> Self::UnderlyingType { - unsafe { RAW_ELT(self.data(), index as R_xlen_t) } + RAW_ELT(self.data(), index as R_xlen_t) } fn convert_value(x: &Self::UnderlyingType) -> Self::Type { diff --git a/crates/harp/src/weak_ref.rs b/crates/harp/src/weak_ref.rs index c81aa2c65..c4fc9ec5d 100644 --- a/crates/harp/src/weak_ref.rs +++ b/crates/harp/src/weak_ref.rs @@ -47,7 +47,7 @@ impl RWeakRef { let finalizer = Box::into_raw(finalizer); // Wrap that address in an R external pointer. - let finalizer = RObject::new(unsafe { + let finalizer = RObject::new({ libr::R_MakeExternalPtr( finalizer as *mut std::ffi::c_void, RObject::null().sexp, @@ -70,7 +70,7 @@ impl RWeakRef { } // Finally, the weakref wraps `obj` and our finalizer. - let weak_ref = RObject::new(unsafe { + let weak_ref = RObject::new({ libr::R_MakeWeakRefC( finalizer.sexp, // Protected by weakref obj, // Not protected by weakref @@ -88,21 +88,19 @@ impl RWeakRef { /// finalizer has been run. pub fn deref(&self) -> Option { // If finalizer is `NULL` we know for sure the weakref is stale - let key = unsafe { libr::R_WeakRefKey(self.weak_ref.sexp) }; + let key = libr::R_WeakRefKey(self.weak_ref.sexp); if key == RObject::null().sexp { return None; } - Some(RObject::new(unsafe { - libr::R_WeakRefValue(self.weak_ref.sexp) - })) + Some(RObject::new({ libr::R_WeakRefValue(self.weak_ref.sexp) })) } } impl Drop for RWeakRef { // It's fine to run this even when weakref is stale fn drop(&mut self) { - unsafe { libr::R_RunWeakRefFinalizer(self.weak_ref.sexp) } + libr::R_RunWeakRefFinalizer(self.weak_ref.sexp) } } diff --git a/crates/libr/src/constant_globals.rs b/crates/libr/src/constant_globals.rs index 0ae410c63..d43d9e851 100644 --- a/crates/libr/src/constant_globals.rs +++ b/crates/libr/src/constant_globals.rs @@ -41,8 +41,8 @@ macro_rules! generate { paste::paste! { $(#[doc=$doc])* $(#[cfg($cfg)])* - pub unsafe fn $name() -> bool { - [<$name _has>] + pub fn $name() -> bool { + unsafe { [<$name _has>] } } } )+ diff --git a/crates/libr/src/functions.rs b/crates/libr/src/functions.rs index b2f283286..5c82f7df9 100644 --- a/crates/libr/src/functions.rs +++ b/crates/libr/src/functions.rs @@ -27,8 +27,8 @@ macro_rules! generate { paste::paste! { $(#[doc=$doc])* $(#[cfg($cfg)])* - pub unsafe fn $name($($pname: $pty), *) $(-> $ret)* { - [<$name _opt>].unwrap_unchecked()($($pname), *) + pub fn $name($($pname: $pty), *) $(-> $ret)* { + unsafe { [<$name _opt>].unwrap_unchecked()($($pname), *) } } } )+ @@ -42,8 +42,8 @@ macro_rules! generate { paste::paste! { $(#[doc=$doc])* $(#[cfg($cfg)])* - pub unsafe fn $name() -> bool { - matches!([<$name _opt>], Some(_)) + pub fn $name() -> bool { + unsafe { matches!([<$name _opt>], Some(_)) } } } )+ diff --git a/crates/libr/src/functions_variadic.rs b/crates/libr/src/functions_variadic.rs index 5893d9593..26b19aebc 100644 --- a/crates/libr/src/functions_variadic.rs +++ b/crates/libr/src/functions_variadic.rs @@ -29,8 +29,8 @@ macro_rules! generate { paste::paste! { $(#[doc=$doc])* $(#[cfg($cfg)])* - pub unsafe fn $name($($pname: $pty), *) $(-> $ret)* { - [<$name _opt>].unwrap_unchecked()($($pname), *) + pub fn $name($($pname: $pty), *) $(-> $ret)* { + unsafe { [<$name _opt>].unwrap_unchecked()($($pname), *) } } } )+ @@ -44,8 +44,8 @@ macro_rules! generate { paste::paste! { $(#[doc=$doc])* $(#[cfg($cfg)])* - pub unsafe fn $name() -> bool { - matches!([<$name _opt>], Some(_)) + pub fn $name() -> bool { + unsafe { matches!([<$name _opt>], Some(_)) } } } )+ diff --git a/crates/libr/src/lib.rs b/crates/libr/src/lib.rs index 42dd11715..31e8b230d 100644 --- a/crates/libr/src/lib.rs +++ b/crates/libr/src/lib.rs @@ -68,14 +68,14 @@ pub mod graphapp { /// /// NOTE: Must be `Copy`, as you can't move out of a raw pointer, so every access to /// the pointer value generates a copy, but it should be cheap. -pub unsafe fn get(x: *mut T) -> T +pub fn get(x: *mut T) -> T where T: Copy, { - *x + unsafe { *x } } /// Set the value of a mutable global using its pointer -pub unsafe fn set(x: *mut T, value: T) { - *x = value; +pub fn set(x: *mut T, value: T) { + unsafe { *x = value }; } diff --git a/crates/libr/src/mutable_globals.rs b/crates/libr/src/mutable_globals.rs index aad28244c..565746f0d 100644 --- a/crates/libr/src/mutable_globals.rs +++ b/crates/libr/src/mutable_globals.rs @@ -27,8 +27,8 @@ macro_rules! generate { paste::paste! { $(#[doc=$doc])* $(#[cfg($cfg)])* - pub unsafe fn $name() -> bool { - !super::$name.is_null() + pub fn $name() -> bool { + unsafe { !super::$name.is_null() } } } )+ From 5b9e7c54e4fb6fb6c1dd2b6def6196019511e9e3 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Sat, 14 Mar 2026 10:50:59 +0100 Subject: [PATCH 4/6] Make libr entry points unsafe again --- crates/libr/src/constant_globals.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/libr/src/constant_globals.rs b/crates/libr/src/constant_globals.rs index d43d9e851..ad4051b53 100644 --- a/crates/libr/src/constant_globals.rs +++ b/crates/libr/src/constant_globals.rs @@ -5,6 +5,8 @@ // // +#![allow(clippy::missing_safety_doc)] + macro_rules! generate { ( $( @@ -41,8 +43,9 @@ macro_rules! generate { paste::paste! { $(#[doc=$doc])* $(#[cfg($cfg)])* - pub fn $name() -> bool { - unsafe { [<$name _has>] } + #[allow(clippy::missing_safety_doc)] + pub unsafe fn $name() -> bool { + [<$name _has>] } } )+ From 9fec61d23ad058da1646fafb5119b35691e64d2f Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Sat, 14 Mar 2026 10:58:17 +0100 Subject: [PATCH 5/6] Hide raw SEXP type for now --- crates/libr/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/libr/src/types.rs b/crates/libr/src/types.rs index 1247966aa..c5b5102e2 100644 --- a/crates/libr/src/types.rs +++ b/crates/libr/src/types.rs @@ -58,7 +58,7 @@ pub struct SEXPREC { /// across FFI boundaries. #[repr(transparent)] #[derive(Copy, Clone, PartialEq, Eq, Hash)] -pub struct SEXP(pub *mut SEXPREC); +pub struct SEXP(*mut SEXPREC); impl SEXP { pub const fn null() -> Self { From 491100b521c7270b0a5c36e335d02cb863c43afc Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Sat, 14 Mar 2026 10:59:07 +0100 Subject: [PATCH 6/6] Remove unnecessary braces --- crates/harp/src/weak_ref.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/harp/src/weak_ref.rs b/crates/harp/src/weak_ref.rs index c4fc9ec5d..b8c694f60 100644 --- a/crates/harp/src/weak_ref.rs +++ b/crates/harp/src/weak_ref.rs @@ -93,7 +93,7 @@ impl RWeakRef { return None; } - Some(RObject::new({ libr::R_WeakRefValue(self.weak_ref.sexp) })) + Some(RObject::new(libr::R_WeakRefValue(self.weak_ref.sexp))) } }