Fix VM panic when function parameter shadows outer local variable#124
Open
CJHwong wants to merge 1 commit intopydantic:mainfrom
Open
Fix VM panic when function parameter shadows outer local variable#124CJHwong wants to merge 1 commit intopydantic:mainfrom
CJHwong wants to merge 1 commit intopydantic:mainfrom
Conversation
In get_id(), the name_map check (function parameters) was ordered after the enclosing_locals check (implicit closure capture). When a nested function parameter shadowed an outer local (e.g. `def inner(scale)` where outer scope also has `scale`), the parameter was incorrectly compiled as a closure cell reference instead of a local variable, causing a VM panic: "index out of bounds: the len is 1 but the index is 1" at vm/mod.rs:1678. Move the name_map check before enclosing_locals so parameters correctly shadow enclosing locals, matching Python's LEGB scoping rules.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
kundan175
approved these changes
Feb 8, 2026
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
I hit a Rust panic in the Monty VM while running a Chudnovsky algorithm for computing pi to 100 digits. The panic occurred at
vm/mod.rs:1678with:The code exercises nested function definitions where a parameter name collides with an outer local variable — valid Python that triggers a name resolution bug in the bytecode compiler.
Reproducer
This valid Python causes a VM panic on v0.0.4:
The trigger:
int_sqrt(n, scale)has a parameterscalethat shadows the outer localscale = 10 ** (precision + 15).Before fix:
After fix:
(Matches CPython output.)
Root Cause
The bug is in
prepare.rs:get_id(), which resolves variable names to their scope during bytecode compilation. The method checks a priority list and returns on the first match.The resolution order before this fix was:
When compiling
int_sqrt's body and encountering a reference toscale:scaleis not global, not nonlocal, not captured by a nested function, and not assigned inint_sqrt's body (it's a parameter, not an assignment)enclosing_locals): findsscaleincompute_pi_chudnovsky's locals → incorrectly treats it as an implicit closure capture → adds it tofree_var_map→ emitsLoadCellbytecode (closure cell load)name_map): would have correctly foundscaleas parameter index 1 and emittedLoadLocal— but this step is never reachedAt runtime, the VM executes
LoadCellwith a cell index that doesn't exist in the frame'scellsarray (which was sized based on actual captures, not the bogus ones), causing the index-out-of-bounds panic.How it got there
Two commits introduced this, neither wrong in isolation:
0038788(Dec 2025) — Original closure implementation. Added theenclosing_localscheck. No parameter check existed at all at this point.c7c196a(Jan 2026, PR fix function parameter binding #4) — "fix function parameter binding". Added thename_mapcheck to fix parameters shadowing globals, but appended it afterenclosing_locals. The fix was focused on the parameter-vs-global conflict and didn't consider parameter-vs-enclosing-local.No test ever combined parameter shadowing with closures, so the interaction went undetected.
Fix
Moved the
name_mapcheck (step 6 → step 5) before theenclosing_localscheck (step 5 → step 6):This placement is precise — it cannot go higher because:
cell_var_map) must still take priority: a parameter captured by a deeper nested function needsCellscope, notLocalassigned_names) must still take priority: a parameter reassigned in the body is already tracked thereLEGB Compliance
I verified this matches Python's LEGB (Local → Enclosing → Global → Built-in) scoping rules using CPython's
dismodule and code object introspection:CPython compiles
inner's reference toscaleasLOAD_FAST(local variable load), notLOAD_DEREF(closure cell load). Parameters are definitively in the L (Local) tier of LEGB and must shadow enclosing scope.Test Plan
closure__param_shadows_outer.pywith 6 test scenarios:scaleparam)