Simplify .Last.value handling (#1062)

* Simplify `.Last.value` handling

* Move `execute_request_invisibly()` to ark_test

This magically works because the ark_test side has the `self.recv_iopub_busy()` and `self.recv_iopub_idle()` overrides that buffer Variables messages

* Move all execution related helpers to ark_test

So they all get proper buffering

* Move `is_installed()` to ark_test

It runs R code, so is very R specific

Author: DavisVaughan

crates/amalthea/src/fixtures/dummy_frontend.rs

@@ -17,7 +17,6 @@ use crate::wire::comm_msg::CommWireMsg;
 use crate::wire::execute_input::ExecuteInput;
 use crate::wire::execute_request::ExecuteRequest;
 use crate::wire::execute_request::ExecuteRequestPositron;
-use crate::wire::execute_request::JupyterPositronLocation;
 use crate::wire::handshake_reply::HandshakeReply;
 use crate::wire::input_reply::InputReply;
 use crate::wire::jupyter_message::JupyterMessage;
@@ -241,112 +240,6 @@ impl DummyFrontend {
         })
     }
 
-    /// Sends an execute request and handles the standard message flow:
-    /// busy -> execute_input -> idle -> execute_reply.
-    /// Asserts that the input code matches and returns the execution count.
-    #[track_caller]
-    pub fn execute_request_invisibly(&self, code: &str) -> u32 {
-        self.send_execute_request(code, ExecuteRequestOptions::default());
-        self.recv_iopub_busy();
-
-        let input = self.recv_iopub_execute_input();
-        assert_eq!(input.code, code);
-
-        self.recv_iopub_idle();
-
-        let execution_count = self.recv_shell_execute_reply();
-        assert_eq!(execution_count, input.execution_count);
-
-        execution_count
-    }
-
-    /// Sends an execute request and handles the standard message flow with a result:
-    /// busy -> execute_input -> execute_result -> idle -> execute_reply.
-    /// Asserts that the input code matches and passes the result to the callback.
-    /// Returns the execution count.
-    #[track_caller]
-    pub fn execute_request<F>(&self, code: &str, result_check: F) -> u32
-    where
-        F: FnOnce(String),
-    {
-        self.execute_request_with_options(code, result_check, Default::default())
-    }
-
-    #[track_caller]
-    pub fn execute_request_with_location<F>(
-        &self,
-        code: &str,
-        result_check: F,
-        code_location: JupyterPositronLocation,
-    ) -> u32
-    where
-        F: FnOnce(String),
-    {
-        self.execute_request_with_options(
-            code,
-            result_check,
-            ExecuteRequestOptions {
-                positron: Some(ExecuteRequestPositron {
-                    code_location: Some(code_location),
-                }),
-                ..Default::default()
-            },
-        )
-    }
-
-    #[track_caller]
-    pub fn execute_request_with_options<F>(
-        &self,
-        code: &str,
-        result_check: F,
-        options: ExecuteRequestOptions,
-    ) -> u32
-    where
-        F: FnOnce(String),
-    {
-        self.send_execute_request(code, options);
-        self.recv_iopub_busy();
-
-        let input = self.recv_iopub_execute_input();
-        assert_eq!(input.code, code);
-
-        let result = self.recv_iopub_execute_result();
-        result_check(result);
-
-        self.recv_iopub_idle();
-
-        let execution_count = self.recv_shell_execute_reply();
-        assert_eq!(execution_count, input.execution_count);
-
-        execution_count
-    }
-
-    /// Sends an execute request that produces an error and handles the standard message flow:
-    /// busy -> execute_input -> execute_error -> idle -> execute_reply_exception.
-    /// Passes the error message to the callback for custom assertions.
-    /// Returns the execution count.
-    #[track_caller]
-    pub fn execute_request_error<F>(&self, code: &str, error_check: F) -> u32
-    where
-        F: FnOnce(String),
-    {
-        self.send_execute_request(code, ExecuteRequestOptions::default());
-        self.recv_iopub_busy();
-
-        let input = self.recv_iopub_execute_input();
-        assert_eq!(input.code, code);
-
-        let error_msg = self.recv_iopub_execute_error();
-        error_check(error_msg);
-
-        self.recv_iopub_idle();
-
-        let execution_count = self.recv_shell_execute_reply_exception();
-        assert_eq!(execution_count, input.execution_count);
-
-        execution_count
-    }
-
     /// Sends a Jupyter message on the Stdin socket
     pub fn send_stdin<T: ProtocolMessage>(&self, msg: T) {
         Self::send(&self.stdin_socket, &self.session, msg);
@@ -713,31 +606,6 @@ impl DummyFrontend {
             (data.content.comm_id, data.content.target_name, data.content.data)
         })
     }
