From d5c6ed72e97ac0505ed3b30bde542f6191e48914 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 13 Sep 2024 21:52:16 -0300 Subject: [PATCH 01/35] Add methods registration and dispatching --- crates/ark/src/variables/methods.rs | 95 +++++++++++++++++++++++++++++ crates/ark/src/variables/mod.rs | 1 + 2 files changed, 96 insertions(+) create mode 100644 crates/ark/src/variables/methods.rs diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs new file mode 100644 index 000000000..b483c72c9 --- /dev/null +++ b/crates/ark/src/variables/methods.rs @@ -0,0 +1,95 @@ +// +// methods.rs +// +// Copyright (C) 2024 by Posit Software, PBC +// +// + +use std::collections::HashMap; +use std::sync::RwLock; + +use anyhow::anyhow; +use harp::environment::r_ns_env; +use harp::environment::R_ENVS; +use harp::exec::RFunction; +use harp::exec::RFunctionExt; +use harp::utils::r_classes; +use harp::RObject; +use libr::SEXP; +use once_cell::sync::Lazy; + +pub static ARK_VARIABLES_METHODS: Lazy>>> = + Lazy::new(|| RwLock::new(HashMap::new())); + +fn register_variables_method(method: String, pkg: String, class: String) { + let mut tables = ARK_VARIABLES_METHODS.write().unwrap(); + + tables + .entry(method) + .or_insert_with(HashMap::new) + .insert(class, pkg); +} + +pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { + let ns = r_ns_env(&pkg)?; + let symbol_names = ns.iter().filter_map(|f| match f { + Ok(binding) => { + let nm: String = binding.name.into(); + Some(nm) + }, + Err(_) => None, + }); + + let methods = vec!["ark_variable_display_value"]; + + for nm in symbol_names { + for method in methods.clone() { + if nm.starts_with(method) { + register_variables_method( + String::from(method), + pkg.clone(), + nm.trim_start_matches(method).to_string(), + ); + break; + } + } + } + + Ok(()) +} + +pub fn dispatch_variables_method(method: String, x: SEXP) -> Option { + // If the object doesn't have classes, just return None + let classes: harp::vector::CharacterVector = r_classes(x)?; + + // Get the method table, if there isn't one return an empty string + let tables = ARK_VARIABLES_METHODS.read().unwrap(); + let method_table = tables.get(&method)?; + + for class in classes.iter().filter_map(|x| x) { + if let Some(pkg) = method_table.get(&class) { + return match call_method(method.clone(), pkg.clone(), class.clone(), x) { + Err(err) => { + log::warn!("Failed dispatching `{pkg}::{method}.{class}`: {err}"); + continue; // Try the method for the next class if there's any + }, + Ok(value) => value, + }; + } + } + None +} + +fn call_method(method: String, pkg: String, class: String, x: SEXP) -> anyhow::Result +where + T: TryFrom, +{ + let result = RFunction::new(pkg.as_str(), format!("{method}.{class}").as_str()) + .add(x) + .call_in(R_ENVS.global)?; + + match result.try_into() { + Err(_) => Err(anyhow!("Failed converting to method return type.")), + Ok(value) => Ok(value), + } +} diff --git a/crates/ark/src/variables/mod.rs b/crates/ark/src/variables/mod.rs index 4f02205e9..ac5c0736b 100644 --- a/crates/ark/src/variables/mod.rs +++ b/crates/ark/src/variables/mod.rs @@ -5,5 +5,6 @@ // // +pub mod methods; pub mod r_variables; pub mod variable; From 63e52e37fa463863d6684e0c5d74b3cc8c11eede Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 13 Sep 2024 22:28:24 -0300 Subject: [PATCH 02/35] Dispatch on methods before relying on defaults --- crates/ark/src/interface.rs | 9 +++++++++ crates/ark/src/variables/methods.rs | 24 ++++++++++++++++++++---- crates/ark/src/variables/variable.rs | 15 +++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 6b5e842a2..7877191f2 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -120,6 +120,8 @@ use crate::ui::UiCommMessage; use crate::ui::UiCommSender; static RE_DEBUG_PROMPT: Lazy = Lazy::new(|| Regex::new(r"Browse\[\d+\]").unwrap()); +use crate::variables::methods::populate_methods_from_loaded_namespaces; +use crate::variables::methods::populate_variable_methods_table; /// An enum representing the different modes in which the R session can run. #[derive(PartialEq, Clone)] @@ -421,6 +423,9 @@ impl RMain { } } + populate_methods_from_loaded_namespaces() + .or_log_error("Can't populate variables pane methods from loaded packages"); + // Set up the global error handler (after support function initialization) errors::initialize(); @@ -1962,6 +1967,10 @@ unsafe extern "C" fn ps_onload_hook(pkg: SEXP, _path: SEXP) -> anyhow::Result>>> = +static ARK_VARIABLES_METHODS: Lazy>>> = Lazy::new(|| RwLock::new(HashMap::new())); fn register_variables_method(method: String, pkg: String, class: String) { @@ -30,6 +31,17 @@ fn register_variables_method(method: String, pkg: String, class: String) { .insert(class, pkg); } +pub fn populate_methods_from_loaded_namespaces() -> anyhow::Result<()> { + let loaded = RFunction::new("base", "loadedNamespaces").call()?; + let loaded: Vec = loaded.try_into()?; + + for pkg in loaded.into_iter() { + populate_variable_methods_table(pkg).or_log_error("Failed populating methods"); + } + + Ok(()) +} + pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { let ns = r_ns_env(&pkg)?; let symbol_names = ns.iter().filter_map(|f| match f { @@ -48,7 +60,8 @@ pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { register_variables_method( String::from(method), pkg.clone(), - nm.trim_start_matches(method).to_string(), + // 1.. is used to remove the `.` that follows the method name + nm.trim_start_matches(method)[1..].to_string(), ); break; } @@ -58,7 +71,10 @@ pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { Ok(()) } -pub fn dispatch_variables_method(method: String, x: SEXP) -> Option { +pub fn dispatch_variables_method(method: String, x: SEXP) -> Option +where + T: TryFrom, +{ // If the object doesn't have classes, just return None let classes: harp::vector::CharacterVector = r_classes(x)?; @@ -73,7 +89,7 @@ pub fn dispatch_variables_method(method: String, x: SEXP) -> Option { log::warn!("Failed dispatching `{pkg}::{method}.{class}`: {err}"); continue; // Try the method for the next class if there's any }, - Ok(value) => value, + Ok(value) => Some(value), }; } } diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 8ee223934..09ab63a5d 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -51,6 +51,8 @@ use libr::*; use stdext::local; use stdext::unwrap; +use crate::variables::methods::dispatch_variables_method; + // Constants. const MAX_DISPLAY_VALUE_ENTRIES: usize = 1_000; const MAX_DISPLAY_VALUE_LENGTH: usize = 100; @@ -284,6 +286,12 @@ impl WorkspaceVariableDisplayValue { } fn from_default(value: SEXP) -> Self { + // Try to use the display method if there's one available + match dispatch_variables_method(String::from("ark_variables_display_value"), value) { + Some(display_value) => return Self::from_untruncated_display_value(display_value), + None => {}, + } + let formatted = unwrap!(FormattedVector::new(value), Err(err) => { return Self::from_error(err); }); @@ -312,6 +320,13 @@ impl WorkspaceVariableDisplayValue { log::trace!("Error while formatting variable: {err:?}"); Self::new(String::from("??"), true) } + + fn from_untruncated_display_value(display_value: String) -> Self { + let mut display_value = display_value.clone(); + let is_truncated = display_value.len() > MAX_DISPLAY_VALUE_LENGTH; + display_value.truncate(MAX_DISPLAY_VALUE_LENGTH); + Self::new(display_value, is_truncated) + } } pub struct WorkspaceVariableDisplayType { From 072c4dde9be3c6607238e440b47e72f847221a7b Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Sat, 14 Sep 2024 11:05:41 -0300 Subject: [PATCH 03/35] add more supported methods --- crates/ark/src/variables/methods.rs | 63 +++++++++++++++++++++------- crates/ark/src/variables/variable.rs | 58 ++++++++++++++++++++++--- 2 files changed, 99 insertions(+), 22 deletions(-) diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index 0989ec29f..da7752f63 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -10,6 +10,7 @@ use std::sync::RwLock; use anyhow::anyhow; use harp::environment::r_ns_env; +use harp::environment::BindingValue; use harp::environment::R_ENVS; use harp::exec::RFunction; use harp::exec::RFunctionExt; @@ -24,7 +25,7 @@ static ARK_VARIABLES_METHODS: Lazy anyhow::Result<()> { pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { let ns = r_ns_env(&pkg)?; - let symbol_names = ns.iter().filter_map(|f| match f { - Ok(binding) => { - let nm: String = binding.name.into(); - Some(nm) - }, - Err(_) => None, - }); - - let methods = vec!["ark_variable_display_value"]; + let symbol_names = ns + .iter() + .filter_map(Result::ok) + .filter(|b| match b.value { + BindingValue::Standard { .. } => true, + BindingValue::Promise { .. } => true, + _ => false, + }) + .map(|b| -> String { b.name.into() }); + + let methods = vec![ + "ark_variable_display_value", + "ark_variable_display_type", + "ark_variable_has_children", + "ark_variable_kind", + ]; for nm in symbol_names { for method in methods.clone() { @@ -61,7 +69,8 @@ pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { String::from(method), pkg.clone(), // 1.. is used to remove the `.` that follows the method name - nm.trim_start_matches(method)[1..].to_string(), + nm.trim_start_matches(format!("{method}.").as_str()) + .to_string(), ); break; } @@ -72,6 +81,17 @@ pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { } pub fn dispatch_variables_method(method: String, x: SEXP) -> Option +where + T: TryFrom, +{ + dispatch_variables_method_with_args(method, x, HashMap::new()) +} + +pub fn dispatch_variables_method_with_args( + method: String, + x: SEXP, + args: HashMap, +) -> Option where T: TryFrom, { @@ -84,7 +104,7 @@ where for class in classes.iter().filter_map(|x| x) { if let Some(pkg) = method_table.get(&class) { - return match call_method(method.clone(), pkg.clone(), class.clone(), x) { + return match call_method(method.clone(), pkg.clone(), class.clone(), x, args.clone()) { Err(err) => { log::warn!("Failed dispatching `{pkg}::{method}.{class}`: {err}"); continue; // Try the method for the next class if there's any @@ -96,13 +116,24 @@ where None } -fn call_method(method: String, pkg: String, class: String, x: SEXP) -> anyhow::Result +fn call_method( + method: String, + pkg: String, + class: String, + x: SEXP, + args: HashMap, +) -> anyhow::Result where T: TryFrom, { - let result = RFunction::new(pkg.as_str(), format!("{method}.{class}").as_str()) - .add(x) - .call_in(R_ENVS.global)?; + let mut call = RFunction::new_internal(pkg.as_str(), format!("{method}.{class}").as_str()); + call.add(x); + + for (name, value) in args.into_iter() { + call.param(name.as_str(), value); + } + + let result = call.call_in(R_ENVS.global)?; match result.try_into() { Err(_) => Err(anyhow!("Failed converting to method return type.")), diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 09ab63a5d..7c0c90d2e 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -5,6 +5,7 @@ // // +use std::collections::HashMap; use std::time::SystemTime; use std::time::UNIX_EPOCH; @@ -12,6 +13,7 @@ use amalthea::comm::variables_comm::ClipboardFormatFormat; use amalthea::comm::variables_comm::Variable; use amalthea::comm::variables_comm::VariableKind; use anyhow::anyhow; +use anyhow::Context; use harp::environment::Binding; use harp::environment::BindingValue; use harp::environment::Environment; @@ -52,6 +54,7 @@ use stdext::local; use stdext::unwrap; use crate::variables::methods::dispatch_variables_method; +use crate::variables::methods::dispatch_variables_method_with_args; // Constants. const MAX_DISPLAY_VALUE_ENTRIES: usize = 1_000; @@ -72,6 +75,14 @@ fn plural(text: &str, n: i32) -> String { impl WorkspaceVariableDisplayValue { pub fn from(value: SEXP) -> Self { + // Try to use the display method if there's one available + match dispatch_variables_method(String::from("ark_variable_display_value"), value) { + Some(display_value) => return Self::from_untruncated_display_value(display_value), + None => { + // No method found, we can just continue + }, + } + match r_typeof(value) { NILSXP => Self::new(String::from("NULL"), false), VECSXP if r_inherits(value, "data.frame") => Self::from_data_frame(value), @@ -286,12 +297,6 @@ impl WorkspaceVariableDisplayValue { } fn from_default(value: SEXP) -> Self { - // Try to use the display method if there's one available - match dispatch_variables_method(String::from("ark_variables_display_value"), value) { - Some(display_value) => return Self::from_untruncated_display_value(display_value), - None => {}, - } - let formatted = unwrap!(FormattedVector::new(value), Err(err) => { return Self::from_error(err); }); @@ -342,6 +347,13 @@ impl WorkspaceVariableDisplayType { /// - include_length: Whether to include the length of the object in the /// display type. pub fn from(value: SEXP, include_length: bool) -> Self { + match Self::try_from_method(value, include_length) { + Ok(display_type) => return display_type, + Err(_) => { + // Continue on to the default approaches + }, + } + if r_is_null(value) { return Self::simple(String::from("NULL")); } @@ -440,6 +452,18 @@ impl WorkspaceVariableDisplayType { } } + fn try_from_method(value: SEXP, include_length: bool) -> anyhow::Result { + let mut args: HashMap = HashMap::new(); + args.insert(String::from("include_length"), include_length.try_into()?); + let display_type: Option = dispatch_variables_method_with_args( + String::from("ark_variable_display_type"), + value, + args, + ); + let value: String = display_type.context("Empty display value")?; + Ok(Self::simple(value)) + } + fn new(display_type: String, type_info: String) -> Self { Self { display_type, @@ -449,6 +473,14 @@ impl WorkspaceVariableDisplayType { } fn has_children(value: SEXP) -> bool { + // Try to use the display method if there's one available + match dispatch_variables_method(String::from("ark_variable_has_children"), value) { + Some(has_children) => return has_children, + None => { + // Just continue, no method was found + }, + } + if RObject::view(value).is_s4() { unsafe { let names = RFunction::new("methods", ".slotNames") @@ -640,6 +672,13 @@ impl PositronVariable { return VariableKind::Empty; } + match variable_kind_try_from_method(x) { + Ok(kind) => return kind, + Err(_) => { + // Continue to default operations + }, + } + let obj = RObject::view(x); if obj.is_s4() { @@ -1281,6 +1320,13 @@ impl PositronVariable { } } +fn variable_kind_try_from_method(value: SEXP) -> anyhow::Result { + let kind: String = dispatch_variables_method(String::from("ark_variable_kind"), value) + .context("No kind found")?; + + Ok(serde_json::from_str(kind.as_str())?) +} + pub fn is_binding_fancy(binding: &Binding) -> bool { match &binding.value { BindingValue::Active { .. } => true, From dbfadc22e79d8e3ddb3e678dc2abf36f4f61788a Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Sat, 14 Sep 2024 12:34:27 -0300 Subject: [PATCH 04/35] Use a strum enum to make less error prone method dispatching --- Cargo.lock | 26 +++++++++-- crates/ark/Cargo.toml | 1 + crates/ark/src/variables/methods.rs | 70 ++++++++++++++++++---------- crates/ark/src/variables/variable.rs | 9 ++-- 4 files changed, 75 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a755739ec..cc84ecfb6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -255,7 +255,7 @@ dependencies = [ "sha2", "stdext", "strum 0.24.1", - "strum_macros", + "strum_macros 0.24.3", "tracing", "uuid", "zmq", @@ -320,6 +320,7 @@ dependencies = [ "stdext", "struct-field-names-as-array", "strum 0.26.2", + "strum_macros 0.26.4", "tempfile", "tokio", "tower-lsp", @@ -1166,6 +1167,12 @@ version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "95505c38b4572b2d910cecb0281560f54b440a19336cbbcb27bf6ce6adc6f5a8" +[[package]] +name = "heck" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" + [[package]] name = "hermit-abi" version = "0.2.6" @@ -2773,13 +2780,26 @@ version = "0.24.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1e385be0d24f186b4ce2f9982191e7101bb737312ad61c1f2f984f34bcf85d59" dependencies = [ - "heck", + "heck 0.4.1", "proc-macro2", "quote", "rustversion", "syn 1.0.109", ] +[[package]] +name = "strum_macros" +version = "0.26.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c6bee85a5a24955dc440386795aa378cd9cf82acd5f764469152d2270e581be" +dependencies = [ + "heck 0.5.0", + "proc-macro2", + "quote", + "rustversion", + "syn 2.0.32", +] + [[package]] name = "subtle" version = "2.6.1" @@ -2824,7 +2844,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5fa6fb9ee296c0dc2df41a656ca7948546d061958115ddb0bcaae43ad0d17d2" dependencies = [ "cfg-expr", - "heck", + "heck 0.4.1", "pkg-config", "toml 0.7.3", "version-compare", diff --git a/crates/ark/Cargo.toml b/crates/ark/Cargo.toml index 944f44c34..17fdd1022 100644 --- a/crates/ark/Cargo.toml +++ b/crates/ark/Cargo.toml @@ -55,6 +55,7 @@ yaml-rust = "0.4.5" winsafe = { version = "0.0.19", features = ["kernel"] } struct-field-names-as-array = "0.3.0" strum = "0.26.2" +strum_macros = "0.26.2" futures = "0.3.30" tracing = "0.1.40" tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index da7752f63..1b1a881ea 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -19,11 +19,49 @@ use harp::RObject; use libr::SEXP; use once_cell::sync::Lazy; use stdext::result::ResultOrLog; +use strum::IntoEnumIterator; +use strum_macros::Display; +use strum_macros::EnumIter; +use strum_macros::EnumString; +use strum_macros::IntoStaticStr; -static ARK_VARIABLES_METHODS: Lazy>>> = +#[derive(Debug, PartialEq, EnumString, EnumIter, IntoStaticStr, Display, Eq, Hash, Clone)] +pub enum ArkVariablesMethods { + #[strum(serialize = "ark_variable_display_value")] + VariableDisplayValue, + + #[strum(serialize = "ark_variable_display_type")] + VariableDisplayType, + + #[strum(serialize = "ark_variable_has_children")] + VariableHasChildren, + + #[strum(serialize = "ark_variable_kind")] + VariableKind, +} + +impl ArkVariablesMethods { + // Checks if a symbol name is a method and returns it's class + fn parse_method(name: &String) -> Option<(Self, String)> { + for method in ArkVariablesMethods::iter() { + let method_str: &str = method.clone().into(); + if name.starts_with::<&str>(method_str) { + return Some(( + method, + name.trim_start_matches::<&str>(method_str) + .trim_start_matches('.') + .to_string(), + )); + } + } + None + } +} + +static ARK_VARIABLES_METHODS: Lazy>>> = Lazy::new(|| RwLock::new(HashMap::new())); -fn register_variables_method(method: String, pkg: String, class: String) { +fn register_variables_method(method: ArkVariablesMethods, pkg: String, class: String) { let mut tables = ARK_VARIABLES_METHODS.write().unwrap(); log::info!("Found method:{method} for class:{class} on pkg:{pkg}"); tables @@ -55,32 +93,16 @@ pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { }) .map(|b| -> String { b.name.into() }); - let methods = vec![ - "ark_variable_display_value", - "ark_variable_display_type", - "ark_variable_has_children", - "ark_variable_kind", - ]; - - for nm in symbol_names { - for method in methods.clone() { - if nm.starts_with(method) { - register_variables_method( - String::from(method), - pkg.clone(), - // 1.. is used to remove the `.` that follows the method name - nm.trim_start_matches(format!("{method}.").as_str()) - .to_string(), - ); - break; - } + for name in symbol_names { + if let Some((method, class)) = ArkVariablesMethods::parse_method(&name) { + register_variables_method(method, pkg.clone(), class); } } Ok(()) } -pub fn dispatch_variables_method(method: String, x: SEXP) -> Option +pub fn dispatch_variables_method(method: ArkVariablesMethods, x: SEXP) -> Option where T: TryFrom, { @@ -88,7 +110,7 @@ where } pub fn dispatch_variables_method_with_args( - method: String, + method: ArkVariablesMethods, x: SEXP, args: HashMap, ) -> Option @@ -117,7 +139,7 @@ where } fn call_method( - method: String, + method: ArkVariablesMethods, pkg: String, class: String, x: SEXP, diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 7c0c90d2e..649c1c0f0 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -55,6 +55,7 @@ use stdext::unwrap; use crate::variables::methods::dispatch_variables_method; use crate::variables::methods::dispatch_variables_method_with_args; +use crate::variables::methods::ArkVariablesMethods; // Constants. const MAX_DISPLAY_VALUE_ENTRIES: usize = 1_000; @@ -76,7 +77,7 @@ fn plural(text: &str, n: i32) -> String { impl WorkspaceVariableDisplayValue { pub fn from(value: SEXP) -> Self { // Try to use the display method if there's one available - match dispatch_variables_method(String::from("ark_variable_display_value"), value) { + match dispatch_variables_method(ArkVariablesMethods::VariableDisplayValue, value) { Some(display_value) => return Self::from_untruncated_display_value(display_value), None => { // No method found, we can just continue @@ -456,7 +457,7 @@ impl WorkspaceVariableDisplayType { let mut args: HashMap = HashMap::new(); args.insert(String::from("include_length"), include_length.try_into()?); let display_type: Option = dispatch_variables_method_with_args( - String::from("ark_variable_display_type"), + ArkVariablesMethods::VariableDisplayType, value, args, ); @@ -474,7 +475,7 @@ impl WorkspaceVariableDisplayType { fn has_children(value: SEXP) -> bool { // Try to use the display method if there's one available - match dispatch_variables_method(String::from("ark_variable_has_children"), value) { + match dispatch_variables_method(ArkVariablesMethods::VariableHasChildren, value) { Some(has_children) => return has_children, None => { // Just continue, no method was found @@ -1321,7 +1322,7 @@ impl PositronVariable { } fn variable_kind_try_from_method(value: SEXP) -> anyhow::Result { - let kind: String = dispatch_variables_method(String::from("ark_variable_kind"), value) + let kind: String = dispatch_variables_method(ArkVariablesMethods::VariableKind, value) .context("No kind found")?; Ok(serde_json::from_str(kind.as_str())?) From 7f973e82ca9f72780c01a85ddd4f8916002091a0 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Sat, 14 Sep 2024 12:53:35 -0300 Subject: [PATCH 05/35] Favor let Some(x) instead of match --- crates/ark/src/variables/variable.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 649c1c0f0..719e71ee0 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -77,11 +77,10 @@ fn plural(text: &str, n: i32) -> String { impl WorkspaceVariableDisplayValue { pub fn from(value: SEXP) -> Self { // Try to use the display method if there's one available - match dispatch_variables_method(ArkVariablesMethods::VariableDisplayValue, value) { - Some(display_value) => return Self::from_untruncated_display_value(display_value), - None => { - // No method found, we can just continue - }, + if let Some(display_value) = + dispatch_variables_method(ArkVariablesMethods::VariableDisplayValue, value) + { + return Self::from_untruncated_display_value(display_value); } match r_typeof(value) { @@ -475,11 +474,10 @@ impl WorkspaceVariableDisplayType { fn has_children(value: SEXP) -> bool { // Try to use the display method if there's one available - match dispatch_variables_method(ArkVariablesMethods::VariableHasChildren, value) { - Some(has_children) => return has_children, - None => { - // Just continue, no method was found - }, + if let Some(has_children) = + dispatch_variables_method(ArkVariablesMethods::VariableHasChildren, value) + { + return has_children; } if RObject::view(value).is_s4() { From 02fcd4e9cf0d4f10e2631a83c93ce8eb4953dcfa Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Sat, 14 Sep 2024 12:55:52 -0300 Subject: [PATCH 06/35] use let Ok() instead --- crates/ark/src/variables/variable.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 719e71ee0..c3dce656f 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -671,11 +671,8 @@ impl PositronVariable { return VariableKind::Empty; } - match variable_kind_try_from_method(x) { - Ok(kind) => return kind, - Err(_) => { - // Continue to default operations - }, + if let Ok(kind) = variable_kind_try_from_method(x) { + return kind; } let obj = RObject::view(x); From 302f9a5eb265584a3ca89d9533e30db23cceb0ed Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Mon, 16 Sep 2024 17:52:15 -0300 Subject: [PATCH 07/35] Move into an R based impl --- crates/ark/src/interface.rs | 2 +- crates/ark/src/modules/positron/methods.R | 52 +++++++++ crates/ark/src/variables/methods.rs | 131 +++++++++++----------- crates/ark/src/variables/variable.rs | 21 ++-- 4 files changed, 131 insertions(+), 75 deletions(-) create mode 100644 crates/ark/src/modules/positron/methods.R diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 7877191f2..5e363ea98 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -1968,7 +1968,7 @@ unsafe extern "C" fn ps_onload_hook(pkg: SEXP, _path: SEXP) -> anyhow::Result anyhow::Result<()> { + let method = RObject::from(unsafe { + Rf_lang3( + r_symbol!(":::"), + r_symbol!(package), + r_symbol!(format!("{generic}.{class}")), + ) + }); + Self::register_method(generic, class, method)?; + Ok(()) + } + + fn register_method(generic: Self, class: &str, method: RObject) -> anyhow::Result<()> { + let generic_name: &str = generic.into(); + RFunction::new("", "register_ark_method") + .add(RObject::try_from(generic_name)?) + .add(RObject::try_from(class)?) + .add(method) + .call_in(ARK_ENVS.positron_ns)?; + Ok(()) + } + // Checks if a symbol name is a method and returns it's class fn parse_method(name: &String) -> Option<(Self, String)> { - for method in ArkVariablesMethods::iter() { + for method in ArkVariablesGenerics::iter() { let method_str: &str = method.clone().into(); if name.starts_with::<&str>(method_str) { return Some(( @@ -58,31 +86,19 @@ impl ArkVariablesMethods { } } -static ARK_VARIABLES_METHODS: Lazy>>> = - Lazy::new(|| RwLock::new(HashMap::new())); - -fn register_variables_method(method: ArkVariablesMethods, pkg: String, class: String) { - let mut tables = ARK_VARIABLES_METHODS.write().unwrap(); - log::info!("Found method:{method} for class:{class} on pkg:{pkg}"); - tables - .entry(method) - .or_insert_with(HashMap::new) - .insert(class, pkg); -} - pub fn populate_methods_from_loaded_namespaces() -> anyhow::Result<()> { let loaded = RFunction::new("base", "loadedNamespaces").call()?; let loaded: Vec = loaded.try_into()?; for pkg in loaded.into_iter() { - populate_variable_methods_table(pkg).or_log_error("Failed populating methods"); + populate_variable_methods_table(pkg.as_str()).or_log_error("Failed populating methods"); } Ok(()) } -pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { - let ns = r_ns_env(&pkg)?; +pub fn populate_variable_methods_table(package: &str) -> anyhow::Result<()> { + let ns = r_ns_env(package)?; let symbol_names = ns .iter() .filter_map(Result::ok) @@ -94,71 +110,60 @@ pub fn populate_variable_methods_table(pkg: String) -> anyhow::Result<()> { .map(|b| -> String { b.name.into() }); for name in symbol_names { - if let Some((method, class)) = ArkVariablesMethods::parse_method(&name) { - register_variables_method(method, pkg.clone(), class); + if let Some((generic, class)) = ArkVariablesGenerics::parse_method(&name) { + ArkVariablesGenerics::register_method_from_package(generic, class.as_str(), package)?; } } Ok(()) } -pub fn dispatch_variables_method(method: ArkVariablesMethods, x: SEXP) -> Option +pub fn dispatch_variables_method(generic: ArkVariablesGenerics, x: SEXP) -> anyhow::Result where T: TryFrom, + >::Error: std::fmt::Debug, { - dispatch_variables_method_with_args(method, x, HashMap::new()) + dispatch_variables_method_with_args(generic, x, HashMap::new()) } pub fn dispatch_variables_method_with_args( - method: ArkVariablesMethods, - x: SEXP, - args: HashMap, -) -> Option -where - T: TryFrom, -{ - // If the object doesn't have classes, just return None - let classes: harp::vector::CharacterVector = r_classes(x)?; - - // Get the method table, if there isn't one return an empty string - let tables = ARK_VARIABLES_METHODS.read().unwrap(); - let method_table = tables.get(&method)?; - - for class in classes.iter().filter_map(|x| x) { - if let Some(pkg) = method_table.get(&class) { - return match call_method(method.clone(), pkg.clone(), class.clone(), x, args.clone()) { - Err(err) => { - log::warn!("Failed dispatching `{pkg}::{method}.{class}`: {err}"); - continue; // Try the method for the next class if there's any - }, - Ok(value) => Some(value), - }; - } - } - None -} - -fn call_method( - method: ArkVariablesMethods, - pkg: String, - class: String, + generic: ArkVariablesGenerics, x: SEXP, args: HashMap, ) -> anyhow::Result where T: TryFrom, + >::Error: std::fmt::Debug, { - let mut call = RFunction::new_internal(pkg.as_str(), format!("{method}.{class}").as_str()); + let generic_name: &str = generic.into(); + let mut call = RFunction::new("", "call_ark_method"); + + call.add(generic_name); call.add(x); for (name, value) in args.into_iter() { call.param(name.as_str(), value); } - let result = call.call_in(R_ENVS.global)?; - - match result.try_into() { - Err(_) => Err(anyhow!("Failed converting to method return type.")), + match call.call_in(ARK_ENVS.positron_ns)?.try_into() { Ok(value) => Ok(value), + Err(err) => Err(anyhow!("Failed converting to type: {err:?}")), } } + +#[harp::register] +extern "C" fn ps_register_ark_method( + generic: SEXP, + class: SEXP, + method: SEXP, +) -> anyhow::Result { + let generic: String = RObject::from(generic).try_into()?; + let class: String = RObject::from(class).try_into()?; + + ArkVariablesGenerics::register_method( + ArkVariablesGenerics::from_str(generic.as_str())?, + class.as_str(), + RObject::from(method), + )?; + Ok(r_null()) +} diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index c3dce656f..8728597ed 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -55,7 +55,7 @@ use stdext::unwrap; use crate::variables::methods::dispatch_variables_method; use crate::variables::methods::dispatch_variables_method_with_args; -use crate::variables::methods::ArkVariablesMethods; +use crate::variables::methods::ArkVariablesGenerics; // Constants. const MAX_DISPLAY_VALUE_ENTRIES: usize = 1_000; @@ -77,8 +77,8 @@ fn plural(text: &str, n: i32) -> String { impl WorkspaceVariableDisplayValue { pub fn from(value: SEXP) -> Self { // Try to use the display method if there's one available - if let Some(display_value) = - dispatch_variables_method(ArkVariablesMethods::VariableDisplayValue, value) + if let Ok(display_value) = + dispatch_variables_method(ArkVariablesGenerics::VariableDisplayValue, value) { return Self::from_untruncated_display_value(display_value); } @@ -455,13 +455,12 @@ impl WorkspaceVariableDisplayType { fn try_from_method(value: SEXP, include_length: bool) -> anyhow::Result { let mut args: HashMap = HashMap::new(); args.insert(String::from("include_length"), include_length.try_into()?); - let display_type: Option = dispatch_variables_method_with_args( - ArkVariablesMethods::VariableDisplayType, + let display_type: String = dispatch_variables_method_with_args( + ArkVariablesGenerics::VariableDisplayType, value, args, - ); - let value: String = display_type.context("Empty display value")?; - Ok(Self::simple(value)) + )?; + Ok(Self::simple(display_type)) } fn new(display_type: String, type_info: String) -> Self { @@ -474,8 +473,8 @@ impl WorkspaceVariableDisplayType { fn has_children(value: SEXP) -> bool { // Try to use the display method if there's one available - if let Some(has_children) = - dispatch_variables_method(ArkVariablesMethods::VariableHasChildren, value) + if let Ok(has_children) = + dispatch_variables_method(ArkVariablesGenerics::VariableHasChildren, value) { return has_children; } @@ -1317,7 +1316,7 @@ impl PositronVariable { } fn variable_kind_try_from_method(value: SEXP) -> anyhow::Result { - let kind: String = dispatch_variables_method(ArkVariablesMethods::VariableKind, value) + let kind: String = dispatch_variables_method(ArkVariablesGenerics::VariableKind, value) .context("No kind found")?; Ok(serde_json::from_str(kind.as_str())?) From 7473e49fd3c232f9b4d5f39d60085269dcf3a29f Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Tue, 17 Sep 2024 12:10:43 -0400 Subject: [PATCH 08/35] minor revision --- crates/ark/src/modules/positron/methods.R | 65 ++++++++++----------- crates/ark/src/variables/methods.rs | 59 ++++++++----------- crates/ark/src/variables/variable.rs | 70 ++++++++++++----------- 3 files changed, 94 insertions(+), 100 deletions(-) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index d65355947..c3f45dbce 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -6,47 +6,48 @@ # ark_methods_table <- new.env(parent = emptyenv()) +ark_methods_table$ark_variable_display_value <- new.env(parent = emptyenv()) +ark_methods_table$ark_variable_display_type <- new.env(parent = emptyenv()) +ark_methods_table$ark_variable_has_children <- new.env(parent = emptyenv()) +ark_methods_table$ark_variable_kind <- new.env(parent = emptyenv()) +lockEnvironment(ark_methods_table, TRUE) -register_ark_method <- function(generic, class, method) { - method <- substitute(method) - if (!exists(generic, envir = ark_methods_table)) { - ark_methods_table[[generic]] <- new.env(parent = emptyenv()) - } - ark_methods_table[[generic]][[class]] <- method +#' Register the methods with the Positron runtime +#' +#' @param generic Generic function name as a character to register +#' @param class Class name as a character +#' @param method A method to be registered. Should be a call object. +#' @export +.ps.register_ark_method <- function(generic, class, method) { + stopifnot( + typeof(generic) == "character", + length(generic) == 1, + generic %in% c( + "ark_variable_display_value", + "ark_variable_display_type", + "ark_variable_has_children", + "ark_variable_kind" + ), + typeof(class) == "character" + ) + for (cls in class) + assign(cls, method, envir = ark_methods_table[[generic]]) + + invisible() } call_ark_method <- function(generic, object, ...) { methods_table <- ark_methods_table[[generic]] - if (is.null(methods_table)) { - stop("No methods registered for generic '", generic, "'") - } + if (is.null(methods_table)) + return(NULL) for (cl in class(object)) { - if (exists(cl, envir = methods_table)) { - return(do.call(eval(methods_table[[cl]], envir = globalenv()), list(object, ...))) + if (is.function(method <- get0(cl, envir = methods_table))) { + return(eval(as.call(list(method, object, ...)), + envir = globalenv())) } } - stop("No methods found for object") -} - -#' Register the methods with the Positron runtime -#' -#' @param generic Generic function name as a character to register -#' @param class class name as a character -#' @param method A method to be registered. Should be a call object. -.ps.register_ark_method <- function(generic, class, method) { - # Even though methods are stored in an R environment, we call into Rust - # to make sure we have a single source of truth for generic names. - - # Functions are inlined, so we always construct the call into them. - # this also allows registering with eg: `pkg::fun` without explicitly - # using the function from that namespace, which might change at points - # in time. - if (is.function(method)) { - method <- substitute(method) - } - - .ps.Call("ps_register_ark_method", generic, class, substitute(method)) + NULL } diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index adbb83e60..677b8d758 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -5,9 +5,6 @@ // // -use std::collections::HashMap; -use std::str::FromStr; - use anyhow::anyhow; use harp::environment::r_ns_env; use harp::environment::BindingValue; @@ -15,6 +12,7 @@ use harp::exec::RFunction; use harp::exec::RFunctionExt; use harp::r_null; use harp::r_symbol; +use harp::utils::r_is_object; use harp::RObject; use libr::Rf_lang3; use libr::SEXP; @@ -61,7 +59,7 @@ impl ArkVariablesGenerics { fn register_method(generic: Self, class: &str, method: RObject) -> anyhow::Result<()> { let generic_name: &str = generic.into(); - RFunction::new("", "register_ark_method") + RFunction::new("", ".ps.register_ark_method") .add(RObject::try_from(generic_name)?) .add(RObject::try_from(class)?) .add(method) @@ -74,12 +72,9 @@ impl ArkVariablesGenerics { for method in ArkVariablesGenerics::iter() { let method_str: &str = method.clone().into(); if name.starts_with::<&str>(method_str) { - return Some(( - method, - name.trim_start_matches::<&str>(method_str) - .trim_start_matches('.') - .to_string(), - )); + if let Some((_, class)) = name.split_once(".") { + return Some((method, class.to_string())); + } } } None @@ -118,52 +113,44 @@ pub fn populate_variable_methods_table(package: &str) -> anyhow::Result<()> { Ok(()) } -pub fn dispatch_variables_method(generic: ArkVariablesGenerics, x: SEXP) -> anyhow::Result +pub fn dispatch_variables_method( + generic: ArkVariablesGenerics, + x: SEXP, +) -> anyhow::Result> where T: TryFrom, >::Error: std::fmt::Debug, { - dispatch_variables_method_with_args(generic, x, HashMap::new()) + dispatch_variables_method_with_args(generic, x, Vec::new()) } pub fn dispatch_variables_method_with_args( generic: ArkVariablesGenerics, x: SEXP, - args: HashMap, -) -> anyhow::Result + args: Vec<(String, RObject)>, +) -> anyhow::Result> where T: TryFrom, >::Error: std::fmt::Debug, { - let generic_name: &str = generic.into(); + if !r_is_object(x) { + return Ok(None); + } + let generic: &str = generic.into(); let mut call = RFunction::new("", "call_ark_method"); - call.add(generic_name); + call.add(generic); call.add(x); for (name, value) in args.into_iter() { call.param(name.as_str(), value); } - match call.call_in(ARK_ENVS.positron_ns)?.try_into() { - Ok(value) => Ok(value), - Err(err) => Err(anyhow!("Failed converting to type: {err:?}")), + let result = call.call_in(ARK_ENVS.positron_ns)?; + if result.sexp == r_null() { + Ok(None) + } else { + let result = result.try_into().map_err(|e| anyhow!("{:?}", e))?; + Ok(Some(result)) } } - -#[harp::register] -extern "C" fn ps_register_ark_method( - generic: SEXP, - class: SEXP, - method: SEXP, -) -> anyhow::Result { - let generic: String = RObject::from(generic).try_into()?; - let class: String = RObject::from(class).try_into()?; - - ArkVariablesGenerics::register_method( - ArkVariablesGenerics::from_str(generic.as_str())?, - class.as_str(), - RObject::from(method), - )?; - Ok(r_null()) -} diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 8728597ed..36ab58bc1 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -5,7 +5,6 @@ // // -use std::collections::HashMap; use std::time::SystemTime; use std::time::UNIX_EPOCH; @@ -13,7 +12,6 @@ use amalthea::comm::variables_comm::ClipboardFormatFormat; use amalthea::comm::variables_comm::Variable; use amalthea::comm::variables_comm::VariableKind; use anyhow::anyhow; -use anyhow::Context; use harp::environment::Binding; use harp::environment::BindingValue; use harp::environment::Environment; @@ -77,10 +75,11 @@ fn plural(text: &str, n: i32) -> String { impl WorkspaceVariableDisplayValue { pub fn from(value: SEXP) -> Self { // Try to use the display method if there's one available - if let Ok(display_value) = - dispatch_variables_method(ArkVariablesGenerics::VariableDisplayValue, value) - { - return Self::from_untruncated_display_value(display_value); + + match try_from_method_display_type(value, MAX_DISPLAY_VALUE_LENGTH) { + Err(e) => log::error!("Error from 'ark_variable_display_value' method: {e}"), + Ok(None) => {}, + Ok(Some(display_value)) => return Self::new(display_value, false), } match r_typeof(value) { @@ -325,13 +324,6 @@ impl WorkspaceVariableDisplayValue { log::trace!("Error while formatting variable: {err:?}"); Self::new(String::from("??"), true) } - - fn from_untruncated_display_value(display_value: String) -> Self { - let mut display_value = display_value.clone(); - let is_truncated = display_value.len() > MAX_DISPLAY_VALUE_LENGTH; - display_value.truncate(MAX_DISPLAY_VALUE_LENGTH); - Self::new(display_value, is_truncated) - } } pub struct WorkspaceVariableDisplayType { @@ -348,10 +340,9 @@ impl WorkspaceVariableDisplayType { /// display type. pub fn from(value: SEXP, include_length: bool) -> Self { match Self::try_from_method(value, include_length) { - Ok(display_type) => return display_type, - Err(_) => { - // Continue on to the default approaches - }, + Err(e) => log::error!("Error from 'ark_variable_display_type' method: {e}"), + Ok(None) => {}, + Ok(Some(display_type)) => return display_type, } if r_is_null(value) { @@ -452,15 +443,14 @@ impl WorkspaceVariableDisplayType { } } - fn try_from_method(value: SEXP, include_length: bool) -> anyhow::Result { - let mut args: HashMap = HashMap::new(); - args.insert(String::from("include_length"), include_length.try_into()?); - let display_type: String = dispatch_variables_method_with_args( + fn try_from_method(value: SEXP, include_length: bool) -> anyhow::Result> { + let args = vec![(String::from("include_length"), include_length.try_into()?)]; + let result: Option = dispatch_variables_method_with_args( ArkVariablesGenerics::VariableDisplayType, value, args, )?; - Ok(Self::simple(display_type)) + Ok(result.map(Self::simple)) } fn new(display_type: String, type_info: String) -> Self { @@ -473,10 +463,11 @@ impl WorkspaceVariableDisplayType { fn has_children(value: SEXP) -> bool { // Try to use the display method if there's one available - if let Ok(has_children) = - dispatch_variables_method(ArkVariablesGenerics::VariableHasChildren, value) - { - return has_children; + + match try_from_method_has_children(value) { + Err(e) => log::error!("Error from 'ark_variable_has_children' method: {e}"), + Ok(None) => {}, + Ok(Some(answer)) => return answer, } if RObject::view(value).is_s4() { @@ -670,8 +661,10 @@ impl PositronVariable { return VariableKind::Empty; } - if let Ok(kind) = variable_kind_try_from_method(x) { - return kind; + match try_from_method_variable_kind(x) { + Err(e) => log::error!("Error from 'ark_variable_kind' method: {e}"), + Ok(None) => {}, + Ok(Some(kind)) => return kind, } let obj = RObject::view(x); @@ -1315,11 +1308,24 @@ impl PositronVariable { } } -fn variable_kind_try_from_method(value: SEXP) -> anyhow::Result { - let kind: String = dispatch_variables_method(ArkVariablesGenerics::VariableKind, value) - .context("No kind found")?; +fn try_from_method_variable_kind(value: SEXP) -> anyhow::Result> { + let kind: Option = + dispatch_variables_method(ArkVariablesGenerics::VariableKind, value)?; + match kind { + None => Ok(None), + Some(kind) => Ok(serde_json::from_str(kind.as_str())?), + } +} + +fn try_from_method_has_children(value: SEXP) -> anyhow::Result> { + dispatch_variables_method(ArkVariablesGenerics::VariableHasChildren, value) +} - Ok(serde_json::from_str(kind.as_str())?) +fn try_from_method_display_type(value: SEXP, width: usize) -> anyhow::Result> { + dispatch_variables_method_with_args(ArkVariablesGenerics::VariableDisplayType, value, vec![( + String::from("width"), + (width as i32).try_into()?, + )]) } pub fn is_binding_fancy(binding: &Binding) -> bool { From 01eda82d929f4cfcc6edda03b61037e2b8d92a07 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Tue, 17 Sep 2024 13:00:18 -0400 Subject: [PATCH 09/35] typo --- crates/ark/src/variables/variable.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 36ab58bc1..46fee4648 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -76,7 +76,7 @@ impl WorkspaceVariableDisplayValue { pub fn from(value: SEXP) -> Self { // Try to use the display method if there's one available - match try_from_method_display_type(value, MAX_DISPLAY_VALUE_LENGTH) { + match try_from_method_display_value(value, MAX_DISPLAY_VALUE_LENGTH) { Err(e) => log::error!("Error from 'ark_variable_display_value' method: {e}"), Ok(None) => {}, Ok(Some(display_value)) => return Self::new(display_value, false), @@ -1321,8 +1321,8 @@ fn try_from_method_has_children(value: SEXP) -> anyhow::Result> { dispatch_variables_method(ArkVariablesGenerics::VariableHasChildren, value) } -fn try_from_method_display_type(value: SEXP, width: usize) -> anyhow::Result> { - dispatch_variables_method_with_args(ArkVariablesGenerics::VariableDisplayType, value, vec![( +fn try_from_method_display_value(value: SEXP, width: usize) -> anyhow::Result> { + dispatch_variables_method_with_args(ArkVariablesGenerics::VariableDisplayValue, value, vec![( String::from("width"), (width as i32).try_into()?, )]) From e0b073d624bc536edfd8be95395c1ec955092375 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Tue, 17 Sep 2024 15:19:50 -0300 Subject: [PATCH 10/35] Make a method instead --- crates/ark/src/modules/positron/methods.R | 7 +- crates/ark/src/variables/methods.rs | 118 +++++++++++----------- crates/ark/src/variables/variable.rs | 55 +++++----- 3 files changed, 89 insertions(+), 91 deletions(-) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index c3f45dbce..f76ee0cd8 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -30,17 +30,18 @@ lockEnvironment(ark_methods_table, TRUE) ), typeof(class) == "character" ) - for (cls in class) + for (cls in class) { assign(cls, method, envir = ark_methods_table[[generic]]) - + } invisible() } call_ark_method <- function(generic, object, ...) { methods_table <- ark_methods_table[[generic]] - if (is.null(methods_table)) + if (is.null(methods_table)) { return(NULL) + } for (cl in class(object)) { if (is.function(method <- get0(cl, envir = methods_table))) { diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index 677b8d758..ecd29733d 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -26,7 +26,7 @@ use strum_macros::IntoStaticStr; use crate::modules::ARK_ENVS; #[derive(Debug, PartialEq, EnumString, EnumIter, IntoStaticStr, Display, Eq, Hash, Clone)] -pub enum ArkVariablesGenerics { +pub enum ArkGenerics { #[strum(serialize = "ark_variable_display_value")] VariableDisplayValue, @@ -40,8 +40,62 @@ pub enum ArkVariablesGenerics { VariableKind, } -impl ArkVariablesGenerics { - fn register_method_from_package( +impl ArkGenerics { + // Dispatches the method on `x` + // Returns + // - `None` if no method was found, + // - `Err` if method was found and errored + // - T, if method was found and was succesfully executed + pub fn try_dispatch( + &self, + x: SEXP, + args: Vec<(String, RObject)>, + ) -> anyhow::Result> + where + // Making this a generic allows us to handle the conversion to the expected output + // type within the dispatch, which is much more ergonomic. + T: TryFrom, + >::Error: std::fmt::Debug, + { + if !r_is_object(x) { + return Ok(None); + } + + let generic: &str = self.into(); + let mut call = RFunction::new("", "call_ark_method"); + + call.add(generic); + call.add(x); + + for (name, value) in args.into_iter() { + call.param(name.as_str(), value); + } + + let result = call.call_in(ARK_ENVS.positron_ns)?; + + // No method for that object + if result.sexp == r_null() { + return Ok(None); + } + + // Convert the result to the expected return type + match result.try_into() { + Ok(value) => Ok(Some(value)), + Err(err) => Err(anyhow!("Conversion failed: {err:?}")), + } + } + + pub fn register_method(generic: Self, class: &str, method: RObject) -> anyhow::Result<()> { + let generic_name: &str = generic.into(); + RFunction::new("", ".ps.register_ark_method") + .add(RObject::try_from(generic_name)?) + .add(RObject::try_from(class)?) + .add(method) + .call_in(ARK_ENVS.positron_ns)?; + Ok(()) + } + + pub fn register_method_from_package( generic: Self, class: &str, package: &str, @@ -57,19 +111,9 @@ impl ArkVariablesGenerics { Ok(()) } - fn register_method(generic: Self, class: &str, method: RObject) -> anyhow::Result<()> { - let generic_name: &str = generic.into(); - RFunction::new("", ".ps.register_ark_method") - .add(RObject::try_from(generic_name)?) - .add(RObject::try_from(class)?) - .add(method) - .call_in(ARK_ENVS.positron_ns)?; - Ok(()) - } - // Checks if a symbol name is a method and returns it's class fn parse_method(name: &String) -> Option<(Self, String)> { - for method in ArkVariablesGenerics::iter() { + for method in ArkGenerics::iter() { let method_str: &str = method.clone().into(); if name.starts_with::<&str>(method_str) { if let Some((_, class)) = name.split_once(".") { @@ -105,52 +149,10 @@ pub fn populate_variable_methods_table(package: &str) -> anyhow::Result<()> { .map(|b| -> String { b.name.into() }); for name in symbol_names { - if let Some((generic, class)) = ArkVariablesGenerics::parse_method(&name) { - ArkVariablesGenerics::register_method_from_package(generic, class.as_str(), package)?; + if let Some((generic, class)) = ArkGenerics::parse_method(&name) { + ArkGenerics::register_method_from_package(generic, class.as_str(), package)?; } } Ok(()) } - -pub fn dispatch_variables_method( - generic: ArkVariablesGenerics, - x: SEXP, -) -> anyhow::Result> -where - T: TryFrom, - >::Error: std::fmt::Debug, -{ - dispatch_variables_method_with_args(generic, x, Vec::new()) -} - -pub fn dispatch_variables_method_with_args( - generic: ArkVariablesGenerics, - x: SEXP, - args: Vec<(String, RObject)>, -) -> anyhow::Result> -where - T: TryFrom, - >::Error: std::fmt::Debug, -{ - if !r_is_object(x) { - return Ok(None); - } - let generic: &str = generic.into(); - let mut call = RFunction::new("", "call_ark_method"); - - call.add(generic); - call.add(x); - - for (name, value) in args.into_iter() { - call.param(name.as_str(), value); - } - - let result = call.call_in(ARK_ENVS.positron_ns)?; - if result.sexp == r_null() { - Ok(None) - } else { - let result = result.try_into().map_err(|e| anyhow!("{:?}", e))?; - Ok(Some(result)) - } -} diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 46fee4648..2291e622d 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -51,9 +51,7 @@ use libr::*; use stdext::local; use stdext::unwrap; -use crate::variables::methods::dispatch_variables_method; -use crate::variables::methods::dispatch_variables_method_with_args; -use crate::variables::methods::ArkVariablesGenerics; +use crate::variables::methods::ArkGenerics; // Constants. const MAX_DISPLAY_VALUE_ENTRIES: usize = 1_000; @@ -74,12 +72,9 @@ fn plural(text: &str, n: i32) -> String { impl WorkspaceVariableDisplayValue { pub fn from(value: SEXP) -> Self { - // Try to use the display method if there's one available - - match try_from_method_display_value(value, MAX_DISPLAY_VALUE_LENGTH) { - Err(e) => log::error!("Error from 'ark_variable_display_value' method: {e}"), - Ok(None) => {}, - Ok(Some(display_value)) => return Self::new(display_value, false), + // Try to use the display method if there's one available\ + if let Some(display_value) = Self::try_from_method(value) { + return display_value; } match r_typeof(value) { @@ -324,6 +319,24 @@ impl WorkspaceVariableDisplayValue { log::trace!("Error while formatting variable: {err:?}"); Self::new(String::from("??"), true) } + + fn try_from_method(value: SEXP) -> Option { + let display_value = + ArkGenerics::VariableDisplayValue.try_dispatch::(value, vec![( + String::from("width"), + RObject::from(MAX_DISPLAY_VALUE_LENGTH as i32), + )]); + + let display_value = unwrap!(display_value, Err(err) => { + log::error!("Failed to apply 'ark_variable_display_value': {err:?}"); + return None; + }); + + match display_value { + None => None, + Some(value) => Some(Self::new(value, false)), + } + } } pub struct WorkspaceVariableDisplayType { @@ -445,11 +458,7 @@ impl WorkspaceVariableDisplayType { fn try_from_method(value: SEXP, include_length: bool) -> anyhow::Result> { let args = vec![(String::from("include_length"), include_length.try_into()?)]; - let result: Option = dispatch_variables_method_with_args( - ArkVariablesGenerics::VariableDisplayType, - value, - args, - )?; + let result: Option = ArkGenerics::VariableDisplayType.try_dispatch(value, args)?; Ok(result.map(Self::simple)) } @@ -462,9 +471,7 @@ impl WorkspaceVariableDisplayType { } fn has_children(value: SEXP) -> bool { - // Try to use the display method if there's one available - - match try_from_method_has_children(value) { + match ArkGenerics::VariableHasChildren.try_dispatch(value, vec![]) { Err(e) => log::error!("Error from 'ark_variable_has_children' method: {e}"), Ok(None) => {}, Ok(Some(answer)) => return answer, @@ -1309,25 +1316,13 @@ impl PositronVariable { } fn try_from_method_variable_kind(value: SEXP) -> anyhow::Result> { - let kind: Option = - dispatch_variables_method(ArkVariablesGenerics::VariableKind, value)?; + let kind: Option = ArkGenerics::VariableKind.try_dispatch(value, vec![])?; match kind { None => Ok(None), Some(kind) => Ok(serde_json::from_str(kind.as_str())?), } } -fn try_from_method_has_children(value: SEXP) -> anyhow::Result> { - dispatch_variables_method(ArkVariablesGenerics::VariableHasChildren, value) -} - -fn try_from_method_display_value(value: SEXP, width: usize) -> anyhow::Result> { - dispatch_variables_method_with_args(ArkVariablesGenerics::VariableDisplayValue, value, vec![( - String::from("width"), - (width as i32).try_into()?, - )]) -} - pub fn is_binding_fancy(binding: &Binding) -> bool { match &binding.value { BindingValue::Active { .. } => true, From 023a9543cc50a7de212261b190eca8e6773de39c Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 18 Sep 2024 11:43:09 -0300 Subject: [PATCH 11/35] Add some unit tests --- crates/ark/src/modules/positron/methods.R | 22 ++++++- crates/ark/src/variables/methods.rs | 11 ++++ crates/ark/src/variables/variable.rs | 73 ++++++++++++++++++++++- 3 files changed, 102 insertions(+), 4 deletions(-) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index f76ee0cd8..be1a65d95 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -36,6 +36,22 @@ lockEnvironment(ark_methods_table, TRUE) invisible() } +has_ark_method <- function(generic, object) { + methods_table <- ark_methods_table[[generic]] + + if (is.null(methods_table)) { + return(FALSE) + } + + for (cl in class(object)) { + if (is.function(method <- get0(cl, envir = methods_table))) { + return(TRUE) + } + } + + FALSE +} + call_ark_method <- function(generic, object, ...) { methods_table <- ark_methods_table[[generic]] @@ -45,8 +61,10 @@ call_ark_method <- function(generic, object, ...) { for (cl in class(object)) { if (is.function(method <- get0(cl, envir = methods_table))) { - return(eval(as.call(list(method, object, ...)), - envir = globalenv())) + return(eval( + as.call(list(method, object, ...)), + envir = globalenv() + )) } } diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index ecd29733d..70730e91e 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -85,6 +85,17 @@ impl ArkGenerics { } } + // Checks if an object has a registered method for it. + pub fn has_method(&self, x: SEXP) -> anyhow::Result { + let generic: &str = self.into(); + let result = RFunction::new("", "has_ark_method") + .add(RObject::from(generic)) + .add(x) + .call_in(ARK_ENVS.positron_ns)? + .try_into()?; + Ok(result) + } + pub fn register_method(generic: Self, class: &str, method: RObject) -> anyhow::Result<()> { let generic_name: &str = generic.into(); RFunction::new("", ".ps.register_ark_method") diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 2291e622d..ee9e098d3 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -320,6 +320,19 @@ impl WorkspaceVariableDisplayValue { Self::new(String::from("??"), true) } + fn from_untruncated_string(value: String) -> Self { + let mut is_truncated = false; + let mut display_value = value.clone(); + + // If an index is found, truncate the string to that index + if let Some((index, _)) = value.char_indices().nth(MAX_DISPLAY_VALUE_LENGTH) { + display_value.truncate(index); + is_truncated = true; + } + + Self::new(display_value, is_truncated) + } + fn try_from_method(value: SEXP) -> Option { let display_value = ArkGenerics::VariableDisplayValue.try_dispatch::(value, vec![( @@ -334,7 +347,7 @@ impl WorkspaceVariableDisplayValue { match display_value { None => None, - Some(value) => Some(Self::new(value, false)), + Some(value) => Some(Self::from_untruncated_string(value)), } } } @@ -1319,7 +1332,9 @@ fn try_from_method_variable_kind(value: SEXP) -> anyhow::Result = ArkGenerics::VariableKind.try_dispatch(value, vec![])?; match kind { None => Ok(None), - Some(kind) => Ok(serde_json::from_str(kind.as_str())?), + // Enum reflection is not beautiful, we want to parse a VariableKind from it's + // string representation by reading from a json which is just `"{kind}"`. + Some(kind) => Ok(serde_json::from_str(format!(r#""{kind}""#).as_str())?), } } @@ -1338,3 +1353,57 @@ pub fn plain_binding_force_with_rollback(binding: &Binding) -> anyhow::Result Err(anyhow!("Unexpected binding type")), } } + +#[cfg(test)] +mod tests { + use harp; + + use super::*; + use crate::test::r_test; + + #[test] + fn test_variable_with_methods() { + r_test(|| { + // Register the display value method + harp::parse_eval_global( + r#" + .ps.register_ark_method("ark_variable_display_value", "foo", function(x, width) { + # We return a large string and make sure it gets truncated. + paste0(rep("a", length.out = 2*width), collapse="") + }) + + .ps.register_ark_method("ark_variable_display_type", "foo", function(x, include_length) { + paste0("foo (", length(x), ")") + }) + + .ps.register_ark_method("ark_variable_has_children", "foo", function(x) { + FALSE + }) + + .ps.register_ark_method("ark_variable_kind", "foo", function(x) { + "other" + }) + + "#, + ) + .unwrap(); + + // Create an object with that class in an env. + let obj = harp::parse_eval_base(r#"structure(list(1,2,3), class = "foo")"#).unwrap(); + + let variable = + PositronVariable::from(String::from("foo"), String::from("foo"), obj.sexp); + + assert_eq!( + variable.var.display_value, + String::from("a".repeat(MAX_DISPLAY_VALUE_LENGTH)) + ); + + assert_eq!(variable.var.display_type, String::from("foo (3)")); + + assert_eq!(variable.var.has_children, false); + + assert_eq!(variable.var.kind, VariableKind::Other); + }) + } +} From 1b8f53892e305aeacbcc87df4345c82389229407 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Tue, 24 Sep 2024 10:36:33 -0300 Subject: [PATCH 12/35] Add some documentation --- doc/variables-pane-extending.md | 103 ++++++++++++++++++++++++++++++++ doc/variables-pane.png | Bin 0 -> 8246 bytes 2 files changed, 103 insertions(+) create mode 100644 doc/variables-pane-extending.md create mode 100644 doc/variables-pane.png diff --git a/doc/variables-pane-extending.md b/doc/variables-pane-extending.md new file mode 100644 index 000000000..c5976f0bf --- /dev/null +++ b/doc/variables-pane-extending.md @@ -0,0 +1,103 @@ +# Extending the variables pane + +Ark enables package authors to customize the variables pane for specific +R objects by implementing custom methods, similar to S3 methods. + +![Variables pane anotated](variables-pane.png) + + +### `ark_variable_display_value` + +Used to customize the display value of an object. It's called by the variables +pane to produce the text that's marked as "1. Display value" in the above screenshot. + +Example: + +```r +#' @param x Object to get the display value for +#' @param width Maximum expected width. This is used as a reference, the UI can truncate +#' the string in different widths. +ark_variable_display_value.foo <- function(x, ..., width) { + "Hello world" # Should return a length 1 character vector. +} +``` + +### `ark_variable_display_type` + +Called by the variables pane to obtain the display type of an object. The display type +is shown in the screenshot above as "2. Display type". + +Example: + +```r +#' @param x Object to get the display type for +#' @param include_length Boolean indicating if the display type should also include the +#' object length. +ark_variable_display_type.foo <– function(x, ..., include_length) { + sprintf("foo(%d)", length(x)) +} +``` + +### `ark_variable_kind` + +Returns the kind of the variable, which can be used by the UI to show the variable in +different orders in the variable pane. Currently only `"table"` is used. You can find +the full list of possible values [here](https://github.com/posit-dev/ark/blob/50f335183c5a13eda561a48d2ce21441caa79937/crates/amalthea/src/comm/variables_comm.rs#L107-L160). + +Example: + +```r +#' @param x Object to get the variable kind for +ark_variable_kind <- fuinction(x, ...) { + "other" +} +``` + +## Inspecting objects + +Package authors can implement methods that allow users to inspect R objects. This is the behavior +that allows the variables pane to display the structure of objects, similar to the `str` function +in R. + +In order to allow inspecting objects, implement the `ark_variable_has_children`: + +```r +#' @param x Check if `x` has children +ark_variable_has_children <- function(x, ...) { + TRUE # TRUE if the object can be inspected, FALSE otherwise +} +``` + +Next, implement a pair of methods + +- `ark_variable_get_children`: Returns a named list of objects that should be displayed. +- `ark_variable_get_child_at`: Get an element from the object + +Example: + +```r +ark_variable_get_children <- function(x, ...) { + list( + a = c(1, 2, 3), + b = "Hello world", + c = list(1, 2, 3) + ) +} + +#' @param name Name to extract from `x`. +ark_variable_get_child_at <- function(x, ..., name) { + # It could be implemented as + # ark_variable_get_children(x)[[name]] + # but we expose an API that doesn't require users to re-build + # if there's a fast path to acess a single name. + if (name == "a") { + c(1, 2, 3) + } else if (name == "b") { + "Hello world" + } else if (name == "c") { + list(1, 2, 3) + } else { + stop("Unknown name: ", name) + } +} +``` diff --git a/doc/variables-pane.png b/doc/variables-pane.png new file mode 100644 index 0000000000000000000000000000000000000000..fceff4298b24f506f29312e9c14d18a89e5df3be GIT binary patch literal 8246 zcmZ{KcQo8j7xyAahzNpU^%g{rvWONnYV=-%5Ovq;HG0XaYjsKVP6!sOw^ftqov1^X28`Jv}|Y4-DMi-b(n?o+kvH%QIJ$ zdx`=8j3cTFa(X^E+taF9S)#!H11EA$!yu;o6RHtE4z691%=-cObTRKSPaz+Z2Yu;Z zo-9&5m7(ZUJGGw_%Lq)$9Sd8LSth&653m>=@&)m4VE!K=kPEyzyGVyCMYp87|0eci zD&}ne@RP#~RPd&!9|q_YM5@Q=Z3^g~=u8fWfBfnh!K)+~_CjY+rK6N8zocV*&c1AP z^=wk>b&OJ_KrFXL!~MaDFVNQ+Ufi-nuAL=6?&W__;#7BJARY(`EK4mc+g;M*YBvx8 zkE%Dm3i$hh)_q-@kNX45OBn|>lV&zsj-Wu@?|)=n$$3YS_Zp~al<BQ@czb8N%4+rX@qg1zC z$>az2&Gf(4HlCMzJfnVo@C@^bfK9nWJyZ5)B+vpv{0Y%b*;zsbiB_!)rm|9zb(ZXAg)7v@XT3GbBl?8@ZLxW?mt1chUl@cNaAmZ!O$K@a z$sP19*ie~Hqlt1?!~FSd#$H#F?3D-s=7SCTL=n}$#J)MU?6DSCAt!d%!4*TUaz&Ol z*$Vs-5=%t0)mqbs=j5{d-nzf~2Cn)%C42h!{0-OAfkSDf*=t3Ds|Lb#_zij$T4s_j-xdbC+@q1kqaA2B*@UO*2WW^#pTOBkm?^giMI81Gz`%n!5)V`oSi6qpt&= zEwI#gJAQLrtk%NUW@S1p)+c~(dZ|cfjSt3_Ao6({29Pa+2k#AZJO83D7rN8?m#scO>?!canMy};u zJ*X9fD}4M~cUh`n1=)AjAADQ3goxwrxz_stEVX+bgW~Kdv{EbbOlFxuCrSM&PX6@f zQuhH(5jmZ~^@X>tYJ^yxkO) zjW_~x9)e}Ayvi(BkEZ|Ay<@|9>p#LS&&a>T7vISJN?JGgz_yuB)arPii7||B&G*w<%vx+Gzvtrp(&MJkSf62y>e|yGU_@%lb_A2RX6_&lw}h~Nsgqq};-_{MZ!RG>>|Kkc z+O%={ib#ogFPwrtuTR!G<}b!vN%cSRVA1QQ_~J}%B3Yh@pV z+n$rtnx9$f_edwN$sAE!s^t)WV7fhyk$%HiHd$ zfR;WzSRr|Wxa9W$@%3YxfQX~>QZHGg?az@_HphdTDCN0N<0gQ&@s4e%sy~m6ex6R7 zMI>T|Gvmz3MB*JmIO(m)YW-i6;aR1rip|72viiaEKyYF3v|sMwB9*;Rh!^Q64#5gseOhQs(8=L zV9RwAVyovKvNOm&%Pz!-kxZeYKSrO7=n#hvR)(*wr zbj*Laz8p2LrgEDEv-H*4SrH2P?^}qxrU=~Y7Jc22sE?*@HJ5xjl z+k(jsLP7>7A8;**ARYWYO&Isi_lm-=^CDS_%W1#w!)9&U7gEAnJWh-BKY`n*wWLoT zMZKsL)HmVi3E%rXmu1kiWDbv;nyR-(3s&WEg7=C8#>F-;R(KVXE!cZj@4!_LSB$(A zBi@0STgSm}Yd>wR?}KOGF8u5lCX;OoVI^}WtRF+1zRUgk@z{O>gX`yNP94QQtsKNT z_D)mNgJ9~`4%5{&FjN@RVaZ9IC)$qtNo>cGVlQY*+JxSdoPTV$)a=HvIw!kNHSV^G z&D6(}=u;3`@Js&_k>%0L4PR#DEjwf55kuy!vzS)OWuE0#k~`Ab<5#Fs1jqH*xqKU6 z`~KYTbuGx`@bAx?!yd8$kU|VY>@t7j9IAHGSku$Pkb|+T7z4;cgcrdqH!en zTBe1~5sEnk#|59jxm3tkqaWnvPi|@>#W&47lg9(n*;wHI2T5p=6f$`CSn%>jB^0zs z(Y-`zj#FI~9Qv(CW^Ln|f4ym8D5hO>X?TYCij8C{3$mGpEvS8`jV!Jtc1&zqFguJH zBAGZ+pnb-NaDyN_BflNrDa0$nP_Q9-Xu*~Tk~7@-XVyzZ;z!@Fo+8<5v*k!F(k39D zt^Xpu4UO7Sfu|LXWZYY=KL*d+uFiJyX%VK+RexTIrU+vzP>O5bj@G7w#`EOwu@!K( z<+L?0L|;`0(F{&D9jwey*96TZ8clp0TIArYOvQQ^T=xLpOCaxya;87AWK&a+!MuFYvrZVJ#Zk+#B>XO=gq)z}y*v92KaD9pAtY&&_$8 z*PRhBzi`H}fi|3%Te&mD*cP)*ES{TpL>=p*F11Qg-o=_ zdjpD`nZ>-jzJ2)P_1FX9Q*6P_8h8vLu9pXv`ZtcPkzCQp>{%pr;BA#(0UC*jP3Z~N-?qG}22kJB5t=Hyz{W4salU}95 z)fnUBvuCco&H2ndLp7&ju;gxY`!?lNzm*icE>ehJmUK#%$iAwqcslpuQBT;++&x;8 zSFc(Pi|7Eo&uSb-BB>?EV@uORT~Q>I=G*8Iw;qGZm;w7R@w&Hk+5@TPMgB=n>U1Bp zEKT6bulc~u;J#Er+fl^E-_pZweN5>=XHLlp>*+}{Q8{gr~B87<9qN#aKj zRXq&1YhV^CruQ~X$}sCS*EfJ${+3kA0#);f+4&&#d(k^{P;-M>|LX1GKPjFGZQ6NW zX@->N9gme9OmsmM98Mk)(sPP6QjT4L%NzA94PGIAqNcVt1Pgtp=E4ZIPo*3>NZaaN z{e$mY=onB2N>yZTTVt!9Pgb#8?)Mi0shZG%7+^#H?j}zGKIyGMYrAsQ1eGJv`);1&x z0o#t?%fpv4hsEIePiajdwcqBm0$GitC@qEr&EUDMKA3ro@O|bG~Q*GG;i5@<#04_@`!W-gm3o2eP(kc(Rz|`Mm|&G#i_7NBtE8Gn;-)6>xD2-J1sUe#(qg z$I%7^;dHoOz8rnotIy&WF+df)J-dyNC##$LHvJ^z?@6D9xQSs`7@`WvD7|=|CYmO5 zFimQAWwiQPAPvk)_it4|a-k}+x4fMb>cD0Q?4K?`*%3Zlc;j=f$)~#~H!Kd4)b`lH z>6u02=&2CbaM+;nyrcS#^Hi3Z4j9xO_I=uetM+y{9Qi9J&8ED*9d!=7E#We@TU)fc z1zm4OLpRR+{UEzdCUjDO zbFg!79ugR?=$91?)kiC=?-}usdk}Plim=*07keaH*c|PH59Zzk>fiVp!(?7|sd{>a zFPcJRZt5k^cb}I3D%xPOuViSncT%)d9%LtJqUEt9fS9@#)EJyFQNJYQ@pWnX$iq`) zQo`D+4XzmppK%u+%*Nm0MSJ&}9h-}MKpg681R8-+dGEipJg1$)EvTYXWZyK!9 z#-Cug1QBw{e^YGTQ?Xi^NcQPi*U}HttHRu6!sT=i_|rhhzAv`XK#8CMM}bQ1g*&aY zF^q9c3+6p^DDZdTjR4`cveqNRcdqy2w*nCjqK@jndVD2aK{(NpyK20tG=5=5JVIl} z5egeRs&6Rs8?>j`2#WAyRm8=q)cJ-EY>TCQJkEOGdLNca_?j!-xC9=YY>yC)sLDPr zt%nyp{Prs~+s<_xlY*wK#^B+P8vJPB+t)CPPO7(MMmS&ANTQ!`w*2+y1 z8uw;H*M$y5){7d;`23vQ;sGu3D5WS-l>p~tqw$sNWJ~qE1m=vz%%Pe$+lFG;Kl8k{ z*EHA}v&L6HS4i19;P}7vQSCG84UBbb)SYHuI(IYFzGAQ!?eO0%xoV~MeYiJ#8r^UW zw0mWI82e;a%WeCG5%sW(WF?*8WUHvG6!h@5Kctt^L#$rsN6FR*uhj;&=woap^2s5# zOKCS5M$=un`!34PT;;jN?9CGhs9w0-!db!sOEn=y1so1*JM>xKaj z4VuB;^E3`OF?hEPy4+aW=*dX+%M@MwVrakKCe+`IJI!Gd&KGzMtLmO8+!ENsu0hj7 z#?4+Nh?gUM>!549oemkJTr$3ZJTRxl zP4xDzZfK0bd%QWlJl_(At*;FrUh%wdB(fCm@S!quvOrR^B;%uHF>~2p4w&4Jy5*7kyux=au#G(Y?V~-m(+r zm~q9Lb$qEfA_i=bH5nt)C-cb?4x61xMfhRdasbU)mGve$%ombUZJe* zLEUC!g2NKe`=P=cI~s!?m|NwlMctb91CxUnN(b*#!2NrIDpN0-{Zt z!z^AO`RNa!F_#tZJ&oH^H8JIIuf?*zd10j$*0XDQIX82$D@RBLrs7pHq$J6*vU5%Pf?Fg%b%5JWO$XV3*ZU|2W4xX&YYAGYzLUdeJQAZE z$_u7?-iyGU z;_BkP_H1BNVpx=n)-L}LGMR#zn!Ud0ViTVwV*NV{$fRG(SV}B4|NZeT#!%tDdG*_^ zic<#SsP(JczkVplbB1J!yr`hgE@qGvWut%3FJ|P4aXp1JTU5J3Dk~^RG`YnP6>2u$ zS;!`+KJ&Ci_VyM26+j#}r0Rp+kU5@^zmY$lly4jrfWN=Tjvkdpg-xv`aPZV=IzJ%* zw|-D_v`Oy~eyGbZ7(p1qH^#aKInwT?j$vnO3jKcnxhV;+>Fflc{c{tN`>R2I4lrd6 zh4=8w+6gA}X-bN*5nuj_nnK#y=P5?y&}6 zg&4p>)EhH-7V6AJ+QI91e?;i<1eD;(R&^*0clSQXoDzI^0+1DQ!%<0lnn<37)@afv zNany3iwlY4d$rQ>!EhzwM7oQXKe|hkki4jL6^owh!^=X74i)7w3BUj%p5fW2RLVQk z`^@V_)#A$yw>+6WcbnE_ zPJicH;>F{tH4aW`4y}1YxXp?06XPwb>)dfVkKaKQ`x$R0i$FUPf4{#WHY>_-9BE`* zUex^I4SJL6!omg{Lf|$tR-F{r*62rr-x^Z%{au_4sqd%Di@Zy)+qXp8tC$N;kSQD- zHjf7w@pKT^QW0#ESzrrz&@r{X#Er3vfQ3xs;SEF^{^%Vi+g%=Anv0ryKo7)e!w=+l zJQLxf28K`B2ENubbYt@#dqKI6P$?h`{!&90U1ZFFhAM-(xDfnugOrirUo@etd8_cK ztY`;6)$yk1#c_z&&JIHA55+@z@RX&#&L^!s>L3J@KKy;3zM32J*VE{gK~^HM>)E3p zdJQro*~QVeS`2u+Tn?1zMAWZ2q5zM&4Ebq0lSaTLZ8Wbs6W z>wsvWeTdXy^rHN1l`qjJ*Fmei*%voDS*tSHQ>!(vb*sSX6@4}`J;q|jHZ<*( z71Kr^BkSo-&k}c#hcHq@cYtza`fdl=pTl?qS?YraX5d5ds8&&SLPJ3DA-kf_jLsts z-Tl;_gg*}-uiq!$5qi8Eh2Ozrv%gff09znE;qz=LWp|lhX()`01ROfg@7_9T4JI!2d1Ntk(@YFquGyg!!Oz5EUTHDt4ayX{ikWO;20gJsj@ zEcio0%^Zw;F#mFW;rrQGKw$k4w13SOFlpV_8oxJM6*1Q<;1E3ZF_^nKWI z`FjFZSSnJ`247c!Ndyr=uRLXfBu0~f=zm+KtLtXa)qRl;Nt=*X&RZgO?I}P#AlLS0 z?rlG*IV!`f#TAfMNFLH^Zo~`ny0?P=*x=i`UVp;MianTS9+0^ij$8fEUj)+>23&Y= z^6E#1a&pVONC=h0L+LQsr0P4yTGmG9b$LSXM7?UqGDQalMOVGYMa+9c?Lr=;P^>g0 zBOBya8Tx)*!GXaXMqF28(fx-liVs5e6pRhHo`uo}Yxj|?lmEiW{Wz&=>z++a4}$D3 z{1u_UZ3x5zS=ZP_fB47xK7 zF_cw=^J0Y7fHO45R=FA9H~X@)>Ly9!IIu#ebv5`<{8VhAZQ&1Ry-@)yZ~ZD=iT>6l z{0_JElC@%3o;f73pH7 zI!;9LFh%$(jU1b53MU>SlEy53+M=d^%0Kxr2KrmBQsDWV7254@Jb2womGyQHeDGcS zD>4SQOc?U_<2ukR)&+0pgXO(uCGCtvV#__P0pz?%Q!DuF>{P#4@}^4gxf#-NR~b%0 zd@pKt634XzS6;&+7 zkg}j%Kmj#pZLXP=00|}3ahe&u1H+u`^b3uUy2VdEr*@5VZv&CTbyB9p6NPD#b7aJq z5x~UmbRYw5r*=iD;*u$&en^gyIt|)|xXw8SN0y8Rf5+%KLDHw6d~CZ?o)_Te?IwhY zrlFEQPuE;{w!z&FldY5cmRBoS;U3GRd#M429s_tCS{ka z4_HGc4(Ouqc+Oh^K$?vvf)Cjl3s7IPkuLahw~dS#a&9I}S`fCQ@-wj7VxD%6&Z#GZ z*lCu?{jX{jLmb}XT-dcX-ms6kqK`4(H-yM@59lq2=}E?rOY7seB`oK=wSM1#Vk7cB z8Skg#Z2jNA%VZCk)=1M1`C{CAwlug^SkGR)+rBM~_?Gv|L0+@b1>=?FBq%Ih6^l@! z=%8#r)*G;V-k2R7tHv1aYRvas&n4b46w_aj-1K zrVM6Nrk8!EMo>Gvx1WIp;td%ZJpVDslTp=S+0ZUNgI0vWbQ<)&io$b zUR&4IcZr~k+J@Ub{9)3{H3V|v#NQW^3#?pY9IM$OCB6x`OXo6_%%@guce-{K3sh6d zoq#ydyUX%hXO8qTm1~wU|2Ql3YT`im&WqRXJa-}QUz5OS0*1olFkgBZv__%tfQH@9 z3`*4huhVM#gJ^$31?`<#KXL+-!aywypxI?8YVnNB;9I7z#j2qropA9a6~6 zL|`TfKmyInce+j=BBBKLtok2mwEMOGLmC4uSmPs{4w)A1JY42W{4;87)cU^e9xh-i z_~lIv-}-1M;{k{n^bkyRdwsz=H8qNw^1H14dQ9ME;%LU0eCe%*zmkJz)ZQe1zH?O^ zx1_qasHnA-6WDRgj65DEw%%_Cz{dLnk=G=~pW0@2B=8Jg&+2dj72O&% z@3Q1O)2*r_VLza4uaPNBzGrd)n~plW1=ENl|CdlE zZ|X%5__TgyE46!BaKLi>DfSX4R`d@uxpM=s#?eY*BJf;sIHhhp{SnlXl-_wHt)(&H z4qo2|2{-H^ZP6>?M&oG5I?RVdeSQUD&fO1!rhImpl5)ikA>0? zC#Re)<`Eq~PIx?=^j}fy(7EBCDJ5&(YI8uObBwjb3&@Noc7WL@qV)UlqT-oT(LWR3 zVTH!(AoiLI-i*4RGl)lg`#~Pdf$C8(NL%bIc>iaU(8NJ^4zn1~#6dNR9VB5W|Nk-5 zvf2#F3}SFFw`;QQ05F}bzlZbivrPxS*{oTOG{*x}a2NmP`u#f~nV z{1Pt4dkmHj#P2>HOA=m#LamWKM{E@R$VOk1as(^yhKMM3EZexZ3v7*gX3r-;XVfWG zbGouS3J+sk9Y<^=G?=5Ezi${^^l06YUgD1Q`LMi9r$L>8yWIQcE|c7~XaBphe?tN* zi0I$M9PyvQ|7*Olu8tz1pDO56)QlKilKi=hSOnL;P(tdvyN3V(RYgsODtW69{|5 Date: Wed, 25 Sep 2024 12:28:32 -0400 Subject: [PATCH 13/35] Revise 'Extending Variables Pane` doc --- doc/variables-pane-extending.md | 110 +++++++++++++++++++++----------- 1 file changed, 71 insertions(+), 39 deletions(-) diff --git a/doc/variables-pane-extending.md b/doc/variables-pane-extending.md index c5976f0bf..78a112043 100644 --- a/doc/variables-pane-extending.md +++ b/doc/variables-pane-extending.md @@ -1,82 +1,108 @@ -# Extending the variables pane +# Extending the Variables Pane in Ark -Ark enables package authors to customize the variables pane for specific -R objects by implementing custom methods, similar to S3 methods. +Ark allows package authors to customize how the variables pane displays specific R objects by defining custom methods, similar to S3 methods. -![Variables pane anotated](variables-pane.png) +![Variables pane annotated](variables-pane.png) +## Defining Ark Methods for S3 Classes -### `ark_variable_display_value` +To implement an Ark Variables Pane method for an S3 class (`"foo"`) in an R package, define pseudo-S3 methods like this: -Used to customize the display value of an object. It's called by the variables -pane to produce the text that's marked as "1. Display value" in the above screenshot. +```r +ark_variable_display_value.foo <- function(x, ..., width = NULL) { + toString(x, width) +} +``` + +These methods don't need to be exported in the `NAMESPACE` file. Ark automatically finds and registers them when the package is loaded. + +You can also register a method outside an R package using `.ps.register_ark_method()`, similar to `base::.S3method()`: + +```r +.ps.register_ark_method("ark_variable_display_value", "foo", + function(x, width) { toString(x, width) }) +``` + +## Available Methods + +Ark currently supports six methods with the following signatures: + +- `ark_variable_display_value(x, ..., width = getOption("width"))` +- `ark_variable_display_type(x, ..., include_length = TRUE)` +- `ark_variable_kind(x, ...)` +- `ark_variable_has_children(x, ...)` +- `ark_variable_get_children(x, ...)` +- `ark_variable_get_child_at(x, ..., index, name)` + +### Customizing Display Value + +The `ark_variable_display_value` method customizes how the display value of an object is shown. This is the text marked as "1. Display value" in the image above. Example: ```r #' @param x Object to get the display value for -#' @param width Maximum expected width. This is used as a reference, the UI can truncate -#' the string in different widths. -ark_variable_display_value.foo <- function(x, ..., width) { - "Hello world" # Should return a length 1 character vector. +#' @param width Maximum expected width. This is just a suggestion, the UI +#' can stil truncate the string to different widths. +ark_variable_display_value.foo <- function(x, ..., width = getOption("width")) { + "Hello world" # Should return a length 1 character vector. } ``` -### `ark_variable_display_type` +### Customizing Display Type -Called by the variables pane to obtain the display type of an object. The display type -is shown in the screenshot above as "2. Display type". +The `ark_variable_display_type` method customizes how the type of an object is shown. This is marked as "2. Display type" in the image. Example: ```r #' @param x Object to get the display type for -#' @param include_length Boolean indicating if the display type should also include the -#' object length. -ark_variable_display_type.foo <– function(x, ..., include_length) { +#' @param include_length Boolean indicating whether to include object length. +ark_variable_display_type.foo <- function(x, ..., include_length = TRUE) { sprintf("foo(%d)", length(x)) } ``` -### `ark_variable_kind` +### Specifying Variable Kind -Returns the kind of the variable, which can be used by the UI to show the variable in -different orders in the variable pane. Currently only `"table"` is used. You can find -the full list of possible values [here](https://github.com/posit-dev/ark/blob/50f335183c5a13eda561a48d2ce21441caa79937/crates/amalthea/src/comm/variables_comm.rs#L107-L160). +The `ark_variable_kind` method defines the kind of the variable. This allows the UI to organize variables in the variables pane differently. Currently, only `"table"` is used, but other possible values are [listed here](https://github.com/posit-dev/ark/blob/50f335183c5a13eda561a48d2ce21441caa79937/crates/amalthea/src/comm/variables_comm.rs#L107-L160). Example: ```r #' @param x Object to get the variable kind for -ark_variable_kind <- fuinction(x, ...) { +ark_variable_kind.foo <- function(x, ...) { "other" } ``` -## Inspecting objects +## Inspecting Objects + +Package authors can also implement methods that allow users to inspect R objects, similar to how the `str()` function works in R. This enables displaying object structures in the variables pane. -Package authors can implement methods that allow users to inspect R objects. This is the behavior -that allows the variables pane to display the structure of objects, similar to the `str` function -in R. +### Checking for Children -In order to allow inspecting objects, implement the `ark_variable_has_children`: +To check if an object has children that can be inspected, implement the `ark_variable_has_children` method: ```r #' @param x Check if `x` has children -ark_variable_has_children <- function(x, ...) { - TRUE # TRUE if the object can be inspected, FALSE otherwise +ark_variable_has_children.foo <- function(x, ...) { + TRUE # Return TRUE if the object can be inspected, FALSE otherwise. } ``` -Next, implement a pair of methods +### Getting Children and Child Elements -- `ark_variable_get_children`: Returns a named list of objects that should be displayed. -- `ark_variable_get_child_at`: Get an element from the object +To allow inspection, implement these methods: + +- `ark_variable_get_children`: Returns a named list of child objects to be displayed. +- `ark_variable_get_child_at`: Retrieves a specific element from the object. Example: ```r -ark_variable_get_children <- function(x, ...) { +ark_variable_get_children.foo <- function(x, ...) { + # return an R list of children. For example: list( a = c(1, 2, 3), b = "Hello world", @@ -84,12 +110,18 @@ ark_variable_get_children <- function(x, ...) { ) } -#' @param name Name to extract from `x`. -ark_variable_get_child_at <- function(x, ..., name) { - # It could be implemented as - # ark_variable_get_children(x)[[name]] - # but we expose an API that doesn't require users to re-build - # if there's a fast path to acess a single name. +#' @param index An integer > 1, representing the index position of the child in the +#' list returned by `ark_variable_get_children()`. +#' @param name The name of the child, corresponding to `names(ark_variable_get_children(x))[index]`. +#' This may be a string or `NULL`. If using the name, it is the method author's responsibility to ensure +#' the name is a valid, unique accessor. Additionally, if the original name from `ark_variable_get_children()` +#' was too long, `ark` may discard the name and supply `name = NULL` instead. +ark_variable_get_child_at.foo <- function(x, ..., name, index) { + # This could be implemented as: + # ark_variable_get_children(x)[[index]] + # However, we expose an API that allows access by either name or index + # without needing to rebuild the full list of children. + if (name == "a") { c(1, 2, 3) } else if (name == "b") { From b27d308b0f09b95e52ad1a5a9a323aff595f39ff Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Wed, 25 Sep 2024 14:02:42 -0400 Subject: [PATCH 14/35] Additional note --- doc/variables-pane-extending.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/variables-pane-extending.md b/doc/variables-pane-extending.md index 78a112043..5afce42ec 100644 --- a/doc/variables-pane-extending.md +++ b/doc/variables-pane-extending.md @@ -102,7 +102,8 @@ Example: ```r ark_variable_get_children.foo <- function(x, ...) { - # return an R list of children. For example: + # Return an R list of children. The order of children should be + # stable between repeated calls on the same object. For example: list( a = c(1, 2, 3), b = "Hello world", From 894c9db16735a2c8c79476d19fb2cd15de4462a7 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 2 Oct 2024 11:05:46 -0300 Subject: [PATCH 15/35] fix tests --- crates/ark/src/variables/variable.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index ee9e098d3..2a43b2f14 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -1359,7 +1359,7 @@ mod tests { use harp; use super::*; - use crate::test::r_test; + use crate::fixtures::r_test; #[test] fn test_variable_with_methods() { From b24eaacc627d5457a5a49091f54028843e22b58e Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Wed, 2 Oct 2024 10:20:39 -0400 Subject: [PATCH 16/35] Find namespaced method calls like `foo:::bar` --- crates/ark/src/modules/positron/methods.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index be1a65d95..5e11c2505 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -44,7 +44,7 @@ has_ark_method <- function(generic, object) { } for (cl in class(object)) { - if (is.function(method <- get0(cl, envir = methods_table))) { + if (!is.null(get0(cl, envir = methods_table))) { return(TRUE) } } @@ -60,7 +60,7 @@ call_ark_method <- function(generic, object, ...) { } for (cl in class(object)) { - if (is.function(method <- get0(cl, envir = methods_table))) { + if (!is.null(method <- get0(cl, envir = methods_table))) { return(eval( as.call(list(method, object, ...)), envir = globalenv() From 9bc5fc320ea81bb00b0c6ab4d1e203f48dbb7d06 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 2 Oct 2024 11:35:15 -0300 Subject: [PATCH 17/35] Remove has_ark_method --- crates/ark/src/modules/positron/methods.R | 16 ---------------- crates/ark/src/variables/methods.rs | 11 ----------- 2 files changed, 27 deletions(-) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index 5e11c2505..e0acf214c 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -36,22 +36,6 @@ lockEnvironment(ark_methods_table, TRUE) invisible() } -has_ark_method <- function(generic, object) { - methods_table <- ark_methods_table[[generic]] - - if (is.null(methods_table)) { - return(FALSE) - } - - for (cl in class(object)) { - if (!is.null(get0(cl, envir = methods_table))) { - return(TRUE) - } - } - - FALSE -} - call_ark_method <- function(generic, object, ...) { methods_table <- ark_methods_table[[generic]] diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index 70730e91e..ecd29733d 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -85,17 +85,6 @@ impl ArkGenerics { } } - // Checks if an object has a registered method for it. - pub fn has_method(&self, x: SEXP) -> anyhow::Result { - let generic: &str = self.into(); - let result = RFunction::new("", "has_ark_method") - .add(RObject::from(generic)) - .add(x) - .call_in(ARK_ENVS.positron_ns)? - .try_into()?; - Ok(result) - } - pub fn register_method(generic: Self, class: &str, method: RObject) -> anyhow::Result<()> { let generic_name: &str = generic.into(); RFunction::new("", ".ps.register_ark_method") From 588b7332cb8ccc52223b17bddc6011f76fcb7774 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 2 Oct 2024 11:39:32 -0300 Subject: [PATCH 18/35] use `r_task` --- crates/ark/src/variables/variable.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 2a43b2f14..3abbdbfb9 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -1359,11 +1359,11 @@ mod tests { use harp; use super::*; - use crate::fixtures::r_test; + use crate::r_task; #[test] fn test_variable_with_methods() { - r_test(|| { + r_task(|| { // Register the display value method harp::parse_eval_global( r#" From c3ee3c6aa6b66abdbe0ec89cac481d93a747dae4 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 08:15:31 -0300 Subject: [PATCH 19/35] Use `if let Err` construct --- crates/ark/src/interface.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 5e363ea98..1debfebf0 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -423,8 +423,9 @@ impl RMain { } } - populate_methods_from_loaded_namespaces() - .or_log_error("Can't populate variables pane methods from loaded packages"); + if let Err(err) = populate_methods_from_loaded_namespaces() { + log::error!("Can't populate variables pane methods from loaded packages: {err:?}"); + } // Set up the global error handler (after support function initialization) errors::initialize(); @@ -1968,8 +1969,9 @@ unsafe extern "C" fn ps_onload_hook(pkg: SEXP, _path: SEXP) -> anyhow::Result Date: Wed, 9 Oct 2024 08:24:35 -0300 Subject: [PATCH 20/35] Use `is_string` helper --- crates/ark/src/modules/positron/methods.R | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index e0acf214c..7c1613b14 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -20,8 +20,7 @@ lockEnvironment(ark_methods_table, TRUE) #' @export .ps.register_ark_method <- function(generic, class, method) { stopifnot( - typeof(generic) == "character", - length(generic) == 1, + is_string(generic), generic %in% c( "ark_variable_display_value", "ark_variable_display_type", From 7997c48d6a8ab12bad083871831cffeadb4a143d Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 08:26:09 -0300 Subject: [PATCH 21/35] USe `cls` for consistency --- crates/ark/src/modules/positron/methods.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index 7c1613b14..90bbeed9c 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -42,8 +42,8 @@ call_ark_method <- function(generic, object, ...) { return(NULL) } - for (cl in class(object)) { - if (!is.null(method <- get0(cl, envir = methods_table))) { + for (cls in class(object)) { + if (!is.null(method <- get0(cls, envir = methods_table))) { return(eval( as.call(list(method, object, ...)), envir = globalenv() From bd26e53288f22854fde04f67dce8bd0a5b71ff00 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 08:33:51 -0300 Subject: [PATCH 22/35] Improve readability Co-authored-by: Lionel Henry --- crates/ark/src/variables/methods.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index ecd29733d..59d2082ac 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -45,6 +45,7 @@ impl ArkGenerics { // Returns // - `None` if no method was found, // - `Err` if method was found and errored + // - `Err`if the method result could not be coerced to `T` // - T, if method was found and was succesfully executed pub fn try_dispatch( &self, @@ -55,7 +56,7 @@ impl ArkGenerics { // Making this a generic allows us to handle the conversion to the expected output // type within the dispatch, which is much more ergonomic. T: TryFrom, - >::Error: std::fmt::Debug, + >::Error: std::fmt::Debug, { if !r_is_object(x) { return Ok(None); From d68648df7fea83a3ee22a576ebe48dc233aef3b4 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 08:34:25 -0300 Subject: [PATCH 23/35] Simplify function Co-authored-by: Lionel Henry --- crates/ark/src/variables/variable.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 3abbdbfb9..896f8f686 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -320,17 +320,14 @@ impl WorkspaceVariableDisplayValue { Self::new(String::from("??"), true) } - fn from_untruncated_string(value: String) -> Self { - let mut is_truncated = false; - let mut display_value = value.clone(); + fn from_untruncated_string(mut value: String) -> Self { + let Some((index, _)) = value.char_indices().nth(MAX_DISPLAY_VALUE_LENGTH) else { + return Self::new(value, false); + }; // If an index is found, truncate the string to that index - if let Some((index, _)) = value.char_indices().nth(MAX_DISPLAY_VALUE_LENGTH) { - display_value.truncate(index); - is_truncated = true; - } - - Self::new(display_value, is_truncated) + value.truncate(index); + Self::new(value, true) } fn try_from_method(value: SEXP) -> Option { From a69c736a74c2f98c70437002690c2e4db7ee848a Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 08:37:32 -0300 Subject: [PATCH 24/35] Simplify note --- crates/ark/src/variables/variable.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 896f8f686..c4bdd249a 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -1329,8 +1329,8 @@ fn try_from_method_variable_kind(value: SEXP) -> anyhow::Result = ArkGenerics::VariableKind.try_dispatch(value, vec![])?; match kind { None => Ok(None), - // Enum reflection is not beautiful, we want to parse a VariableKind from it's - // string representation by reading from a json which is just `"{kind}"`. + // We want to parse a VariableKind from it's string representation. + // We do that by reading from a json which is just `"{kind}"`. Some(kind) => Ok(serde_json::from_str(format!(r#""{kind}""#).as_str())?), } } From 87cd6da7125c371dae1e712992a1e303ca2943e2 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 08:49:37 -0300 Subject: [PATCH 25/35] Use `&self` instead --- crates/ark/src/variables/methods.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index 59d2082ac..2bfe90e7e 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -86,8 +86,8 @@ impl ArkGenerics { } } - pub fn register_method(generic: Self, class: &str, method: RObject) -> anyhow::Result<()> { - let generic_name: &str = generic.into(); + pub fn register_method(&self, class: &str, method: RObject) -> anyhow::Result<()> { + let generic_name: &str = self.into(); RFunction::new("", ".ps.register_ark_method") .add(RObject::try_from(generic_name)?) .add(RObject::try_from(class)?) @@ -96,19 +96,15 @@ impl ArkGenerics { Ok(()) } - pub fn register_method_from_package( - generic: Self, - class: &str, - package: &str, - ) -> anyhow::Result<()> { + pub fn register_method_from_package(&self, class: &str, package: &str) -> anyhow::Result<()> { let method = RObject::from(unsafe { Rf_lang3( r_symbol!(":::"), r_symbol!(package), - r_symbol!(format!("{generic}.{class}")), + r_symbol!(format!("{self}.{class}")), ) }); - Self::register_method(generic, class, method)?; + self.register_method(class, method)?; Ok(()) } @@ -151,7 +147,7 @@ pub fn populate_variable_methods_table(package: &str) -> anyhow::Result<()> { for name in symbol_names { if let Some((generic, class)) = ArkGenerics::parse_method(&name) { - ArkGenerics::register_method_from_package(generic, class.as_str(), package)?; + generic.register_method_from_package(class.as_str(), package)?; } } From 2cad01ed74760568ddee59238abbaa7ad15761e7 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 09:43:46 -0300 Subject: [PATCH 26/35] Use `RCall` instead --- crates/ark/src/variables/methods.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index 2bfe90e7e..cf8c91b55 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -6,6 +6,7 @@ // use anyhow::anyhow; +use harp::call::RCall; use harp::environment::r_ns_env; use harp::environment::BindingValue; use harp::exec::RFunction; @@ -14,7 +15,6 @@ use harp::r_null; use harp::r_symbol; use harp::utils::r_is_object; use harp::RObject; -use libr::Rf_lang3; use libr::SEXP; use stdext::result::ResultOrLog; use strum::IntoEnumIterator; @@ -97,13 +97,10 @@ impl ArkGenerics { } pub fn register_method_from_package(&self, class: &str, package: &str) -> anyhow::Result<()> { - let method = RObject::from(unsafe { - Rf_lang3( - r_symbol!(":::"), - r_symbol!(package), - r_symbol!(format!("{self}.{class}")), - ) - }); + let method = RCall::new(unsafe { r_symbol!(":::") }) + .add(RObject::from(package)) + .add(RObject::from(format!("{self}.{class}"))) + .build(); self.register_method(class, method)?; Ok(()) } From c8a5634ee8204ab2e45c9dfe76894208edacae67 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 10:08:46 -0300 Subject: [PATCH 27/35] Use `RArgument` --- crates/ark/src/variables/methods.rs | 9 +++------ crates/ark/src/variables/variable.rs | 10 +++++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index cf8c91b55..e30dfbc1e 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -6,6 +6,7 @@ // use anyhow::anyhow; +use harp::call::RArgument; use harp::call::RCall; use harp::environment::r_ns_env; use harp::environment::BindingValue; @@ -47,11 +48,7 @@ impl ArkGenerics { // - `Err` if method was found and errored // - `Err`if the method result could not be coerced to `T` // - T, if method was found and was succesfully executed - pub fn try_dispatch( - &self, - x: SEXP, - args: Vec<(String, RObject)>, - ) -> anyhow::Result> + pub fn try_dispatch(&self, x: SEXP, args: Vec) -> anyhow::Result> where // Making this a generic allows us to handle the conversion to the expected output // type within the dispatch, which is much more ergonomic. @@ -68,7 +65,7 @@ impl ArkGenerics { call.add(generic); call.add(x); - for (name, value) in args.into_iter() { + for RArgument { name, value } in args.into_iter() { call.param(name.as_str(), value); } diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index c4bdd249a..7707c9c11 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -12,6 +12,7 @@ use amalthea::comm::variables_comm::ClipboardFormatFormat; use amalthea::comm::variables_comm::Variable; use amalthea::comm::variables_comm::VariableKind; use anyhow::anyhow; +use harp::call::RArgument; use harp::environment::Binding; use harp::environment::BindingValue; use harp::environment::Environment; @@ -332,8 +333,8 @@ impl WorkspaceVariableDisplayValue { fn try_from_method(value: SEXP) -> Option { let display_value = - ArkGenerics::VariableDisplayValue.try_dispatch::(value, vec![( - String::from("width"), + ArkGenerics::VariableDisplayValue.try_dispatch::(value, vec![RArgument::new( + "width", RObject::from(MAX_DISPLAY_VALUE_LENGTH as i32), )]); @@ -467,7 +468,10 @@ impl WorkspaceVariableDisplayType { } fn try_from_method(value: SEXP, include_length: bool) -> anyhow::Result> { - let args = vec![(String::from("include_length"), include_length.try_into()?)]; + let args = vec![RArgument::new( + "include_length", + RObject::try_from(include_length)?, + )]; let result: Option = ArkGenerics::VariableDisplayType.try_dispatch(value, args)?; Ok(result.map(Self::simple)) } From ea9c749d9ecbd5b94c8d7afd58a2ce7dae5367c5 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 10:14:20 -0300 Subject: [PATCH 28/35] Add note on why we store a call --- crates/ark/src/variables/methods.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index e30dfbc1e..28c16e21f 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -94,6 +94,11 @@ impl ArkGenerics { } pub fn register_method_from_package(&self, class: &str, package: &str) -> anyhow::Result<()> { + // We store a call instead of the function itself to make sure: + // 1. Everything worked seamlessly with devtools::load_all() and similar functions (no risk of calling an + // outdated method from a stale cache); + // 2. An escape hatch remained available for monkey-patching a method in the package namespace + // (e.g., to enable workarounds for misbehaving methods). let method = RCall::new(unsafe { r_symbol!(":::") }) .add(RObject::from(package)) .add(RObject::from(format!("{self}.{class}"))) From 17ebe9a521b2d21aafb6dcad6d18e22ea3d5ba55 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 10:30:03 -0300 Subject: [PATCH 29/35] Use different namespacing. `.ps` -> `.ark` and `ark_variables` -> `positron_variables`. --- crates/ark/src/modules/positron/methods.R | 18 +++++++++--------- crates/ark/src/variables/methods.rs | 10 +++++----- crates/ark/src/variables/variable.rs | 8 ++++---- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index 90bbeed9c..4b480ab97 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -6,10 +6,10 @@ # ark_methods_table <- new.env(parent = emptyenv()) -ark_methods_table$ark_variable_display_value <- new.env(parent = emptyenv()) -ark_methods_table$ark_variable_display_type <- new.env(parent = emptyenv()) -ark_methods_table$ark_variable_has_children <- new.env(parent = emptyenv()) -ark_methods_table$ark_variable_kind <- new.env(parent = emptyenv()) +ark_methods_table$positron_variable_display_value <- new.env(parent = emptyenv()) +ark_methods_table$positron_variable_display_type <- new.env(parent = emptyenv()) +ark_methods_table$positron_variable_has_children <- new.env(parent = emptyenv()) +ark_methods_table$positron_variable_kind <- new.env(parent = emptyenv()) lockEnvironment(ark_methods_table, TRUE) #' Register the methods with the Positron runtime @@ -18,14 +18,14 @@ lockEnvironment(ark_methods_table, TRUE) #' @param class Class name as a character #' @param method A method to be registered. Should be a call object. #' @export -.ps.register_ark_method <- function(generic, class, method) { +.ark.register_ark_method <- function(generic, class, method) { stopifnot( is_string(generic), generic %in% c( - "ark_variable_display_value", - "ark_variable_display_type", - "ark_variable_has_children", - "ark_variable_kind" + "positron_variable_display_value", + "positron_variable_display_type", + "positron_variable_has_children", + "positron_variable_kind" ), typeof(class) == "character" ) diff --git a/crates/ark/src/variables/methods.rs b/crates/ark/src/variables/methods.rs index 28c16e21f..317fb9aae 100644 --- a/crates/ark/src/variables/methods.rs +++ b/crates/ark/src/variables/methods.rs @@ -28,16 +28,16 @@ use crate::modules::ARK_ENVS; #[derive(Debug, PartialEq, EnumString, EnumIter, IntoStaticStr, Display, Eq, Hash, Clone)] pub enum ArkGenerics { - #[strum(serialize = "ark_variable_display_value")] + #[strum(serialize = "positron_variable_display_value")] VariableDisplayValue, - #[strum(serialize = "ark_variable_display_type")] + #[strum(serialize = "positron_variable_display_type")] VariableDisplayType, - #[strum(serialize = "ark_variable_has_children")] + #[strum(serialize = "positron_variable_has_children")] VariableHasChildren, - #[strum(serialize = "ark_variable_kind")] + #[strum(serialize = "positron_variable_kind")] VariableKind, } @@ -85,7 +85,7 @@ impl ArkGenerics { pub fn register_method(&self, class: &str, method: RObject) -> anyhow::Result<()> { let generic_name: &str = self.into(); - RFunction::new("", ".ps.register_ark_method") + RFunction::new("", ".ark.register_ark_method") .add(RObject::try_from(generic_name)?) .add(RObject::try_from(class)?) .add(method) diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 7707c9c11..2b3c2edfd 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -1368,20 +1368,20 @@ mod tests { // Register the display value method harp::parse_eval_global( r#" - .ps.register_ark_method("ark_variable_display_value", "foo", function(x, width) { + .ark.register_ark_method("positron_variable_display_value", "foo", function(x, width) { # We return a large string and make sure it gets truncated. paste0(rep("a", length.out = 2*width), collapse="") }) - .ps.register_ark_method("ark_variable_display_type", "foo", function(x, include_length) { + .ark.register_ark_method("positron_variable_display_type", "foo", function(x, include_length) { paste0("foo (", length(x), ")") }) - .ps.register_ark_method("ark_variable_has_children", "foo", function(x) { + .ark.register_ark_method("positron_variable_has_children", "foo", function(x) { FALSE }) - .ps.register_ark_method("ark_variable_kind", "foo", function(x) { + .ark.register_ark_method("positron_variable_kind", "foo", function(x) { "other" }) From 9eebd2be8f10833b07d4bbcd73eb5f7b5d6a3119 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Wed, 9 Oct 2024 10:44:00 -0300 Subject: [PATCH 30/35] Move `methods.rs` to it's own module one level above. --- crates/ark/src/interface.rs | 6 +++--- crates/ark/src/lib.rs | 1 + crates/ark/src/{variables => }/methods.rs | 4 ++-- crates/ark/src/variables/mod.rs | 1 - crates/ark/src/variables/variable.rs | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) rename crates/ark/src/{variables => }/methods.rs (96%) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 1debfebf0..0cf8dab0c 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -98,6 +98,8 @@ use crate::lsp::main_loop::Event; use crate::lsp::main_loop::KernelNotification; use crate::lsp::main_loop::TokioUnboundedSender; use crate::lsp::state_handlers::ConsoleInputs; +use crate::methods::populate_methods_from_loaded_namespaces; +use crate::methods::populate_methods_table; use crate::modules; use crate::plots::graphics_device; use crate::r_task; @@ -120,8 +122,6 @@ use crate::ui::UiCommMessage; use crate::ui::UiCommSender; static RE_DEBUG_PROMPT: Lazy = Lazy::new(|| Regex::new(r"Browse\[\d+\]").unwrap()); -use crate::variables::methods::populate_methods_from_loaded_namespaces; -use crate::variables::methods::populate_variable_methods_table; /// An enum representing the different modes in which the R session can run. #[derive(PartialEq, Clone)] @@ -1969,7 +1969,7 @@ unsafe extern "C" fn ps_onload_hook(pkg: SEXP, _path: SEXP) -> anyhow::Result anyhow::Result<()> { let loaded: Vec = loaded.try_into()?; for pkg in loaded.into_iter() { - populate_variable_methods_table(pkg.as_str()).or_log_error("Failed populating methods"); + populate_methods_table(pkg.as_str()).or_log_error("Failed populating methods"); } Ok(()) } -pub fn populate_variable_methods_table(package: &str) -> anyhow::Result<()> { +pub fn populate_methods_table(package: &str) -> anyhow::Result<()> { let ns = r_ns_env(package)?; let symbol_names = ns .iter() diff --git a/crates/ark/src/variables/mod.rs b/crates/ark/src/variables/mod.rs index ac5c0736b..4f02205e9 100644 --- a/crates/ark/src/variables/mod.rs +++ b/crates/ark/src/variables/mod.rs @@ -5,6 +5,5 @@ // // -pub mod methods; pub mod r_variables; pub mod variable; diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 2b3c2edfd..68bf14ab0 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -52,7 +52,7 @@ use libr::*; use stdext::local; use stdext::unwrap; -use crate::variables::methods::ArkGenerics; +use crate::methods::ArkGenerics; // Constants. const MAX_DISPLAY_VALUE_ENTRIES: usize = 1_000; From 1ef4d1abb664714d83ac9f3a586f3c2e99388277 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 17 Oct 2024 10:58:17 -0300 Subject: [PATCH 31/35] Method naming - Changed naming scheme to include `ark_` prefix for all methods. - Reuse the methods table names to avoid duplication. - Use the `to_string()` method to generate the error messages. --- crates/ark/src/methods.rs | 8 ++++---- crates/ark/src/modules/positron/methods.R | 15 +++++--------- crates/ark/src/variables/variable.rs | 25 +++++++++++++++-------- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/crates/ark/src/methods.rs b/crates/ark/src/methods.rs index be9fe4386..64301aa96 100644 --- a/crates/ark/src/methods.rs +++ b/crates/ark/src/methods.rs @@ -28,16 +28,16 @@ use crate::modules::ARK_ENVS; #[derive(Debug, PartialEq, EnumString, EnumIter, IntoStaticStr, Display, Eq, Hash, Clone)] pub enum ArkGenerics { - #[strum(serialize = "positron_variable_display_value")] + #[strum(serialize = "ark_positron_variable_display_value")] VariableDisplayValue, - #[strum(serialize = "positron_variable_display_type")] + #[strum(serialize = "ark_positron_variable_display_type")] VariableDisplayType, - #[strum(serialize = "positron_variable_has_children")] + #[strum(serialize = "ark_positron_variable_has_children")] VariableHasChildren, - #[strum(serialize = "positron_variable_kind")] + #[strum(serialize = "ark_positron_variable_kind")] VariableKind, } diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index 4b480ab97..97ac892e8 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -6,10 +6,10 @@ # ark_methods_table <- new.env(parent = emptyenv()) -ark_methods_table$positron_variable_display_value <- new.env(parent = emptyenv()) -ark_methods_table$positron_variable_display_type <- new.env(parent = emptyenv()) -ark_methods_table$positron_variable_has_children <- new.env(parent = emptyenv()) -ark_methods_table$positron_variable_kind <- new.env(parent = emptyenv()) +ark_methods_table$ark_positron_variable_display_value <- new.env(parent = emptyenv()) +ark_methods_table$ark_positron_variable_display_type <- new.env(parent = emptyenv()) +ark_methods_table$ark_positron_variable_has_children <- new.env(parent = emptyenv()) +ark_methods_table$ark_positron_variable_kind <- new.env(parent = emptyenv()) lockEnvironment(ark_methods_table, TRUE) #' Register the methods with the Positron runtime @@ -21,12 +21,7 @@ lockEnvironment(ark_methods_table, TRUE) .ark.register_ark_method <- function(generic, class, method) { stopifnot( is_string(generic), - generic %in% c( - "positron_variable_display_value", - "positron_variable_display_type", - "positron_variable_has_children", - "positron_variable_kind" - ), + generic %in% names(ark_methods_table), typeof(class) == "character" ) for (cls in class) { diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index 68bf14ab0..a70968256 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -339,7 +339,7 @@ impl WorkspaceVariableDisplayValue { )]); let display_value = unwrap!(display_value, Err(err) => { - log::error!("Failed to apply 'ark_variable_display_value': {err:?}"); + log::error!("Failed to apply '{}': {err:?}", ArkGenerics::VariableDisplayValue.to_string()); return None; }); @@ -364,7 +364,10 @@ impl WorkspaceVariableDisplayType { /// display type. pub fn from(value: SEXP, include_length: bool) -> Self { match Self::try_from_method(value, include_length) { - Err(e) => log::error!("Error from 'ark_variable_display_type' method: {e}"), + Err(err) => log::error!( + "Error from '{}' method: {err}", + ArkGenerics::VariableDisplayType.to_string() + ), Ok(None) => {}, Ok(Some(display_type)) => return display_type, } @@ -486,7 +489,10 @@ impl WorkspaceVariableDisplayType { fn has_children(value: SEXP) -> bool { match ArkGenerics::VariableHasChildren.try_dispatch(value, vec![]) { - Err(e) => log::error!("Error from 'ark_variable_has_children' method: {e}"), + Err(err) => log::error!( + "Error from '{}' method: {err}", + ArkGenerics::VariableHasChildren.to_string() + ), Ok(None) => {}, Ok(Some(answer)) => return answer, } @@ -683,7 +689,10 @@ impl PositronVariable { } match try_from_method_variable_kind(x) { - Err(e) => log::error!("Error from 'ark_variable_kind' method: {e}"), + Err(err) => log::error!( + "Error from '{}' method: {err}", + ArkGenerics::VariableKind.to_string() + ), Ok(None) => {}, Ok(Some(kind)) => return kind, } @@ -1368,20 +1377,20 @@ mod tests { // Register the display value method harp::parse_eval_global( r#" - .ark.register_ark_method("positron_variable_display_value", "foo", function(x, width) { + .ark.register_ark_method("ark_positron_variable_display_value", "foo", function(x, width) { # We return a large string and make sure it gets truncated. paste0(rep("a", length.out = 2*width), collapse="") }) - .ark.register_ark_method("positron_variable_display_type", "foo", function(x, include_length) { + .ark.register_ark_method("ark_positron_variable_display_type", "foo", function(x, include_length) { paste0("foo (", length(x), ")") }) - .ark.register_ark_method("positron_variable_has_children", "foo", function(x) { + .ark.register_ark_method("ark_positron_variable_has_children", "foo", function(x) { FALSE }) - .ark.register_ark_method("positron_variable_kind", "foo", function(x) { + .ark.register_ark_method("ark_positron_variable_kind", "foo", function(x) { "other" }) From c6e52bcf497ac3ea318c56a039ac2e580036dd4e Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 17 Oct 2024 11:09:52 -0300 Subject: [PATCH 32/35] Typos Co-authored-by: Davis Vaughan --- crates/ark/src/methods.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ark/src/methods.rs b/crates/ark/src/methods.rs index 64301aa96..b8a3540d1 100644 --- a/crates/ark/src/methods.rs +++ b/crates/ark/src/methods.rs @@ -46,8 +46,8 @@ impl ArkGenerics { // Returns // - `None` if no method was found, // - `Err` if method was found and errored - // - `Err`if the method result could not be coerced to `T` - // - T, if method was found and was succesfully executed + // - `Err` if the method result could not be coerced to `T` + // - T, if method was found and was successfully executed pub fn try_dispatch(&self, x: SEXP, args: Vec) -> anyhow::Result> where // Making this a generic allows us to handle the conversion to the expected output @@ -107,7 +107,7 @@ impl ArkGenerics { Ok(()) } - // Checks if a symbol name is a method and returns it's class + // Checks if a symbol name is a method and returns its class fn parse_method(name: &String) -> Option<(Self, String)> { for method in ArkGenerics::iter() { let method_str: &str = method.clone().into(); From 2ee62d766f1293fcec6837bec8a65570165d5b71 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 17 Oct 2024 11:14:43 -0300 Subject: [PATCH 33/35] Require explicit registration --- crates/ark/src/interface.rs | 11 ------- crates/ark/src/methods.rs | 65 ------------------------------------- 2 files changed, 76 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 0cf8dab0c..6b5e842a2 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -98,8 +98,6 @@ use crate::lsp::main_loop::Event; use crate::lsp::main_loop::KernelNotification; use crate::lsp::main_loop::TokioUnboundedSender; use crate::lsp::state_handlers::ConsoleInputs; -use crate::methods::populate_methods_from_loaded_namespaces; -use crate::methods::populate_methods_table; use crate::modules; use crate::plots::graphics_device; use crate::r_task; @@ -423,10 +421,6 @@ impl RMain { } } - if let Err(err) = populate_methods_from_loaded_namespaces() { - log::error!("Can't populate variables pane methods from loaded packages: {err:?}"); - } - // Set up the global error handler (after support function initialization) errors::initialize(); @@ -1968,11 +1962,6 @@ unsafe extern "C" fn ps_onload_hook(pkg: SEXP, _path: SEXP) -> anyhow::Result anyhow::Result<()> { - // We store a call instead of the function itself to make sure: - // 1. Everything worked seamlessly with devtools::load_all() and similar functions (no risk of calling an - // outdated method from a stale cache); - // 2. An escape hatch remained available for monkey-patching a method in the package namespace - // (e.g., to enable workarounds for misbehaving methods). - let method = RCall::new(unsafe { r_symbol!(":::") }) - .add(RObject::from(package)) - .add(RObject::from(format!("{self}.{class}"))) - .build(); - self.register_method(class, method)?; - Ok(()) - } - - // Checks if a symbol name is a method and returns its class - fn parse_method(name: &String) -> Option<(Self, String)> { - for method in ArkGenerics::iter() { - let method_str: &str = method.clone().into(); - if name.starts_with::<&str>(method_str) { - if let Some((_, class)) = name.split_once(".") { - return Some((method, class.to_string())); - } - } - } - None - } -} - -pub fn populate_methods_from_loaded_namespaces() -> anyhow::Result<()> { - let loaded = RFunction::new("base", "loadedNamespaces").call()?; - let loaded: Vec = loaded.try_into()?; - - for pkg in loaded.into_iter() { - populate_methods_table(pkg.as_str()).or_log_error("Failed populating methods"); - } - - Ok(()) -} - -pub fn populate_methods_table(package: &str) -> anyhow::Result<()> { - let ns = r_ns_env(package)?; - let symbol_names = ns - .iter() - .filter_map(Result::ok) - .filter(|b| match b.value { - BindingValue::Standard { .. } => true, - BindingValue::Promise { .. } => true, - _ => false, - }) - .map(|b| -> String { b.name.into() }); - - for name in symbol_names { - if let Some((generic, class)) = ArkGenerics::parse_method(&name) { - generic.register_method_from_package(class.as_str(), package)?; - } - } - - Ok(()) } From ee2f34e6c63b46153af1cfd3164ddcdf37a63d9d Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 17 Oct 2024 11:18:21 -0300 Subject: [PATCH 34/35] Rename to `.ark.register_method()` --- crates/ark/src/methods.rs | 2 +- crates/ark/src/modules/positron/methods.R | 2 +- crates/ark/src/variables/variable.rs | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ark/src/methods.rs b/crates/ark/src/methods.rs index d8ed1cd93..02c3135d2 100644 --- a/crates/ark/src/methods.rs +++ b/crates/ark/src/methods.rs @@ -79,7 +79,7 @@ impl ArkGenerics { pub fn register_method(&self, class: &str, method: RObject) -> anyhow::Result<()> { let generic_name: &str = self.into(); - RFunction::new("", ".ark.register_ark_method") + RFunction::new("", ".ark.register_method") .add(RObject::try_from(generic_name)?) .add(RObject::try_from(class)?) .add(method) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index 97ac892e8..f64c0735b 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -18,7 +18,7 @@ lockEnvironment(ark_methods_table, TRUE) #' @param class Class name as a character #' @param method A method to be registered. Should be a call object. #' @export -.ark.register_ark_method <- function(generic, class, method) { +.ark.register_method <- function(generic, class, method) { stopifnot( is_string(generic), generic %in% names(ark_methods_table), diff --git a/crates/ark/src/variables/variable.rs b/crates/ark/src/variables/variable.rs index a70968256..facaefa04 100644 --- a/crates/ark/src/variables/variable.rs +++ b/crates/ark/src/variables/variable.rs @@ -1377,20 +1377,20 @@ mod tests { // Register the display value method harp::parse_eval_global( r#" - .ark.register_ark_method("ark_positron_variable_display_value", "foo", function(x, width) { + .ark.register_method("ark_positron_variable_display_value", "foo", function(x, width) { # We return a large string and make sure it gets truncated. paste0(rep("a", length.out = 2*width), collapse="") }) - .ark.register_ark_method("ark_positron_variable_display_type", "foo", function(x, include_length) { + .ark.register_method("ark_positron_variable_display_type", "foo", function(x, include_length) { paste0("foo (", length(x), ")") }) - .ark.register_ark_method("ark_positron_variable_has_children", "foo", function(x) { + .ark.register_method("ark_positron_variable_has_children", "foo", function(x) { FALSE }) - .ark.register_ark_method("ark_positron_variable_kind", "foo", function(x) { + .ark.register_method("ark_positron_variable_kind", "foo", function(x) { "other" }) From e6e2e18872f28be9cccb6aab9cd21a34ddbfd58a Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 17 Oct 2024 16:22:10 -0300 Subject: [PATCH 35/35] Use an allow list of packages --- crates/ark/src/modules/positron/methods.R | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/ark/src/modules/positron/methods.R b/crates/ark/src/modules/positron/methods.R index f64c0735b..c0779b5a8 100644 --- a/crates/ark/src/modules/positron/methods.R +++ b/crates/ark/src/modules/positron/methods.R @@ -12,6 +12,8 @@ ark_methods_table$ark_positron_variable_has_children <- new.env(parent = emptyen ark_methods_table$ark_positron_variable_kind <- new.env(parent = emptyenv()) lockEnvironment(ark_methods_table, TRUE) +ark_methods_allowed_packages <- c("torch", "reticulate") + #' Register the methods with the Positron runtime #' #' @param generic Generic function name as a character to register @@ -19,6 +21,15 @@ lockEnvironment(ark_methods_table, TRUE) #' @param method A method to be registered. Should be a call object. #' @export .ark.register_method <- function(generic, class, method) { + + # Check if the caller is an allowed package + if (!in_ark_tests()) { + calling_env <- .ps.env_name(topenv(parent.frame())) + if (!(calling_env %in% paste0("namespace:", ark_methods_allowed_packages))) { + stop("Only allowed packages can register methods. Called from ", calling_env) + } + } + stopifnot( is_string(generic), generic %in% names(ark_methods_table),