From a1ecd068b2a9cbc260534ed25fe16d7125b96d9d Mon Sep 17 00:00:00 2001
From: "Stefan J. Wernli" <swernli@microsoft.com>
Date: Mon, 29 Apr 2024 13:52:56 -0700
Subject: [PATCH] Track nested scopes during debugging (#1442)

This adds new execution graph nodes for pushing and popping variable
scopes that are used only while debugging (like statement nodes) so that
the evaluator can correctly respond to local variable requests during
debugging.

Fixes #1436
---
 compiler/qsc_eval/src/lib.rs    | 66 ++++++++++++++++++++++-----------
 compiler/qsc_fir/src/fir.rs     |  4 ++
 compiler/qsc_lowerer/src/lib.rs | 15 ++++++++
 3 files changed, 64 insertions(+), 21 deletions(-)

diff --git a/compiler/qsc_eval/src/lib.rs b/compiler/qsc_eval/src/lib.rs
index 2e2b69cce6..b1fa440435 100644
--- a/compiler/qsc_eval/src/lib.rs
+++ b/compiler/qsc_eval/src/lib.rs
@@ -563,27 +563,9 @@ impl State {
                     self.idx += 1;
                     self.current_span = globals.get_stmt((self.package, *stmt).into()).span;
 
-                    if let Some(bp) = breakpoints
-                        .iter()
-                        .find(|&bp| *bp == *stmt && self.package == self.source_package)
-                    {
-                        StepResult::BreakpointHit(*bp)
-                    } else {
-                        if self.current_span == Span::default() {
-                            // if there is no span, we are in generated code, so we should skip
-                            continue;
-                        }
-                        // no breakpoint, but we may stop here
-                        if step == StepAction::In {
-                            StepResult::StepIn
-                        } else if step == StepAction::Next && current_frame >= self.call_stack.len()
-                        {
-                            StepResult::Next
-                        } else if step == StepAction::Out && current_frame > self.call_stack.len() {
-                            StepResult::StepOut
-                        } else {
-                            continue;
-                        }
+                    match self.check_for_break(breakpoints, *stmt, step, current_frame) {
+                        Some(value) => value,
+                        None => continue,
                     }
                 }
                 Some(ExecGraphNode::Jump(idx)) => {
@@ -623,6 +605,16 @@ impl State {
                     env.leave_scope();
                     continue;
                 }
+                Some(ExecGraphNode::PushScope) => {
+                    self.push_scope(env);
+                    self.idx += 1;
+                    continue;
+                }
+                Some(ExecGraphNode::PopScope) => {
+                    env.leave_scope();
+                    self.idx += 1;
+                    continue;
+                }
                 None => {
                     // We have reached the end of the current graph without reaching an explicit return node,
                     // usually indicating the partial execution of a single sub-expression.
@@ -644,6 +636,38 @@ impl State {
         Ok(StepResult::Return(self.get_result()))
     }
 
+    fn check_for_break(
+        &self,
+        breakpoints: &[StmtId],
+        stmt: StmtId,
+        step: StepAction,
+        current_frame: usize,
+    ) -> Option<StepResult> {
+        Some(
+            if let Some(bp) = breakpoints
+                .iter()
+                .find(|&bp| *bp == stmt && self.package == self.source_package)
+            {
+                StepResult::BreakpointHit(*bp)
+            } else {
+                if self.current_span == Span::default() {
+                    // if there is no span, we are in generated code, so we should skip
+                    return None;
+                }
+                // no breakpoint, but we may stop here
+                if step == StepAction::In {
+                    StepResult::StepIn
+                } else if step == StepAction::Next && current_frame >= self.call_stack.len() {
+                    StepResult::Next
+                } else if step == StepAction::Out && current_frame > self.call_stack.len() {
+                    StepResult::StepOut
+                } else {
+                    return None;
+                }
+            },
+        )
+    }
+
     pub fn get_result(&mut self) -> Value {
         // Some executions don't have any statements to execute,
         // such as a fragment that has only item definitions.
diff --git a/compiler/qsc_fir/src/fir.rs b/compiler/qsc_fir/src/fir.rs
index df84098a4a..5f8d84fcf1 100644
--- a/compiler/qsc_fir/src/fir.rs
+++ b/compiler/qsc_fir/src/fir.rs
@@ -900,6 +900,10 @@ pub enum ExecGraphNode {
     Unit,
     /// The end of the control flow graph.
     Ret,
+    /// A push of a new scope, used when tracking variables for debugging.
+    PushScope,
+    /// A pop of the current scope, used when tracking variables for debugging.
+    PopScope,
 }
 
 /// A sequenced block of statements.
diff --git a/compiler/qsc_lowerer/src/lib.rs b/compiler/qsc_lowerer/src/lib.rs
index c38e98090c..8a238e44da 100644
--- a/compiler/qsc_lowerer/src/lib.rs
+++ b/compiler/qsc_lowerer/src/lib.rs
@@ -274,6 +274,18 @@ impl Lowerer {
 
     fn lower_block(&mut self, block: &hir::Block) -> BlockId {
         let id = self.assigner.next_block();
+        // When lowering for debugging, we need to be more strict about scoping for variables
+        // otherwise variables that are not in scope will be visible in the locals view.
+        // We push a scope entry marker, `PushScope`, here and then a `PopScope` marker at the
+        // end of the block, which will cause the evaluation logic to track local variables
+        // for this block in the innermost scope matching their actual accessibility.
+        // When not in debug mode, variables may persist across block boundaries, but all access
+        // is performed via their lowered local variable ID, so they cannot be accessed outside of
+        // their scope. Associated memory is still cleaned up at callable exit rather than block
+        // exit.
+        if self.enable_debug {
+            self.exec_graph.push(ExecGraphNode::PushScope);
+        }
         let set_unit = block.stmts.is_empty()
             || !matches!(
                 block.stmts.last().expect("block should be non-empty").kind,
@@ -288,6 +300,9 @@ impl Lowerer {
         if set_unit {
             self.exec_graph.push(ExecGraphNode::Unit);
         }
+        if self.enable_debug {
+            self.exec_graph.push(ExecGraphNode::PopScope);
+        }
         self.blocks.insert(id, block);
         id
     }