From 66a8ea7706295e454153c4270d783f080b55b086 Mon Sep 17 00:00:00 2001 From: Mine Starks <16928427+minestarks@users.noreply.github.com> Date: Fri, 19 Jan 2024 17:44:55 -0800 Subject: [PATCH] Separate Debugger into a distinct struct from Interpreter (#1039) The immediate, selfish motivation for this change is that I want to add a parameter to the `Interpreter` constructor (in #1038) that is really only relevant to debugger scenarios, and I didn't want to update every single call to `Interpreter::new`. But I think this is a good change regardless. Some methods and fields in `Interpreter` are only applicable to debugging, and the rest are only applicable to not-debugging. It's really two separate use cases and two separate sets of data. I wanted to make this distinction clearer. Very open to feedback and opinions, and suggestions for further cleanup. --- compiler/qsc/src/interpret.rs | 131 ++++++++------- .../{stepping_tests.rs => debugger_tests.rs} | 153 ++++++++---------- compiler/qsc/src/interpret/tests.rs | 15 +- npm/test/basics.js | 10 -- wasm/src/debug_service.rs | 49 +++--- 5 files changed, 173 insertions(+), 185 deletions(-) rename compiler/qsc/src/interpret/{stepping_tests.rs => debugger_tests.rs} (50%) diff --git a/compiler/qsc/src/interpret.rs b/compiler/qsc/src/interpret.rs index 1768ad2898..2db6e9e3a5 100644 --- a/compiler/qsc/src/interpret.rs +++ b/compiler/qsc/src/interpret.rs @@ -7,7 +7,7 @@ mod debug; mod tests; #[cfg(test)] -mod stepping_tests; +mod debugger_tests; pub use qsc_eval::{ debug::Frame, @@ -106,8 +106,6 @@ pub struct Interpreter { classical_seed: Option, /// The evaluator environment. env: Env, - /// The current state of the evaluator. - state: State, } #[allow(clippy::module_name_repetitions)] @@ -149,7 +147,6 @@ impl Interpreter { sim: SparseSim::new(), quantum_seed: None, classical_seed: None, - state: State::new(map_hir_package_to_fir(source_package_id), None), package: map_hir_package_to_fir(package_id), source_package: map_hir_package_to_fir(source_package_id), }) @@ -163,46 +160,6 @@ impl Interpreter { pub fn set_classical_seed(&mut self, seed: Option) { self.classical_seed = seed; } - - /// Loads the entry expression to the top of the evaluation stack. - /// This is needed for debugging so that when begging to debug with - /// a step action the system is already in the correct state. - /// # Errors - /// Returns a vector of errors if loading the entry point fails. - pub fn set_entry(&mut self) -> Result<(), Vec> { - let expr = self.get_entry_expr()?; - qsc_eval::eval_push_expr(&mut self.state, expr); - Ok(()) - } - - /// Resumes execution with specified `StepAction`. - /// # Errors - /// Returns a vector of errors if evaluating the entry point fails. - pub fn eval_step( - &mut self, - receiver: &mut impl Receiver, - breakpoints: &[StmtId], - step: StepAction, - ) -> Result> { - self.state - .eval( - &self.fir_store, - &mut self.env, - &mut self.sim, - receiver, - breakpoints, - step, - ) - .map_err(|(error, call_stack)| { - eval_error( - self.compiler.package_store(), - &self.fir_store, - call_stack, - error, - ) - }) - } - /// Executes the entry expression until the end of execution. /// # Errors /// Returns a vector of errors if evaluating the entry point fails. @@ -382,18 +339,72 @@ impl Interpreter { .lower_and_update_package(fir_package, &unit_addition.hir) } - fn source_package(&self) -> &CompileUnit { - self.compiler - .package_store() - .get(map_fir_package_to_hir(self.source_package)) - .expect("Could not load package") - } - fn next_line_label(&mut self) -> String { let label = format!("line_{}", self.lines); self.lines += 1; label } +} + +/// A debugger that enables step-by-step evaluation of code +/// and inspecting state in the interpreter. +pub struct Debugger { + interpreter: Interpreter, + /// The current state of the evaluator. + state: State, +} + +impl Debugger { + pub fn new( + sources: SourceMap, + capabilities: RuntimeCapabilityFlags, + ) -> Result> { + let interpreter = Interpreter::new(true, sources, PackageType::Exe, capabilities)?; + let source_package_id = interpreter.source_package; + Ok(Self { + interpreter, + state: State::new(source_package_id, None), + }) + } + + /// Loads the entry expression to the top of the evaluation stack. + /// This is needed for debugging so that when begging to debug with + /// a step action the system is already in the correct state. + /// # Errors + /// Returns a vector of errors if loading the entry point fails. + pub fn set_entry(&mut self) -> Result<(), Vec> { + let expr = self.interpreter.get_entry_expr()?; + qsc_eval::eval_push_expr(&mut self.state, expr); + Ok(()) + } + + /// Resumes execution with specified `StepAction`. + /// # Errors + /// Returns a vector of errors if evaluating the entry point fails. + pub fn eval_step( + &mut self, + receiver: &mut impl Receiver, + breakpoints: &[StmtId], + step: StepAction, + ) -> Result> { + self.state + .eval( + &self.interpreter.fir_store, + &mut self.interpreter.env, + &mut self.interpreter.sim, + receiver, + breakpoints, + step, + ) + .map_err(|(error, call_stack)| { + eval_error( + self.interpreter.compiler.package_store(), + &self.interpreter.fir_store, + call_stack, + error, + ) + }) + } #[must_use] pub fn get_stack_frames(&self) -> Vec { @@ -402,6 +413,7 @@ impl Interpreter { .iter() .map(|frame| { let callable = self + .interpreter .fir_store .get_global(frame.id) .expect("frame should exist"); @@ -412,6 +424,7 @@ impl Interpreter { }; let hir_package = self + .interpreter .compiler .package_store() .get(map_fir_package_to_hir(frame.id.package)) @@ -434,7 +447,7 @@ impl Interpreter { } pub fn capture_quantum_state(&mut self) -> (Vec<(BigUint, Complex)>, usize) { - self.sim.capture_quantum_state() + self.interpreter.sim.capture_quantum_state() } #[must_use] @@ -443,8 +456,9 @@ impl Interpreter { if let Some(source) = unit.sources.find_by_name(path) { let package = self + .interpreter .fir_store - .get(self.source_package) + .get(self.interpreter.source_package) .expect("package should have been lowered"); let mut collector = BreakpointCollector::new(&unit.sources, source.offset, package); collector.visit_package(package); @@ -466,12 +480,21 @@ impl Interpreter { #[must_use] pub fn get_locals(&self) -> Vec { - self.env + self.interpreter + .env .get_variables_in_top_frame() .into_iter() .filter(|v| !v.name.starts_with('@')) .collect() } + + fn source_package(&self) -> &CompileUnit { + self.interpreter + .compiler + .package_store() + .get(map_fir_package_to_hir(self.interpreter.source_package)) + .expect("Could not load package") + } } /// Wrapper function for `qsc_eval::eval` that handles error conversion. diff --git a/compiler/qsc/src/interpret/stepping_tests.rs b/compiler/qsc/src/interpret/debugger_tests.rs similarity index 50% rename from compiler/qsc/src/interpret/stepping_tests.rs rename to compiler/qsc/src/interpret/debugger_tests.rs index 415e498372..91cb3dc0dd 100644 --- a/compiler/qsc/src/interpret/stepping_tests.rs +++ b/compiler/qsc/src/interpret/debugger_tests.rs @@ -3,22 +3,21 @@ #![allow(clippy::needless_raw_string_hashes)] -use crate::interpret::Interpreter; +use crate::interpret::Debugger; use qsc_eval::{output::CursorReceiver, StepAction, StepResult}; use qsc_fir::fir::StmtId; use qsc_frontend::compile::{RuntimeCapabilityFlags, SourceMap}; -use qsc_passes::PackageType; use std::io::Cursor; -fn get_breakpoint_ids(interpreter: &Interpreter, path: &str) -> Vec { - let mut bps = interpreter.get_breakpoints(path); +fn get_breakpoint_ids(debugger: &Debugger, path: &str) -> Vec { + let mut bps = debugger.get_breakpoints(path); bps.sort_by_key(|f| f.id); let ids = bps.iter().map(|f| f.id.into()).collect::>(); ids } -fn expect_return(mut interpreter: Interpreter, expected: &str) { - let r = step_next(&mut interpreter, &[]); +fn expect_return(mut debugger: Debugger, expected: &str) { + let r = step_next(&mut debugger, &[]); match r.0 { Ok(StepResult::Return(value)) => assert_eq!(value.to_string(), expected), Ok(v) => panic!("Expected Return, got {v:?}"), @@ -26,8 +25,8 @@ fn expect_return(mut interpreter: Interpreter, expected: &str) { } } -fn expect_bp(interpreter: &mut Interpreter, ids: &[StmtId], expected_id: StmtId) { - let r = step_next(interpreter, ids); +fn expect_bp(debugger: &mut Debugger, ids: &[StmtId], expected_id: StmtId) { + let r = step_next(debugger, ids); match r.0 { Ok(StepResult::BreakpointHit(actual_id)) => assert!(actual_id == expected_id), Ok(v) => panic!("Expected BP, got {v:?}"), @@ -36,41 +35,41 @@ fn expect_bp(interpreter: &mut Interpreter, ids: &[StmtId], expected_id: StmtId) } fn step_in( - interpreter: &mut Interpreter, + debugger: &mut Debugger, breakpoints: &[StmtId], ) -> (Result>, String) { - step(interpreter, breakpoints, qsc_eval::StepAction::In) + step(debugger, breakpoints, qsc_eval::StepAction::In) } fn step_next( - interpreter: &mut Interpreter, + debugger: &mut Debugger, breakpoints: &[StmtId], ) -> (Result>, String) { - step(interpreter, breakpoints, qsc_eval::StepAction::Next) + step(debugger, breakpoints, qsc_eval::StepAction::Next) } fn step_out( - interpreter: &mut Interpreter, + debugger: &mut Debugger, breakpoints: &[StmtId], ) -> (Result>, String) { - step(interpreter, breakpoints, qsc_eval::StepAction::Out) + step(debugger, breakpoints, qsc_eval::StepAction::Out) } fn step( - interpreter: &mut Interpreter, + debugger: &mut Debugger, breakpoints: &[StmtId], step: StepAction, ) -> (Result>, String) { let mut cursor = Cursor::new(Vec::::new()); let mut receiver = CursorReceiver::new(&mut cursor); ( - interpreter.eval_step(&mut receiver, breakpoints, step), + debugger.eval_step(&mut receiver, breakpoints, step), receiver.dump(), ) } -fn expect_next(interpreter: &mut Interpreter) { - let result = step_next(interpreter, &[]); +fn expect_next(debugger: &mut Debugger) { + let result = step_next(debugger, &[]); match result.0 { Ok(StepResult::Next) => (), Ok(v) => panic!("Expected Next, got {v:?}"), @@ -78,8 +77,8 @@ fn expect_next(interpreter: &mut Interpreter) { } } -fn expect_in(interpreter: &mut Interpreter) { - let result = step_in(interpreter, &[]); +fn expect_in(debugger: &mut Debugger) { + let result = step_in(debugger, &[]); match result.0 { Ok(StepResult::StepIn) => (), Ok(v) => panic!("Expected StepIn, got {v:?}"), @@ -87,8 +86,8 @@ fn expect_in(interpreter: &mut Interpreter) { } } -fn expect_out(interpreter: &mut Interpreter) { - let result = step_out(interpreter, &[]); +fn expect_out(debugger: &mut Debugger) { + let result = step_out(debugger, &[]); match result.0 { Ok(StepResult::StepOut) => (), Ok(v) => panic!("Expected StepOut, got {v:?}"), @@ -97,7 +96,7 @@ fn expect_out(interpreter: &mut Interpreter) { } #[cfg(test)] -mod given_interpreter_with_sources { +mod given_debugger { use super::*; static STEPPING_SOURCE: &str = r#" @@ -127,96 +126,76 @@ mod given_interpreter_with_sources { #[test] fn in_one_level_operation_works() -> Result<(), Vec> { let sources = SourceMap::new([("test".into(), STEPPING_SOURCE.into())], None); - let mut interpreter = Interpreter::new( - true, - sources, - PackageType::Exe, - RuntimeCapabilityFlags::all(), - )?; - interpreter.set_entry()?; - let ids = get_breakpoint_ids(&interpreter, "test"); + let mut debugger = Debugger::new(sources, RuntimeCapabilityFlags::all())?; + debugger.set_entry()?; + let ids = get_breakpoint_ids(&debugger, "test"); let expected_id = ids[0]; - expect_bp(&mut interpreter, &ids, expected_id); - expect_in(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); + expect_bp(&mut debugger, &ids, expected_id); + expect_in(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); let expected = "42"; - expect_return(interpreter, expected); + expect_return(debugger, expected); Ok(()) } #[test] fn next_crosses_operation_works() -> Result<(), Vec> { let sources = SourceMap::new([("test".into(), STEPPING_SOURCE.into())], None); - let mut interpreter = Interpreter::new( - true, - sources, - PackageType::Exe, - RuntimeCapabilityFlags::all(), - )?; - interpreter.set_entry()?; - let ids = get_breakpoint_ids(&interpreter, "test"); + let mut debugger = Debugger::new(sources, RuntimeCapabilityFlags::all())?; + debugger.set_entry()?; + let ids = get_breakpoint_ids(&debugger, "test"); let expected_id = ids[0]; - expect_bp(&mut interpreter, &ids, expected_id); - expect_next(&mut interpreter); - expect_next(&mut interpreter); + expect_bp(&mut debugger, &ids, expected_id); + expect_next(&mut debugger); + expect_next(&mut debugger); let expected = "42"; - expect_return(interpreter, expected); + expect_return(debugger, expected); Ok(()) } #[test] fn in_multiple_operations_works() -> Result<(), Vec> { let sources = SourceMap::new([("test".into(), STEPPING_SOURCE.into())], None); - let mut interpreter = Interpreter::new( - true, - sources, - PackageType::Exe, - RuntimeCapabilityFlags::all(), - )?; - interpreter.set_entry()?; - let ids = get_breakpoint_ids(&interpreter, "test"); + let mut debugger = Debugger::new(sources, RuntimeCapabilityFlags::all())?; + debugger.set_entry()?; + let ids = get_breakpoint_ids(&debugger, "test"); let expected_id = ids[0]; - expect_bp(&mut interpreter, &ids, expected_id); - expect_in(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); - expect_in(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); + expect_bp(&mut debugger, &ids, expected_id); + expect_in(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); + expect_in(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); let expected = "42"; - expect_return(interpreter, expected); + expect_return(debugger, expected); Ok(()) } #[test] fn out_multiple_operations_works() -> Result<(), Vec> { let sources = SourceMap::new([("test".into(), STEPPING_SOURCE.into())], None); - let mut interpreter = Interpreter::new( - true, - sources, - PackageType::Exe, - RuntimeCapabilityFlags::all(), - )?; - interpreter.set_entry()?; - let ids = get_breakpoint_ids(&interpreter, "test"); + let mut debugger = Debugger::new(sources, RuntimeCapabilityFlags::all())?; + debugger.set_entry()?; + let ids = get_breakpoint_ids(&debugger, "test"); let expected_id = ids[0]; - expect_bp(&mut interpreter, &ids, expected_id); - expect_in(&mut interpreter); - expect_next(&mut interpreter); - expect_next(&mut interpreter); - expect_in(&mut interpreter); - expect_out(&mut interpreter); - expect_out(&mut interpreter); - expect_next(&mut interpreter); + expect_bp(&mut debugger, &ids, expected_id); + expect_in(&mut debugger); + expect_next(&mut debugger); + expect_next(&mut debugger); + expect_in(&mut debugger); + expect_out(&mut debugger); + expect_out(&mut debugger); + expect_next(&mut debugger); let expected = "42"; - expect_return(interpreter, expected); + expect_return(debugger, expected); Ok(()) } } diff --git a/compiler/qsc/src/interpret/tests.rs b/compiler/qsc/src/interpret/tests.rs index e2706e2ab2..cddf4fa43e 100644 --- a/compiler/qsc/src/interpret/tests.rs +++ b/compiler/qsc/src/interpret/tests.rs @@ -1127,6 +1127,7 @@ mod given_interpreter { use std::sync::Arc; use super::*; + use crate::interpret::Debugger; use expect_test::expect; use indoc::indoc; use qsc_frontend::compile::{RuntimeCapabilityFlags, SourceMap}; @@ -1223,6 +1224,7 @@ mod given_interpreter { r#" namespace Test2 { open Test; + @EntryPoint() operation Main() : String { Hello(); Hello() @@ -1233,16 +1235,11 @@ mod given_interpreter { ]; let sources = SourceMap::new(sources, None); - let interpreter = Interpreter::new( - true, - sources, - PackageType::Lib, - RuntimeCapabilityFlags::all(), - ) - .expect("interpreter should be created"); - let bps = interpreter.get_breakpoints("a.qs"); + let debugger = Debugger::new(sources, RuntimeCapabilityFlags::all()) + .expect("debugger should be created"); + let bps = debugger.get_breakpoints("a.qs"); assert_eq!(1, bps.len()); - let bps = interpreter.get_breakpoints("b.qs"); + let bps = debugger.get_breakpoints("b.qs"); assert_eq!(2, bps.len()); } diff --git a/npm/test/basics.js b/npm/test/basics.js index 0245dbba64..808005dc11 100644 --- a/npm/test/basics.js +++ b/npm/test/basics.js @@ -697,16 +697,6 @@ async function testCompilerError(useWorker) { test("compiler error on run", () => testCompilerError(false)); test("compiler error on run - worker", () => testCompilerError(true)); -test("debug service get breakpoints without service loaded returns empty - web worker", async () => { - const debugService = getDebugServiceWorker(); - try { - const emptyBps = await debugService.getBreakpoints("test.qs"); - assert.equal(0, emptyBps.length); - } finally { - debugService.terminate(); - } -}); - test("debug service loading source without entry point attr fails - web worker", async () => { const debugService = getDebugServiceWorker(); try { diff --git a/wasm/src/debug_service.rs b/wasm/src/debug_service.rs index eac5db6ab6..41c94d05ac 100644 --- a/wasm/src/debug_service.rs +++ b/wasm/src/debug_service.rs @@ -4,8 +4,8 @@ use std::str::FromStr; use qsc::fir::StmtId; -use qsc::interpret::{Error, Interpreter, StepAction, StepResult}; -use qsc::{fmt_complex, target::Profile, PackageType, SourceMap}; +use qsc::interpret::{Debugger, Error, StepAction, StepResult}; +use qsc::{fmt_complex, target::Profile}; use crate::{get_source_map, serializable_type, CallbackReceiver}; use serde::{Deserialize, Serialize}; @@ -13,23 +13,16 @@ use serde_json::json; use wasm_bindgen::prelude::*; #[wasm_bindgen] +#[derive(Default)] pub struct DebugService { - interpreter: Interpreter, + debugger: Option, } #[wasm_bindgen] impl DebugService { #[wasm_bindgen(constructor)] pub fn new() -> Self { - Self { - interpreter: Interpreter::new( - false, - SourceMap::default(), - PackageType::Lib, - Profile::Unrestricted.into(), - ) - .expect("Couldn't create interpreter"), - } + Self::default() } pub fn load_source( @@ -41,10 +34,10 @@ impl DebugService { let source_map = get_source_map(sources, entry); let target = Profile::from_str(&target_profile) .unwrap_or_else(|_| panic!("Invalid target : {}", target_profile)); - match Interpreter::new(true, source_map, PackageType::Exe, target.into()) { - Ok(interpreter) => { - self.interpreter = interpreter; - match self.interpreter.set_entry() { + match Debugger::new(source_map, target.into()) { + Ok(debugger) => { + self.debugger = Some(debugger); + match self.debugger_mut().set_entry() { Ok(()) => "".to_string(), Err(e) => render_errors(e), } @@ -54,7 +47,7 @@ impl DebugService { } pub fn capture_quantum_state(&mut self) -> IQuantumStateList { - let state = self.interpreter.capture_quantum_state(); + let state = self.debugger_mut().capture_quantum_state(); let entries = state .0 .iter() @@ -68,7 +61,7 @@ impl DebugService { } pub fn get_stack_frames(&self) -> IStackFrameList { - let frames = self.interpreter.get_stack_frames(); + let frames = self.debugger().get_stack_frames(); StackFrameList { frames: frames @@ -150,7 +143,7 @@ impl DebugService { F: Fn(&str), { let mut out = CallbackReceiver { event_cb }; - let result = self.interpreter.eval_step(&mut out, bps, step); + let result = self.debugger_mut().eval_step(&mut out, bps, step); let mut success = true; let msg: Option = match &result { @@ -183,7 +176,7 @@ impl DebugService { } pub fn get_breakpoints(&self, path: &str) -> IBreakpointSpanList { - let bps = self.interpreter.get_breakpoints(path); + let bps = self.debugger().get_breakpoints(path); BreakpointSpanList { spans: bps @@ -199,7 +192,7 @@ impl DebugService { } pub fn get_locals(&self) -> IVariableList { - let locals = self.interpreter.get_locals(); + let locals = self.debugger().get_locals(); let variables: Vec<_> = locals .into_iter() .map(|local| Variable { @@ -210,11 +203,17 @@ impl DebugService { .collect(); VariableList { variables }.into() } -} -impl Default for DebugService { - fn default() -> Self { - Self::new() + fn debugger(&self) -> &Debugger { + self.debugger + .as_ref() + .expect("debugger should be initialized") + } + + fn debugger_mut(&mut self) -> &mut Debugger { + self.debugger + .as_mut() + .expect("debugger should be initialized") } }