From a992b56ce57a39de8f86b2c91c27eda53ae9ece6 Mon Sep 17 00:00:00 2001 From: proboscis Date: Sat, 14 Mar 2026 16:49:10 +0900 Subject: [PATCH 1/2] Add GIL cleanup regression tests --- .semgrep.yaml | 41 ++++++++ .../tests/test_gil_cleanup_architecture.py | 96 +++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 packages/doeff-vm-core/tests/test_gil_cleanup_architecture.py diff --git a/.semgrep.yaml b/.semgrep.yaml index 0759a8d3..088b9d3e 100644 --- a/.semgrep.yaml +++ b/.semgrep.yaml @@ -1846,6 +1846,47 @@ rules: languages: [rust] severity: ERROR + - id: vm-gil-cleanup-no-caller-attach-on-kleisli-apply + pattern-either: + - pattern: Python::attach(|$PY| $KLEISLI.apply($PY, $ARGS)) + - pattern: Python::attach(|$PY| $KLEISLI.apply_with_run_token($PY, $ARGS, $RUN_TOKEN)) + message: | + GIL ownership belongs inside Kleisli implementations. + Call kleisli.apply(...) / apply_with_run_token(...) directly without forcing Python::attach + at the call site. Ref: vm-perf-gil-cleanup. + languages: [rust] + severity: ERROR + paths: + include: + - /packages/doeff-vm-core/src/** + + - id: vm-gil-cleanup-no-caller-attach-on-ir-stream-program + pattern-either: + - pattern: | + Python::attach(|$PY| { + ... + $PROGRAM.start($PY, $EFFECT, $K, $STORE, $SCOPE) + }) + - pattern: | + Python::attach(|$PY| { + ... + $PROGRAM.resume($VALUE, $STORE, $SCOPE) + }) + - pattern: | + Python::attach(|$PY| { + ... + $PROGRAM.throw($EXC, $STORE, $SCOPE) + }) + message: | + IRStreamProgram implementations must acquire the GIL internally when needed. + Do not wrap start/resume/throw in Python::attach at the RustKleisliStream call site. + Ref: vm-perf-gil-cleanup. + languages: [rust] + severity: ERROR + paths: + include: + - /packages/doeff-vm-core/src/kleisli.rs + - id: no-getattr-in-classify-yielded patterns: - pattern-inside: | diff --git a/packages/doeff-vm-core/tests/test_gil_cleanup_architecture.py b/packages/doeff-vm-core/tests/test_gil_cleanup_architecture.py new file mode 100644 index 00000000..fa3d6c8a --- /dev/null +++ b/packages/doeff-vm-core/tests/test_gil_cleanup_architecture.py @@ -0,0 +1,96 @@ +from pathlib import Path +import re + + +CORE_ROOT = Path(__file__).resolve().parents[1] +KLEISLI_RS = CORE_ROOT / "src/kleisli.rs" +HANDLER_RS = CORE_ROOT / "src/handler.rs" +VM_DISPATCH_RS = CORE_ROOT / "src/vm/dispatch.rs" +VM_STEP_RS = CORE_ROOT / "src/vm/step.rs" +VM_TRACE_RS = CORE_ROOT / "src/vm/vm_trace.rs" + + +def _runtime_source(path: Path) -> str: + source = path.read_text(encoding="utf-8") + return source.split("#[cfg(test)]", 1)[0] + + +def _block(source: str, token: str) -> str: + start = source.find(token) + assert start != -1, f"missing block token: {token}" + + brace = source.find("{", start) + assert brace != -1, f"missing opening brace for block: {token}" + + depth = 0 + for index in range(brace, len(source)): + char = source[index] + if char == "{": + depth += 1 + elif char == "}": + depth -= 1 + if depth == 0: + return source[start : index + 1] + + raise AssertionError(f"unterminated block: {token}") + + +def test_kleisli_apply_signatures_do_not_require_python_at_call_site() -> None: + source = _runtime_source(KLEISLI_RS) + block = _block(source, "pub trait Kleisli") + + assert "fn apply(&self, py: Python<'_>, args: Vec)" not in block + assert "py: Python<'_>" not in block + assert re.search(r"fn apply\(&self,\s*args: Vec\)", block) + assert re.search( + r"fn apply_with_run_token\(\s*&self,\s*args: Vec,\s*run_token: Option", + block, + ) + + +def test_ir_stream_program_start_no_longer_requires_python_at_call_site() -> None: + source = _runtime_source(HANDLER_RS) + block = _block(source, "pub trait IRStreamProgram") + + assert "_py: Python<'_>" not in block + assert "py: Python<'_>" not in block + assert re.search( + r"fn start\(\s*&mut self,\s*effect: DispatchEffect,\s*k: Continuation,", + block, + ) + + +def test_step_kleisli_calls_do_not_force_python_attach() -> None: + source = _runtime_source(VM_STEP_RS) + + assert "Python::attach(|py| kleisli.apply_with_run_token(py, args_values, run_token))" not in source + assert source.count("kleisli.apply_with_run_token(args_values, run_token)") >= 2 + + +def test_rust_kleisli_stream_does_not_wrap_program_callbacks_in_python_attach() -> None: + source = _runtime_source(KLEISLI_RS) + block = _block(source, "impl IRStream for RustKleisliStream") + + assert "Python::attach(|py|" not in block + assert "Python::attach(|_py|" not in block + + +def test_dispatch_reuses_materialized_effect_object_for_handler_invocation() -> None: + dispatch_source = _runtime_source(VM_DISPATCH_RS) + start_dispatch = _block( + dispatch_source, + "pub fn start_dispatch(&mut self, effect: DispatchEffect) -> Result", + ) + + assert re.search(r"invoke_kleisli_handler_expr\(\s*handler,\s*effect_obj,", start_dispatch) + assert not re.search( + r"let\s+effect_obj\s*=\s*Python::attach\(\|py\|\s*dispatch_to_pyobject\(py,\s*&effect\)", + start_dispatch, + ) + + +def test_handler_expr_builder_does_not_reconvert_dispatch_effect() -> None: + source = _runtime_source(VM_TRACE_RS) + block = _block(source, "pub(super) fn invoke_kleisli_handler_expr(") + + assert "dispatch_to_pyobject" not in block From cab31ca41d75be4cc15fd7f3a096def2c716c17f Mon Sep 17 00:00:00 2001 From: proboscis Date: Sat, 14 Mar 2026 17:28:53 +0900 Subject: [PATCH 2/2] Clean up VM GIL handoff --- .../doeff-core-effects/src/effects/mod.rs | 5 +- .../doeff-core-effects/src/handlers/mod.rs | 28 ++---- packages/doeff-core-effects/src/lib.rs | 4 +- .../doeff-core-effects/src/scheduler/mod.rs | 61 +++++------- packages/doeff-vm-core/src/handler.rs | 10 +- packages/doeff-vm-core/src/kleisli.rs | 77 +++++++-------- packages/doeff-vm-core/src/segment.rs | 3 +- packages/doeff-vm-core/src/value.rs | 3 +- packages/doeff-vm-core/src/vm/dispatch.rs | 99 ++++++++++--------- packages/doeff-vm-core/src/vm/handler.rs | 4 +- packages/doeff-vm-core/src/vm/step.rs | 5 +- packages/doeff-vm-core/src/vm/vm_trace.rs | 34 +++++-- .../tests/test_gil_cleanup_architecture.py | 4 +- packages/doeff-vm/src/pyvm.rs | 2 +- tests/core/test_sa001_spec_gaps.py | 8 +- tests/core/test_spec_gaps.py | 9 +- ...state_handler_unwrap_invariant_messages.py | 10 +- 17 files changed, 185 insertions(+), 181 deletions(-) diff --git a/packages/doeff-core-effects/src/effects/mod.rs b/packages/doeff-core-effects/src/effects/mod.rs index 5f34b1e4..eb661f54 100644 --- a/packages/doeff-core-effects/src/effects/mod.rs +++ b/packages/doeff-core-effects/src/effects/mod.rs @@ -938,7 +938,10 @@ pub fn register_effect_classes(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_class::()?; - m.add("TaskCancelledError", m.py().get_type::())?; + m.add( + "TaskCancelledError", + m.py().get_type::(), + )?; m.add_class::()?; m.add_class::()?; m.add_class::()?; diff --git a/packages/doeff-core-effects/src/handlers/mod.rs b/packages/doeff-core-effects/src/handlers/mod.rs index fdec7a10..bb233919 100644 --- a/packages/doeff-core-effects/src/handlers/mod.rs +++ b/packages/doeff-core-effects/src/handlers/mod.rs @@ -367,7 +367,6 @@ impl AwaitHandlerProgram { impl IRStreamProgram for AwaitHandlerProgram { fn start( &mut self, - _py: Python<'_>, effect: DispatchEffect, k: Continuation, _store: &mut RustStore, @@ -541,7 +540,6 @@ impl StateHandlerProgram { impl IRStreamProgram for StateHandlerProgram { fn start( &mut self, - _py: Python<'_>, effect: DispatchEffect, k: Continuation, store: &mut RustStore, @@ -641,9 +639,10 @@ impl IRStreamProgram for StateHandlerProgram { } // Modify case: store modifier result but resume caller with OLD value. // SPEC-008 L1271: Modify is read-then-modify, returns the old value. - let key = self.pending_key.take().expect( - "StateHandler Modify invariant violated: pending key missing during resume", - ); + let key = self + .pending_key + .take() + .expect("StateHandler Modify invariant violated: pending key missing during resume"); let continuation = self.pending_k.take().expect( "StateHandler Modify invariant violated: pending continuation missing during resume", ); @@ -1020,7 +1019,6 @@ impl LazyAskHandlerProgram { impl IRStreamProgram for LazyAskHandlerProgram { fn start( &mut self, - _py: Python<'_>, effect: DispatchEffect, k: Continuation, store: &mut RustStore, @@ -1310,7 +1308,6 @@ impl ReaderHandlerProgram { impl IRStreamProgram for ReaderHandlerProgram { fn start( &mut self, - _py: Python<'_>, effect: DispatchEffect, k: Continuation, store: &mut RustStore, @@ -1446,7 +1443,6 @@ impl WriterHandlerProgram { impl IRStreamProgram for WriterHandlerProgram { fn start( &mut self, - _py: Python<'_>, effect: DispatchEffect, k: Continuation, store: &mut RustStore, @@ -1614,7 +1610,6 @@ impl ResultSafeHandlerProgram { impl IRStreamProgram for ResultSafeHandlerProgram { fn start( &mut self, - _py: Python<'_>, effect: DispatchEffect, k: Continuation, _store: &mut RustStore, @@ -1765,7 +1760,6 @@ impl std::fmt::Debug for DoubleCallHandlerProgram { impl IRStreamProgram for DoubleCallHandlerProgram { fn start( &mut self, - _py: Python<'_>, effect: DispatchEffect, k: Continuation, _store: &mut RustStore, @@ -2022,14 +2016,8 @@ mod tests { let obj = locals.get_item("obj").unwrap().unwrap().unbind(); let effect = Effect::from_shared(PyShared::new(obj)); - let step = IRStreamProgram::start( - &mut program, - py, - effect, - continuation, - &mut store, - &mut scope, - ); + let step = + IRStreamProgram::start(&mut program, effect, continuation, &mut store, &mut scope); assert!(matches!( step, IRStreamStep::Yield(DoCtrl::EvalInScope { .. }) @@ -2374,7 +2362,7 @@ mod tests { let ok_program = ResultSafeHandlerFactory.create_program(); let start_step = { let mut guard = ok_program.lock().unwrap(); - guard.start(py, effect.clone(), k.clone(), &mut store, &mut scope) + guard.start(effect.clone(), k.clone(), &mut store, &mut scope) }; assert!(matches!( start_step, @@ -2402,7 +2390,7 @@ mod tests { let err_program = ResultSafeHandlerFactory.create_program(); let _ = { let mut guard = err_program.lock().unwrap(); - guard.start(py, effect, k, &mut store, &mut scope) + guard.start(effect, k, &mut store, &mut scope) }; let err_step = { diff --git a/packages/doeff-core-effects/src/lib.rs b/packages/doeff-core-effects/src/lib.rs index 95284325..0fcf3e36 100644 --- a/packages/doeff-core-effects/src/lib.rs +++ b/packages/doeff-core-effects/src/lib.rs @@ -63,8 +63,10 @@ pub mod effect { } pub mod handler { - pub use doeff_vm_core::{IRStreamFactory, IRStreamFactoryRef, IRStreamProgram, IRStreamProgramRef}; pub use crate::handlers::*; + pub use doeff_vm_core::{ + IRStreamFactory, IRStreamFactoryRef, IRStreamProgram, IRStreamProgramRef, + }; } pub fn register_all(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> { diff --git a/packages/doeff-core-effects/src/scheduler/mod.rs b/packages/doeff-core-effects/src/scheduler/mod.rs index 33a21cdd..79247c93 100644 --- a/packages/doeff-core-effects/src/scheduler/mod.rs +++ b/packages/doeff-core-effects/src/scheduler/mod.rs @@ -20,8 +20,8 @@ use crate::effect::{ dispatch_from_shared, dispatch_into_python, dispatch_ref_as_python, make_execution_context_object, DispatchEffect, PyAcquireSemaphore, PyCancelEffect, PyCompletePromise, PyCreateExternalPromise, PyCreatePromise, PyCreateSemaphore, PyFailPromise, - PyGather, PyGetExecutionContext, PyRace, PyReleaseSemaphore, PySemaphore, PySpawn, PyTaskCompleted, - TaskCancelledError, + PyGather, PyGetExecutionContext, PyRace, PyReleaseSemaphore, PySemaphore, PySpawn, + PyTaskCompleted, TaskCancelledError, }; use crate::error::VMError; use crate::handler::{IRStreamFactory, IRStreamProgram, IRStreamProgramRef}; @@ -528,10 +528,13 @@ pub fn debug_semaphore_exists(semaphore_id: u64) -> bool { registry .iter() .filter_map(|(state_id, weak_state)| { - weak_state.upgrade().map(|state| (*state_id, state)).or_else(|| { - stale_state_ids.push(*state_id); - None - }) + weak_state + .upgrade() + .map(|state| (*state_id, state)) + .or_else(|| { + stale_state_ids.push(*state_id); + None + }) }) .collect() }; @@ -559,7 +562,10 @@ pub fn notify_semaphore_handle_dropped(state_id: u64, semaphore_id: u64) { let mut notifications = semaphore_drop_notifications() .lock() .expect("Scheduler lock poisoned"); - notifications.entry(state_id).or_default().push(semaphore_id); + notifications + .entry(state_id) + .or_default() + .push(semaphore_id); } fn parse_task_completed_result( @@ -586,7 +592,9 @@ fn parse_task_completed_result( } fn extract_semaphore_id(obj: &Bound<'_, PyAny>) -> Option { - obj.extract::>().ok().map(|semaphore| semaphore.id) + obj.extract::>() + .ok() + .map(|semaphore| semaphore.id) } fn is_internal_source_file(source_file: &str) -> bool { @@ -910,12 +918,12 @@ fn make_python_semaphore_value(semaphore_id: u64, state_id: u64) -> Result bool { - self.waiters - .iter() - .any(|(item, waiters)| { - matches!(item, Waitable::ExternalPromise(_)) && !waiters.is_empty() - }) + self.waiters.iter().any(|(item, waiters)| { + matches!(item, Waitable::ExternalPromise(_)) && !waiters.is_empty() + }) } fn parse_external_completion_item( @@ -2661,7 +2667,6 @@ impl SchedulerProgram { impl IRStreamProgram for SchedulerProgram { fn start( &mut self, - _py: Python<'_>, effect: DispatchEffect, k_user: Continuation, store: &mut RustStore, @@ -3815,7 +3820,6 @@ mod tests { let step = IRStreamProgram::start( &mut program, - py, complete, idle_k.clone(), &mut store, @@ -4045,7 +4049,6 @@ mod tests { let first_step = IRStreamProgram::start( &mut program, - py, complete, idle_driver_k.clone(), &mut store, @@ -4154,7 +4157,6 @@ mod tests { let resolver_step = IRStreamProgram::start( &mut resolver_program, - py, complete, resolver_k.clone(), &mut store, @@ -4298,7 +4300,6 @@ mod tests { ); let first_step = IRStreamProgram::start( &mut program, - py, wake_normal, idle_k.clone(), &mut store, @@ -4325,7 +4326,6 @@ mod tests { ); let second_step = IRStreamProgram::start( &mut program, - py, wake_high, normal_k.clone(), &mut store, @@ -4413,14 +4413,8 @@ mod tests { .into_any(); let fail = make_fail_promise_effect(py, promise_obj, error_obj); - let step = IRStreamProgram::start( - &mut program, - py, - fail, - idle_k.clone(), - &mut store, - &mut _scope, - ); + let step = + IRStreamProgram::start(&mut program, fail, idle_k.clone(), &mut store, &mut _scope); assert!( matches!( &step, @@ -4996,7 +4990,6 @@ mod tests { let step = IRStreamProgram::start( &mut program, - py, make_acquire_semaphore_effect(py, make_semaphore_object(py, semaphore_id)), driver_k, &mut store, diff --git a/packages/doeff-vm-core/src/handler.rs b/packages/doeff-vm-core/src/handler.rs index f7e3ead4..8ef0244a 100644 --- a/packages/doeff-vm-core/src/handler.rs +++ b/packages/doeff-vm-core/src/handler.rs @@ -2,8 +2,6 @@ use std::sync::{Arc, Mutex}; -use pyo3::prelude::*; - use crate::continuation::Continuation; use crate::do_ctrl::DoCtrl; use crate::effect::DispatchEffect; @@ -20,7 +18,6 @@ use crate::value::Value; pub trait IRStreamProgram: std::fmt::Debug + Send { fn start( &mut self, - py: Python<'_>, effect: DispatchEffect, k: Continuation, store: &mut RustStore, @@ -74,13 +71,12 @@ impl Kleisli for T where T: IRStreamFactory + Clone + std::fmt::Debug + Send + Sync + 'static, { - fn apply(&self, py: Python<'_>, args: Vec) -> Result { - self.apply_with_run_token(py, args, None) + fn apply(&self, args: Vec) -> Result { + self.apply_with_run_token(args, None) } fn apply_with_run_token( &self, - py: Python<'_>, args: Vec, run_token: Option, ) -> Result { @@ -88,7 +84,7 @@ where Arc::new(self.clone()), ::handler_name(self).to_string(), ); - kleisli.apply_with_run_token(py, args, run_token) + kleisli.apply_with_run_token(args, run_token) } fn debug_info(&self) -> KleisliDebugInfo { diff --git a/packages/doeff-vm-core/src/kleisli.rs b/packages/doeff-vm-core/src/kleisli.rs index c03b958a..57a5a0ac 100644 --- a/packages/doeff-vm-core/src/kleisli.rs +++ b/packages/doeff-vm-core/src/kleisli.rs @@ -37,19 +37,18 @@ pub struct KleisliDebugInfo { /// SPEC-VM-017 R1-A. pub trait Kleisli: std::fmt::Debug + Send + Sync { /// Apply the arrow to arguments, producing a DoCtrl to evaluate. - fn apply(&self, py: Python<'_>, args: Vec) -> Result; + fn apply(&self, args: Vec) -> Result; /// Apply with optional VM run token for run-scoped handler state. /// /// Non-Rust handlers ignore the run token and defer to `apply`. fn apply_with_run_token( &self, - py: Python<'_>, args: Vec, run_token: Option, ) -> Result { let _ = run_token; - self.apply(py, args) + self.apply(args) } /// Debug metadata for tracing/error reporting. @@ -101,17 +100,16 @@ impl IdentityKleisli { } impl Kleisli for IdentityKleisli { - fn apply(&self, py: Python<'_>, args: Vec) -> Result { - self.inner.apply(py, args) + fn apply(&self, args: Vec) -> Result { + self.inner.apply(args) } fn apply_with_run_token( &self, - py: Python<'_>, args: Vec, run_token: Option, ) -> Result { - self.inner.apply_with_run_token(py, args, run_token) + self.inner.apply_with_run_token(args, run_token) } fn debug_info(&self) -> KleisliDebugInfo { @@ -364,10 +362,8 @@ impl PyKleisli { | Value::List(_) => value.to_pyobject(py).map_err(Self::map_pyerr), } } -} -impl Kleisli for PyKleisli { - fn apply(&self, py: Python<'_>, args: Vec) -> Result { + fn apply_attached(&self, py: Python<'_>, args: Vec) -> Result { let arg_values: Vec> = args .iter() .map(|value| Self::runtime_arg_to_pyobject(py, value)) @@ -429,6 +425,12 @@ impl Kleisli for PyKleisli { metadata: Some(metadata), }) } +} + +impl Kleisli for PyKleisli { + fn apply(&self, args: Vec) -> Result { + Python::attach(|py| self.apply_attached(py, args)) + } fn debug_info(&self) -> KleisliDebugInfo { KleisliDebugInfo { @@ -471,8 +473,8 @@ impl DgfnKleisli { } impl Kleisli for DgfnKleisli { - fn apply(&self, py: Python<'_>, args: Vec) -> Result { - self.inner.apply(py, args) + fn apply(&self, args: Vec) -> Result { + self.inner.apply(args) } fn debug_info(&self) -> KleisliDebugInfo { @@ -524,19 +526,21 @@ impl PyCallableKleisli { } impl Kleisli for PyCallableKleisli { - fn apply(&self, py: Python<'_>, args: Vec) -> Result { - let arg_values: Vec> = args - .iter() - .map(|value| PyKleisli::runtime_arg_to_pyobject(py, value)) - .collect::, VMError>>()?; - let arg_tuple = PyTuple::new(py, &arg_values).map_err(PyKleisli::map_pyerr)?; - let produced = self - .func - .bind(py) - .call1(arg_tuple) - .map_err(PyKleisli::map_pyerr)?; - Ok(DoCtrl::Pure { - value: Value::Python(PyShared::new(produced.unbind())), + fn apply(&self, args: Vec) -> Result { + Python::attach(|py| { + let arg_values: Vec> = args + .iter() + .map(|value| PyKleisli::runtime_arg_to_pyobject(py, value)) + .collect::, VMError>>()?; + let arg_tuple = PyTuple::new(py, &arg_values).map_err(PyKleisli::map_pyerr)?; + let produced = self + .func + .bind(py) + .call1(arg_tuple) + .map_err(PyKleisli::map_pyerr)?; + Ok(DoCtrl::Pure { + value: Value::Python(PyShared::new(produced.unbind())), + }) }) } @@ -600,15 +604,11 @@ impl IRStream for RustKleisliStream { if let (Some(effect), Some(continuation)) = (self.start_effect.take(), self.start_continuation.take()) { - return Python::attach(|py| { - let mut guard = self.program.lock().expect("Rust program lock poisoned"); - guard.start(py, effect, continuation, store, scope) - }); - } - Python::attach(|_py| { let mut guard = self.program.lock().expect("Rust program lock poisoned"); - guard.resume(value, store, scope) - }) + return guard.start(effect, continuation, store, scope); + } + let mut guard = self.program.lock().expect("Rust program lock poisoned"); + guard.resume(value, store, scope) } fn throw( @@ -617,21 +617,18 @@ impl IRStream for RustKleisliStream { store: &mut RustStore, scope: &mut ScopeStore, ) -> IRStreamStep { - Python::attach(|_py| { - let mut guard = self.program.lock().expect("Rust program lock poisoned"); - guard.throw(exc, store, scope) - }) + let mut guard = self.program.lock().expect("Rust program lock poisoned"); + guard.throw(exc, store, scope) } } impl Kleisli for RustKleisli { - fn apply(&self, py: Python<'_>, args: Vec) -> Result { - self.apply_with_run_token(py, args, None) + fn apply(&self, args: Vec) -> Result { + self.apply_with_run_token(args, None) } fn apply_with_run_token( &self, - _py: Python<'_>, args: Vec, run_token: Option, ) -> Result { diff --git a/packages/doeff-vm-core/src/segment.rs b/packages/doeff-vm-core/src/segment.rs index f0ad4a1c..5a4e853a 100644 --- a/packages/doeff-vm-core/src/segment.rs +++ b/packages/doeff-vm-core/src/segment.rs @@ -155,7 +155,6 @@ impl Segment { #[cfg(test)] mod tests { use super::*; - use pyo3::Python; use crate::do_ctrl::DoCtrl; use crate::error::VMError; @@ -165,7 +164,7 @@ mod tests { struct DummyKleisli; impl Kleisli for DummyKleisli { - fn apply(&self, _py: Python<'_>, _args: Vec) -> Result { + fn apply(&self, _args: Vec) -> Result { unreachable!("test dummy should never be invoked") } diff --git a/packages/doeff-vm-core/src/value.rs b/packages/doeff-vm-core/src/value.rs index 28d2a734..4255bbfd 100644 --- a/packages/doeff-vm-core/src/value.rs +++ b/packages/doeff-vm-core/src/value.rs @@ -699,7 +699,6 @@ impl From<()> for Value { #[cfg(test)] mod tests { use super::*; - use pyo3::Python; use crate::do_ctrl::DoCtrl; use crate::error::VMError; @@ -709,7 +708,7 @@ mod tests { struct DummyKleisli; impl Kleisli for DummyKleisli { - fn apply(&self, _py: Python<'_>, _args: Vec) -> Result { + fn apply(&self, _args: Vec) -> Result { unreachable!("test dummy should never be invoked") } diff --git a/packages/doeff-vm-core/src/vm/dispatch.rs b/packages/doeff-vm-core/src/vm/dispatch.rs index 6fbd58a6..8bf82824 100644 --- a/packages/doeff-vm-core/src/vm/dispatch.rs +++ b/packages/doeff-vm-core/src/vm/dispatch.rs @@ -921,14 +921,7 @@ impl VM { handler_chain: &[Marker], effect: &DispatchEffect, ) -> Result<(usize, Marker, KleisliRef), VMError> { - let effect_obj = - Python::attach(|py| dispatch_to_pyobject(py, effect).map(|obj| obj.unbind())).map_err( - |err| { - VMError::python_error(format!( - "failed to convert dispatch effect to Python object: {err}" - )) - }, - )?; + let mut effect_obj = None; for (idx, marker) in handler_chain.iter().copied().enumerate() { let Some((prompt_seg_id, handler, types)) = self.find_prompt_boundary_by_marker(marker) else { @@ -938,9 +931,10 @@ impl VM { idx ))); }; - if handler.can_handle(effect)? - && self - .should_invoke_handler( + if handler.can_handle(effect)? { + let should_invoke = if types.is_some() { + let effect_obj = Self::ensure_dispatch_effect_object(effect, &mut effect_obj)?; + self.should_invoke_handler( &HandlerChainEntry { marker, prompt_seg_id, @@ -954,8 +948,12 @@ impl VM { "failed to evaluate WithHandler type filter: {err:?}" )) })? - { - return Ok((idx, marker, handler)); + } else { + true + }; + if should_invoke { + return Ok((idx, marker, handler)); + } } } Err(VMError::no_matching_handler(effect.clone())) @@ -1030,13 +1028,7 @@ impl VM { if let Some(seg) = self.current_segment_mut() { seg.pending_error_context = None; } - let effect_obj = - Python::attach(|py| dispatch_to_pyobject(py, &effect).map(|obj| obj.unbind())) - .map_err(|err| { - VMError::python_error(format!( - "failed to convert dispatch effect to Python object: {err}" - )) - })?; + let mut effect_obj = None; let mut selected: Option<(usize, Marker, SegmentId, KleisliRef)> = None; let mut first_type_filtered_skip: Option<(usize, Marker, SegmentId, KleisliRef)> = None; @@ -1071,13 +1063,18 @@ impl VM { }); if handler.can_handle(&effect)? { - let should_invoke = self - .should_invoke_handler_types(types.as_ref(), &effect_obj) - .map_err(|err| { - VMError::python_error(format!( - "failed to evaluate WithHandler type filter: {err:?}" - )) - })?; + let should_invoke = if types.is_some() { + let effect_obj = + Self::ensure_dispatch_effect_object(&effect, &mut effect_obj)?; + self.should_invoke_handler_types(types.as_ref(), &effect_obj) + .map_err(|err| { + VMError::python_error(format!( + "failed to evaluate WithHandler type filter: {err:?}" + )) + })? + } else { + true + }; if should_invoke { if selected.is_none() { selected = Some(( @@ -1196,7 +1193,8 @@ impl VM { if handler.py_identity().is_some() { self.register_continuation(k_user.clone()); } - let ir_node = Self::invoke_kleisli_handler_expr(handler, effect, k_user)?; + let effect_obj = Self::ensure_dispatch_effect_object(&effect, &mut effect_obj)?; + let ir_node = Self::invoke_kleisli_handler_expr(handler, effect_obj, k_user)?; Ok(self.evaluate(ir_node)) } @@ -1688,15 +1686,7 @@ impl VM { ForwardKind::Delegate | ForwardKind::Pass => {} } - let effect_obj = - match Python::attach(|py| dispatch_to_pyobject(py, &effect).map(|obj| obj.unbind())) { - Ok(obj) => obj, - Err(err) => { - return StepEvent::Error(VMError::python_error(format!( - "failed to convert dispatch effect to Python object: {err}" - ))) - } - }; + let mut effect_obj = None; for entry in visible_chain { let handler = entry.handler.clone(); @@ -1705,13 +1695,22 @@ impl VM { Err(err) => return StepEvent::Error(err), }; if can_handle { - let should_invoke = match self.should_invoke_handler(&entry, &effect_obj) { - Ok(value) => value, - Err(err) => { - return StepEvent::Error(VMError::python_error(format!( - "failed to evaluate WithHandler type filter: {err:?}" - ))) + let should_invoke = if entry.types.is_some() { + let effect_obj = + match Self::ensure_dispatch_effect_object(&effect, &mut effect_obj) { + Ok(effect_obj) => effect_obj, + Err(err) => return StepEvent::Error(err), + }; + match self.should_invoke_handler(&entry, &effect_obj) { + Ok(value) => value, + Err(err) => { + return StepEvent::Error(VMError::python_error(format!( + "failed to evaluate WithHandler type filter: {err:?}" + ))) + } } + } else { + true }; if !should_invoke { continue; @@ -1781,14 +1780,16 @@ impl VM { if handler.py_identity().is_some() { self.register_continuation(next_k.clone()); } - let ir_node = match Self::invoke_kleisli_handler_expr( - handler, - effect.clone(), - next_k.clone(), - ) { - Ok(node) => node, + let effect_obj = match Self::ensure_dispatch_effect_object(&effect, &mut effect_obj) + { + Ok(effect_obj) => effect_obj, Err(err) => return StepEvent::Error(err), }; + let ir_node = + match Self::invoke_kleisli_handler_expr(handler, effect_obj, next_k.clone()) { + Ok(node) => node, + Err(err) => return StepEvent::Error(err), + }; return self.evaluate(ir_node); } } diff --git a/packages/doeff-vm-core/src/vm/handler.rs b/packages/doeff-vm-core/src/vm/handler.rs index 5036a90a..10050159 100644 --- a/packages/doeff-vm-core/src/vm/handler.rs +++ b/packages/doeff-vm-core/src/vm/handler.rs @@ -165,7 +165,7 @@ impl VM { pub(super) fn should_invoke_handler( &self, entry: &HandlerChainEntry, - effect_obj: &Py, + effect_obj: &PyShared, ) -> Result { self.should_invoke_handler_types(entry.types.as_ref(), effect_obj) } @@ -173,7 +173,7 @@ impl VM { pub(super) fn should_invoke_handler_types( &self, types: Option<&Vec>, - effect_obj: &Py, + effect_obj: &PyShared, ) -> Result { let Some(types) = types else { return Ok(true); diff --git a/packages/doeff-vm-core/src/vm/step.rs b/packages/doeff-vm-core/src/vm/step.rs index 43f49041..441faa4a 100644 --- a/packages/doeff-vm-core/src/vm/step.rs +++ b/packages/doeff-vm-core/src/vm/step.rs @@ -1563,7 +1563,7 @@ impl VM { let run_token = self.current_run_token(); let result = - Python::attach(|py| kleisli.apply_with_run_token(py, args_values, run_token)); + Python::attach(|_py| kleisli.apply_with_run_token(args_values, run_token)); return match result { Ok(doctrl) => self.evaluate(doctrl), Err(vm_err) => StepEvent::Error(vm_err), @@ -1654,8 +1654,7 @@ impl VM { } let run_token = self.current_run_token(); - let result = - Python::attach(|py| kleisli.apply_with_run_token(py, args_values, run_token)); + let result = kleisli.apply_with_run_token(args_values, run_token); return match result { Ok(doctrl) => { if let DoCtrl::IRStream { stream, metadata } = doctrl { diff --git a/packages/doeff-vm-core/src/vm/vm_trace.rs b/packages/doeff-vm-core/src/vm/vm_trace.rs index aa436cae..5e6414ef 100644 --- a/packages/doeff-vm-core/src/vm/vm_trace.rs +++ b/packages/doeff-vm-core/src/vm/vm_trace.rs @@ -90,17 +90,35 @@ impl VM { (info.name, kind, info.file, info.line) } - pub(super) fn invoke_kleisli_handler_expr( - kleisli: KleisliRef, - effect: DispatchEffect, - continuation: Continuation, - ) -> Result { - let effect_obj = Python::attach(|py| dispatch_to_pyobject(py, &effect).map(|v| v.unbind())) + pub(super) fn materialize_dispatch_effect_object( + effect: &DispatchEffect, + ) -> Result { + Python::attach(|py| dispatch_to_pyobject(py, effect).map(|v| PyShared::new(v.unbind()))) .map_err(|err| { VMError::python_error(format!( "failed to convert dispatch effect to Python object: {err}" )) - })?; + }) + } + + pub(super) fn ensure_dispatch_effect_object( + effect: &DispatchEffect, + effect_obj: &mut Option, + ) -> Result { + if let Some(effect_obj) = effect_obj { + return Ok(effect_obj.clone()); + } + + let materialized = Self::materialize_dispatch_effect_object(effect)?; + *effect_obj = Some(materialized.clone()); + Ok(materialized) + } + + pub(super) fn invoke_kleisli_handler_expr( + kleisli: KleisliRef, + effect_obj: PyShared, + continuation: Continuation, + ) -> Result { let debug = kleisli.debug_info(); let metadata = CallMetadata::new( debug.name, @@ -116,7 +134,7 @@ impl VM { }), args: vec![ DoCtrl::Pure { - value: Value::Python(PyShared::new(effect_obj)), + value: Value::Python(effect_obj), }, DoCtrl::Pure { value: Value::Continuation(continuation), diff --git a/packages/doeff-vm-core/tests/test_gil_cleanup_architecture.py b/packages/doeff-vm-core/tests/test_gil_cleanup_architecture.py index fa3d6c8a..6b28b54b 100644 --- a/packages/doeff-vm-core/tests/test_gil_cleanup_architecture.py +++ b/packages/doeff-vm-core/tests/test_gil_cleanup_architecture.py @@ -60,10 +60,10 @@ def test_ir_stream_program_start_no_longer_requires_python_at_call_site() -> Non ) -def test_step_kleisli_calls_do_not_force_python_attach() -> None: +def test_step_kleisli_calls_use_new_kleisli_signature() -> None: source = _runtime_source(VM_STEP_RS) - assert "Python::attach(|py| kleisli.apply_with_run_token(py, args_values, run_token))" not in source + assert "kleisli.apply_with_run_token(py, args_values, run_token)" not in source assert source.count("kleisli.apply_with_run_token(args_values, run_token)") >= 2 diff --git a/packages/doeff-vm/src/pyvm.rs b/packages/doeff-vm/src/pyvm.rs index 4ddf4167..5cfab77a 100644 --- a/packages/doeff-vm/src/pyvm.rs +++ b/packages/doeff-vm/src/pyvm.rs @@ -2938,7 +2938,7 @@ mod tests { } impl Kleisli for DummyRustHandler { - fn apply(&self, _py: Python<'_>, _args: Vec) -> Result { + fn apply(&self, _args: Vec) -> Result { unreachable!("dummy test handler should never be applied") } diff --git a/tests/core/test_sa001_spec_gaps.py b/tests/core/test_sa001_spec_gaps.py index ef931c19..3d4a8755 100644 --- a/tests/core/test_sa001_spec_gaps.py +++ b/tests/core/test_sa001_spec_gaps.py @@ -335,14 +335,14 @@ def test_scheduler_not_placeholder(self): class TestSA001G15HandlerSigs: """G15: Handler trait sigs diverge.""" - def test_start_receives_py_and_bound(self): - """ASTStreamProgram::start must receive py: Python<'_>. SPEC-008 L1111.""" + def test_start_does_not_require_py_at_call_site(self): + """IRStreamProgram::start should no longer require py: Python<'_> at the call site.""" src = _read_rust("handler.rs") m = re.search(r"fn start\s*\(\s*&mut self,\s*([^)]*)\)", src, re.S) assert m, "Could not find ASTStreamProgram::start" params = m.group(1) - assert "py:" in params or "Python" in params, ( - f"start() missing py parameter: start(&mut self, {params})" + assert "py:" not in params and "Python" not in params, ( + f"start() should not require py parameter anymore: start(&mut self, {params})" ) diff --git a/tests/core/test_spec_gaps.py b/tests/core/test_spec_gaps.py index 409b3a53..4e4c0d42 100644 --- a/tests/core/test_spec_gaps.py +++ b/tests/core/test_spec_gaps.py @@ -276,13 +276,18 @@ class TestG22FrozenBases: def test_pyclass_declarations_include_frozen(self) -> None: """All three base class #[pyclass] macros must include 'frozen'.""" - pyvm_src = (ROOT / "packages" / "doeff-vm" / "src" / "pyvm.rs").read_text() + core_sources = "\n".join( + [ + (ROOT / "packages" / "doeff-vm-core" / "src" / "do_ctrl.rs").read_text(), + (ROOT / "packages" / "doeff-vm-core" / "src" / "effect.rs").read_text(), + ] + ) bases = ["DoExprBase", "EffectBase", "DoCtrlBase"] for name in bases: # Find the #[pyclass(...)] line preceding the struct with this name pattern = rf"#\[pyclass\(([^)]*)\)\]\s*pub struct Py{name}" - m = re.search(pattern, pyvm_src) + m = re.search(pattern, core_sources) assert m is not None, f"Could not find #[pyclass] for Py{name}" attrs = m.group(1) assert "frozen" in attrs, ( diff --git a/tests/core/test_state_handler_unwrap_invariant_messages.py b/tests/core/test_state_handler_unwrap_invariant_messages.py index c0fdf63c..4aebf0c7 100644 --- a/tests/core/test_state_handler_unwrap_invariant_messages.py +++ b/tests/core/test_state_handler_unwrap_invariant_messages.py @@ -1,4 +1,5 @@ from pathlib import Path +import re ROOT = Path(__file__).resolve().parents[2] @@ -18,9 +19,12 @@ def test_modify_resume_does_not_use_ambiguous_take_unwrap() -> None: def test_modify_resume_has_explicit_invariant_messages() -> None: source = _handler_source() - assert "self.pending_key.take().expect(" in source - assert "self.pending_k.take().expect(" in source - assert "self.pending_old_value.take().expect(" in source + assert re.search(r"self\s*\.\s*pending_key\s*\.\s*take\(\)\s*\.\s*expect\(", source) + assert re.search(r"self\s*\.\s*pending_k\s*\.\s*take\(\)\s*\.\s*expect\(", source) + assert re.search( + r"self\s*\.\s*pending_old_value\s*\.\s*take\(\)\s*\.\s*expect\(", + source, + ) assert "StateHandler Modify invariant violated: pending key missing during resume" in source assert "StateHandler Modify invariant violated: pending continuation missing during resume" in source assert "StateHandler Modify invariant violated: pending old_value missing during resume" in source