From 3cc899f30145584967c0f35a0d35617ffd94a1fd Mon Sep 17 00:00:00 2001
From: Caleb Walch <cawalch@gmail.com>
Date: Sun, 15 Dec 2024 09:37:20 -0600
Subject: [PATCH] fix(scope): improve variable lookup and null handling in
 scope chain

* use hasOwnProperty for more precise scope checks
* add getter for current scope data
* fix variable reference error handling
* add comprehensive tests for scope chain behavior
<!-- ps-id: 54d45955-fef8-47f4-95d9-ca34d734a696 -->
---
 src/Scope.ts           | 13 +++++++----
 src/TreeInterpreter.ts |  8 ++++---
 test/scopes.spec.ts    | 52 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/src/Scope.ts b/src/Scope.ts
index e95db16..7383fc7 100644
--- a/src/Scope.ts
+++ b/src/Scope.ts
@@ -4,6 +4,10 @@ export class ScopeChain {
   private inner?: ScopeChain = undefined;
   private data: JSONObject = {};
 
+  get currentScopeData(): JSONObject {
+    return this.data;
+  }
+
   public withScope(data: JSONObject): ScopeChain {
     const outer: ScopeChain = new ScopeChain();
     outer.inner = this;
@@ -12,15 +16,14 @@ export class ScopeChain {
   }
 
   public getValue(identifier: string): JSONValue {
-    if (identifier in this.data) {
-      const result = this.data[identifier];
-      if (result !== null && result !== undefined) {
-        return result;
-      }
+    if (Object.prototype.hasOwnProperty.call(this.data, identifier)) {
+      return this.data[identifier];
     }
+
     if (this.inner) {
       return this.inner.getValue(identifier);
     }
+
     return null;
   }
 }
diff --git a/src/TreeInterpreter.ts b/src/TreeInterpreter.ts
index b73745c..1fd1f7b 100644
--- a/src/TreeInterpreter.ts
+++ b/src/TreeInterpreter.ts
@@ -59,11 +59,13 @@ export class TreeInterpreter {
       }
       case 'Variable': {
         const variable = node.name;
-        const result = this._scope.getValue(variable) ?? null;
-        if (result === null || result === undefined) {
+        if (
+          !this._scope.getValue(variable) &&
+          !Object.prototype.hasOwnProperty.call(this._scope.currentScopeData, variable)
+        ) {
           throw new Error(`Error referencing undefined variable ${variable}`);
         }
-        return result;
+        return this._scope.getValue(variable);
       }
       case 'IndexExpression':
         return this.visit(node.right, this.visit(node.left, value));
diff --git a/test/scopes.spec.ts b/test/scopes.spec.ts
index 15690a7..a9ed263 100644
--- a/test/scopes.spec.ts
+++ b/test/scopes.spec.ts
@@ -1,3 +1,4 @@
+import { describe, it, expect } from 'vitest';
 import { Scope } from '../src';
 
 describe('scopes', () => {
@@ -24,4 +25,55 @@ describe('scopes', () => {
       expect(outer.getValue('foo')).toEqual('bar');
     }
   });
+  it('should not return value for non-existent identifiers', () => {
+    const scope = Scope();
+    {
+      const scoped = scope.withScope({ foo: 'bar' });
+      expect(scoped.getValue('baz')).toEqual(null);
+      expect(scoped.getValue('foo')).toEqual('bar');
+    }
+  });
+  it('should return null for identifiers even in nested scopes if absent', () => {
+    const scope = Scope();
+    {
+      const outer = scope.withScope({ foo: 'bar' });
+      {
+        const inner = outer.withScope({ bar: 'baz' });
+        expect(inner.getValue('qux')).toEqual(null);
+      }
+      expect(outer.getValue('qux')).toEqual(null);
+    }
+  });
+  it('should handle values in nested scopes differently from outer scopes', () => {
+    const scope = Scope();
+    {
+      const outer = scope.withScope({ foo: 'bar' });
+      {
+        const inner = outer.withScope({ bar: 'baz' });
+        expect(inner.getValue('foo')).toEqual('bar');
+        expect(inner.getValue('bar')).toEqual('baz');
+      }
+    }
+  });
+  it('should not fall through to outer scope when key is in current scope with null/undefined', () => {
+    const scope = Scope();
+    {
+      const outer = scope.withScope({ foo: 'bar' });
+      {
+        const inner = outer.withScope({ foo: null });
+        expect(inner.getValue('foo')).toEqual(null);
+      }
+    }
+  });
+  it('should properly differentiate between keys absent entirely and those in outer scopes', () => {
+    const scope = Scope();
+    {
+      const outer = scope.withScope({ foo: 'bar' });
+      {
+        const inner = outer.withScope({});
+        expect(inner.getValue('foo')).toEqual('bar');
+        expect(inner.getValue('baz')).toEqual(null);
+      }
+    }
+  });
 });