Skip to content

Simplify .Last.value handling #1062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DavisVaughan merged 4 commits into from
Mar 5, 2026
+215 −475
Merged

Simplify .Last.value handling #1062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad0dcc9
Simplify `.Last.value` handling
DavisVaughan Feb 25, 2026
533010d
Move `execute_request_invisibly()` to ark_test
DavisVaughan Feb 27, 2026
e57a70e
Move all execution related helpers to ark_test
DavisVaughan Feb 27, 2026
b535d96
Move `is_installed()` to ark_test
DavisVaughan Feb 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 0 additions & 132 deletions crates/amalthea/src/fixtures/dummy_frontend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down
123 changes: 20 additions & 103 deletions crates/ark/src/variables/r_variables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Copy link
Contributor Author

@DavisVaughan DavisVaughan Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only used by tests before, but we don't use this now. We literally set the option like an R user would.


/// 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.
Expand All @@ -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.
Expand All @@ -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]) {
Expand All @@ -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();
Expand Down Expand Up @@ -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() {
Comment on lines 220 to 222
Copy link
Contributor Author

@DavisVaughan DavisVaughan Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was very strange that we had all this special handling around .Last.value.

I realized it could be greatly simplified by shoving .Last.value handling into bindings().

Either:

  • The option is on, and .Last.value is returned from bindings()
  • The option is off, and .Last.value is not returned from bindings()

Everything else (including assigned and removed handling below) just correctly falls out from this.

variables.push(PositronVariable::new(binding).var());
}
Expand Down Expand Up @@ -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(|| {
Expand All @@ -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());
}
Expand All @@ -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();
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
}
}
Comment on lines +635 to +639
Copy link
Contributor Author

@DavisVaughan DavisVaughan Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one special place we handle it now


bindings.sort_by(|a, b| a.name.cmp(&b.name));

RThreadSafe::new(bindings)
Expand Down
Loading