Env.lookup: walk full parent chain; cache validates intermediates

The previous Env.lookup had a shortcut that checked self.g (global)
right after self.b (local). This skipped any intermediate parent
frame that shadowed a global name. The concrete bug was a let-loop
parameter named `count` (also a SRFI-1 builtin): when an inner
(let ((next ...))) pushed a new frame between the loop body and
the loop binding, `count` was not in self.b, the shortcut found
the global builtin, and returned it — instead of walking up one
more parent to the loop's parameter frame.

Fix: remove the shortcut, walk self → self.p → ... → global in
order. O(chain depth) instead of O(1) for the common case, but
correct. Chain depths are small in practice.

The inline cache (bytecode VM's OP_LOOKUP) had the mirror issue —
it verified only `arg not in env.b`, missing parent shadows.
Updated to walk the chain from env up to the cached env (always
global) and check each intermediate frame before returning the
cached value. The cache still reads the value fresh from the
cached env's bindings dict so `set!` on a global is observed
immediately (previously a cached value would go stale on set!
even though the test suite's test_compile_mutual_recursion
depended on this behavior).

Cache is now populated only when the lookup resolved identity-
equal to the global's current binding — i.e. no intermediate
shadow — using `val is g.b[arg]` as the guard.

Regression: all 975 tests still green (571 py + 132 asm + 189
shared + 83 c), including test_compile_mutual_recursion that
exercises set!-after-compile.

Discovered while debugging portal-http-client.lsp, where the
portal body's (define counter ...) form landed correctly but a
nearby let-loop accumulator named `count` resolved to the Python
builtin `count` (SRFI-1 count procedure). The server itself, and
asm and C clients, were unaffected — asm's env lookup walks the
chain, and C's env lookup has no equivalent shortcut.
This commit is contained in:
russell@unturf.com 2026-04-17 14:39:26 -04:00
parent 18e68f8838
commit 68c3d3a928

View file

@ -489,13 +489,19 @@ class Env:
self.g = parent.g if parent else None # global env shortcut
def lookup(self, k):
b = self.b
if k in b: return b[k]
g = self.g
if g is not None and k in g.b: return g.b[k]
e = self.p
while e:
if k in e.b: return e.b[k]
# Walk local → parents → global. Previously there was a
# shortcut that checked self.g (global) right after self.b
# (local), which skipped any intermediate parent frame that
# shadowed a global name. That broke e.g. a let-loop named
# `count` (a SRFI-1 builtin) when an inner `(let ((next ...)))`
# pushed a new frame between the loop body and the loop
# binding: self.b lacked `count`, global had the builtin, and
# the shortcut returned the builtin instead of walking up to
# the parent frame that held the loop parameter.
e = self
while e is not None:
b = e.b
if k in b: return b[k]
e = e.p
raise LispErr(f'undefined: {k}')
@ -1676,14 +1682,27 @@ def _vm_loop(instrs, ip, stack, env, frames, vm_id):
elif op == OP_LOOKUP:
idx = ip - 1
cached = _ic.get(idx)
if cached is not None and arg not in env.b:
ce, cv = cached
if arg in ce.b:
if cached is not None:
ce, _ = cached
# Cache valid only if no intermediate frame shadows
# the name between env and ce (the cached env, always
# the global env). Previously only `arg not in env.b`
# was checked, which missed parent-frame shadows such
# as a let-loop param sharing a global builtin name.
# We still read the value fresh from ce.b so that
# set! on globals is observed immediately.
e = env; shadowed = False
while e is not ce:
if e is None: shadowed = True; break
if arg in e.b: shadowed = True; break
e = e.p
if not shadowed and arg in ce.b:
_ap(ce.b[arg]); continue
val = env.lookup(arg)
# Cache if resolved to global (safe — globals rarely change)
# Cache only if the resolved value came from global —
# i.e. no intermediate frame shadowed on the way.
g = env.g
if g is not None and arg in g.b:
if g is not None and arg in g.b and val is g.b[arg]:
_ic[idx] = (g, val)
_ap(val)
elif op == OP_SET: env.set(arg, _po())