From 3c4d4e9a24045539de319170f8cb5d8e130d82b5 Mon Sep 17 00:00:00 2001 From: Tristan Ang Date: Sun, 8 Mar 2026 04:40:49 -0700 Subject: [PATCH 1/3] Avoid cloning REPL global name maps --- crates/monty/src/prepare.rs | 58 +++++++++++++++++++++++++------ crates/monty/src/repl.rs | 69 ++++++++++++++++--------------------- crates/monty/tests/repl.rs | 47 +++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 51 deletions(-) diff --git a/crates/monty/src/prepare.rs b/crates/monty/src/prepare.rs index b44a4b2b5..10c637f1c 100644 --- a/crates/monty/src/prepare.rs +++ b/crates/monty/src/prepare.rs @@ -1,4 +1,4 @@ -use std::collections::hash_map::Entry; +use std::{collections::hash_map::Entry, mem}; use ahash::{AHashMap, AHashSet}; @@ -30,7 +30,7 @@ pub struct PrepareResult { /// /// This map is used by: /// - ref-count tests for looking up variables by name - /// - REPL incremental compilation to preserve stable global slot IDs across snippets + /// - initial REPL compilation to seed stable global slot IDs across later snippets pub name_map: AHashMap, /// The prepared AST nodes with all names resolved to namespace indices. /// Function definitions are inline as `PreparedFunctionDef` variants. @@ -39,6 +39,20 @@ pub struct PrepareResult { pub interner: InternerBuilder, } +/// Result of preparing a REPL snippet against an existing mutable global name map. +/// +/// Unlike [`PrepareResult`], this does not return the module name map because the +/// caller-owned map is mutated in place during preparation. On failure, any newly +/// reserved global slots remain in that map so later REPL snippets can reuse them. +pub struct PrepareExistingNamesResult { + /// Number of items in the namespace (at module level, this IS the global namespace) + pub namespace_size: usize, + /// The prepared AST nodes with all names resolved to namespace indices. + pub nodes: Vec, + /// The string interner containing all interned identifiers and filenames. + pub interner: InternerBuilder, +} + /// Prepares parsed nodes for compilation by resolving names and building the initial namespace. /// /// The namespace will be converted to runtime Objects when execution begins and the heap is available. @@ -69,15 +83,22 @@ pub(crate) fn prepare(parse_result: ParseResult, input_names: Vec) -> Re /// Prepares parsed nodes for REPL-style incremental compilation using an existing global namespace map. /// -/// Existing bindings keep their original namespace slots; any new names are appended with new slots. -/// This ensures snippets can be compiled independently while sharing one persistent global namespace. +/// Existing bindings keep their original namespace slots; any new names are appended with new slots +/// directly in `existing_name_map`. This ensures snippets can be compiled independently while sharing +/// one persistent global namespace. +/// +/// If preparation fails, any newly reserved slots stay in `existing_name_map`. That is intentional for +/// REPL compilation: the corresponding namespace entries remain `Undefined`, so preserving the slots is +/// harmless and avoids cloning the full map before each feed. pub(crate) fn prepare_with_existing_names( parse_result: ParseResult, - existing_name_map: AHashMap, -) -> Result { + existing_name_map: &mut AHashMap, +) -> Result { let ParseResult { nodes, interner } = parse_result; let mut p = Prepare::new_module_with_name_map(existing_name_map, &interner); - let mut prepared_nodes = p.prepare_nodes(nodes)?; + let prepare_result = p.prepare_nodes(nodes); + p.write_back_module_name_map(existing_name_map); + let mut prepared_nodes = prepare_result?; // In the root frame, the last expression is implicitly returned to match REPL behavior. if let Some(Node::Expr(expr_loc)) = prepared_nodes.last() @@ -88,9 +109,8 @@ pub(crate) fn prepare_with_existing_names( prepared_nodes.push(Node::Return(new_expr_loc)); } - Ok(PrepareResult { + Ok(PrepareExistingNamesResult { namespace_size: p.namespace_size, - name_map: p.name_map, nodes: prepared_nodes, interner, }) @@ -184,10 +204,13 @@ impl<'i> Prepare<'i> { } } - /// Creates a module-scope Prepare instance from an existing global name map. + /// Creates a module-scope Prepare instance from an existing mutable global name map. /// /// Used by incremental REPL compilation to keep stable slot assignments across snippets. - fn new_module_with_name_map(name_map: AHashMap, interner: &'i InternerBuilder) -> Self { + /// The caller's map is moved into this preparer and must be written back with + /// [`Self::write_back_module_name_map`] before returning. + fn new_module_with_name_map(name_map: &mut AHashMap, interner: &'i InternerBuilder) -> Self { + let name_map = mem::take(name_map); let namespace_size = name_map .values() .map(|id| id.index()) @@ -210,6 +233,19 @@ impl<'i> Prepare<'i> { } } + /// Writes the mutated module-level name map back to the caller-owned REPL state. + /// + /// REPL preparation takes ownership of the map so new slots can be appended without cloning. + /// Call this before returning from the incremental prepare path, even on errors, so newly + /// reserved slots remain visible to later snippets. + fn write_back_module_name_map(&mut self, target_name_map: &mut AHashMap) { + debug_assert!( + self.is_module_scope, + "module name maps can only be written back from module scope" + ); + mem::swap(target_name_map, &mut self.name_map); + } + /// Creates a new Prepare instance for function-level code. /// /// Pre-populates `free_var_map` with nonlocal declarations and implicit captures, diff --git a/crates/monty/src/repl.rs b/crates/monty/src/repl.rs index 7ac18940c..ff0eb47ca 100644 --- a/crates/monty/src/repl.rs +++ b/crates/monty/src/repl.rs @@ -113,7 +113,7 @@ impl MontyRepl { let executor = match ReplExecutor::new_repl_snippet( code.to_owned(), &input_script_name, - this.global_name_map.clone(), + &mut this.global_name_map, &this.interns, input_names, ) { @@ -154,6 +154,9 @@ impl MontyRepl { /// partially mutating globals, those mutations remain visible in later feeds, /// matching Python REPL semantics. /// + /// Compile-time failures may still reserve new global slots in the REPL map. + /// Those slots remain `Undefined` until a later successful snippet writes them. + /// /// # Errors /// Returns `MontyException` for syntax/compile/runtime failures. pub fn feed_run( @@ -172,7 +175,7 @@ impl MontyRepl { let executor = ReplExecutor::new_repl_snippet( code.to_owned(), &input_script_name, - self.global_name_map.clone(), + &mut self.global_name_map, &self.interns, input_names, )?; @@ -205,16 +208,9 @@ impl MontyRepl { self.globals = vm.take_globals(); vm.cleanup(); - // Commit compiler metadata even on runtime errors. - // Snippets can mutate globals before raising, and those values may contain - // FunctionId/StringId values that must be interpreted with the updated tables. - let ReplExecutor { - name_map, - interns, - code, - .. - } = executor; - self.global_name_map = name_map; + // Commit intern/function metadata even on runtime errors. The REPL global + // name map was already updated in place during preparation. + let ReplExecutor { interns, code, .. } = executor; self.interns = interns; result.map_err(|e| e.into_python_exception(&self.interns, &code)) @@ -803,22 +799,18 @@ pub fn detect_repl_continuation_mode(source: &str) -> ReplContinuationMode { struct ReplExecutor { /// Number of slots needed in the global namespace. namespace_size: usize, - /// Maps variable names to their indices in the namespace. - /// - /// Stable slot assignment is required across snippets so previously created - /// objects continue to resolve names correctly. - name_map: AHashMap, /// Compiled bytecode for the snippet. module_code: Code, /// Interned strings and compiled functions for this snippet. interns: Interns, /// Source code used for traceback/error rendering. code: String, - /// Input variable names that were injected for this snippet. + /// Global slots assigned to inputs for this snippet. /// - /// Stored so that `inject_inputs` can look up their namespace slots - /// after compilation assigns them. - input_names: Vec, + /// Inputs are pre-registered in the REPL-global name map before preparation. + /// We store only the assigned slots here so execution can inject values without + /// needing a cloned copy of the full global name map. + input_slots: Vec, } impl ReplExecutor { @@ -828,23 +820,29 @@ impl ReplExecutor { /// no-replay execution: /// - Seeds parsing from `existing_interns` so old `StringId` values stay stable. /// - Seeds compilation with existing functions so old `FunctionId` values remain valid. - /// - Reuses `existing_name_map` and appends new global names only. + /// - Reuses `existing_name_map` in place and appends new global names only. /// - /// `input_names` are pre-registered in the name map before preparation so they + /// `input_names` are pre-registered in the REPL-global name map before preparation so they /// receive stable namespace slots that `inject_inputs` can use to store values. + /// + /// If snippet preparation fails, newly reserved global slots remain in the + /// caller-owned name map. That is harmless because the corresponding runtime + /// global entries stay `Undefined` until a later successful assignment. fn new_repl_snippet( code: String, script_name: &str, - mut existing_name_map: AHashMap, + existing_name_map: &mut AHashMap, existing_interns: &Interns, input_names: Vec, ) -> Result { // Pre-register input names so they get stable slots before preparation. + let mut input_slots = Vec::with_capacity(input_names.len()); for name in &input_names { let next_slot = existing_name_map.len(); - existing_name_map + let slot = *existing_name_map .entry(name.clone()) .or_insert_with(|| NamespaceId::new(next_slot)); + input_slots.push(slot.index()); } let seeded_interner = InternerBuilder::from_interns(existing_interns, &code); @@ -863,11 +861,10 @@ impl ReplExecutor { Ok(Self { namespace_size: prepared.namespace_size, - name_map: prepared.name_map, module_code: compile_result.code, interns, code, - input_names, + input_slots, }) } } @@ -965,12 +962,7 @@ fn inject_inputs_into_vm( input_values: Vec, vm: &mut VM<'_, '_, impl ResourceTracker>, ) -> Result<(), MontyException> { - for (name, obj) in executor.input_names.iter().zip(input_values) { - let slot = executor - .name_map - .get(name) - .expect("input name should have a namespace slot") - .index(); + for (slot, obj) in executor.input_slots.iter().copied().zip(input_values) { let value = obj .to_value(vm) .map_err(|e| MontyException::runtime_error(format!("invalid input type: {e}")))?; @@ -1048,8 +1040,7 @@ fn build_repl_progress( match converted { ConvertedExit::Complete(obj) => { - let ReplExecutor { name_map, interns, .. } = executor; - repl.global_name_map = name_map; + let ReplExecutor { interns, .. } = executor; repl.interns = interns; Ok(ReplProgress::Complete { repl, value: obj }) } @@ -1097,11 +1088,9 @@ fn build_repl_progress( })), ConvertedExit::Error(err) => { let error = err.into_python_exception(&executor.interns, &executor.code); - // Commit compiler metadata even on runtime errors, matching feed() behavior. - // Snippets can create new variables or functions before raising, and those - // values may reference FunctionId/StringId values from the new tables. - let ReplExecutor { name_map, interns, .. } = executor; - repl.global_name_map = name_map; + // Commit intern/function metadata even on runtime errors, matching feed() behavior. + // The global name map was already updated in place during preparation. + let ReplExecutor { interns, .. } = executor; repl.interns = interns; Err(Box::new(ReplStartError { repl, error })) } diff --git a/crates/monty/tests/repl.rs b/crates/monty/tests/repl.rs index d51e2de9e..5b27bc011 100644 --- a/crates/monty/tests/repl.rs +++ b/crates/monty/tests/repl.rs @@ -81,6 +81,29 @@ fn repl_runtime_error_keeps_partial_state_consistent() { assert_eq!(feed_run_print(&mut repl, "x").unwrap(), MontyObject::Int(1)); } +#[test] +fn repl_feed_compile_error_keeps_reserved_global_slot() { + let (mut repl, init_output) = init_repl(""); + assert_eq!(init_output, MontyObject::None); + + let error = feed_run_print(&mut repl, "reserved_name = 1\nnonlocal missing_name") + .expect_err("snippet should raise a prepare-time SyntaxError"); + assert_eq!(error.exc_type(), monty::ExcType::SyntaxError); + assert_eq!( + error.message(), + Some("nonlocal declaration not allowed at module level") + ); + + assert_eq!( + feed_run_print(&mut repl, "reserved_name = 42").unwrap(), + MontyObject::None + ); + assert_eq!( + feed_run_print(&mut repl, "reserved_name").unwrap(), + MontyObject::Int(42) + ); +} + #[test] fn repl_heap_mutations_are_not_replayed() { let (mut repl, _) = init_repl("items = []"); @@ -260,6 +283,30 @@ fn repl_start_runtime_error_preserves_repl_state() { assert_eq!(feed_run_print(&mut repl, "x + y + 12").unwrap(), MontyObject::Int(42)); } +#[test] +fn repl_start_compile_error_keeps_reserved_global_slot() { + let (repl, _) = init_repl(""); + + let err = repl + .feed_start("reserved_name = 1\nnonlocal missing_name", vec![], PrintWriter::Stdout) + .expect_err("expected ReplStartError"); + let ReplStartError { mut repl, error } = *err; + assert_eq!(error.exc_type(), monty::ExcType::SyntaxError); + assert_eq!( + error.message(), + Some("nonlocal declaration not allowed at module level") + ); + + assert_eq!( + feed_run_print(&mut repl, "reserved_name = 42").unwrap(), + MontyObject::None + ); + assert_eq!( + feed_run_print(&mut repl, "reserved_name").unwrap(), + MontyObject::Int(42) + ); +} + #[test] fn repl_start_runtime_error_during_external_call_preserves_repl_state() { // An external function returns an error, which should come back as ReplStartError From 1ee8084bf24a9248104b871a01c8829fe6220144 Mon Sep 17 00:00:00 2001 From: Tristan Ang Date: Tue, 10 Mar 2026 15:14:55 -0700 Subject: [PATCH 2/3] fix: preserve REPL metadata on abandoned progress --- crates/monty/src/prepare.rs | 11 +-- crates/monty/src/repl.rs | 138 +++++++++++++++++++++++++++--------- crates/monty/tests/repl.rs | 47 ++++++++++++ 3 files changed, 158 insertions(+), 38 deletions(-) diff --git a/crates/monty/src/prepare.rs b/crates/monty/src/prepare.rs index 10c637f1c..6950b682d 100644 --- a/crates/monty/src/prepare.rs +++ b/crates/monty/src/prepare.rs @@ -20,9 +20,9 @@ use crate::{ /// /// This struct holds the outputs of name resolution and AST transformation: /// - The namespace size (number of slots needed at module level) -/// - A mapping from variable names to their namespace indices (for ref-count testing) /// - The transformed AST nodes with all names resolved, ready for compilation /// - The string interner containing all interned identifiers and filenames +/// - In `ref-count-return` builds, a name map for looking up variables by slot pub struct PrepareResult { /// Number of items in the namespace (at module level, this IS the global namespace) pub namespace_size: usize, @@ -31,6 +31,7 @@ pub struct PrepareResult { /// This map is used by: /// - ref-count tests for looking up variables by name /// - initial REPL compilation to seed stable global slot IDs across later snippets + #[cfg(feature = "ref-count-return")] pub name_map: AHashMap, /// The prepared AST nodes with all names resolved to namespace indices. /// Function definitions are inline as `PreparedFunctionDef` variants. @@ -75,6 +76,7 @@ pub(crate) fn prepare(parse_result: ParseResult, input_names: Vec) -> Re Ok(PrepareResult { namespace_size: p.namespace_size, + #[cfg(feature = "ref-count-return")] name_map: p.name_map, nodes: prepared_nodes, interner, @@ -87,9 +89,10 @@ pub(crate) fn prepare(parse_result: ParseResult, input_names: Vec) -> Re /// directly in `existing_name_map`. This ensures snippets can be compiled independently while sharing /// one persistent global namespace. /// -/// If preparation fails, any newly reserved slots stay in `existing_name_map`. That is intentional for -/// REPL compilation: the corresponding namespace entries remain `Undefined`, so preserving the slots is -/// harmless and avoids cloning the full map before each feed. +/// If preparation fails, any newly reserved entries remain in `existing_name_map`. +/// That is intentional for REPL compilation: the extra entries are harmless +/// because the namespace slots stay `Undefined`. This avoids cloning the +/// full map on every feed. pub(crate) fn prepare_with_existing_names( parse_result: ParseResult, existing_name_map: &mut AHashMap, diff --git a/crates/monty/src/repl.rs b/crates/monty/src/repl.rs index ff0eb47ca..4a89c32bf 100644 --- a/crates/monty/src/repl.rs +++ b/crates/monty/src/repl.rs @@ -115,7 +115,7 @@ impl MontyRepl { &input_script_name, &mut this.global_name_map, &this.interns, - input_names, + &input_names, ) { Ok(exec) => exec, Err(error) => return Err(Box::new(ReplStartError { repl: this, error })), @@ -127,9 +127,11 @@ impl MontyRepl { // Inject inputs with VM alive if let Err(error) = inject_inputs_into_vm(&executor, input_values, &mut vm) { - this.globals = vm.take_globals(); - vm.cleanup(); - return Err(Box::new(ReplStartError { repl: this, error })); + let globals = reclaim_globals_from_vm(&mut vm); + drop(vm); + this.globals = globals; + let repl = commit_repl_compiler_metadata(this, executor); + return Err(Box::new(ReplStartError { repl, error })); } let vm_result = vm.run_module(&executor.module_code); @@ -177,7 +179,7 @@ impl MontyRepl { &input_script_name, &mut self.global_name_map, &self.interns, - input_names, + &input_names, )?; self.ensure_globals_size(executor.namespace_size); @@ -185,8 +187,11 @@ impl MontyRepl { let mut vm = VM::new(mem::take(&mut self.globals), &mut self.heap, &executor.interns, print); if let Err(e) = inject_inputs_into_vm(&executor, input_values, &mut vm) { - self.globals = vm.take_globals(); - vm.cleanup(); + let globals = reclaim_globals_from_vm(&mut vm); + drop(vm); + self.globals = globals; + let ReplExecutor { interns, .. } = executor; + self.interns = interns; return Err(e); } @@ -353,8 +358,9 @@ impl ReplProgress { /// /// Use this to recover the REPL when you need to abandon the current /// snippet (e.g. because `feed_run` doesn't support async futures). - /// The REPL state reflects any mutations that occurred before the - /// snapshot was taken. + /// The returned REPL includes any globals and compiler metadata produced + /// before the snapshot was taken, so later snippets can keep using the + /// session without replaying the abandoned code. #[must_use] pub fn into_repl(self) -> MontyRepl { match self { @@ -415,7 +421,8 @@ pub struct ReplFunctionCall { impl ReplFunctionCall { /// Extracts the REPL session, discarding the in-flight execution state. /// - /// Restores globals from the VM snapshot so the REPL remains usable. + /// Restores globals and compiler metadata from the VM snapshot so the REPL + /// remains usable without replaying the abandoned snippet. #[must_use] pub fn into_repl(self) -> MontyRepl { self.snapshot.into_repl() @@ -463,7 +470,8 @@ pub struct ReplOsCall { impl ReplOsCall { /// Extracts the REPL session, discarding the in-flight execution state. /// - /// Restores globals from the VM snapshot so the REPL remains usable. + /// Restores globals and compiler metadata from the VM snapshot so the REPL + /// remains usable without replaying the abandoned snippet. #[must_use] pub fn into_repl(self) -> MontyRepl { self.snapshot.into_repl() @@ -504,7 +512,8 @@ pub struct ReplNameLookup { impl ReplNameLookup { /// Extracts the REPL session, discarding the in-flight execution state. /// - /// Restores globals from the VM snapshot so the REPL remains usable. + /// Restores globals and compiler metadata from the VM snapshot so the REPL + /// remains usable without replaying the abandoned snippet. #[must_use] pub fn into_repl(self) -> MontyRepl { self.snapshot.into_repl() @@ -547,9 +556,11 @@ impl ReplNameLookup { let value = match obj.to_value(&mut vm) { Ok(v) => v, Err(e) => { - repl.globals = vm.take_globals(); - vm.cleanup(); let error = MontyException::runtime_error(format!("invalid name lookup result: {e}")); + let globals = reclaim_globals_from_vm(&mut vm); + drop(vm); + repl.globals = globals; + let repl = commit_repl_compiler_metadata(repl, executor); return Err(Box::new(ReplStartError { repl, error })); } }; @@ -611,9 +622,18 @@ pub struct ReplResolveFutures { impl ReplResolveFutures { /// Extracts the REPL session, discarding the in-flight execution state. + /// + /// Restores globals and compiler metadata from the suspended VM state so + /// later snippets can keep using any values created before suspension. #[must_use] pub fn into_repl(self) -> MontyRepl { - self.repl + let Self { + repl, + executor, + vm_state, + .. + } = self; + restore_repl_from_suspended_state(repl, executor, vm_state) } /// Returns unresolved call IDs for this suspended state. @@ -656,11 +676,13 @@ impl ReplResolveFutures { ); if let Some(call_id) = invalid_call_id { - repl.globals = vm.take_globals(); - vm.cleanup(); let error = MontyException::runtime_error(format!( "unknown call_id {call_id}, expected one of: {pending_call_ids:?}" )); + let globals = reclaim_globals_from_vm(&mut vm); + drop(vm); + repl.globals = globals; + let repl = commit_repl_compiler_metadata(repl, executor); return Err(Box::new(ReplStartError { repl, error })); } @@ -668,10 +690,12 @@ impl ReplResolveFutures { match ext_result { ExtFunctionResult::Return(obj) => { if let Err(e) = vm.resolve_future(call_id, obj) { - repl.globals = vm.take_globals(); - vm.cleanup(); let error = MontyException::runtime_error(format!("Invalid return type for call {call_id}: {e}")); + let globals = reclaim_globals_from_vm(&mut vm); + drop(vm); + repl.globals = globals; + let repl = commit_repl_compiler_metadata(repl, executor); return Err(Box::new(ReplStartError { repl, error })); } } @@ -684,9 +708,11 @@ impl ReplResolveFutures { } if let Some(error) = vm.take_failed_task_error() { - repl.globals = vm.take_globals(); - vm.cleanup(); let error = error.into_python_exception(&executor.interns, &executor.code); + let globals = reclaim_globals_from_vm(&mut vm); + drop(vm); + repl.globals = globals; + let repl = commit_repl_compiler_metadata(repl, executor); return Err(Box::new(ReplStartError { repl, error })); } @@ -695,9 +721,11 @@ impl ReplResolveFutures { let loaded_task = match vm.load_ready_task_if_needed() { Ok(loaded) => loaded, Err(e) => { - repl.globals = vm.take_globals(); - vm.cleanup(); let error = e.into_python_exception(&executor.interns, &executor.code); + let globals = reclaim_globals_from_vm(&mut vm); + drop(vm); + repl.globals = globals; + let repl = commit_repl_compiler_metadata(repl, executor); return Err(Box::new(ReplStartError { repl, error })); } }; @@ -833,11 +861,11 @@ impl ReplExecutor { script_name: &str, existing_name_map: &mut AHashMap, existing_interns: &Interns, - input_names: Vec, + input_names: &[String], ) -> Result { // Pre-register input names so they get stable slots before preparation. let mut input_slots = Vec::with_capacity(input_names.len()); - for name in &input_names { + for name in input_names { let next_slot = existing_name_map.len(); let slot = *existing_name_map .entry(name.clone()) @@ -889,15 +917,19 @@ pub(crate) struct ReplSnapshot { } impl ReplSnapshot { - /// Extracts the REPL session, restoring globals from the VM snapshot. + /// Extracts the REPL session, restoring globals and compiler metadata. /// /// When a snapshot is taken, globals live inside the `VMSnapshot`. - /// This method creates an empty snapshot from just the globals so the REPL - /// can be used for further snippets. + /// The matching intern/function tables live in the `ReplExecutor`. This + /// method restores both so the REPL can be used for further snippets without + /// replaying the abandoned execution. fn into_repl(self) -> MontyRepl { - let Self { mut repl, vm_state, .. } = self; - repl.globals = vm_state.globals; - repl + let Self { + repl, + executor, + vm_state, + } = self; + restore_repl_from_suspended_state(repl, executor, vm_state) } /// Continues snippet execution with an external result. @@ -953,6 +985,44 @@ impl ReplSnapshot { // Private helper functions // --------------------------------------------------------------------------- +/// Commits compiler metadata from a compiled REPL snippet back to the session. +/// +/// Incremental compilation can append global slots and compile new intern/function +/// tables before snippet execution completes. Whenever a compiled snippet hands the +/// REPL back to the caller, these tables must move with it so any globals restored +/// from execution snapshots remain interpretable. +fn commit_repl_compiler_metadata(mut repl: MontyRepl, executor: ReplExecutor) -> MontyRepl { + let ReplExecutor { interns, .. } = executor; + repl.interns = interns; + repl +} + +/// Restores a REPL from suspended execution by committing globals and compiler metadata. +/// +/// Suspended snippets keep globals inside the VM snapshot and compiler metadata inside +/// the `ReplExecutor`. If the caller abandons that suspended snippet via `into_repl()`, +/// both pieces must be restored together to avoid mixing new global slots with stale +/// intern/function tables. +fn restore_repl_from_suspended_state( + mut repl: MontyRepl, + executor: ReplExecutor, + vm_state: VMSnapshot, +) -> MontyRepl { + repl.globals = vm_state.globals; + commit_repl_compiler_metadata(repl, executor) +} + +/// Reclaims globals from a live VM before the caller drops it. +/// +/// This is used on host-side error paths that occur after a snippet has already been +/// compiled or partially executed but before `build_repl_progress` can consume the VM. +/// The caller must drop the VM before moving the REPL or executor back out. +fn reclaim_globals_from_vm(vm: &mut VM<'_, '_, T>) -> Vec { + let globals = vm.take_globals(); + vm.cleanup(); + globals +} + /// Injects input values into the VM's global namespace slots. /// /// Converts each `MontyObject` to a `Value` while the VM is alive, then stores @@ -1019,9 +1089,9 @@ fn frame_exit_to_object( /// Assembles a `ReplProgress` from already-converted data. /// -/// This is the REPL equivalent of `build_run_progress`. On completion/error, -/// compiler metadata is committed to the REPL so subsequent snippets see -/// updated intern tables and name maps. +/// This is the REPL equivalent of `build_run_progress`. Completion and runtime-error +/// paths commit compiler metadata immediately; suspended states keep the executor so +/// `into_repl()` can commit the same metadata if the caller abandons execution. fn build_repl_progress( converted: ConvertedExit, vm_state: Option, diff --git a/crates/monty/tests/repl.rs b/crates/monty/tests/repl.rs index 5b27bc011..02264a7c1 100644 --- a/crates/monty/tests/repl.rs +++ b/crates/monty/tests/repl.rs @@ -33,6 +33,13 @@ fn init_repl(code: &str) -> (MontyRepl, MontyObject) { (repl, output) } +fn assert_name_error(result: Result, name: &str) { + let error = result.expect_err("snippet should raise NameError"); + let expected_message = format!("name '{name}' is not defined"); + assert_eq!(error.exc_type(), monty::ExcType::NameError); + assert_eq!(error.message(), Some(expected_message.as_str())); +} + #[test] fn repl_persists_state_and_definitions() { let (mut repl, _) = init_repl("x = 10"); @@ -93,6 +100,7 @@ fn repl_feed_compile_error_keeps_reserved_global_slot() { error.message(), Some("nonlocal declaration not allowed at module level") ); + assert_name_error(feed_run_print(&mut repl, "reserved_name"), "reserved_name"); assert_eq!( feed_run_print(&mut repl, "reserved_name = 42").unwrap(), @@ -296,6 +304,7 @@ fn repl_start_compile_error_keeps_reserved_global_slot() { error.message(), Some("nonlocal declaration not allowed at module level") ); + assert_name_error(feed_run_print(&mut repl, "reserved_name"), "reserved_name"); assert_eq!( feed_run_print(&mut repl, "reserved_name = 42").unwrap(), @@ -307,6 +316,44 @@ fn repl_start_compile_error_keeps_reserved_global_slot() { ); } +#[test] +fn repl_abandon_function_call_preserves_globals_and_metadata() { + let (repl, _) = init_repl(""); + + let progress = repl + .feed_start( + "captured = 41\ndef helper():\n return captured + 1\next_fn()", + vec![], + PrintWriter::Stdout, + ) + .unwrap(); + let call = progress.into_function_call().expect("expected function call"); + let mut repl = call.into_repl(); + + assert_eq!(feed_run_print(&mut repl, "captured").unwrap(), MontyObject::Int(41)); + assert_eq!(feed_run_print(&mut repl, "helper()").unwrap(), MontyObject::Int(42)); +} + +#[test] +fn repl_abandon_resolve_futures_preserves_globals_and_metadata() { + let (repl, _) = init_repl(""); + + let progress = repl + .feed_start( + "captured = 41\ndef helper():\n return captured + 1\nasync def main():\n value = await foo()\n return value\nawait main()", + vec![], + PrintWriter::Stdout, + ) + .unwrap(); + let call = progress.into_function_call().expect("expected function call"); + let progress = call.resume_pending(PrintWriter::Stdout).unwrap(); + let state = progress.into_resolve_futures().expect("expected resolve futures"); + let mut repl = state.into_repl(); + + assert_eq!(feed_run_print(&mut repl, "captured").unwrap(), MontyObject::Int(41)); + assert_eq!(feed_run_print(&mut repl, "helper()").unwrap(), MontyObject::Int(42)); +} + #[test] fn repl_start_runtime_error_during_external_call_preserves_repl_state() { // An external function returns an error, which should come back as ReplStartError From 1632a02b2e9b14c965aa8b514e0c272d9f58d839 Mon Sep 17 00:00:00 2001 From: Tristan Ang Date: Tue, 10 Mar 2026 16:00:09 -0700 Subject: [PATCH 3/3] test: cover REPL error-path state preservation --- crates/monty/tests/repl.rs | 124 ++++++++++++++++++++++++++++++++++++- 1 file changed, 122 insertions(+), 2 deletions(-) diff --git a/crates/monty/tests/repl.rs b/crates/monty/tests/repl.rs index 02264a7c1..54ff8ca5c 100644 --- a/crates/monty/tests/repl.rs +++ b/crates/monty/tests/repl.rs @@ -4,8 +4,8 @@ //! only the newly fed snippet each time. use monty::{ - ExtFunctionResult, MontyException, MontyObject, MontyRepl, NoLimitTracker, PrintWriter, ReplContinuationMode, - ReplProgress, ReplStartError, ResourceTracker, detect_repl_continuation_mode, + ExtFunctionResult, MontyException, MontyObject, MontyRepl, NameLookupResult, NoLimitTracker, PrintWriter, + ReplContinuationMode, ReplProgress, ReplStartError, ResourceTracker, detect_repl_continuation_mode, }; #[test] @@ -40,6 +40,16 @@ fn assert_name_error(result: Result, name: &str) { assert_eq!(error.message(), Some(expected_message.as_str())); } +fn assert_runtime_error_exception(error: &MontyException, expected_message: &str) { + assert_eq!(error.exc_type(), monty::ExcType::RuntimeError); + assert_eq!(error.message(), Some(expected_message)); +} + +fn assert_runtime_error(result: Result, expected_message: &str) { + let error = result.expect_err("snippet should raise RuntimeError"); + assert_runtime_error_exception(&error, expected_message); +} + #[test] fn repl_persists_state_and_definitions() { let (mut repl, _) = init_repl("x = 10"); @@ -316,6 +326,59 @@ fn repl_start_compile_error_keeps_reserved_global_slot() { ); } +#[test] +fn repl_feed_invalid_input_keeps_reserved_slots_and_repl_usable() { + let (mut repl, init_output) = init_repl(""); + assert_eq!(init_output, MontyObject::None); + + assert_runtime_error( + repl.feed_run( + "reserved_output = incoming_input", + vec![("incoming_input".to_string(), MontyObject::Repr("bad".to_string()))], + PrintWriter::Stdout, + ), + "invalid input type: 'Repr' is not a valid input value", + ); + assert_name_error(feed_run_print(&mut repl, "incoming_input"), "incoming_input"); + assert_name_error(feed_run_print(&mut repl, "reserved_output"), "reserved_output"); + + assert_eq!( + feed_run_print(&mut repl, "incoming_input = 41\nreserved_output = incoming_input + 1").unwrap(), + MontyObject::None + ); + assert_eq!( + feed_run_print(&mut repl, "reserved_output").unwrap(), + MontyObject::Int(42) + ); +} + +#[test] +fn repl_start_invalid_input_keeps_reserved_slots_and_repl_usable() { + let (repl, init_output) = init_repl(""); + assert_eq!(init_output, MontyObject::None); + + let err = repl + .feed_start( + "reserved_output = incoming_input", + vec![("incoming_input".to_string(), MontyObject::Repr("bad".to_string()))], + PrintWriter::Stdout, + ) + .expect_err("expected invalid input error"); + let ReplStartError { mut repl, error } = *err; + assert_runtime_error_exception(&error, "invalid input type: 'Repr' is not a valid input value"); + assert_name_error(feed_run_print(&mut repl, "incoming_input"), "incoming_input"); + assert_name_error(feed_run_print(&mut repl, "reserved_output"), "reserved_output"); + + assert_eq!( + feed_run_print(&mut repl, "incoming_input = 41\nreserved_output = incoming_input + 1").unwrap(), + MontyObject::None + ); + assert_eq!( + feed_run_print(&mut repl, "reserved_output").unwrap(), + MontyObject::Int(42) + ); +} + #[test] fn repl_abandon_function_call_preserves_globals_and_metadata() { let (repl, _) = init_repl(""); @@ -334,6 +397,33 @@ fn repl_abandon_function_call_preserves_globals_and_metadata() { assert_eq!(feed_run_print(&mut repl, "helper()").unwrap(), MontyObject::Int(42)); } +#[test] +fn repl_name_lookup_invalid_host_value_preserves_globals_and_metadata() { + let (repl, _) = init_repl(""); + + let progress = repl + .feed_start( + "captured = 41\ndef helper():\n return captured + 1\nmissing_name", + vec![], + PrintWriter::Stdout, + ) + .unwrap(); + let lookup = progress.into_name_lookup().expect("expected name lookup"); + assert_eq!(lookup.name, "missing_name"); + + let err = lookup + .resume( + NameLookupResult::Value(MontyObject::Repr("bad".to_string())), + PrintWriter::Stdout, + ) + .expect_err("expected invalid name lookup result"); + let ReplStartError { mut repl, error } = *err; + assert_runtime_error_exception(&error, "invalid name lookup result: 'Repr' is not a valid input value"); + + assert_eq!(feed_run_print(&mut repl, "captured").unwrap(), MontyObject::Int(41)); + assert_eq!(feed_run_print(&mut repl, "helper()").unwrap(), MontyObject::Int(42)); +} + #[test] fn repl_abandon_resolve_futures_preserves_globals_and_metadata() { let (repl, _) = init_repl(""); @@ -354,6 +444,36 @@ fn repl_abandon_resolve_futures_preserves_globals_and_metadata() { assert_eq!(feed_run_print(&mut repl, "helper()").unwrap(), MontyObject::Int(42)); } +#[test] +fn repl_resolve_futures_unknown_call_id_preserves_globals_and_metadata() { + let (repl, _) = init_repl(""); + + let progress = repl + .feed_start( + "captured = 41\ndef helper():\n return captured + 1\nasync def main():\n value = await foo()\n return value\nawait main()", + vec![], + PrintWriter::Stdout, + ) + .unwrap(); + let call = progress.into_function_call().expect("expected function call"); + let progress = call.resume_pending(PrintWriter::Stdout).unwrap(); + let state = progress.into_resolve_futures().expect("expected resolve futures"); + let expected_pending = state.pending_call_ids().to_vec(); + + let err = state + .resume( + vec![(9999, ExtFunctionResult::Return(MontyObject::Int(1)))], + PrintWriter::Stdout, + ) + .expect_err("expected unknown call_id error"); + let ReplStartError { mut repl, error } = *err; + let expected_message = format!("unknown call_id 9999, expected one of: {expected_pending:?}"); + assert_runtime_error_exception(&error, &expected_message); + + assert_eq!(feed_run_print(&mut repl, "captured").unwrap(), MontyObject::Int(41)); + assert_eq!(feed_run_print(&mut repl, "helper()").unwrap(), MontyObject::Int(42)); +} + #[test] fn repl_start_runtime_error_during_external_call_preserves_repl_state() { // An external function returns an error, which should come back as ReplStartError