fix(eval STRING): preserve 'our' decls so caller-side local() is visible#532
Merged
fix(eval STRING): preserve 'our' decls so caller-side local() is visible#532
Conversation
Inside an eval STRING, variables declared with our in an outer scope were being treated as plain lexicals bound to the scalar captured at eval-entry time. That made local $OurVar / local @OurArr / local %OurHash in the caller invisible to the eval body: it kept reading the pre-local scalar (or the empty array/hash whose slot had been swapped out by makeLocal). The root cause was in the bytecode-interpreter eval path: RuntimeCode and EvalStringHandler build an adjustedRegistry of captured variables and hand it to BytecodeCompiler, which seeded every captured variable in its symbol table with decl="my". compileVariableReference then took the register path for 'our' vars instead of emitting LOAD_GLOBAL_SCALAR. Changes: - RuntimeCode.evalStringWithInterpreter now builds a parallel adjustedDecls map from the captured symbol table and passes it to BytecodeCompiler so 'our' stays 'our'. - BytecodeCompiler gets a new constructor overload that accepts parentDecls and preserves the outer decl when seeding its symbol table. - handleArrayElementAccess and handleHashElementAccess now check isOurVariable and route 'our' aggregates through LOAD_GLOBAL_ARRAY / LOAD_GLOBAL_HASH so element reads see the current (possibly localized) slot. - EvalStringHandler keeps the old behaviour: for nested evals the symbol table intentionally seeds captured outer `my` vars as `our` in a synthetic package so the parser accepts them, but at runtime those must stay register-bound. A comment explains why we do not propagate adjustedDecls here. Observable impact: jcpan -t TimeDate goes from 340/1215 failing subtests across 10 files to 8/1215 in 1 file (t/lang-encoding.t, a pre-existing utf8-flag issue unrelated to eval/local). make and make test-bundled-modules both pass. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
d8503f9 to
cbab500
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes
eval STRINGso thatlocalapplied to outerour-declaredpackage variables is visible inside the eval body.
Minimal reproducer that broke before this patch:
The same issue affected
local @our_array/local %our_hashseenfrom inside
eval STRING, including element access like$our_hash{k}.Root cause
The bytecode-interpreter eval path (default on PerlOnJava) builds an
adjustedRegistryof captured variables inRuntimeCode/EvalStringHandlerand hands it toBytecodeCompiler, which seededevery captured variable in its symbol table with
decl="my".compileVariableReferencethen took the register path forourvarsinstead of emitting
LOAD_GLOBAL_SCALAR, so the eval body read thescalar that was in the global slot at eval-entry time — not the
currently-localized one.
Changes
RuntimeCode.evalStringWithInterpreternow builds a paralleladjustedDeclsmap from the captured symbol table and passes it toBytecodeCompilersoourstaysour.BytecodeCompilergets a new constructor overload that acceptsparentDeclsand preserves the outer decl when seeding its symboltable.
handleArrayElementAccessandhandleHashElementAccessnow checkisOurVariableand routeouraggregates throughLOAD_GLOBAL_ARRAY/LOAD_GLOBAL_HASHso element reads see thecurrent (possibly localized) slot.
EvalStringHandlerdeliberately does not propagateadjustedDecls: nested evals seed outermyvars asourin asynthetic package purely for the parser, and at runtime those must
stay register-bound. A comment explains this.
Observable impact
jcpan -t TimeDategoes from 340/1215 failing subtests across 10files to 8/1215 in 1 file (
t/lang-encoding.t, a pre-existingutf8::is_utf8issue unrelated toeval/local). The originalDate::Parse::str2timefailures ("Fri Dec 17 00:00:00 1901 GMT","Wed, 16 Jun 94 07:29:35 CST", …) were all caused by this bug:Date::Parsebuilds its regex closures viaeval "$strptime"insidelocal($day_ref,$mon_ref,$suf_ref), and the eval was seeing thepre-local (undef) scalars instead of the freshly-localized ones.
Test plan
make(all unit tests pass, includingunit/eval_nested_lexicals.twhich exercises theEvalStringHandlerpath we intentionally left alone)make test-bundled-modulesjcpan -t TimeDate— 1/28 test files fail, 8/1215 subtestsfail (down from 10/28 and 340/1215)
Generated with Devin