-
-    pub fn is_installed(&self, package: &str) -> bool {
-        let code = format!(".ps.is_installed('{package}')");
-        self.send_execute_request(&code, ExecuteRequestOptions::default());
-        self.recv_iopub_busy();
-
-        let input = self.recv_iopub_execute_input();
-        assert_eq!(input.code, code);
-
-        let result = self.recv_iopub_execute_result();
-
-        let out = if result == "[1] TRUE" {
-            true
-        } else if result == "[1] FALSE" {
-            false
-        } else {
-            panic!("Expected `TRUE` or `FALSE`, got '{result}'.");
-        };
-
-        self.recv_iopub_idle();
-
-        assert_eq!(self.recv_shell_execute_reply(), input.execution_count);
-
-        out
-    }
 }
 
 impl Default for ExecuteRequestOptions {

crates/ark/src/variables/r_variables.rs

@@ -35,6 +35,8 @@ use harp::utils::r_assert_type;
 use harp::utils::r_is_function;
 use harp::vector::CharacterVector;
 use harp::vector::Vector;
+use harp::RSymbol;
+use harp::R_ENVS;
 use libr::R_GlobalEnv;
 use libr::Rf_ScalarLogical;
 use libr::ENVSXP;
@@ -53,17 +55,6 @@ use crate::variables::variable::try_dispatch_view;
 use crate::variables::variable::PositronVariable;
 use crate::view::view;
 
-/// Enumeration of treatments for the .Last.value variable
-pub enum LastValue {
-    /// Always show the .Last.value variable in the Variables pane. This is used
-    /// by tests to show the value without changing the global option.
-    Always,
-
-    /// Use the value of the global option `positron.show_last_value` to
-    /// determine whether to show the .Last.value variable
-    UseOption,
-}
-
 /**
  * The R Variables handler provides the server side of Positron's Variables panel, and is
  * responsible for creating and updating the list of variables.
@@ -90,14 +81,6 @@ pub struct RVariables {
     current_bindings: RThreadSafe<Vec<Binding>>,
     version: u64,
 
-    /// Whether to always show the .Last.value in the Variables pane, regardless
-    /// of the value of positron.show_last_value
-    show_last_value: LastValue,
-
-    /// Whether we are currently showing the .Last.value variable in the Variables
-    /// pane.
-    showing_last_value: bool,
-
     /// Whether we need to send an initial `Refresh` event to the frontend,
     /// which is required for frontend initialization. Set on construction,
     /// cleared after the first `update()` call.
@@ -116,24 +99,6 @@ impl RVariables {
         comm: CommSocket,
         comm_event_tx: Sender<CommEvent>,
         iopub_tx: Sender<IOPubMessage>,
-    ) {
-        // Start with default settings
-        Self::start_with_config(env, comm, comm_event_tx, iopub_tx, LastValue::UseOption);
-    }
-
-    /**
-     * Creates a new RVariables instance with specific configuration.
-     *
-     * - `env`: An R environment to scan for variables, typically R_GlobalEnv
-     * - `comm`: A channel used to send messages to the frontend
-     * - `show_last_value`: Whether to include .Last.value in the variables list
-     */
-    pub fn start_with_config(
-        env: RObject,
-        comm: CommSocket,
-        comm_event_tx: Sender<CommEvent>,
-        iopub_tx: Sender<IOPubMessage>,
-        show_last_value: LastValue,
     ) {
         // Validate that the RObject we were passed is actually an environment
         if let Err(err) = r_assert_type(env.sexp, &[ENVSXP]) {
@@ -159,8 +124,6 @@ impl RVariables {
                 env,
                 current_bindings,
                 version: 0,
-                show_last_value,
-                showing_last_value: false,
                 needs_initial_refresh: true,
             };
             environment.execution_thread();
@@ -256,10 +219,6 @@ impl RVariables {
     fn list_variables(&mut self) -> Vec<Variable> {
         let mut variables: Vec<Variable> = vec![];
         r_task(|| {
-            if let Some(last_value) = self.last_value() {
-                variables.push(last_value.var());
-            }
-
             for binding in self.current_bindings.get() {
                 variables.push(PositronVariable::new(binding).var());
             }
@@ -526,45 +485,21 @@ impl RVariables {
         }
     }
 
-    /// Gets the value of the special variable '.Last.value' (the value of the
-    /// last expression evaluated at the top level), if enabled.
-    ///
-    /// Returns None in all other cases.
-    fn last_value(&self) -> Option<PositronVariable> {
-        // Check the cached value first
-        let show_last_value = match self.show_last_value {
-            LastValue::Always => true,
-            LastValue::UseOption => {
-                // If we aren't always showing the last value, update from the
-                // global option
-                let use_last_value = get_option("positron.show_last_value");
-                match use_last_value.get_bool(0) {
-                    Ok(Some(true)) => true,
-                    _ => false,
-                }
-            },
-        };
-
-        if show_last_value {
-            match harp::environment::last_value() {
-                Ok(last_robj) => Some(PositronVariable::from(
-                    String::from(".Last.value"),
-                    String::from(".Last.value"),
-                    last_robj.sexp,
-                )),
-                Err(err) => {
-                    // This isn't a critical error but would also be very
-                    // unexpected.
-                    log::error!("Variables: Could not evaluate .Last.value ({err:?})");
-                    None
-                },
-            }
-        } else {
-            // Last value display is disabled
-            None
+    /// Determines whether or not we should be showing the special `.Last.value` object
+    fn show_last_value(&self) -> bool {
+        match get_option("positron.show_last_value").get_bool(0) {
+            Ok(Some(true)) => true,
+            _ => false,
         }
     }
 
+    fn last_value() -> harp::Result<Binding> {
+        Binding::new(
+            &Environment::view(R_ENVS.base),
+            RSymbol::from(".Last.value"),
+        )
+    }
+
     #[tracing::instrument(level = "trace", skip_all)]
     fn update(&mut self) {
         r_task(|| {
@@ -580,10 +515,6 @@ impl RVariables {
 
                 let mut variables: Vec<Variable> = vec![];
 
-                if let (Some(var), _) = self.track_last_value() {
-                    variables.push(var);
-                }
-
                 for binding in self.current_bindings.get() {
                     variables.push(PositronVariable::new(binding).var());
                 }
@@ -602,12 +533,6 @@ impl RVariables {
             let mut assigned: Vec<Variable> = vec![];
             let mut removed: Vec<String> = vec![];
 
-            match self.track_last_value() {
-                (Some(var), _) => assigned.push(var),
-                (None, true) => removed.push(".Last.value".to_string()),
-                (None, false) => {},
-            }
-
             let new_bindings = self.bindings();
 
             let mut old_iter = self.current_bindings.get().iter();
@@ -684,20 +609,6 @@ impl RVariables {
 
     // SAFETY: The following methods must be called in an `r_task()`
 
-    /// Updates `self.showing_last_value` and returns the last value variable
-    /// if it should be shown, along with whether it was previously shown.
-    fn track_last_value(&mut self) -> (Option<Variable>, bool) {
-        let was_showing = self.showing_last_value;
-
-        if let Some(last_value) = self.last_value() {
-            self.showing_last_value = true;
-            (Some(last_value.var()), was_showing)
-        } else {
-            self.showing_last_value = false;
-            (None, was_showing)
-        }
-    }
-
     /// Updates `self.env` to the current effective environment if it changed
     /// (e.g. entering or exiting debug mode). Returns `true` if switched.
     fn update_env(&mut self) -> bool {
@@ -721,6 +632,12 @@ impl RVariables {
 
         let mut bindings: Vec<Binding> = env.iter().filter_map(|b| b.ok()).collect();
 
+        if self.show_last_value() {
+            if let Some(last_value) = Self::last_value().log_err() {
+                bindings.push(last_value);
+            }
+        }
+
         bindings.sort_by(|a, b| a.name.cmp(&b.name));
 
         RThreadSafe::new(bindings)