Extract InterpreterFrame from Frame with Deref wrapper - #7353
Conversation
Sorry, something went wrong.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughFrame internals were moved into a new InterpreterFrame type; Frame (pyclass) now contains pub(crate) iframe: FrameUnsafeCell<InterpreterFrame>. All frame methods, GC traversal, ExecutingFrame accessors, construction, and debug/display were updated to route through iframe. 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin interpreter-frame |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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
📥 CommitsReviewing files that changed from the base of the PR and between 9a0511b and 1380396.
📒 Files selected for processing (1)- crates/vm/src/frame.rs
Sorry, something went wrong.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)crates/vm/src/frame.rs (1)🤖 Prompt for all review comments with AI agents768-770: ⚠️ Potential issue | 🟡 Minor
Doc 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🤖 Prompt for AI Agents- /// 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()`.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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 1380396 and 86fd5d1.
📒 Files selected for processing (1)- crates/vm/src/frame.rs
Sorry, something went wrong.
| 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() } | ||
| } | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
🧩 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 AgentsVerify 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.
Sorry, something went wrong.
There was a problem hiding this comment.
crates/vm/src/frame.rs (2)🤖 Prompt for all review comments with AI agents768-774: ⚠️ Potential issue | 🟡 Minor
clear_stack_and_cells comment 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 AgentsVerify 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 &InterpreterFrame from UnsafeCell, while safe methods mutate the same storage via &self (e.g., Line [770], Line [780], Line [863], and with_exec at Line [929]). This permits holding a shared reference while mutating behind it, which violates Rust aliasing guarantees.
🔍 Read-only verification script🤖 Prompt for AI Agents#!/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 -B2Verify 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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 86fd5d1 and b1f07db.
📒 Files selected for processing (1)- crates/vm/src/frame.rs
Sorry, something went wrong.
* Remove cells_frees duplicate storage from Frame 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. * Extract InterpreterFrame from Frame with Deref wrapper 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. * Auto-format: cargo fmt --all --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
close #7351
Summary
This is a preparatory step toward removing PyObject allocation for normal function calls (future PR will allow InterpreterFrame to live on the Rust stack + DataStack without creating a Frame PyObject).
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit