From 796da46ae89a4f832c6a1e6fd482f2af0e3a2034 Mon Sep 17 00:00:00 2001
From: Anand Krishnamoorthi <35780660+anakrish@users.noreply.github.com>
Date: Fri, 23 Jun 2023 08:57:47 -0700
Subject: [PATCH] Ensure that scopes are cleaned up correctly upon error. (#20)

When evaluating rules, upon error the last pushed scope wasn't being
popped from the stack of scopes. This causes incorrect behavior
when there are multiple definitions for the same rule name.

The fix is to make sure that the scopes are popped manually upon encountering errors.

Once the interpreter logic is locked down, then we need to clean up scope management
using Drop functions so that the cleanup happens even during short circuited return.

Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
---
 src/interpreter.rs                   | 24 +++++++++++++++---------
 src/scheduler.rs                     |  1 -
 tests/interpreter/cases/call/or.yaml |  8 +++-----
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/src/interpreter.rs b/src/interpreter.rs
index ebc91d8..34eb9a0 100644
--- a/src/interpreter.rs
+++ b/src/interpreter.rs
@@ -1310,6 +1310,7 @@ impl<'source> Interpreter<'source> {
                 Err(e) => {
                     // If the rule produces an error, save the error.
                     errors.push(e);
+                    self.scopes = scopes;
                     continue;
                 }
             };
@@ -1550,33 +1551,38 @@ impl<'source> Interpreter<'source> {
         span: &'source Span<'source>,
         bodies: &'source Vec<RuleBody<'source>>,
     ) -> Result<Value> {
-        let mut result = true;
-        self.scopes.push(Scope::new());
-
-        if bodies.is_empty() {
+        let result = if bodies.is_empty() {
             self.contexts.push(ctx.clone());
-            result = self.eval_output_expr()?;
+            self.eval_output_expr()
         } else {
+            let mut result = Ok(true);
             for body in bodies {
                 self.contexts.push(ctx.clone());
-                result = self.eval_query(&body.query)?;
+                result = self.eval_query(&body.query);
 
-                // The body evaluated successfully.
-                if result {
+                if matches!(&result, Ok(true) | Err(_)) {
                     break;
                 }
 
+                // TODO: Manage other scoped data.
+                self.scopes.pop();
                 if bodies.len() > 1 {
                     unimplemented!("else bodies");
                 }
             }
-        }
+            result
+        };
 
         let ctx = match self.contexts.pop() {
             Some(ctx) => ctx,
             _ => bail!("internal error: rule's context already popped"),
         };
 
+        let result = match result {
+            Ok(r) => r,
+            Err(e) => return Err(e),
+        };
+
         // Drop local variables and leave the local scope
         self.scopes.pop();
 
diff --git a/src/scheduler.rs b/src/scheduler.rs
index 09f618b..3158074 100644
--- a/src/scheduler.rs
+++ b/src/scheduler.rs
@@ -39,7 +39,6 @@ pub enum SortResult {
 }
 
 pub fn schedule<'a>(infos: &mut [StmtInfo<'a>]) -> Result<SortResult> {
-    println!("infos: {infos:?}");
     let num_statements = infos.len();
 
     // Mapping from each var to the list of statements that define it.
diff --git a/tests/interpreter/cases/call/or.yaml b/tests/interpreter/cases/call/or.yaml
index b63b6bd..42982ad 100644
--- a/tests/interpreter/cases/call/or.yaml
+++ b/tests/interpreter/cases/call/or.yaml
@@ -130,10 +130,8 @@ cases:
         a2 = fcn(5)
     query: data.test
     want_result:
-      a1: 
-        set!: [hello world]
-      a2: 
-        set!: [6]
+      a1: "hello world"
+      a2: 6
 
   - note: or-all-error
     data: {}
@@ -151,4 +149,4 @@ cases:
 
         a1 = fcn("world")
     query: data.test
-    error: "`add` expects numeric argument. Got `\"world\"` instead"
\ No newline at end of file
+    error: "`add` expects numeric argument. Got `\"world\"` instead"