Extract InterpreterFrame from Frame with Deref wrapper#7353
Merged
youknowone merged 3 commits intoRustPython:mainfrom Mar 5, 2026
Merged
Extract InterpreterFrame from Frame with Deref wrapper#7353youknowone merged 3 commits intoRustPython:mainfrom
youknowone merged 3 commits intoRustPython:mainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughFrame internals were moved into a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin interpreter-frame |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 768-774: Update the doc comment for the function
clear_stack_and_cells to accurately reflect its behavior: state that it clears
only the evaluation stack (via localsplus.stack_clear()) and does not clear
cell/free references, and mention that full local/cell cleanup is performed by
clear_locals_and_stack (which clears cell/free refs later); adjust the comment
wording accordingly so it no longer claims cell/free refs are cleared here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 176e8188-d06f-4423-a2b6-bb9b7ba82885
📒 Files selected for processing (1)
crates/vm/src/frame.rs
Cell/free variable objects were stored in both a separate `Box<[PyCellRef]>` (cells_frees field) and in the localsplus fastlocals array. Remove the redundant cells_frees field and access cell objects directly through localsplus, eliminating one Box allocation and N clone operations per frame creation.
Introduce InterpreterFrame struct containing all execution state fields previously on Frame. Frame now wraps InterpreterFrame via FrameUnsafeCell and implements Deref for transparent field access. localsplus and prev_line are plain fields on InterpreterFrame (no longer individually wrapped in FrameUnsafeCell) since the entire InterpreterFrame is wrapped at the Frame level.
youknowone
force-pushed
the
interpreter-frame
branch
from
1380396 to
86fd5d1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/vm/src/frame.rs (1)
768-770:⚠️ Potential issue | 🟡 MinorDoc comment still overstates
clear_stack_and_cells()behavior.The function clears only the evaluation stack; cell/free refs are cleared in
clear_locals_and_stack().📝 Proposed comment-only fix
- /// Clear evaluation stack and state-owned cell/free references. - /// For full local/cell cleanup, call `clear_locals_and_stack()`. + /// Clear only the evaluation stack. + /// For full local/cell cleanup (including cell/free refs), call `clear_locals_and_stack()`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 768 - 770, The doc comment for clear_stack_and_cells() incorrectly claims it clears "state-owned cell/free references"; update the documentation to state precisely that clear_stack_and_cells() only clears the evaluation stack (and any immediate stack state) and does not clear cell/free references, and point readers to clear_locals_and_stack() for full local/cell cleanup; ensure the summary and the one-line description around the pub(crate) fn clear_stack_and_cells() signature are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 626-639: The Deref impl for Frame (fn deref(&self) ->
&InterpreterFrame) unsafely exposes a shared reference to InterpreterFrame while
methods like clear_stack_and_cells, clear_locals_and_stack, and locals_to_fast
mutate InterpreterFrame::localsplus via UnsafeCell with only &self, allowing
soundness-violating aliasing. Fix by removing or restricting the Deref
implementation and instead provide explicit accessor methods that return only
immutable views of truly immutable fields, or change the mutating methods
(clear_stack_and_cells, clear_locals_and_stack, locals_to_fast) to require &mut
self (or use a runtime borrow like RefCell/Mutex) so callers cannot hold a
shared &InterpreterFrame from deref() while invoking those mutators; update call
sites to use the new accessors or &mut Frame accordingly.
---
Duplicate comments:
In `@crates/vm/src/frame.rs`:
- Around line 768-770: The doc comment for clear_stack_and_cells() incorrectly
claims it clears "state-owned cell/free references"; update the documentation to
state precisely that clear_stack_and_cells() only clears the evaluation stack
(and any immediate stack state) and does not clear cell/free references, and
point readers to clear_locals_and_stack() for full local/cell cleanup; ensure
the summary and the one-line description around the pub(crate) fn
clear_stack_and_cells() signature are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5fea980b-d520-4c42-a405-cfd8e3514e5e
📒 Files selected for processing (1)
crates/vm/src/frame.rs
Comment on lines
+626
to
+639
| impl core::ops::Deref for Frame { | ||
| type Target = InterpreterFrame; | ||
| /// Transparent access to InterpreterFrame fields. | ||
| /// | ||
| /// # Safety argument | ||
| /// Immutable fields (code, globals, builtins, func_obj, locals) are safe | ||
| /// to access at any time. Atomic/mutex fields (lasti, trace, owner, etc.) | ||
| /// provide their own synchronization. Mutable fields (localsplus, prev_line) | ||
| /// are only mutated during single-threaded execution via `with_exec`. | ||
| #[inline(always)] | ||
| fn deref(&self) -> &InterpreterFrame { | ||
| unsafe { &*self.iframe.get() } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/vm/src/frame.rs"
echo "=== Frame struct definition and iframe field type ==="
rg -n "^pub struct Frame" "$file" -A30 | head -50
echo
echo "=== Deref implementation for Frame ==="
rg -n "impl.*Deref.*Frame" "$file" -A15Repository: RustPython/RustPython
Length of output: 1876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/vm/src/frame.rs"
echo "=== Methods mentioned: clear_stack_and_cells ==="
rg -n "fn clear_stack_and_cells\(&self" "$file" -A20
echo
echo "=== Methods mentioned: clear_locals_and_stack ==="
rg -n "fn clear_locals_and_stack\(&self" "$file" -A20
echo
echo "=== Methods mentioned: locals_to_fast ==="
rg -n "fn locals_to_fast\(&self" "$file" -A20Repository: RustPython/RustPython
Length of output: 3199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/vm/src/frame.rs"
echo "=== InterpreterFrame struct definition ==="
rg -n "^pub struct InterpreterFrame|^struct InterpreterFrame" "$file" -A50 | head -80Repository: RustPython/RustPython
Length of output: 2508
Deref<Target = InterpreterFrame> allows unsound aliasing with safe mutation methods.
Frame::deref() exposes a safe shared reference to InterpreterFrame, but clear_stack_and_cells(), clear_locals_and_stack(), and locals_to_fast() all mutate localsplus through UnsafeCell while taking only &self. Code can hold a reference from Deref while simultaneously calling these mutating methods, violating Rust's aliasing rules in safe code. The safety invariants ("frame not executing") are documented but not enforced by the type system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/frame.rs` around lines 626 - 639, The Deref impl for Frame (fn
deref(&self) -> &InterpreterFrame) unsafely exposes a shared reference to
InterpreterFrame while methods like clear_stack_and_cells,
clear_locals_and_stack, and locals_to_fast mutate InterpreterFrame::localsplus
via UnsafeCell with only &self, allowing soundness-violating aliasing. Fix by
removing or restricting the Deref implementation and instead provide explicit
accessor methods that return only immutable views of truly immutable fields, or
change the mutating methods (clear_stack_and_cells, clear_locals_and_stack,
locals_to_fast) to require &mut self (or use a runtime borrow like
RefCell/Mutex) so callers cannot hold a shared &InterpreterFrame from deref()
while invoking those mutators; update call sites to use the new accessors or
&mut Frame accordingly.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/vm/src/frame.rs (2)
768-774:⚠️ Potential issue | 🟡 Minor
clear_stack_and_cellscomment still contradicts behavior.Line [768] says cell/free refs are cleared here, but Line [774] only clears the stack, and Line [772] explicitly says cell refs are cleared in
clear_locals_and_stack().📝 Comment-only fix
- /// Clear evaluation stack and state-owned cell/free references. - /// For full local/cell cleanup, call `clear_locals_and_stack()`. + /// Clear only the evaluation stack. + /// Cell/free refs are cleared by `clear_locals_and_stack()`.As per coding guidelines, "Do not delete or rewrite existing comments unless they are factually wrong or directly contradict the new code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 768 - 774, The doc-comment for clear_stack_and_cells is contradictory: it claims "Clear evaluation stack and state-owned cell/free references" but the implementation only calls (*self.iframe.get()).localsplus.stack_clear() and the comment inside already says cell refs are cleared by clear_locals_and_stack(); update the comment on clear_stack_and_cells to accurately describe behavior (i.e., that it only clears the evaluation stack and state-owned stack slots and that cell/free references are handled by clear_locals_and_stack()), mentioning the related symbols clear_locals_and_stack, localsplus.stack_clear, and iframe to make the intent clear.
626-639:⚠️ Potential issue | 🔴 Critical
Deref<Target = InterpreterFrame>exposes unsound aliasing in safe code.Line [636] returns
&InterpreterFramefromUnsafeCell, while safe methods mutate the same storage via&self(e.g., Line [770], Line [780], Line [863], andwith_execat Line [929]). This permits holding a shared reference while mutating behind it, which violates Rust aliasing guarantees.🔍 Read-only verification script
#!/bin/bash set -euo pipefail file="crates/vm/src/frame.rs" echo "=== Deref exposing &InterpreterFrame from UnsafeCell ===" rg -n "impl core::ops::Deref for Frame|fn deref\(&self\)|iframe\.get\(\)" "$file" -A6 -B3 echo echo "=== Safe methods mutating frame internals via iframe.get() ===" rg -n "fn clear_stack_and_cells\(&self\)|fn clear_locals_and_stack\(&self\)|fn locals_to_fast\(&self|fn with_exec<" "$file" -A20 -B2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 626 - 639, The Deref impl for Frame currently returns &InterpreterFrame from an UnsafeCell, enabling safe code to hold a shared reference while other safe methods (clear_stack_and_cells, clear_locals_and_stack, locals_to_fast, with_exec) mutate the same storage — this is unsound; remove or change the impl so safe code cannot obtain a long-lived &InterpreterFrame from UnsafeCell. Specifically, either delete impl core::ops::Deref for Frame and replace call sites with explicit safe accessor methods (e.g., borrow_immutable() that returns a restricted read-only view type or with_read/with_exec closures), or change the Target to a new immutable view type that contains only truly immutable fields and update callers to use that view; ensure all mutation paths (with_exec, clear_*, locals_to_fast) continue to use iframe.get() internally and that no public method can produce a &InterpreterFrame to callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/vm/src/frame.rs`:
- Around line 768-774: The doc-comment for clear_stack_and_cells is
contradictory: it claims "Clear evaluation stack and state-owned cell/free
references" but the implementation only calls
(*self.iframe.get()).localsplus.stack_clear() and the comment inside already
says cell refs are cleared by clear_locals_and_stack(); update the comment on
clear_stack_and_cells to accurately describe behavior (i.e., that it only clears
the evaluation stack and state-owned stack slots and that cell/free references
are handled by clear_locals_and_stack()), mentioning the related symbols
clear_locals_and_stack, localsplus.stack_clear, and iframe to make the intent
clear.
- Around line 626-639: The Deref impl for Frame currently returns
&InterpreterFrame from an UnsafeCell, enabling safe code to hold a shared
reference while other safe methods (clear_stack_and_cells,
clear_locals_and_stack, locals_to_fast, with_exec) mutate the same storage —
this is unsound; remove or change the impl so safe code cannot obtain a
long-lived &InterpreterFrame from UnsafeCell. Specifically, either delete impl
core::ops::Deref for Frame and replace call sites with explicit safe accessor
methods (e.g., borrow_immutable() that returns a restricted read-only view type
or with_read/with_exec closures), or change the Target to a new immutable view
type that contains only truly immutable fields and update callers to use that
view; ensure all mutation paths (with_exec, clear_*, locals_to_fast) continue to
use iframe.get() internally and that no public method can produce a
&InterpreterFrame to callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7353e605-655c-49a1-a671-ee87f26c26b8
📒 Files selected for processing (1)
crates/vm/src/frame.rs
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
1 participant
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.
close #7351
Summary
InterpreterFramestruct containing all execution state fields previously onFrame, analogous to CPython's_PyInterpreterFrameFrame(PyFrameObject) now wrapsInterpreterFrameviaFrameUnsafeCelland implementsDereffor transparent field accesslocalsplusandprev_lineare plain fields onInterpreterFrame(no longer individually wrapped inFrameUnsafeCell) since the entireInterpreterFrameis wrapped at theFramelevelThis is a preparatory step toward removing PyObject allocation for normal function calls (future PR will allow
InterpreterFrameto live on the Rust stack + DataStack without creating aFramePyObject).Test plan
cargo buildpasses with no warningscargo testpassescargo run --release -- -m test test_scope test_generators test_coroutines test_sys test_traceback test_frame -v— all 6 test suites pass (721 tests)🤖 Generated with Claude Code
Summary by CodeRabbit